Skip to content

fix(init): resolve hooks path relative to repo root #1446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions git-branchless-init/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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: {:?}",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()?;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changes readonly_config to be read from the current worktree instead of the root. (It's used for modifying the root config to include branchless/config) This doesn't seem to break any tests, and also uninstall uses the current worktree instead of the root. I'm not sure if this is a problem or not, since I'm not clear on how much of the config file init/uninstall process is covered in the worktree tests.

Expand Down
21 changes: 17 additions & 4 deletions git-branchless-lib/src/core/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ use super::eventlog::EventTransactionId;
/// overridden it.
#[instrument]
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"))
Ok(repo.get_shared_path().join("hooks"))
}

/// Get the path where the main worktree's Git hooks are stored on disk.
Expand Down Expand Up @@ -54,7 +52,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)?
};
Expand Down
49 changes: 13 additions & 36 deletions git-branchless-lib/src/git/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),

Expand Down Expand Up @@ -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/<name>`
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")
Expand Down Expand Up @@ -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<Option<Self>> {
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
Expand All @@ -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<PathBuf> {
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(),
Expand Down
6 changes: 5 additions & 1 deletion git-branchless-lib/src/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ pub struct GitRunOptions {

/// Additional environment variables to start the process with.
pub env: HashMap<String, String>,

/// The working directory to start the process in.
pub working_dir: Option<PathBuf>,
}

impl Git {
Expand Down Expand Up @@ -217,6 +220,7 @@ impl Git {
expected_exit_code,
input,
env,
working_dir,
} = options;

let env: BTreeMap<_, _> = self
Expand All @@ -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);
Expand Down
Loading
Loading