Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b12fd4a

Browse filesBrowse files
EliahKaganEliahKagan
committed
Let Windows subprocess open or rename onto temp file
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 #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 typically cannot be replaced, just as, on Windows, they typically 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 #1630. The following 8 tests go from failing to passing due to this change on a local Windows development machine (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 On CI, one test still fails, but later and for an unrelated reason: - test/test_index.py:428 TestIndex.test_index_mutation That `open(fake_symlink_path, "rt")` call raises FileNotFoundError.
1 parent e359718 commit b12fd4a
Copy full SHA for b12fd4a

File tree

Expand file treeCollapse file tree

1 file changed

+2
-0
lines changed
Filter options
Expand file treeCollapse file tree

1 file changed

+2
-0
lines changed

‎git/index/base.py

Copy file name to clipboardExpand all lines: git/index/base.py
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,8 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
362362
# works - /tmp/ dirs could be on another device.
363363
with ExitStack() as stack:
364364
tmp_index = stack.enter_context(tempfile.NamedTemporaryFile(dir=repo.git_dir))
365+
if os.name == "nt":
366+
tmp_index.close()
365367
arg_list.append("--index-output=%s" % tmp_index.name)
366368
arg_list.extend(treeish)
367369

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.