Repository: gitpython-developers/GitPython
Pull request #1776 (2023-12-21 05:21:30)

Fix TemporaryFileSwap regression where file_path could not be Path
As noted in https://github.com/gitpython-developers/GitPython/pull/1770#discussion_r1433438311, in 9e86053 (#1770) I accidentally introduced a regression in `git.index.util.TemporaryFileSwap` where the `file_path` parameter could no longer be a `Path`, even though it was (and remained) annotated in a way compatible with that.

Passing a `Path` object instead of `str` failed with `TypeError`, with a message like this (with `WindowsPath` instead of `PosixPath` on Windows, but otherwise the same):

```text
TypeError: unsupported operand type(s) for +: 'PosixPath' and 'str'
```

Although much code that results in `TemporaryFileSwap` being used is under test, which would reveal some kinds of regressions, `TemporaryFileSwap` itself did not have any tests. It is public, so even if a `Path` were never passed to it in GitPython's own use of it, applications that use GitPython may be making direct use of it and are justified in assuming it accepts both a `str` and a `str`-based `Path`, as annotated.

This pull request:

- **Adds a general test of `TemporaryFileSwap`.** It does not test every code path that is supposed to be supported, such as code paths involving the file not existing in the first place, or the temporary file being deleted before it would be swapped back (those are deliberately covered in the code and may thus be valuable to test, and perhaps I, or someone else, could add tests for that later). However, I parameterized the test by the type of the `file_path` argument passed to `TemporaryFileSwap`, and the test shows both that it works when the type is `str` and that it fails, as described above, when the type is `Path`.
- **Fixes the bug**, and also changes how `mkstemp` is called to make it less unidiomatic. (However, it is still opening the file to reserve its name, and then immediately closing it, so this does not address the original misgivings noted in [#1770 (review)](https://github.com/gitpython-developers/GitPython/pull/1770#pullrequestreview-1779282078). See my comment in that review thread.)

There are some further details, which I'd consider less essential but potentially of value, in the commit messages.


Patch:
diff --git a/git/index/util.py b/git/index/util.py
index 1c3b1c4a..61039fe7 100644
--- a/git/index/util.py
+++ b/git/index/util.py
@@ -41,7 +41,8 @@ class TemporaryFileSwap:
 
     def __init__(self, file_path: PathLike) -> None:
         self.file_path = file_path
-        fd, self.tmp_file_path = tempfile.mkstemp(prefix=self.file_path, dir="")
+        dirname, basename = osp.split(file_path)
+        fd, self.tmp_file_path = tempfile.mkstemp(prefix=basename, dir=dirname)
         os.close(fd)
         with contextlib.suppress(OSError):  # It may be that the source does not exist.
             os.replace(self.file_path, self.tmp_file_path)


Test patch:
diff --git a/test/test_blob_filter.py b/test/test_blob_filter.py
index a91f211b..ddd83079 100644
--- a/test/test_blob_filter.py
+++ b/test/test_blob_filter.py
@@ -14,14 +14,15 @@ from git.objects import Blob
 from git.types import PathLike
 
 
-# fmt: off
-@pytest.mark.parametrize('paths, path, expected_result', [
-    ((Path("foo"),), Path("foo"), True),
-    ((Path("foo"),), Path("foo/bar"), True),
-    ((Path("foo/bar"),), Path("foo"), False),
-    ((Path("foo"), Path("bar")), Path("foo"), True),
-])
-# fmt: on
+@pytest.mark.parametrize(
+    "paths, path, expected_result",
+    [
+        ((Path("foo"),), Path("foo"), True),
+        ((Path("foo"),), Path("foo/bar"), True),
+        ((Path("foo/bar"),), Path("foo"), False),
+        ((Path("foo"), Path("bar")), Path("foo"), True),
+    ],
+)
 def test_blob_filter(paths: Sequence[PathLike], path: PathLike, expected_result: bool) -> None:
     """Test the blob filter."""
     blob_filter = BlobFilter(paths)
diff --git a/test/test_index.py b/test/test_index.py
index 2f97f0af..c3f3b4fa 100644
--- a/test/test_index.py
+++ b/test/test_index.py
@@ -34,10 +34,11 @@ from git.exc import (
 )
 from git.index.fun import hook_path
 from git.index.typ import BaseIndexEntry, IndexEntry
+from git.index.util import TemporaryFileSwap
 from git.objects import Blob
-from test.lib import TestBase, fixture, fixture_path, with_rw_directory, with_rw_repo
 from git.util import Actor, hex_to_bin, rmtree
 from gitdb.base import IStream
+from test.lib import TestBase, fixture, fixture_path, with_rw_directory, with_rw_repo
 
 HOOKS_SHEBANG = "#!/usr/bin/env sh\n"
 
@@ -1087,3 +1088,25 @@ class TestIndex(TestBase):
         file.touch()
 
         rw_repo.index.add(file)
+
+
+class TestIndexUtils:
+    @pytest.mark.parametrize("file_path_type", [str, Path])
+    def test_temporary_file_swap(self, tmp_path, file_path_type):
+        file_path = tmp_path / "foo"
+        file_path.write_bytes(b"some data")
+
+        with TemporaryFileSwap(file_path_type(file_path)) as ctx:
+            assert Path(ctx.file_path) == file_path
+            assert not file_path.exists()
+
+            # Recreate it with new data, so we can observe that they're really separate.
+            file_path.write_bytes(b"other data")
+
+            temp_file_path = Path(ctx.tmp_file_path)
+            assert temp_file_path.parent == file_path.parent
+            assert temp_file_path.name.startswith(file_path.name)
+            assert temp_file_path.read_bytes() == b"some data"
+
+        assert not temp_file_path.exists()
+        assert file_path.read_bytes() == b"some data"  # Not b"other data".