Fix Repo() autodiscovery in linked worktrees when GIT_DIR is set#2128
Fix Repo() autodiscovery in linked worktrees when GIT_DIR is set#2128meliezer wants to merge 1 commit intogitpython-developers:mainfrom
Conversation
a98c43b to
b0354d5
Compare
There was a problem hiding this comment.
Pull request overview
Fixes git.Repo() autodiscovery when invoked from within a linked worktree and GIT_DIR is set to that worktree’s internal git directory (e.g. .git/worktrees/<id>), preventing erroneous InvalidGitRepositoryError.
Changes:
- Added a regression test covering
Repo()autodiscovery in a linked worktree withGIT_DIRset. - Updated repository discovery to recognize linked-worktree git directories by their metadata files.
- Derived the correct working tree directory from linked worktree metadata during discovery.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/test_repo.py |
Adds regression coverage ensuring Repo() works when GIT_DIR is set inside a linked worktree. |
git/repo/base.py |
Extends autodiscovery logic to detect linked-worktree git dirs and compute the correct working tree directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Byron
left a comment
There was a problem hiding this comment.
Thanks.
In all honesty, I'm unable to tell if this makes any sense and if this is what Git does. But all we have is tests and as it doesn't break anything probably it's good enough.
Something I'd appreciate is the error message of the test when the patch here is not applied.
Besides that, I think a couple of changes are necessary.
| # It's important to normalize the paths, as submodules will otherwise | ||
| # initialize their repo instances with paths that depend on path-portions | ||
| # that will not exist after being removed. It's just cleaner. | ||
| if ( |
There was a problem hiding this comment.
It seems the comment above this hunk was actually meant for the block below it. So that should be changed. I also think the code block can use some documentation on what it's trying to do.
There was a problem hiding this comment.
@meliezer this wasn't addressed though. The comment above wants to be above if is_git_dir(curpath):.
| and osp.isfile(osp.join(curpath, "HEAD")) | ||
| ): | ||
| git_dir = curpath | ||
| worktree_gitfile = Path(osp.join(git_dir, "gitdir")).read_text().splitlines()[0].strip() |
There was a problem hiding this comment.
Read text is probably going to fail given that the path can contain any set of characters without necessarily being in the default encoding.
Is there some utility in the code pase already that can read files that contain a gitdir?
| if "GIT_WORK_TREE" in os.environ: | ||
| self._working_tree_dir = os.getenv("GIT_WORK_TREE") |
There was a problem hiding this comment.
The _working_tree_dir probably starts out as None, and should be set by whatever has the final say, probably the environment variable.
Then if it's not yet set, all this extra work can be done to figure out the worktree dir.
b0354d5 to
3a94f7c
Compare
|
Addressed, thanks — I gave precedence to GIT_WORK_TREE, adjusted the comment, and replaced Path.read_text(). The linked worktree gitdir file uses a different format, so I handled it locally. |
Handle linked worktree git directories when GIT_DIR points to .git/worktrees/<name>. Previously Repo() could fail with InvalidGitRepositoryError in this scenario, while Repo(os.getcwd()) worked correctly. Add regression test to cover autodiscovery in linked worktrees.
3a94f7c to
b5eb249
Compare
|
Thanks, just one piece missing. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with mock.patch.dict(os.environ, {"GIT_DIR": git_dir}, clear=False): | ||
| old_cwd = os.getcwd() | ||
| try: | ||
| os.chdir(worktree_path) | ||
|
|
||
| explicit = Repo(os.getcwd()) | ||
| autodiscovered = Repo() |
There was a problem hiding this comment.
This test patches only GIT_DIR but leaves any pre-existing GIT_WORK_TREE in the environment intact. If a user/CI environment has GIT_WORK_TREE set, Repo() will yield to it and the assertions will fail, making the test non-hermetic. Consider explicitly unsetting/removing GIT_WORK_TREE (and possibly GIT_COMMON_DIR) within this context so the test isolates the intended behavior.
Fixes #2022
This PR fixes repository autodiscovery in linked worktrees when GIT_DIR is set.
Summary
Fix
Repo()autodiscovery whenGIT_DIRpoints to a linked worktree git directory.Previously, calling
Repo()inside a linked worktree withGIT_DIR=$(git rev-parse --git-dir)could fail withInvalidGitRepositoryError, whileRepo(os.getcwd())resolved the repository correctly.Changes
GIT_DIRis setReproduction
Inside a linked worktree:
Before: raises
InvalidGitRepositoryErrorAfter: resolves repository correctly