Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix IndexFile.from_tree on Windows #1751

Merged
merged 6 commits into from
Dec 1, 2023
+51 −88
Merged

Fix IndexFile.from_tree on Windows #1751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e359718
Revert "Add xfail marks for IndexFile.from_tree failures"
EliahKagan Nov 29, 2023
b12fd4a
Let Windows subprocess open or rename onto temp file
EliahKagan Nov 7, 2023
12bbace
Add xfail mark for new test_index_mutation failure
EliahKagan Nov 30, 2023
928ca1e
Minor formatting consistency improvement
EliahKagan Nov 7, 2023
9e5d0aa
Avoid subprocess-writable temp file race condition
EliahKagan Nov 7, 2023
782c062
Add myself to AUTHORS
EliahKagan Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About time :D!


Portions derived from other open source works and are clearly marked.
38 changes: 30 additions & 8 deletions git/index/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -67,6 +67,7 @@
BinaryIO,
Callable,
Dict,
Generator,
IO,
Iterable,
Iterator,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down
11 changes: 2 additions & 9 deletions test/test_docs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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]
Expand Down
17 changes: 3 additions & 14 deletions test/test_fun.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
61 changes: 15 additions & 46 deletions test/test_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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",
Copy link
Contributor Author

@EliahKagan EliahKagan Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure if this is the best way for the xfail condition to check the core.symlinks configuration variable. This will typically run with the CWD as the GitPython repository, thereby including the local scope, and maybe only the system and global scopes should be used. (I had thought to use GitConfigParser, which I think can omit the local scope since #950, but it looks like its use of the system scope is limited on Windows.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think GitPython has the same problem that gitoxide would have: It fails to see the configuration coming with the git installation itself, which has to be discovered by asking git itself.

That's the configuration file that contains these values, on Windows at least. Maybe Windows will also put that value into the repository-local configuration file, which might be the reason this condition works at all.

And since it works, I suppose it's good enough for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Windows will also put that value into the repository-local configuration file, which might be the reason this condition works at all.

At least in the more common false case, I've found that it is coped into the local repository's configuration. But using Git().config(...) doesn't rely on that. It works outside any repository and finds the system configuration:

(.venv) C:\Users\ek\source\repos\GitPython [fromtree ≡]> pushd ~/tmp
(.venv) C:\Users\ek\tmp> python
Python 3.12.0 (tags/v3.12.0:0fb18b0, Oct  2 2023, 13:03:39) [MSC v.1935 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import git
>>> git.Git().config("core.symlinks", show_origin=True)
'file:C:/Users/ek/scoop/apps/git/2.43.0/etc/gitconfig\tfalse'

(This is the same GitPython as when I run it from the GitPython directory, since the virtual environment is activated and the package is installed in it using pip install -e ..)

So I think the condition is working because config is not being treated specially--unlike if GitConfigParser is used, the command in that condition it really is running git config.

(.venv) C:\Users\ek\tmp> python
Python 3.12.0 (tags/v3.12.0:0fb18b0, Oct  2 2023, 13:03:39) [MSC v.1935 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import logging
>>> logging.basicConfig(level=logging.INFO)
>>> import git
>>> git.Git.GIT_PYTHON_TRACE = "full"
>>> git.Git().config("core.symlinks")
INFO:git.cmd:git config core.symlinks -> 0; stdout: 'false'
'false'

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):
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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."""
Expand Down
11 changes: 0 additions & 11 deletions test/test_refs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
Loading