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.

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

Commits on Nov 30, 2023

  1. Revert "Add xfail marks for IndexFile.from_tree failures"

    This removes the xfail marks from 8 tests that fail due to gitpython-developers#1630,
    to be fixed in the subsequent commits.
    
    This reverts commit 6e477e3.
    EliahKagan committed Nov 30, 2023
    Configuration menu
    Copy the full SHA
    e359718 View commit details
    Browse the repository at this point in the history

Commits on Dec 1, 2023

  1. 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 gitpython-developers#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 gitpython-developers#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.
    EliahKagan committed Dec 1, 2023
    Configuration menu
    Copy the full SHA
    b12fd4a View commit details
    Browse the repository at this point in the history
  2. Add xfail mark for new test_index_mutation failure

    The test assumes symlinks are never created on Windows. This often
    turns out to be correct, because core.symlinks defaults to false on
    Windows. (On some Windows systems, creating them is a privileged
    operation; this can be reconfigured, and unprivileged creation of
    symlinks is often enabled on systems used for development.)
    
    However, on a Windows system with core.symlinks set to true, git
    will create symlinks if it can, when checking out entries committed
    to a repository as symlinks. GitHub Actions runners for Windows do
    this ever since actions/runner-images#1186
    (the file is now at images/windows/scripts/build/Install-Git.ps1;
    the `/o:EnableSymlinks=Enabled` option continues to be passed,
    causing Git for Windows to be installed with core.symlinks set to
    true in the system scope).
    
    For now, this adds an xfail marking to test_index_mutation, for the
    FileNotFoundError raised when a symlink, which is expected not to
    be a symlink, is passed to `open`, causing an attempt to open its
    nonexistent target. (The check itself might bear refinement: as
    written, it reads the core.symlinks variable from any scope,
    including the local scope, which at the time of the check will
    usually be the cloned GitPython directory, where pytest is run.)
    
    While adding an import, this commit also improves the grouping and
    sorting of existing ones.
    EliahKagan committed Dec 1, 2023
    Configuration menu
    Copy the full SHA
    12bbace View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    928ca1e View commit details
    Browse the repository at this point in the history
  4. Avoid subprocess-writable temp file race condition

    This lets the Windows subprocess open or rename onto the temporary
    file using a more robust approach than in b12fd4a, avoiding the
    race condition described there, where the filename could be
    inadvertently reused between deletion and recreation of the file.
    
    This creates a context manager helper for the temporary index file
    used in IndexFile.from_tree, whose implementation differs by
    operating system:
    
    - Delegating straightforwardly to NamedTempoaryFile on POSIX
      systems where an open file can replaced by having another file
      renamed to it (just as it can be deleted).
    
    - Employing custom logic on Windows, using mkstemp, closing the
      temporary file without immediately deleting it (so it won't be
      reused by any process seeking to create a temporary file), and
      then deleting it on context manager exit.
    
    IndexFile.from_tree now calls this helper instead of
    NamedTemporaryFile. For convenience, the helper provides the path,
    i.e. the "name", when entered, so tmp_index is now just that path.
    
    (At least for now, this helper is implemented as a nonpublic
    function in the git.index.base module, rather than in the
    git.index.util module. If it were public in git.index.util like the
    other facilities there, then some later changes to it, or its later
    removal, would be a breaking change to GitPython. If it were
    nonpublic in git.index.util, then this would not be a concern, but
    it would be unintuitive for it to be accessed from code in the
    git.index.base module. In the future, one way to address this might
    be to have one or more nonpublic _util modules with public members.
    Because it would still be a breaking change to drop existing public
    util modules, that would be more utility modules in total, so such
    a change isn't included here just for this one used-once function.)
    EliahKagan committed Dec 1, 2023
    Configuration menu
    Copy the full SHA
    9e5d0aa View commit details
    Browse the repository at this point in the history
  5. Add myself to AUTHORS

    EliahKagan committed Dec 1, 2023
    Configuration menu
    Copy the full SHA
    782c062 View commit details
    Browse the repository at this point in the history