fix(submit): run in correct repo working copy directory when in a worktree

Author: arxanas

git-branchless-lib/src/git/repo.rs

@@ -516,8 +516,42 @@ impl Repo {
 
     /// Get the path to the working copy for this repository. If the repository
     /// is bare (has no working copy), returns `None`.
-    pub fn get_working_copy_path(&self) -> Option<&Path> {
-        self.inner.workdir()
+    pub fn get_working_copy_path(&self) -> Option<PathBuf> {
+        let workdir = self.inner.workdir()?;
+        if !self.inner.is_worktree() {
+            return Some(workdir.to_owned());
+        }
+
+        // Under some circumstances (not sure exactly when),
+        // `git2::Repository::workdir` on a worktree returns a path like
+        // `/path/to/repo/.git/worktrees/worktree-name/` instead of
+        // `/path/to/worktree/`.
+        let gitdir_file = workdir.join("gitdir");
+        let gitdir = match std::fs::read_to_string(&gitdir_file) {
+            Ok(gitdir) => gitdir,
+            Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
+                return Some(workdir.to_path_buf());
+            }
+            Err(err) => {
+                warn!(
+                    ?workdir,
+                    ?gitdir_file,
+                    ?err,
+                    "gitdir file for worktree could not be read; cannot get workdir path"
+                );
+                return None;
+            }
+        };
+        let gitdir = match gitdir.strip_suffix('\n') {
+            Some(gitdir) => gitdir,
+            None => gitdir.as_str(),
+        };
+        let gitdir = PathBuf::from(gitdir);
+        let workdir = gitdir.parent()?; // remove `.git` suffix
+        std::fs::canonicalize(workdir).ok().or_else(|| {
+            warn!(?workdir, "Failed to canonicalize workdir");
+            None
+        })
     }
 
     /// Get the index file for this repository.
@@ -1469,17 +1503,18 @@ impl Repo {
         let repo_path = self
             .get_working_copy_path()
             .ok_or(Error::NoWorkingCopyPath)?;
+        let repo_path = &repo_path;
         let new_tree_entries: HashMap<PathBuf, Option<(NonZeroOid, FileMode)>> = match opts {
             AmendFastOptions::FromWorkingCopy { status_entries } => status_entries
                 .iter()
                 .flat_map(|entry| {
                     entry.paths().into_iter().map(
                         move |path| -> Result<(PathBuf, Option<(NonZeroOid, FileMode)>)> {
-                            let file_path = &repo_path.join(&path);
+                            let file_path = repo_path.join(&path);
                             // Try to create a new blob OID based on the current on-disk
                             // contents of the file in the working copy.
                             let entry = self
-                                .create_blob_from_path(file_path)?
+                                .create_blob_from_path(&file_path)?
                                 .map(|oid| (oid, entry.working_copy_file_mode));
                             Ok((path, entry))
                         },

git-branchless-lib/src/git/run.rs

@@ -3,7 +3,7 @@ use std::convert::TryInto;
 use std::ffi::{OsStr, OsString};
 use std::fmt::Write;
 use std::io::{BufRead, BufReader, Read, Write as WriteIo};
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::process::{Command, ExitStatus, Stdio};
 use std::sync::Arc;
 use std::thread::{self, JoinHandle};
@@ -256,7 +256,7 @@ impl GitRunInfo {
     ///
     /// This contains additional logic for symlinked `.git` directories to work
     /// around an upstream `libgit2` issue.
-    fn working_directory<'a>(&'a self, repo: &'a Repo) -> &'a Path {
+    fn working_directory<'a>(&'a self, repo: &'a Repo) -> PathBuf {
         repo.get_working_copy_path()
             // `libgit2` returns the working copy path as the parent of the
             // `.git` directory. However, if the `.git` directory is a symlink,
@@ -274,13 +274,13 @@ impl GitRunInfo {
             // root of the working tree.
             .map(|working_copy| {
                 // Both paths are already "canonicalized".
-                if !self.working_directory.starts_with(working_copy) {
-                    &self.working_directory
+                if !self.working_directory.starts_with(&working_copy) {
+                    self.working_directory.clone()
                 } else {
                     working_copy
                 }
             })
-            .unwrap_or_else(|| repo.get_path())
+            .unwrap_or_else(|| repo.get_path().to_owned())
     }
 
     fn run_silent_inner(

git-branchless-lib/tests/test_repo.rs

@@ -1,8 +1,10 @@
+use std::path::PathBuf;
+
 use branchless::git::{
-    AmendFastOptions, BranchType, CherryPickFastOptions, FileMode, FileStatus, GitVersion,
+    AmendFastOptions, BranchType, CherryPickFastOptions, FileMode, FileStatus, GitVersion, Repo,
     StatusEntry,
 };
-use branchless::testing::make_git;
+use branchless::testing::{make_git, make_git_worktree, GitWorktreeWrapper};
 
 #[test]
 fn test_parse_git_version_output() {
@@ -262,3 +264,42 @@ fn test_branch_debug() -> eyre::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_worktree_working_copy_path() -> eyre::Result<()> {
+    let git = make_git()?;
+    git.init_repo()?;
+    git.commit_file("test1", 1)?;
+
+    let GitWorktreeWrapper { temp_dir, worktree } = make_git_worktree(&git, "new-worktree")?;
+    {
+        let stdout = worktree.smartlog()?;
+        insta::assert_snapshot!(stdout, @r###"
+        :
+        @ 62fc20d (master) create test1.txt
+        "###);
+    }
+
+    fn canonicalize(path: Option<PathBuf>) -> PathBuf {
+        match path {
+            None => PathBuf::from("<none>"),
+            Some(path) => {
+                // On macOS, it looks like the temporary directory has to be
+                // canonicalized as it may be `/var` vs. `/private/var`.
+                std::fs::canonicalize(path).unwrap_or_else(|err| format!("<error: {err}>").into())
+            }
+        }
+    }
+    let worktree_repo = worktree.get_repo()?;
+    assert_eq!(
+        canonicalize(worktree_repo.get_working_copy_path()),
+        canonicalize(Some(temp_dir.path().join("new-worktree")))
+    );
+    let directly_opened_worktree_repo = Repo::from_dir(&temp_dir.path().join("new-worktree"))?;
+    assert_eq!(
+        canonicalize(directly_opened_worktree_repo.get_working_copy_path()),
+        canonicalize(worktree_repo.get_working_copy_path())
+    );
+
+    Ok(())
+}

git-branchless-submit/src/lib.rs

@@ -38,7 +38,7 @@ use git_branchless_opts::{
 };
 use git_branchless_revset::resolve_commits;
 use phabricator::PhabricatorForge;
-use tracing::{debug, warn};
+use tracing::{debug, info, instrument, warn};
 
 lazy_static! {
     /// The style for branches which were successfully submitted.
@@ -473,6 +473,7 @@ create and push them, retry this operation with the --create option."
     Ok(Ok(()))
 }
 
+#[instrument]
 fn select_forge<'a>(
     effects: &'a Effects,
     git_run_info: &'a GitRunInfo,
@@ -484,10 +485,20 @@ fn select_forge<'a>(
     forge: Option<ForgeKind>,
 ) -> Box<dyn Forge + 'a> {
     let forge_kind = match forge {
-        Some(forge_kind) => forge_kind,
+        Some(forge_kind) => {
+            info!(?forge_kind, "Forge kind was explicitly set");
+            forge_kind
+        }
         None => {
             let use_phabricator = if let Some(working_copy_path) = repo.get_working_copy_path() {
-                working_copy_path.join(".arcconfig").is_file()
+                let arcconfig_path = &working_copy_path.join(".arcconfig");
+                let arcconfig_present = arcconfig_path.is_file();
+                debug!(
+                    ?arcconfig_path,
+                    ?arcconfig_present,
+                    "Checking arcconfig path to decide whether to use Phabricator"
+                );
+                arcconfig_present
             } else {
                 false
             };
@@ -499,6 +510,7 @@ fn select_forge<'a>(
         }
     };
 
+    info!(?forge_kind, "Selected forge kind");
     match forge_kind {
         ForgeKind::Branch => Box::new(BranchForge {
             effects,