Skip to content

Commit

Permalink
Refactor TemporaryFileSwap.__init__ for clarity
Browse files Browse the repository at this point in the history
This changes the mkstemp call TemporaryFileSwap uses to atomically
create (and thus reserve) the temporary file, so that it passes
only a filename (i.e., basename) prefix as `prefix`, and passes all
other path components (i.e., the directory) as `dir`. (If the
original path has no directory, then `dir` is "" as before.)

This makes the mkstemp call *slightly* more idiomatic. This also
makes it clearer, as it is no longer using `prefix` for something
that feels like it should be possible to pass as a Path object.

Although mkstemp does not accept a Path as `prefix`, it does (as
expected) accept one as `dir`. However, to keep the code simple,
I'm passing str for both. The os.path.split function accepts both
str and Path (since Python 3.6), and returns str objects, which are
now used for the `dir` and `prefix` arguments to mkstemp.

For unusual cases, this may technically not be a refactoring. For
example, a file_path of "a/b//c" will be split into "a/b" and "c".
If the automatically generated temporary file suffix is "xyz", then
that results in a tmp_file_path of "a/b/cxyz" where "a/b//cxyz"
would have been used before. The tmp_file_path attribute of a
TemporaryFileSwap object is public (and used in filesystem calls).

However, no guarantee has ever been given that the temporary file
path have the original path as an exact string prefix. I believe
the slightly weaker relationship I expressed in the recently
introduced test_temporary_file_swap -- another file in the same
directory, named with the original filename with more characters,
consisting of equivalent path components in the same order -- has
always been the intended one.

Note that this slight possible variation does not apply to the
file_path attribute. That attribute is always kept exactly as it
was, both in its type and its value, and it always used unmodified
in calls that access the filesystem.
  • Loading branch information
EliahKagan committed Dec 21, 2023
1 parent 1ddf953 commit b438c45
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion git/index/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -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=str(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)
Expand Down

0 comments on commit b438c45

Please sign in to comment.