Fix IndexFile.from_tree on Windows

[EliahKagan]

Fixes #1630

IndexFile.from_tree uses NamedTemporaryFile to create a temporary file in the repository's .git directory to pass to git read-tree as the operand of --index-output. But on Windows, the command always fails: the git subprocess reports it cannot write to that path. IndexFile.from_tree and everything that uses it are thus broken on Windows. This is the cause of #1630, as IndexFile.reset uses IndexFile.from_tree.

git read-tree does not actually write to the file itself. Instead, it attempts to rename its own separate temporary file to have its name. git read-tree does not require that any file exist there already. The reason we create it is to get a filename that was definitely available, and prevent an unintended race condition on the name. As git-read-tree(1) says:

--index-output=<file>
Instead of writing the results out to $GIT_INDEX_FILE, write the resulting index in the named file. While the command is operating, the original index file is locked with the same mechanism as usual. The file must allow to be rename(2)ed into from a temporary file that is created next to the usual index file; typically this means it needs to be on the same filesystem as the index file itself, and you need write permission to the directories the index file and index output file are located in.

Using NamedTemporaryFile for this works on Unix-like systems, but not on Windows, because git needs to be able to replace the file by renaming its own temporary file to the same name, which is a form of deletion: it deletes the file being replaced. NamedTemporaryFile opens the file, which remains open until the context manager object is exited. But on Windows, one cannot typically delete an open file. (As noted in b12fd4a, NamedTemporaryFile is often unsuitable for files shared with other processes on Windows even when they do not attempt to delete the file.)

This fixes the problem on Windows by using mkstemp to create the file, and managing cleanup explicitly. Both NamedTemporaryFile and mkstemp will create a file by opening it in a way that fails if the file already exists (and retrying other names if that happens). In this way they avoid the problems of mktemp (see also smmap#41). Immediately after opening the file for creation to get the name, we close it. When we pass its name for git read-tree to rename its own temporary file to, git is able to do so.

More information about the problem, and how it is solved, appears in the commit messages. Especially:

• b12fd4a describes the problem in detail. It fixes it in a way that is very simple, but unsatisfactory because it introduces a race condition of the same kind one would get with mktemp. But this verifies the nature of the bug.
• 12bbace describes why test_index_mutation still fails (later than before, and unlike the other seven affected tests that now pass). It adds a conditional xfail mark. This is due to a test bug, but I think there are tradeoffs to be considered in deciding how to fix it, so I have not attempted to do so in this PR.
• 9e5d0aa describes the race condition from b12fd4a and how to overcome it. It applies the better fix. (Although the race condition would not be as severe here as in the common case where the file is being created in a shared temporary directory, it should still be avoided.)

---

I have not added any new test cases related to this bug. IndexFile.from_tree does not seem to have any of its own tests, and perhaps should. However, as described in the commit messages (and viewable in diffs removing xfail markings), eight tests were already failing due to it. Of those, seven now pass, while one, which tests a number of things, gets past the point where it failed before and fails later, differently and for an unrelated reason.

To further confirm specifically that this fixes #1630, I followed the procedure described there, which currently shows the bug on the tip of the main branch, but produces no error and successfully resets the change in the working tree at the tip of this topic branch:

(.venv) C:\Users\ek\source\repos\GitPython [fromtree ≡]> rm pyproject.toml
(.venv) C:\Users\ek\source\repos\GitPython [fromtree ≡ +0 ~0 -1 !]> git status -sb
## fromtree...origin/fromtree
 D pyproject.toml
(.venv) C:\Users\ek\source\repos\GitPython [fromtree ≡ +0 ~0 -1 !]> 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.
>>> from git import Repo
>>> Repo().index.reset(working_tree=True)
<git.index.base.IndexFile object at 0x0000023657EF27F0>
>>> exit()
(.venv) C:\Users\ek\source\repos\GitPython [fromtree ≡]> git status -sb
## fromtree...origin/fromtree
(.venv) C:\Users\ek\source\repos\GitPython [fromtree ≡]>

[EliahKagan]

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.)

[Byron]

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?

[EliahKagan]

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'

[Byron]

Thanks a lot for your incredible support!

It's always good to see a PR that removes more than it adds, and this one even doubly so as it adds test-coverage to windows while removing xfails in one fell swoop!

It would be interesting to know if GitPython was previously used on Windows at all, given its shortcomings there, and if there is a user-base that can benefit from these improvements.

[Byron]

About time :D!

[EliahKagan]

It would be interesting to know if GitPython was previously used on Windows at all, given its shortcomings there, and if there is a user-base that can benefit from these improvements.

I use nbdime, including on Windows, which depends on GitPython. That tool is pretty popular, I think including on Windows. My guess is that there are numerous libraries and tools that use GitPython and are used on Windows by many. However, it may be that most of them are doing fine, using parts of GitPython that are working well. Even if many Windows developers use GitPython directly, I don't know what functionality of the library tends in practice to be the most widely used or important. Anyway, if the tests results are any indication--of course a rough one since it's not like each test tests the same amount of significant functionality--most of which are passing on Windows, then I would expect GitPython to be pretty usable on Windows already for many practical applications.