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

PROOF-OF-CONCEPT: allow deleting files that are in use #4719

Closed

Conversation

dscho
Copy link
Member

@dscho dscho commented Nov 29, 2023

This should not be merged as-is, but is intended to act as a discussion-starter.

Applying the insights of https://github.com/LloydLabs/delete-self-poc, this commit implements support for deleting files that are in-use, even if that is theoretically impossible on Windows.

The ramifications are somewhat non-trivial, though:

  • Many Windows applications are totally unhappy when the contract "files that are in use cannot be deleted" is broken.

  • Even git.exe itself can be used to demonstrate the dire downsides of this here patch: when switching branches in git-sdk-64 where, say, mingw64/bin/libintl-8.dll needs to be updated (which is a dependency of mingw64/bin/git.exe), a checkout.workers value greater than 1 will let git.exe attempt to spawn git checkout--worker which will fail to load because of the missing dependency (which is missing because git.exe just deleted it).

This is a potential alternative to #1666, without the loss of performance of that Pull Request.

This should not be merged as-is, but is intended to act as a
discussion-starter.

Applying the insights of https://github.com/LloydLabs/delete-self-poc,
this commit implements support for deleting files that are in-use, even
if that is theoretically impossible on Windows.

The ramifications are somewhat non-trivial, though:

- Many Windows applications are totally unhappy when the contract "files
  that are in use cannot be deleted" is broken.

- Even `git.exe` itself can be used to demonstrate the dire downsides of
  this here patch: when switching branches in `git-sdk-64` where, say,
  `mingw64/bin/libintl-8.dll` needs to be updated (which is a dependency
  of `mingw64/bin/git.exe`), a `checkout.workers` value greater than 1
  will let `git.exe` attempt to spawn `git checkout--worker` which will
  fail to load because of the missing dependency (which is missing
  because `git.exe` _just_ deleted it).

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member Author

dscho commented Dec 2, 2023

I tried using this a couple of times with git pull in the git-sdk-64 checkout I have over here, and ran into serious trouble:

  • Since there are many files to update, I use checkout.workers=56. This means that 56 (!) git checkout--helper processes are spawned. However, they are spawned only after libintl-8.dll, a dependency of git.exe, is deleted, which means that the git checkout--helper processes all fail because of that missing DLL.
  • When the in-use msys-2.0.dll was updated, everything started failing afterwards (I had a lot of sessions open, so that was kind of unpleasant).

Granted, not many users want to use Git to upgrade Git.

Having said that, I expect many similar problems should we merge this PR: It is such a long-standing contract that Windows programs can rely on files remaining present as long as they are in use.

I guess we could guard this behind a new config setting, but I am worried that this is a real foot gun.

@dscho
Copy link
Member Author

dscho commented Dec 4, 2023

Looks like there are more problems: https://github.com/git-for-windows/git/actions/runs/7038769040/job/19156545431?pr=4719#step:5:390 (and it's not only win test, win+VS test agrees):

failure: t5516.95 fetch exact SHA1
[...]
  ++ git fetch -v ../testrepo 9ad36e1e54b2130a20d55abb4f0f3ca8494ead3f:refs/heads/copy main:refs/heads/extra
  remote: error: object file ./objects/50/b820aea6d3503362343cdc0e699b760c700b2b is empty        
  remote: error: object file ./objects/50/b820aea6d3503362343cdc0e699b760c700b2b is empty        
  remote: fatal: bad tree object 50b820aea6d3503362343cdc0e699b760c700b2b        
  error: git upload-pack: git-pack-objects died with error.
  fatal: git upload-pack: aborting due to possible repository corruption on the remote side.
  remote: aborting due to possible repository corruption on the remote side.
  fatal: protocol error: bad pack header
  error: last command exited with $?=128
  ++ rm -rf child
  ++ exit 128
  ++ eval_ret=128
  ++ rm -rf testrepo
  ++ exit 128
  ++ eval_ret=128
  ++ :
  not ok 95 - fetch exact SHA1

failure: t5516.96 fetch exact SHA1 in protocol v2 
[...]
  ++ git -C child fetch -v ../testrepo 9ad36e1e54b2130a20d55abb4f0f3ca8494ead3f:refs/heads/copy
  remote: error: object file ./objects/50/b820aea6d3503362343cdc0e699b760c700b2b is empty        
  remote: error: object file ./objects/50/b820aea6d3503362343cdc0e699b760c700b2b is empty        
  remote: fatal: bad tree object 50b820aea6d3503362343cdc0e699b760c700b2b        
  error: git upload-pack: git-pack-objects died with error.
  fatal: git upload-pack: aborting due to possible repository corruption on the remote side.
  remote: aborting due to possible repository corruption on the remote side.
  fatal: protocol error: bad pack header
  error: last command exited with $?=128
  ++ rm -rf child
  ++ exit 128
  ++ eval_ret=128
  ++ rm -rf testrepo
  ++ exit 128
  ++ eval_ret=128
  ++ :
  not ok 96 - fetch exact SHA1 in protocol v2

@dscho
Copy link
Member Author

dscho commented Jan 10, 2024

I made up my mind: This change would cause more problems than it solves, so I'll abandon the idea.

@dscho dscho closed this Jan 10, 2024
@dscho dscho deleted the allow-deleting-files-that-are-in-use branch January 10, 2024 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant