pre-commit can delete/revert unstaged files if error occurs during git diff-index

[strubbly]

search you tried in the issue tracker

diff-index

describe your issue

I performed a git commit with some modifications unstaged. After the commit, most of the modifications had been reverted and my work was lost. The diff saved in the patch directory had only a few of the modifications in - the ones that survived. The rest were gone.

To reproduce:

• Modify four files and stage one with git add
• Use git status to determine the order of the three unstaged files.
• Change the permission on the middle one so that git will not be able to read it
• Now do git commit: the changes to the first unstaged file will be preserved but the other two will be lost.

The key point, I think, is that the code in staged_files_only.py checks that the return code when creating the diff is non-zero which it takes to mean that the code is 1 meaning that there were diffs. However, in this case the return code is 128 which is non-zero but does not mean success - it means error. So the code assumes the diff is OK even though it is incomplete.

pre-commit --version

2.17.0

.pre-commit-config.yaml

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
  rev: 56b4a7e506901ff86f8de5c2551bc41f8eacf717
  hooks:
     -   id: check-yaml
#     -   id: end-of-file-fixer
     -   id: trailing-whitespace
- repo: https://github.com/psf/black
  rev: 21.11b0
  hooks:
    -   id: black
        language_version: python3.6
- repo: https://github.com/PyCQA/isort
  rev: 5.10.1
  hooks:
    -   id: isort
        args: ["--profile", "black", "--filter-files"]

~/.cache/pre-commit/pre-commit.log (if present)

No response

[asottile]

quite a weird case -- but I guess it makes sense. in the case of diff-index producing an error I think there's not too much pre-commit can do other than throwing that error back at the user and hoping they can figure out how to unbreak their checkout

[strubbly]

quite a weird case -- but I guess it makes sense. in the case of diff-index producing an error I think there's not too much pre-commit can do other than throwing that error back at the user and hoping they can figure out how to unbreak their checkout

Yes. I would much rather it didn't delete my morning's work and pretend everything is OK. Crashing out at that point is what is required.