From 16cab733b0976086e3a5063beab6f5e863147cfe Mon Sep 17 00:00:00 2001 From: Ethan Wu Date: Mon, 27 Jan 2025 17:22:51 -0800 Subject: [PATCH 1/3] feat(tests): add working_dir option to GitRunOptions Allow running git commands in a different working directory in tests. --- git-branchless-lib/src/testing.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/git-branchless-lib/src/testing.rs b/git-branchless-lib/src/testing.rs index cae3432d7..cca6ab7e5 100644 --- a/git-branchless-lib/src/testing.rs +++ b/git-branchless-lib/src/testing.rs @@ -81,6 +81,9 @@ pub struct GitRunOptions { /// Additional environment variables to start the process with. pub env: HashMap, + + /// The working directory to start the process in. + pub working_dir: Option, } impl Git { @@ -217,6 +220,7 @@ impl Git { expected_exit_code, input, env, + working_dir, } = options; let env: BTreeMap<_, _> = self @@ -229,7 +233,7 @@ impl Git { .collect(); let mut command = Command::new(&self.path_to_git); command - .current_dir(&self.repo_path) + .current_dir(working_dir.as_deref().unwrap_or(&self.repo_path)) .args(args) .env_clear() .envs(&env); From 4cc38121caed0183079f2115d2725d2a5fa919a8 Mon Sep 17 00:00:00 2001 From: Ethan Wu Date: Thu, 14 Nov 2024 18:50:28 -0800 Subject: [PATCH 2/3] fix(init): resolve hooks path relative to repo root Ensure that `core.hooksPath` is resolved relative to the repository root of the current worktree, or the git dir if it is bare. This should be the same logic as git uses for resolution. --- git-branchless-lib/src/core/config.rs | 17 ++++- git-branchless/tests/test_init.rs | 97 ++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 3 deletions(-) diff --git a/git-branchless-lib/src/core/config.rs b/git-branchless-lib/src/core/config.rs index 248525a36..1d2ba9391 100644 --- a/git-branchless-lib/src/core/config.rs +++ b/git-branchless-lib/src/core/config.rs @@ -54,7 +54,22 @@ pub fn get_main_worktree_hooks_dir( let hooks_path = if result.exit_code.is_success() { let path = String::from_utf8(result.stdout) .context("Decoding git config output for hooks path")?; - PathBuf::from(path.strip_suffix('\n').unwrap_or(&path)) + + let path = PathBuf::from(path.strip_suffix('\n').unwrap_or(&path)); + + // Relative hook paths are resolved relative to where hooks are run[1], + // which is the root of the current working tree in a non-bare + // repository, or $GIT_DIR (the .git directory) for a bare + // repository[2]. + // [1]: https://git-scm.com/docs/git-config#Documentation/git-config.txt-corehooksPath + // [2]: https://git-scm.com/docs/githooks#_description + repo.get_working_copy_path() + .as_deref() + .unwrap_or_else( + // Bare repo + || repo.get_path(), + ) + .join(path) } else { get_default_hooks_dir(repo)? }; diff --git a/git-branchless/tests/test_init.rs b/git-branchless/tests/test_init.rs index 82be1b2d1..cc3ec5673 100644 --- a/git-branchless/tests/test_init.rs +++ b/git-branchless/tests/test_init.rs @@ -279,6 +279,99 @@ fn test_init_prompt_for_main_branch() -> eyre::Result<()> { Ok(()) } +#[cfg(unix)] +#[test] +fn test_init_in_subdir() -> eyre::Result<()> { + let git = make_git()?; + + if !git.supports_reference_transactions()? { + return Ok(()); + } + + git.init_repo_with_options(&GitInitOptions { + run_branchless_init: false, + ..Default::default() + })?; + + let subdir = git.repo_path.join("subdir"); + std::fs::create_dir(&subdir)?; + + { + let (stdout, stderr) = git.branchless_with_options( + "init", + &[], + &GitRunOptions { + working_dir: Some(subdir), + ..Default::default() + }, + )?; + insta::assert_snapshot!(stderr, @""); + insta::assert_snapshot!(stdout, @r###" + Created config file at /.git/branchless/config + Auto-detected your main branch as: master + If this is incorrect, run: git branchless init --main-branch + Installing hooks: post-applypatch, post-checkout, post-commit, post-merge, post-rewrite, pre-auto-gc, reference-transaction + Successfully installed git-branchless. + To uninstall, run: git branchless init --uninstall + "###); + } + + let hook_path = git.repo_path.join(".git").join("hooks").join("post-commit"); + assert!(hook_path.exists()); + + Ok(()) +} + +#[cfg(unix)] +#[test] +fn test_init_in_subdir_with_hooks_path() -> eyre::Result<()> { + let git = make_git()?; + + if !git.supports_reference_transactions()? { + return Ok(()); + } + + git.init_repo_with_options(&GitInitOptions { + run_branchless_init: false, + ..Default::default() + })?; + + let hooks_path = git.repo_path.join("my-hooks"); + std::fs::create_dir(&hooks_path)?; + git.run(&["config", "core.hooksPath", "my-hooks"])?; + + let subdir = git.repo_path.join("subdir"); + std::fs::create_dir(&subdir)?; + + { + let (stdout, stderr) = git.branchless_with_options( + "init", + &[], + &GitRunOptions { + working_dir: Some(subdir), + ..Default::default() + }, + )?; + insta::assert_snapshot!(stderr, @""); + insta::assert_snapshot!(stdout, @r###" + Created config file at /.git/branchless/config + Auto-detected your main branch as: master + If this is incorrect, run: git branchless init --main-branch + Installing hooks: post-applypatch, post-checkout, post-commit, post-merge, post-rewrite, pre-auto-gc, reference-transaction + Warning: the configuration value core.hooksPath was set to: /my-hooks, + which is not the expected default value of: /.git/hooks + The Git hooks above may have been installed to an unexpected global location. + Successfully installed git-branchless. + To uninstall, run: git branchless init --uninstall + "###); + } + + let hook_path = hooks_path.join("post-commit"); + assert!(hook_path.exists()); + + Ok(()) +} + #[cfg(unix)] #[test] fn test_main_branch_not_found_error_message() -> eyre::Result<()> { @@ -547,7 +640,7 @@ fn test_init_core_hooks_path_warning() -> eyre::Result<()> { Auto-detected your main branch as: master If this is incorrect, run: git branchless init --main-branch Installing hooks: post-applypatch, post-checkout, post-commit, post-merge, post-rewrite, pre-auto-gc, reference-transaction - Warning: the configuration value core.hooksPath was set to: my-hooks, + Warning: the configuration value core.hooksPath was set to: /my-hooks, which is not the expected default value of: /.git/hooks The Git hooks above may have been installed to an unexpected global location. Successfully installed git-branchless. @@ -600,7 +693,7 @@ hooksPath = my-hooks Auto-detected your main branch as: master If this is incorrect, run: git branchless init --main-branch Installing hooks: post-applypatch, post-checkout, post-commit, post-merge, post-rewrite, pre-auto-gc, reference-transaction - Warning: the configuration value core.hooksPath was set to: my-hooks, + Warning: the configuration value core.hooksPath was set to: /my-hooks, which is not the expected default value of: /.git/hooks The Git hooks above may have been installed to an unexpected global location. Successfully installed git-branchless. From c2f48403075976cc54dc45aca02ba306903d86bc Mon Sep 17 00:00:00 2001 From: Ethan Wu Date: Mon, 27 Jan 2025 19:50:09 -0800 Subject: [PATCH 3/3] fix(init): make relative hook path work in worktrees Since init previously used the root worktree for all of its actions, it failed to resolve `core.hooksPath` relative to the worktree it was being run in. Change `init` to work with the current worktree, and update code to explicitly refer to the shared git directory when necessary. This also entirely removes the need for `open_worktree_parent_repo`. --- git-branchless-init/src/lib.rs | 22 +++--- git-branchless-lib/src/core/config.rs | 4 +- git-branchless-lib/src/git/repo.rs | 49 ++++-------- git-branchless/tests/test_init.rs | 107 ++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 49 deletions(-) diff --git a/git-branchless-init/src/lib.rs b/git-branchless-init/src/lib.rs index 5097e6895..c7bd6c405 100644 --- a/git-branchless-init/src/lib.rs +++ b/git-branchless-init/src/lib.rs @@ -430,13 +430,16 @@ fn install_man_pages(effects: &Effects, repo: &Repo, config: &mut Config) -> eyr 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!( - "Getting relative path for {:?} with respect to {:?}", - &man_dir, - repo.get_path() - ) - })?; + let man_dir_relative = + man_dir + .strip_prefix(repo.get_shared_path()) + .wrap_err_with(|| { + format!( + "Getting relative path for {:?} with respect to {:?}", + &man_dir, + repo.get_path() + ) + })?; &man_dir_relative.to_str().ok_or_else(|| { eyre::eyre!( "Could not convert man dir to UTF-8 string: {:?}", @@ -544,7 +547,7 @@ fn create_isolated_config( let config = Config::open(&config_path)?; let config_path_relative = config_path - .strip_prefix(repo.get_path()) + .strip_prefix(repo.get_shared_path()) .wrap_err("Getting relative config path")?; // Be careful when setting paths on Windows. Since the path would have a // backslash, naively using it produces @@ -606,8 +609,7 @@ fn command_init( main_branch_name: Option<&str>, ) -> EyreExitOr<()> { let mut in_ = BufReader::new(stdin()); - let repo = Repo::from_current_dir()?; - let mut repo = repo.open_worktree_parent_repo()?.unwrap_or(repo); + let mut repo = Repo::from_current_dir()?; let default_config = Config::open_default()?; let readonly_config = repo.get_readonly_config()?; diff --git a/git-branchless-lib/src/core/config.rs b/git-branchless-lib/src/core/config.rs index 1d2ba9391..e8e077c0b 100644 --- a/git-branchless-lib/src/core/config.rs +++ b/git-branchless-lib/src/core/config.rs @@ -19,9 +19,7 @@ use super::eventlog::EventTransactionId; /// overridden it. #[instrument] pub fn get_default_hooks_dir(repo: &Repo) -> eyre::Result { - let parent_repo = repo.open_worktree_parent_repo()?; - let repo = parent_repo.as_ref().unwrap_or(repo); - Ok(repo.get_path().join("hooks")) + Ok(repo.get_shared_path().join("hooks")) } /// Get the path where the main worktree's Git hooks are stored on disk. diff --git a/git-branchless-lib/src/git/repo.rs b/git-branchless-lib/src/git/repo.rs index 9725e1037..507830dd8 100644 --- a/git-branchless-lib/src/git/repo.rs +++ b/git-branchless-lib/src/git/repo.rs @@ -49,9 +49,6 @@ pub enum Error { #[error("could not open repository: {0}")] OpenRepo(#[source] git2::Error), - #[error("could not find repository to open for worktree {path:?}")] - OpenParentWorktreeRepository { path: PathBuf }, - #[error("could not open repository: {0}")] UnsupportedExtensionWorktreeConfig(#[source] git2::Error), @@ -505,11 +502,22 @@ impl Repo { Ok(Repo { inner: repo }) } - /// Get the path to the `.git` directory for the repository. + /// Get the path to the local `.git` directory for the repository. + /// + /// If this repository is a non-root worktree, this will return the path to + /// the worktree's local directory, i.e. `.git/worktrees/` pub fn get_path(&self) -> &Path { self.inner.path() } + /// Get the path to the shared common `.git` directory for the repository. + /// + /// This always gets the shared common `.git` directory instead of the + /// per-worktree directory (`.git/worktrees/...`). + pub fn get_shared_path(&self) -> &Path { + self.inner.commondir() + } + /// Get the path to the `packed-refs` file for the repository. pub fn get_packed_refs_path(&self) -> PathBuf { self.inner.path().join("packed-refs") @@ -569,32 +577,6 @@ impl Repo { Ok(Index { inner: index }) } - /// If this repository is a worktree for another "parent" repository, return a [`Repo`] object - /// corresponding to that repository. - #[instrument] - pub fn open_worktree_parent_repo(&self) -> Result> { - if !self.inner.is_worktree() { - return Ok(None); - } - - // `git2` doesn't seem to support a way to directly look up the parent repository for the - // worktree. - let worktree_info_dir = self.get_path(); - let parent_repo_path = match worktree_info_dir - .parent() // remove `.git` - .and_then(|path| path.parent()) // remove worktree name - .and_then(|path| path.parent()) // remove `worktrees` - { - Some(path) => path, - None => { - return Err(Error::OpenParentWorktreeRepository { - path: worktree_info_dir.to_owned()}); - }, - }; - let parent_repo = Self::from_dir(parent_repo_path)?; - Ok(Some(parent_repo)) - } - /// Get the configuration object for the repository. /// /// **Warning**: This object should only be used for read operations. Write @@ -608,12 +590,7 @@ impl Repo { /// Get the directory where all repo-specific git-branchless state is stored. pub fn get_branchless_dir(&self) -> Result { - let maybe_worktree_parent_repo = self.open_worktree_parent_repo()?; - let repo = match maybe_worktree_parent_repo.as_ref() { - Some(repo) => repo, - None => self, - }; - let dir = repo.get_path().join("branchless"); + let dir = self.get_shared_path().join("branchless"); std::fs::create_dir_all(&dir).map_err(|err| Error::CreateBranchlessDir { source: err, path: dir.clone(), diff --git a/git-branchless/tests/test_init.rs b/git-branchless/tests/test_init.rs index cc3ec5673..5b6055653 100644 --- a/git-branchless/tests/test_init.rs +++ b/git-branchless/tests/test_init.rs @@ -730,6 +730,113 @@ fn test_init_worktree() -> eyre::Result<()> { Ok(()) } +#[test] +fn test_init_worktree_from_subdir() -> eyre::Result<()> { + let git = make_git()?; + git.init_repo_with_options(&GitInitOptions { + run_branchless_init: false, + make_initial_commit: true, + })?; + git.commit_file("test1", 1)?; + git.commit_file("test2", 2)?; + + let GitWorktreeWrapper { + temp_dir: _temp_dir, + worktree, + } = make_git_worktree(&git, "new-worktree")?; + + let subdir = worktree.repo_path.join("subdir"); + std::fs::create_dir(&subdir)?; + + worktree.branchless_with_options( + "init", + &[], + &GitRunOptions { + working_dir: Some(subdir), + ..Default::default() + }, + )?; + { + let (stdout, stderr) = worktree.run(&["commit", "--allow-empty", "-m", "test"])?; + insta::assert_snapshot!(stdout, @"[detached HEAD a3e6886] test +"); + insta::assert_snapshot!(stderr, @r###" + branchless: processing 1 update: ref HEAD + branchless: processed commit: a3e6886 test + "###); + } + + Ok(()) +} + +#[test] +fn test_init_worktree_from_subdir_with_hooks_path() -> eyre::Result<()> { + let git = make_git()?; + git.init_repo_with_options(&GitInitOptions { + run_branchless_init: false, + make_initial_commit: true, + })?; + git.commit_file("test1", 1)?; + git.commit_file("test2", 2)?; + + let GitWorktreeWrapper { + temp_dir: _temp_dir, + worktree, + } = make_git_worktree(&git, "new-worktree")?; + + let hooks_path = git.get_repo()?.get_path().join("my-hooks"); + std::fs::create_dir(hooks_path)?; + git.run(&["config", "core.hooksPath", "my-hooks"])?; + + let subdir = worktree.repo_path.join("subdir"); + std::fs::create_dir(&subdir)?; + + { + let (stdout, stderr) = worktree.branchless_with_options( + "init", + &[], + &GitRunOptions { + working_dir: Some(subdir), + ..Default::default() + }, + )?; + + // Copied from [Git::preprocess_output] - replace the path of the root + // repo. + let stdout = stdout.replace( + std::fs::canonicalize(&git.repo_path)? + .to_str() + .ok_or_else(|| eyre::eyre!("Could not convert repo path to string"))?, + "", + ); + + insta::assert_snapshot!(stderr, @""); + insta::assert_snapshot!(stdout, @r###" + Created config file at /.git/branchless/config + Auto-detected your main branch as: master + If this is incorrect, run: git branchless init --main-branch + Installing hooks: post-applypatch, post-checkout, post-commit, post-merge, post-rewrite, pre-auto-gc, reference-transaction + Warning: the configuration value core.hooksPath was set to: /my-hooks, + which is not the expected default value of: /.git/hooks + The Git hooks above may have been installed to an unexpected global location. + Successfully installed git-branchless. + To uninstall, run: git branchless init --uninstall + "###); + } + + { + let (stdout, stderr) = worktree.run(&["commit", "--allow-empty", "-m", "test"])?; + insta::assert_snapshot!(stdout, @"[detached HEAD a3e6886] test +"); + insta::assert_snapshot!(stderr, @r###" + branchless: processing 1 update: ref HEAD + branchless: processed commit: a3e6886 test + "###); + } + + Ok(()) +} + #[test] fn test_install_man_pages() -> eyre::Result<()> { let git = make_git()?;