Wp-calypso: Do not modify the staging area on commit

[sirreal]

When using the new /** @format */ facility to format files on commit, _all changes in staged files will be wholesale commited_.

Within pre-commit-hook.js is the following, which _adds these files to your commit_:

execSync( `git add ${ file }` );

I do my best to keep a clean and coherent commit history, and with the power of git I can usually do a pretty good job. That doesn't mean the way I actually build things is just as clean. It usually isn't.

That means I often patch my commits together with git add -p or git reset -p until I feel that they are coherent and represent a logical step.

Here are two commits that I crafted recently, only to have my care and attention to detail ignored by the pre-commit hook:

• 154605862deb5284aa15ac5319123bc407b3c4e8
• f6bc33a3bbe6c432dbc3f8418be63c607965dc7f

I feel pretty strongly that native git behavior should not be significantly modified by our commit hooks, and we violate that right now.

The hook was introduced in #16394

Steps to reproduce

1. Open a file with /** @format */ directive.
2. Make 2 changes in the file.
3. Stage one of the changes (git add -p).
4. Commit git commit -m 'test'.
5. Notice that the file is now clean because all of the changes were committed.

What I expected

Staged changes are committed, unstaged changes are not.

What happened instead

Complete files are committed, regardless of staging.

/cc @samouri

[sirreal]

I've been loving prettier, but I found this to be very irritating.

An elegant solution may be to use the clean filter.

[sirreal]

A simple and less elegant solution might be git stash before pretty + add + commit and git stash pop after.

[samouri]

Thanks for opening up and researching this issue @sirreal!

I agree we shouldn't be breaking any git flows with our pre-commit hooks. I know that integrating prettier into pre-commit hooks is a very popular thing to do, so I'd expect others to have run into the same issue.

[samouri]

I don't think partially staged gitting is a very popular workflow, so I haven't found many references to people running into this issue. It seems to be mostly ignored.

I did notice one person suggesting the same proposal you made about using git stash.

Want to spin up a PR and I can test it?

[sirreal]

Want to spin up a PR and I can test it?

I am hoping to propose something for this, just need to find the time for it. Glad you're aware of it too so you can keep your eyes and ears open 🙂

[sirreal]

I have not yet found a good solution. I played with stashing and clean filter concepts, but hit complications pretty quickly. @jsnajdr just proposed a very nice solution, we can check whether the file is "dirty" with unstaged changes or "clean" with all changes staged. In the dirty case, we the abort before adding and perhaps formatting as well.

[asottile]

fwiw, our framework handles this nicely -- the "clever" bits are here

[sirreal]

@asottile Thanks for the input! I'll have a look to see if we can borrow something 🙂

[jsnajdr]

I use the partial staging workflow quite often and I've run into this issue several times. After when we add @format to all our JS files, partial staging will be effectively disabled, which is sad.

One solution is to stash before formatting and committing and unstash after, as @asottile suggested. There's one thing that worries me about this: after formatting, will the stashed changes still apply cleanly? How often will I end up with stashed changes that have conflicts when applied to the formatted code? I'm afraid that could often disrupt the workflow.

Another solution is to skip formatting files that have both staged and unstaged changes. Either with just a warning, or even by aborting the commit and forcing me to do commit -n. This relies on an observation that the unstaged changes are usually commited just a moment later, in the next commit. Then we can do the formatting only in the last commit, the one that doesn't leave anything unstaged, can't we?

The upside of this solution is that it never disrupts the workflow. The downside is that unformatted changes can be committed if the assumption about all changes getting eventually committed doesn't hold.

It would be very interesting to investigate git's clean and smudge filters. Maybe they have some elegant solution? I had no idea that such a feature even exists before I read @sirreal's comment today.

[asottile]

fwiw pre-commit takes the most timid approach here: never modify the
staging area (no git add) and on format conflicts with stash, prefer the
stashed files.

I think this is probably a product of developing for least surprise:

• give the user a chance to inspect changes, software has bugs and hooks
  are no exception
• worst case they end up where they started with no changes (even then, let
  the user know why they're back to square one such that they can manage
  their stash / add for the next commit)