fix(hooks): use same branchless dir for all worktrees

Cargo.toml

@@ -25,6 +25,7 @@ members = [
     "scm-bisect",
     "scm-record",
 ]
+resolver = "2"
 
 [workspace.metadata.release]
 consolidate-commits = true

git-branchless-hook/tests/test_hook.rs

@@ -1,4 +1,4 @@
-use lib::testing::{make_git, GitRunOptions};
+use lib::testing::{make_git, make_git_worktree, GitRunOptions, GitWorktreeWrapper};
 
 #[test]
 fn test_is_rebase_underway() -> eyre::Result<()> {
@@ -148,3 +148,86 @@ fn test_rebase_no_process_new_commits_until_conclusion() -> eyre::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_hooks_in_worktree() -> eyre::Result<()> {
+    let git = make_git()?;
+
+    if !git.supports_reference_transactions()? {
+        return Ok(());
+    }
+    git.init_repo()?;
+
+    git.commit_file("test1", 1)?;
+    git.detach_head()?;
+
+    let GitWorktreeWrapper {
+        temp_dir: _temp_dir,
+        worktree,
+    } = make_git_worktree(&git, "new-worktree")?;
+
+    {
+        let (stdout, stderr) =
+            worktree.run(&["commit", "--allow-empty", "-m", "new empty commit"])?;
+        insta::assert_snapshot!(stderr, @r###"
+        branchless: processing 1 update: ref HEAD
+        branchless: processed commit: 1bed0d8 new empty commit
+        "###);
+        insta::assert_snapshot!(stdout, @r###"
+        [detached HEAD 1bed0d8] new empty commit
+        "###);
+    }
+
+    {
+        let stdout = git.smartlog()?;
+        insta::assert_snapshot!(stdout, @r###"
+        :
+        @ 62fc20d (master) create test1.txt
+        |
+        o 1bed0d8 new empty commit
+        "###);
+    }
+    {
+        let stdout = worktree.smartlog()?;
+        insta::assert_snapshot!(stdout, @r###"
+        :
+        O 62fc20d (master) create test1.txt
+        |
+        @ 1bed0d8 new empty commit
+        "###);
+    }
+
+    {
+        let (stdout, stderr) =
+            worktree.run(&["commit", "--amend", "--allow-empty", "--message", "amended"])?;
+        insta::assert_snapshot!(stderr, @r###"
+        branchless: processing 1 update: ref HEAD
+        branchless: processed commit: cc4313e amended
+        branchless: processing 1 rewritten commit
+        "###);
+        insta::assert_snapshot!(stdout, @r###"
+        [detached HEAD cc4313e] amended
+         Date: Thu Oct 29 12:34:56 2020 +0000
+        "###);
+    }
+    {
+        let stdout = git.smartlog()?;
+        insta::assert_snapshot!(stdout, @r###"
+        :
+        @ 62fc20d (master) create test1.txt
+        |
+        o cc4313e amended
+        "###);
+    }
+    {
+        let stdout = worktree.smartlog()?;
+        insta::assert_snapshot!(stdout, @r###"
+        :
+        O 62fc20d (master) create test1.txt
+        |
+        @ cc4313e amended
+        "###);
+    }
+
+    Ok(())
+}

git-branchless-init/src/lib.rs

@@ -23,7 +23,9 @@ use path_slash::PathExt;
 use tracing::{instrument, warn};
 
 use git_branchless_opts::{write_man_pages, InitArgs, InstallManPagesArgs};
-use lib::core::config::{get_default_branch_name, get_default_hooks_dir, get_hooks_dir};
+use lib::core::config::{
+    get_default_branch_name, get_default_hooks_dir, get_main_worktree_hooks_dir,
+};
 use lib::core::dag::Dag;
 use lib::core::effects::Effects;
 use lib::core::eventlog::{EventLogDb, EventReplayer};
@@ -249,12 +251,12 @@ fn install_hooks(effects: &Effects, git_run_info: &GitRunInfo, repo: &Repo) -> e
             .map(|(hook_type, _hook_script)| hook_type)
             .join(", ")
     )?;
-    let hooks_dir = get_hooks_dir(git_run_info, repo, None)?;
+    let hooks_dir = get_main_worktree_hooks_dir(git_run_info, repo, None)?;
     for (hook_type, hook_script) in ALL_HOOKS {
         install_hook(repo, &hooks_dir, hook_type, hook_script)?;
     }
 
-    let default_hooks_dir = get_default_hooks_dir(repo);
+    let default_hooks_dir = get_default_hooks_dir(repo)?;
     if hooks_dir != default_hooks_dir {
         writeln!(
             effects.get_output_stream(),
@@ -281,7 +283,7 @@ fn uninstall_hooks(effects: &Effects, git_run_info: &GitRunInfo, repo: &Repo) ->
             .map(|(hook_type, _hook_script)| hook_type)
             .join(", ")
     )?;
-    let hooks_dir = get_hooks_dir(git_run_info, repo, None)?;
+    let hooks_dir = get_main_worktree_hooks_dir(git_run_info, repo, None)?;
     for (hook_type, _hook_script) in ALL_HOOKS {
         install_hook(
             repo,
@@ -426,7 +428,7 @@ fn install_man_pages(effects: &Effects, repo: &Repo, config: &mut Config) -> eyr
         return Ok(());
     }
 
-    let man_dir = repo.get_man_dir();
+    let man_dir = repo.get_man_dir()?;
     let man_dir_relative = {
         let man_dir_relative = man_dir.strip_prefix(repo.get_path()).wrap_err_with(|| {
             format!(
@@ -534,7 +536,7 @@ fn create_isolated_config(
     repo: &Repo,
     mut parent_config: Config,
 ) -> eyre::Result<Config> {
-    let config_path = repo.get_config_path();
+    let config_path = repo.get_config_path()?;
     let config_dir = config_path
         .parent()
         .ok_or_else(|| eyre::eyre!("Could not get parent config directory"))?;
@@ -575,13 +577,14 @@ fn delete_isolated_config(
     repo: &Repo,
     mut parent_config: Config,
 ) -> eyre::Result<()> {
+    let config_path = repo.get_config_path()?;
     writeln!(
         effects.get_output_stream(),
         "Removing config file: {}",
-        repo.get_config_path().to_string_lossy()
+        config_path.to_string_lossy()
     )?;
     parent_config.remove_multivar("include.path", INCLUDE_PATH_REGEX)?;
-    let result = match std::fs::remove_file(repo.get_config_path()) {
+    let result = match std::fs::remove_file(config_path) {
         Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
             writeln!(
                 effects.get_output_stream(),

git-branchless-lib/src/core/config.rs

@@ -18,13 +18,24 @@ use super::eventlog::EventTransactionId;
 /// Get the expected hooks dir inside `.git`, assuming that the user has not
 /// overridden it.
 #[instrument]
-pub fn get_default_hooks_dir(repo: &Repo) -> PathBuf {
-    repo.get_path().join("hooks")
+pub fn get_default_hooks_dir(repo: &Repo) -> eyre::Result<PathBuf> {
+    let parent_repo = repo.open_worktree_parent_repo()?;
+    let repo = parent_repo.as_ref().unwrap_or(repo);
+    Ok(repo.get_path().join("hooks"))
 }
 
-/// Get the path where Git hooks are stored on disk.
+/// Get the path where the main worktree's Git hooks are stored on disk.
+///
+/// Git hooks live at `$GIT_DIR/hooks` by default, which means that they will be
+/// different per wortkree. Most people, when creating a new worktree, will not
+/// also reinstall hooks or reinitialize git-branchless in that worktree, so we
+/// instead look up hooks for the main worktree, which is most likely to have them
+/// installed.
+///
+/// This could in theory cause problems for users who have different
+/// per-worktree hooks.
 #[instrument]
-pub fn get_hooks_dir(
+pub fn get_main_worktree_hooks_dir(
     git_run_info: &GitRunInfo,
     repo: &Repo,
     event_tx_id: Option<EventTransactionId>,
@@ -45,7 +56,7 @@ pub fn get_hooks_dir(
             .context("Decoding git config output for hooks path")?;
         PathBuf::from(path.strip_suffix('\n').unwrap_or(&path))
     } else {
-        get_default_hooks_dir(repo)
+        get_default_hooks_dir(repo)?
     };
     Ok(hooks_path)
 }

git-branchless-lib/src/core/dag.rs

@@ -256,7 +256,7 @@ impl Dag {
 
     #[instrument]
     fn open_inner_dag(repo: &Repo) -> eyre::Result<eden_dag::Dag> {
-        let dag_dir = repo.get_dag_dir();
+        let dag_dir = repo.get_dag_dir()?;
         std::fs::create_dir_all(&dag_dir).wrap_err("Creating .git/branchless/dag dir")?;
         let dag = eden_dag::Dag::open(&dag_dir)
             .wrap_err_with(|| format!("Opening DAG directory at: {:?}", &dag_dir))?;

git-branchless-lib/src/core/effects.rs

@@ -829,13 +829,6 @@ trait WriteProgress {
     fn drop(&mut self) {
         let buffer = self.get_buffer();
         if !buffer.is_empty() {
-            // In practice, we're expecting that every sequence of `write` calls
-            // will shortly end with a `write` call that ends with a newline, in
-            // such a way that only full lines are written to the output stream.
-            // This assumption might be wrong, in which case we should revisit
-            // this warning and the behavior on `Drop`.
-            warn!(?buffer, "WriteProgress dropped while buffer was not empty");
-
             // NB: this only flushes completely-written lines, which is not
             // correct. We should flush the entire buffer contents, and possibly
             // force a newline at the end in the case of a progress meter being