Skip to content

Fix IndexFile.from_tree on Windows #1751

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Fix IndexFile.from_tree on Windows #1751

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e359718
Revert "Add xfail marks for IndexFile.from_tree failures"
EliahKagan Nov 29, 2023
b12fd4a
Let Windows subprocess open or rename onto temp file
EliahKagan Nov 7, 2023
12bbace
Add xfail mark for new test_index_mutation failure
EliahKagan Nov 30, 2023
928ca1e
Minor formatting consistency improvement
EliahKagan Nov 7, 2023
9e5d0aa
Avoid subprocess-writable temp file race condition
EliahKagan Nov 7, 2023
782c062
Add myself to AUTHORS
EliahKagan Nov 29, 2023
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
Prev Previous commit
Next Next commit
Add xfail mark for new test_index_mutation failure 
The test assumes symlinks are never created on Windows. This often
turns out to be correct, because core.symlinks defaults to false on
Windows. (On some Windows systems, creating them is a privileged
operation; this can be reconfigured, and unprivileged creation of
symlinks is often enabled on systems used for development.)

However, on a Windows system with core.symlinks set to true, git
will create symlinks if it can, when checking out entries committed
to a repository as symlinks. GitHub Actions runners for Windows do
this ever since actions/runner-images#1186
(the file is now at images/windows/scripts/build/Install-Git.ps1;
the `/o:EnableSymlinks=Enabled` option continues to be passed,
causing Git for Windows to be installed with core.symlinks set to
true in the system scope).

For now, this adds an xfail marking to test_index_mutation, for the
FileNotFoundError raised when a symlink, which is expected not to
be a symlink, is passed to `open`, causing an attempt to open its
nonexistent target. (The check itself might bear refinement: as
written, it reads the core.symlinks variable from any scope,
including the local scope, which at the time of the check will
usually be the cloned GitPython directory, where pytest is run.)

While adding an import, this commit also improves the grouping and
sorting of existing ones.
  • Loading branch information
@EliahKagan
EliahKagan committed Dec 1, 2023
commit 12bbacee90f3810367a2ce986f42c3172c598c58
25 changes: 17 additions & 8 deletions test/test_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,21 @@
from sumtypes import constructor, sumtype

from git import (
BlobFilter,
Diff,
Git,
IndexFile,
Object,
Repo,
BlobFilter,
UnmergedEntriesError,
Tree,
Object,
Diff,
GitCommandError,
)
from git.exc import (
CheckoutError,
GitCommandError,
HookExecutionError,
InvalidGitRepositoryError,
UnmergedEntriesError,
)
from git.exc import HookExecutionError, InvalidGitRepositoryError
from git.index.fun import hook_path
from git.index.typ import BaseIndexEntry, IndexEntry
from git.objects import Blob
Expand Down Expand Up @@ -530,6 +534,11 @@ def _count_existing(self, repo, files):

# END num existing helper

@pytest.mark.xfail(
os.name == "nt" and Git().config("core.symlinks") == "true",
Copy link
Member Author

@EliahKagan EliahKagan Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm unsure if this is the best way for the xfail condition to check the core.symlinks configuration variable. This will typically run with the CWD as the GitPython repository, thereby including the local scope, and maybe only the system and global scopes should be used. (I had thought to use GitConfigParser, which I think can omit the local scope since #950, but it looks like its use of the system scope is limited on Windows.)

Copy link
Member

Choose a reason for hiding this comment

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

I think GitPython has the same problem that gitoxide would have: It fails to see the configuration coming with the git installation itself, which has to be discovered by asking git itself.

That's the configuration file that contains these values, on Windows at least. Maybe Windows will also put that value into the repository-local configuration file, which might be the reason this condition works at all.

And since it works, I suppose it's good enough for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe Windows will also put that value into the repository-local configuration file, which might be the reason this condition works at all.

At least in the more common false case, I've found that it is coped into the local repository's configuration. But using Git().config(...) doesn't rely on that. It works outside any repository and finds the system configuration:

(.venv) C:\Users\ek\source\repos\GitPython [fromtree ≡]> pushd ~/tmp
(.venv) C:\Users\ek\tmp> python
Python 3.12.0 (tags/v3.12.0:0fb18b0, Oct  2 2023, 13:03:39) [MSC v.1935 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import git
>>> git.Git().config("core.symlinks", show_origin=True)
'file:C:/Users/ek/scoop/apps/git/2.43.0/etc/gitconfig\tfalse'

(This is the same GitPython as when I run it from the GitPython directory, since the virtual environment is activated and the package is installed in it using pip install -e ..)

So I think the condition is working because config is not being treated specially--unlike if GitConfigParser is used, the command in that condition it really is running git config.

(.venv) C:\Users\ek\tmp> python
Python 3.12.0 (tags/v3.12.0:0fb18b0, Oct  2 2023, 13:03:39) [MSC v.1935 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import logging
>>> logging.basicConfig(level=logging.INFO)
>>> import git
>>> git.Git.GIT_PYTHON_TRACE = "full"
>>> git.Git().config("core.symlinks")
INFO:git.cmd:git config core.symlinks -> 0; stdout: 'false'
'false'

reason="Assumes symlinks are not created on Windows and opens a symlink to a nonexistent target.",
raises=FileNotFoundError,
)
@with_rw_repo("0.1.6")
def test_index_mutation(self, rw_repo):
index = rw_repo.index
Expand Down Expand Up @@ -740,7 +749,7 @@ def mixed_iterator():
# END for each target
# END real symlink test

# Add fake symlink and assure it checks-our as symlink.
# Add fake symlink and assure it checks out as a symlink.
fake_symlink_relapath = "my_fake_symlink"
link_target = "/etc/that"
fake_symlink_path = self._make_file(fake_symlink_relapath, link_target, rw_repo)
Expand Down Expand Up @@ -774,7 +783,7 @@ def mixed_iterator():
os.remove(fake_symlink_path)
index.checkout(fake_symlink_path)

# On Windows, we will never get symlinks.
# On Windows, we currently assume we will never get symlinks.
if os.name == "nt":
# Symlinks should contain the link as text (which is what a
# symlink actually is).
Expand Down