Skip to content

Commit

Permalink
Let Windows subprocess open or rename onto temp file
Browse files Browse the repository at this point in the history
On Windows, a file created by NamedTemporaryFile cannot be reopened
while already open, under most circumstances. This applies to
attempts to open it both from the original process and from other
processes. This differs from the behavior on other operating
systems, and it works this way to help ensure the file can be
automatically deleted (since having a file open on Windows usually
prevents the file from being deleted, unlike on other OSes).

However, this can cause problems when NamedTemporaryFile is used to
produce a safe filename for accessing with an external process, as
IndexFile.from_tree does. On Windows, git subprocess can't open and
write to the file. This breaks IndexFile.from_tree on Windows. This
is the cause of gitpython-developers#1630 -- IndexFile.reset uses IndexFile.from_tree,
and the Repo.index property returns an IndexFile instance, so
Repo.index.reset fails -- and some other breakages.

While passing delete=False to NamedTemporaryFile (and deleting
separately) often overcomes that, it won't fix things here, because
IndexFile.from_tree uses "git read-tree --index-output=<file>".
That needs more than the ability to open the file for write. It
needs to be able to create or overwrite the given path by renaming
an existing file to it. The description of "--index-output=<file>"
in git-read-tree(1) notes:

    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.

On Windows, more is required: there must be no currently open file
with that path, because on Windows, open files cannot be replaced,
just as, on Windows, they cannot be deleted (nor renamed). This is
to say that passing delete=False to NamedTemporaryFile isn't enough
to solve this because it merely causes NamedTemporaryFile to behave
like the lower-level mkstemp function, leaving the responsibility
of deletion to the caller but still *opening* the file -- and while
the file is open, the git subprocess can't replace it. Switching to
mkstemp, with no further change, would likewise not solve this.

This commit is an initial fix for the problem, but not the best
fix. On Windows, it closes the temporary file created by
NamedTemporaryFile -- so the file can be opened by name, including
by a git subprocess, and also overwritten, include by a file move.

As currently implemented, this is not ideal, as it creates a race
condition, because closing the file actually deletes it. During the
time the file does not exist, the filename may end up being reused
by another call to NamedTemporaryFile, mkstemp, or similar facility
(in any process, written in any language) before the git subprocess
can recreate it. Passing delete=False would avoid this, but then
deletion would have to be handled separately, and at that point it
is not obvious that NamedTemporaryFile is the best facility to use.

This fix, though not yet adequate, confirms the impact on gitpython-developers#1630.
The following 8 tests go from failing to passing due to this change
on Windows (as listed in the summary at the end of pytest output):

- test/test_docs.py:210 Tutorials.test_references_and_objects
- test/test_fun.py:37 TestFun.test_aggressive_tree_merge
- test/test_index.py:781 TestIndex.test_compare_write_tree
- test/test_index.py:294 TestIndex.test_index_file_diffing
- test/test_index.py:182 TestIndex.test_index_file_from_tree
- test/test_index.py:232 TestIndex.test_index_merge_tree
- test/test_index.py:428 TestIndex.test_index_mutation
- test/test_refs.py:218 TestRefs.test_head_reset
  • Loading branch information
EliahKagan committed Nov 8, 2023
1 parent 74cc671 commit fbf3311
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions git/index/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,8 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
# works - /tmp/ dirs could be on another device.
with ExitStack() as stack:
tmp_index = stack.enter_context(tempfile.NamedTemporaryFile(dir=repo.git_dir))
if os.name == "nt":
tmp_index.close()
arg_list.append("--index-output=%s" % tmp_index.name)
arg_list.extend(treeish)

Expand Down

0 comments on commit fbf3311

Please sign in to comment.