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

Repo.index.reset fails on windows #1630

Closed
suvayu opened this issue Aug 19, 2023 · 3 comments · Fixed by #1751
Closed

Repo.index.reset fails on windows #1630

suvayu opened this issue Aug 19, 2023 · 3 comments · Fixed by #1751

Comments

@suvayu
Copy link

suvayu commented Aug 19, 2023

How to reproduce

  1. clone any repo
  2. edit a tracked file
  3. in python, try to reset with
    Repo().index.reset(working_tree=True)
    You will get a stack trace.

Step by step recipe:

$ git clone https://github.com/gitpython-developers/GitPython git-python
$ cd git-python
$ python -m venv --prompt git venv
$ .\venv\Scirpts\Activate.ps1  # or whatever script is appropriate for you
$ rm pyproject.toml  # make any edit
$ git status -sb
## main...origin/main
 D pyproject.toml
$ python
>>> from git import Repo
>>> repo.index.reset(working_tree=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\<user>\code\git-python\git\index\util.py", line 103, in check_default_index
    return func(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\<user>\code\git-python\git\index\base.py", line 1320, in reset
    new_inst = type(self).from_tree(self.repo, commit)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\<user>\code\git-python\git\index\base.py", line 367, in from_tree
    repo.git.read_tree(*arg_list, **kwargs)
  File "C:\Users\<user>\code\git-python\git\cmd.py", line 739, in <lambda>
    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\user>\code\git-python\git\cmd.py", line 1312, in _call_process
    return self.execute(call, **exec_kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\<user>\code\git-python\git\cmd.py", line 1107, in execute
    raise GitCommandError(redacted_command, status, stderr_value, stdout_value)
git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
  cmdline: git read-tree --index-output=C:\Users\<user>\code\git-python\.git\tmpxnmxtmmu HEAD
  stderr: 'fatal: unable to write new index file'

Expectation

I was expecting the result would be the equivalent of git reset --hard HEAD. Am I doing something incorrectly here? I'm unfamiliar with Windows, so it's quite possible.

@Byron
Copy link
Member

Byron commented Aug 19, 2023

Maybe git itself is reading the index and GitPython fails to overwrite it shortly after. It's a known Windows filesystem issue.
As a workaround, one should be able to use repo.git.reset(…) to invoke git directly, possibly after changing the index using GitPython.

@suvayu
Copy link
Author

suvayu commented Aug 20, 2023

Thanks for the confirmation. For now I have worked around it with Repo.head.reset(working_tree=True) When I get some time, I'll experiment a bit and try to understand this better.

EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 7, 2023
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 opening with an external process, as
IndexFile.from_tree does. On Windows, the git subprocess can't open
and write to the file. This breaks IndexFile.from_tree on Windows,
causing gitpython-developers#1630 (because IndexFile.reset uses IndexFile.from_tree,
and the Repo.index property returns an IndexFile instance, so
Repo.index.reset fails), and causes a number of other breakages.

This commit is an initial fix for the problem, but likely 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. It is less than ideal because it at least appears
to create a race condition: without furher changes not yet made
here, closing the file actually deletes it, which conveniently lets
the name be reused, but because the file does not exist, the
filename might end up being reused before it is recreated.

The following 8 tests go from failing to passing due to this change
on Windows (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
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 8, 2023
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 cannot be replaced,
just as, on Windows, they 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 Windows (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
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 14, 2023
8 of the tests that fail on native Windows systems fail due to
IndexFile.from_tree being broken on Windows, causing gitpython-developers#1630.

This commit marks those tests as xfail. This is part, though not
all, of the changes to get CI test jobs for native Windows that
are passing, to guard against new regressions and to allow the code
and tests to be gradually fixed (see discussion in gitpython-developers#1654).

When fixing the bug, this commit can be reverted.
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 14, 2023
8 of the tests that fail on native Windows systems fail due to
IndexFile.from_tree being broken on Windows, causing gitpython-developers#1630.

This commit marks those tests as xfail. This is part, though not
all, of the changes to get CI test jobs for native Windows that
are passing, to guard against new regressions and to allow the code
and tests to be gradually fixed (see discussion in gitpython-developers#1654).

When fixing the bug, this commit can be reverted.
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 15, 2023
8 of the tests that fail on native Windows systems fail due to
IndexFile.from_tree being broken on Windows, causing gitpython-developers#1630.

This commit marks those tests as xfail. This is part, though not
all, of the changes to get CI test jobs for native Windows that
are passing, to guard against new regressions and to allow the code
and tests to be gradually fixed (see discussion in gitpython-developers#1654).

When fixing the bug, this commit can be reverted.
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 16, 2023
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 cannot be replaced,
just as, on Windows, they 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 Windows (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
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 29, 2023
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 added a commit to EliahKagan/GitPython that referenced this issue Nov 29, 2023
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 cannot be replaced,
just as, on Windows, they 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 Windows (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
EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Nov 29, 2023
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 cannot be replaced,
just as, on Windows, they 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 added a commit to EliahKagan/GitPython that referenced this issue Nov 30, 2023
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 added a commit to EliahKagan/GitPython that referenced this issue Nov 30, 2023
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 cannot be replaced,
just as, on Windows, they 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 added a commit to EliahKagan/GitPython that referenced this issue Dec 1, 2023
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
Copy link
Contributor

This happens because IndexFile.from_tree (used by IndexFile.reset) uses NamedTemporaryFile in a way that works on Unix-like systems but on Windows prevents git read-tree from overwriting the file by moving its own temporary file to its location. I've proposed a fix in #1751.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants