Fix Repo() autodiscovery in linked worktrees when GIT_DIR is set by meliezer · Pull Request #2128 · gitpython-developers/GitPython
Merged
Conversation
Fixes #2022
This PR fixes repository autodiscovery in linked worktrees when GIT_DIR is set.
Summary
Fix Repo() autodiscovery when GIT_DIR points to a linked worktree git directory.
Previously, calling Repo() inside a linked worktree with GIT_DIR=$(git rev-parse --git-dir) could fail with InvalidGitRepositoryError, while Repo(os.getcwd()) resolved the repository correctly.
Changes
- add regression coverage for autodiscovery in linked worktrees when
GIT_DIRis set - detect linked worktree git directories during repository discovery
- derive the working tree directory correctly from the linked worktree metadata
Reproduction
Inside a linked worktree:
export GIT_DIR=$(git rev-parse --git-dir)
python -c "from git import Repo; Repo()"
Before: raises InvalidGitRepositoryError
After: resolves repository correctly
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ( |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Comment on lines +255 to +256
| if "GIT_WORK_TREE" in os.environ: | ||
| self._working_tree_dir = os.getenv("GIT_WORK_TREE") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on lines +1146 to +1152
| 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() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot uses AI. Check for mistakes.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks — I’ve updated the test to make the environment hermetic by clearing any pre-existing GIT_WORK_TREE and GIT_COMMON_DIR while keeping GIT_DIR set for the scenario under test.
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.
Thanks — I can’t re-run the failed jobs from my side since I don’t have write access to the upstream repository.
The remaining failures are on the Windows 3.11/3.12 jobs, and the one visible failure is test/test_index.py::TestIndex::test_index_mutation (git-checkout-index / symlink warning), which looks unrelated to this PR since this change only touches git/repo/base.py and test/test_repo.py.
Could you please re-run the failed Windows jobs once? If they still fail consistently, I can investigate further.
Thanks — after the retry, only one Windows job is still failing, in test/test_index.py::TestIndex::test_index_mutation with a git-checkout-index symlink warning.
That looks unrelated to this change, since this PR only touches git/repo/base.py and test/test_repo.py. The linked-worktree tests added here are passing.
If you’d like, I can investigate that Windows failure separately, but it seems outside the scope of this PR.
I could simply push the following code in this PR. There are already many other XFAIL. @Byron, do you agree?
@pytest.mark.xfail(sys.platform == "win32", reason="Git checkout-index fallisce sui symlink in Windows CI") def test_index_mutation(self, rw_repo):
It's strange that it works on one runner, but on on the other. Or maybe related to the python version.
But… it seems to be a new failure that everyone is seeing right now, so let's just neuter it and get this merged :).
Byron
mentioned this pull request