Merge pull request #1751 from EliahKagan/fromtree

Fix IndexFile.from_tree on Windows

Author: Byron

AUTHORS

@@ -52,5 +52,6 @@ Contributors are:
 -Joseph Hale <me _at_ jhale.dev>
 -Santos Gallegos <stsewd _at_ proton.me>
 -Wenhan Zhu <wzhu.cosmos _at_ gmail.com>
+-Eliah Kagan <eliah.kagan _at_ gmail.com>
 
 Portions derived from other open source works and are clearly marked.

git/index/base.py

@@ -6,7 +6,7 @@
 """Module containing IndexFile, an Index implementation facilitating all kinds of index
 manipulations such as querying and merging."""
 
-from contextlib import ExitStack
+import contextlib
 import datetime
 import glob
 from io import BytesIO
@@ -67,6 +67,7 @@
     BinaryIO,
     Callable,
     Dict,
+    Generator,
     IO,
     Iterable,
     Iterator,
@@ -96,10 +97,30 @@
 __all__ = ("IndexFile", "CheckoutError", "StageType")
 
 
-class IndexFile(LazyMixin, git_diff.Diffable, Serializable):
+@contextlib.contextmanager
+def _named_temporary_file_for_subprocess(directory: PathLike) -> Generator[str, None, None]:
+    """Create a named temporary file git subprocesses can open, deleting it afterward.
+
+    :param directory: The directory in which the file is created.
+
+    :return: A context manager object that creates the file and provides its name on
+        entry, and deletes it on exit.
     """
-    An Index that can be manipulated using a native implementation in order to save git
-    command function calls wherever possible.
+    if os.name == "nt":
+        fd, name = tempfile.mkstemp(dir=directory)
+        os.close(fd)
+        try:
+            yield name
+        finally:
+            os.remove(name)
+    else:
+        with tempfile.NamedTemporaryFile(dir=directory) as ctx:
+            yield ctx.name
+
+
+class IndexFile(LazyMixin, git_diff.Diffable, Serializable):
+    """An Index that can be manipulated using a native implementation in order to save
+    git command function calls wherever possible.
 
     This provides custom merging facilities allowing to merge without actually changing
     your index or your working tree. This way you can perform own test-merges based
@@ -360,9 +381,9 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
 
         # tmp file created in git home directory to be sure renaming
         # works - /tmp/ dirs could be on another device.
-        with ExitStack() as stack:
-            tmp_index = stack.enter_context(tempfile.NamedTemporaryFile(dir=repo.git_dir))
-            arg_list.append("--index-output=%s" % tmp_index.name)
+        with contextlib.ExitStack() as stack:
+            tmp_index = stack.enter_context(_named_temporary_file_for_subprocess(repo.git_dir))
+            arg_list.append("--index-output=%s" % tmp_index)
             arg_list.extend(treeish)
 
             # Move current index out of the way - otherwise the merge may fail
@@ -372,12 +393,13 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
 
             stack.enter_context(TemporaryFileSwap(join_path_native(repo.git_dir, "index")))
             repo.git.read_tree(*arg_list, **kwargs)
-            index = cls(repo, tmp_index.name)
+            index = cls(repo, tmp_index)
             index.entries  # Force it to read the file as we will delete the temp-file.
             return index
             # END index merge handling
 
     # UTILITIES
+
     @unbare_repo
     def _iter_expand_paths(self: "IndexFile", paths: Sequence[PathLike]) -> Iterator[PathLike]:
         """Expand the directories in list of paths to the corresponding paths accordingly.

test/test_docs.py

@@ -8,10 +8,11 @@
 
 import pytest
 
-from git.exc import GitCommandError
 from test.lib import TestBase
 from test.lib.helper import with_rw_directory
 
+import os.path
+
 
 class Tutorials(TestBase):
     def tearDown(self):
@@ -206,14 +207,6 @@ def update(self, op_code, cur_count, max_count=None, message=""):
         assert sm.module_exists()  # The submodule's working tree was checked out by update.
         # ![14-test_init_repo_object]
 
-    @pytest.mark.xfail(
-        os.name == "nt",
-        reason=(
-            "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n"
-            "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'."
-        ),
-        raises=GitCommandError,
-    )
     @with_rw_directory
     def test_references_and_objects(self, rw_dir):
         # [1-test_references_and_objects]

test/test_fun.py

@@ -3,13 +3,10 @@
 
 from io import BytesIO
 from stat import S_IFDIR, S_IFREG, S_IFLNK, S_IXUSR
-import os
+from os import stat
 import os.path as osp
 
-import pytest
-
 from git import Git
