Precommit hook is unsafe

[bbrtj]

When I remove a file normally (rm, not git rm) and commit, the precommit hook for git restores these files. Worse yet, it manages to remove all my unstaged changes when I follow this sequence:

• remove a file normally
• stage the removal, don't stage other changes
• commit

What ends up happening is all my unstaged changes are deleted and the deleted file is restored. Seems it only happens after removing a file. It already managed to wipe my changes once, thankfully vim kept the undo history for me so I was able to restore them.

[autarch]

I've seen deleted files restored after a commit succeeds, but I haven't had any other data destruction issues.

Can you provide a sequence of commands to replicate this?

[bbrtj]

It seems I had to:

• set up a repository
• add .tidyallrc
• add a first commit
• add the git hook

and then:

touch test1
echo "asd" > test2
touch test3
git add -A
git commit -m "1"
rm test3
git add test3
git commit -m "2" # test3 file gets restored
echo "def" >> test2
rm test1
git add test1
git commit -m "3" # change to test2 is lost, test1 gets restored

For some reason, the change does not seem to get lost after the first file deletion and commit. Some messages / errors are present:

Could not restore untracked files from stash entry
        (in cleanup) "git" unexpectedly returned exit value 1 at .../Code/TidyAll/Git/Precommit.pm line 98.

git version 2.16.4

[autarch]

Ah, that makes a lot more sense. Git can be a bit weird in interactions around the first commit.

[bbrtj]

Not sure if I understand what you mean, but I had the same problem in a repository with ~35 commits, so it is not only happening in fresh repositories.

[autarch]

Ah, I misunderstood. I thought you were saying this only happened on the first commit.

[autarch]

So I followed the steps you described, but I cannot reproduce your results. I do see the test3 file coming back after the commit, but in the final bit, I don't lose the changes to the test2 file.

The deleted files getting restored is some sort of weird interaction with the git stash. The precommit hooks stashes all unstaged changes before running so it only checks the changes that are about to be committed. Then it pops those changes off the stash. For some reason this restore deleted files (though git still knows they're deleted).

[bbrtj]

Okay so I dug a bit deeper and it is reproducible every time on my setup. The key is the git stash pop ran to restore the files from the stash. For me, it ends with the message I mentioned before:

$ git commit -m '2'
Saved working directory and index state On master: TidyAll pre-commit guard
.tidyall.d/cache/e/e1493a09988fdea2c47b8b41e181a4f0470fe22d.dat already exists, no checkout
Could not restore untracked files from stash entry
        (in cleanup) "git" unexpectedly returned exit value 1 at /home/bartosz/perl5/perlbrew/perls/perl-5.30.1/lib/site_perl/5.30.1/Code/TidyAll/Git/Precommit.pm line 98.

Lost changes are actually there, but I can't get to them because of that error. For this simple fresh repository case, the problem was the .tidyall.d getting in the way, so a simple gitignore for it was enough. Check out this question: https://stackoverflow.com/questions/51275777/why-does-git-stash-pop-say-that-it-could-not-restore-untracked-files-from-stash

For a more complex repository like the one I'm working on right now, the stash popping fails on the same file it restores after deletion, with the same message file already exists, no checkout. I'm having problems reproducing it in a clean environment though, will let you know if I figure it out

[bbrtj]

It got weirder pretty fast: I can reproduce it when commiting in the git tool I'm usually using (vim plugin - fugitive), but not in straight command line git. I'm not sure what's so special about the way fugitive is doing a commit, but seems like the git stash and restoring of deleted files is the root of the problem here. Not having tidyall.d in gitignore should make it reproducible without fugitive plugin, as it was for me at first. Do you have it in a global gitignore by any chance?

[autarch]

I have nothing configured for core.excludesfile.

The only other thing I can think of is that maybe fugitive is also doing something with the stash, and maybe that's causing issues?

[autarch]

As for not having .tidyall.d in the git ignore list, that's definitely an issue, and probably one that could use a documentation patch somewhere.

[bbrtj]

It struck me that I can't remember last updating fugitive. Turned out to be at the end of 2019... those 2500 changed lines fixed the problem, I no longer get stash errors while commiting. Whoops.

Of course it would be good to fix the deleted files being restored somehow, but if that's off the table then at least I'd suggest moving the documentation note about stashing from the check method section to description section, so it is more visible. I managed to overlook it when I was first looking for the reason my files got changed.

Anyway, thank you very much for your assistance Dave!

[autarch]

AFAICT the restoring deleted files is git working as intended. https://stackoverflow.com/questions/48446845/git-stash-restoring-index-state-of-deleted-and-renamed-files has a good discussion of this.

It might be possible to be extra clever and have the hook code figure out which files are deleted pre-stash and then re-delete them after, though that could be a bit dangerous.

[bbrtj]

Yeah it's probably better to leave it as it is. File deletion is not done often enough to justify possibly dangerous changes.

Pretty sure the documentation change settles it, the changes are in stash and can be restored even if the hook fails to do that. That should be hard to overlook right now