[Feature] Move working tree changes prior to running pre-commit hook

[ramsey]

Use Case

Developers often use pre-commit hooks to run style-checkers that automatically fix style issues and add them to the staged commit before committing. This is problematic when the index contains changes to a file that also has changes in the working tree (that are not in the index). Unless we first move these changes out of the way, the pre-commit hook will see that changes exist in these files and try to add them to the index.

As a developer, I want any working tree changes to be moved out of the way before applying pre-commit hooks, so that these changes do not get added to the index by pre-commit hooks. After applying all pre-commit hooks, the working tree changes should be moved back to the working tree.

Discussion

You'll note that I didn't use the word "stash" in the use case description. This is because git stash does not effectively perform the operation we need. git stash [push] by itself adds everything in the working tree (including the index) to the stash. When using git stash push --keep-index, the current index remains active, but it is also stored to the stash along with the working tree changes. There's no way to stash only the working tree changes, which is what we need.

I've taken a look at various approaches for this, and I've landed on the following two popular tools:

• lint-staged
• pre-commit

After examining both and discussing pre-commit's approach with the maintainer on Twitter, I've decided to use pre-commit as a model.

In short, pre-commit's approach is to return a non-zero status if any pre-commit hook changes a file.

This may not be what everyone wants to see, however. Some folks might expect these changes to be added to the index and commited as part of the current commit. This is something we can allow, but I think the risk for unwanted changes being added to a commit is too great. It also increases the chances of a conflict occurring when we add the moved working tree changes back into the working tree.

Here's what I propose:

1. User enters git commit
2. Before running the pre-commit hooks, CaptainHook moves any working tree changes not staged out of the way (by creating a patch in a temporary file; this is how pre-commit works)
3. CaptainHook runs the pre-commit hooks

• For each hook, we compare the staged files to see if the hook made any changes
• If the hook made any changes, we fail the commit and print a message that contains the name of the hook and a diff of the changes it made

4. After all pre-commit hooks have run successfully, we apply the patch created in step 2 to the working tree

• There should be no conflicts at this point, since we denied pre-commit hooks from making any changes to the files
• But, in the event that conflicts are detected, we rollback any changes made by the pre-commit hooks, apply the working tree patch, and fail the commit

5. At this point, if all the pre-commit hooks were successful and no files were changed, Git proceeds with making the commit

By failing the commit if any hook makes changes, we require the developer to analyze the reasons for failure, fix them, stage any new changes, and try the commit again.

Thoughts?

Implemention

I've begun stubbing out an implementation, so I can see if this is a workable solution. So far, it looks good. I'm opening this issue to get feedback on the approach before I continue with more development. The Status operator PR (sebastianfeldmann/git#23) on sebastianfeldmann/git is one of the tools I needed to make this work in CaptainHook.

[sebastianfeldmann]

So the idea is to prevent pre-commit hooks to commit unwanted changes to the repository.

That would mean that we had to configure an additional pre-commit hook that cleans up the working tree before any other action just as you described.
If we removed any changes from the working directory we have to make sure no other action changes anything in the index otherwise re-applying the removed changes could potentially fail.

I'm not yet sure how you want to detect those potential changes but if it is doable I think it is a valid safe guard to implement.