-from git.exc import GitCommandError
 from git.index import IndexFile
 from git.index.fun import (
     aggressive_tree_merge,
@@ -37,14 +34,6 @@ def _assert_index_entries(self, entries, trees):
             assert (entry.path, entry.stage) in index.entries
         # END assert entry matches fully
 
-    @pytest.mark.xfail(
-        os.name == "nt",
-        reason=(
-            "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n"
-            "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'."
-        ),
-        raises=GitCommandError,
-    )
     def test_aggressive_tree_merge(self):
         # Head tree with additions, removals and modification compared to its predecessor.
         odb = self.rorepo.odb
@@ -302,12 +291,12 @@ def test_linked_worktree_traversal(self, rw_dir):
         rw_master.git.worktree("add", worktree_path, branch.name)
 
         dotgit = osp.join(worktree_path, ".git")
-        statbuf = os.stat(dotgit)
+        statbuf = stat(dotgit)
         self.assertTrue(statbuf.st_mode & S_IFREG)
 
         gitdir = find_worktree_git_dir(dotgit)
         self.assertIsNotNone(gitdir)
-        statbuf = os.stat(gitdir)
+        statbuf = stat(gitdir)
         self.assertTrue(statbuf.st_mode & S_IFDIR)
 
     def test_tree_entries_from_data_with_failing_name_decode_py3(self):

test/test_index.py

@@ -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
@@ -284,14 +288,6 @@ def add_bad_blob():
         except Exception as ex:
             assert "index.lock' could not be obtained" not in str(ex)
 
-    @pytest.mark.xfail(
-        os.name == "nt",
-        reason=(
-            "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n"
-            "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'."
-        ),
-        raises=GitCommandError,
-    )
     @with_rw_repo("0.1.6")
     def test_index_file_from_tree(self, rw_repo):
         common_ancestor_sha = "5117c9c8a4d3af19a9958677e45cda9269de1541"
@@ -342,14 +338,6 @@ def test_index_file_from_tree(self, rw_repo):
         # END for each blob
         self.assertEqual(num_blobs, len(three_way_index.entries))
 
-    @pytest.mark.xfail(
-        os.name == "nt",
-        reason=(
-            "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n"
-            "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'."
-        ),
-        raises=GitCommandError,
-    )
     @with_rw_repo("0.1.6")
     def test_index_merge_tree(self, rw_repo):
         # A bit out of place, but we need a different repo for this:
@@ -412,14 +400,6 @@ def test_index_merge_tree(self, rw_repo):
         self.assertEqual(len(unmerged_blobs), 1)
         self.assertEqual(list(unmerged_blobs.keys())[0], manifest_key[0])
 
-    @pytest.mark.xfail(
-        os.name == "nt",
-        reason=(
-            "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n"
-            "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'."
-        ),
-        raises=GitCommandError,
-    )
     @with_rw_repo("0.1.6")
     def test_index_file_diffing(self, rw_repo):
         # Default Index instance points to our index.
@@ -555,12 +535,9 @@ def _count_existing(self, repo, files):
     # END num existing helper
 
     @pytest.mark.xfail(
-        os.name == "nt",
-        reason=(
-            "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n"
-            "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'."
-        ),
-        raises=GitCommandError,
+        os.name == "nt" and Git().config("core.symlinks") == "true",
+        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):
@@ -772,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)
@@ -806,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).
@@ -915,14 +892,6 @@ def make_paths():
         for absfile in absfiles:
             assert osp.isfile(absfile)
 
-    @pytest.mark.xfail(
-        os.name == "nt",
-        reason=(
-            "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n"
-            "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'."
-        ),
-        raises=GitCommandError,
-    )
     @with_rw_repo("HEAD")
     def test_compare_write_tree(self, rw_repo):
         """Test writing all trees, comparing them for equality."""

test/test_refs.py

@@ -4,11 +4,8 @@
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
 from itertools import chain
-import os
 from pathlib import Path
 
-import pytest
-
 from git import (
     Reference,
     Head,
@@ -218,14 +215,6 @@ def test_head_checkout_detached_head(self, rw_repo):
         assert isinstance(res, SymbolicReference)
         assert res.name == "HEAD"
 
-    @pytest.mark.xfail(
-        os.name == "nt",
-        reason=(
-            "IndexFile.from_tree is broken on Windows (related to NamedTemporaryFile), see #1630.\n"
-            "'git read-tree --index-output=...' fails with 'fatal: unable to write new index file'."
-        ),
-        raises=GitCommandError,
-    )
     @with_rw_repo("0.1.6")
     def test_head_reset(self, rw_repo):
         cur_head = rw_repo.head