Fix IndexFile.from_tree on Windows by EliahKagan · Pull Request #1751 · gitpython-developers/GitPython (original) (raw)

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 ≡]>