refactor: use native git stashes in git workflow

[iiroj]

This PR overhauls and simplifies the behaviour of the git workflow to work like this:

When run, lint-staged will:

1. Create a stash using git stash save "temporary lint-staged stash".
2. Apply that stash back using git stash apply --index. This will "remember" staged files from other modifications. We are now back to square one, but we have still have the stash.
3. Run all the linters, possible modifiying files, possibly throwing errors
4. If there are linting errors:
   1. Do a hard reset using git reset --hard to clear everything.
   2. Restore temporary stash, going back to square one. This will in effect always delete all possibly-modified files during the linter step, if there's a failure
5. Drop the temporary stash

This seems very simple and performs fast. If I understood the workflow correctly, this is the wanted behaviour. Besides, if anything fails before the final step, the stash will be left for the user to revert.

I didn't touch the tests yet, because I wanted to open this PR for discussion. I did try this out on our large monorepo, where it performs correctly as in the git state remains unchanged after commits, and in the case partial commits fail.

Thoughts?

[okonet]

I believe it's missing the step 6 from #75

Compare new commit index and working copy and apply as many formatting changes to working copy as possible. If there are conflicting hunks, drop formatter's changes. User modifications that were stashed should always take precedence over formatters.

That's the crucial part of it that took me 2 years to realize and implement. And I'm not sure it's possible to achieve with git stash.

I mentioned before I'm open to any suggestion that would keep tests green. Please update tests in order to see if some are broken and we can discuss further from there. How does that sound?

[iiroj]

Do you mean that in the case of linter failures during formatting changes, the changes should be left in the working copy?

This branch will always revert any changes during error, and return to the identical status before committing. Maybe I'm misunderstading the current behaviour, but I did see I was only running the stash/pop with partial files (should run always).

[okonet]

Not quite: the step 6 is for the case when there are no errors during formatting but the contents did change. Since there is a new content in files that were stashed git stash apply will lead to a conflict state on the working copy tree.

See https://github.com/okonet/lint-staged/pull/633/files#diff-cf453db1a0c4f207753345e32ef28aa3L121 for the explanation on how it works.

[okonet]

I think easiest way is to do the refactoring without changing function names that are being used in the runAll so you can see what tests are going to fail.

[iiroj]

Maybe this branch then completely circumvents that issue, since in this case the stash is only applied before running linters (but not removed, since we use apply instead of pop).

If there are no errors, the process will just remove the stash but not try to do anything else. This might actually change behaviour in the sense that running git add in the linter script shouldn't be necessary.

EDIT: git add is still necessary because the changes wouldn't otherwise be staged.

[iiroj]

I ”fixed” most of the tests. There’s a lot of tests that mostly test the old stash/pop functions, but this method doesn’t really rely on those so they are not needed.

Overall things seem to work around the same considering the start and end results. Maybe this needs some new tests, but first we should validate the logic is the same before investing into those.

What do you think?

[codecov]

Codecov Report

Merging #633 into master will increase coverage by 0.6%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #633     +/-   ##
=========================================
+ Coverage   98.14%   98.75%   +0.6%     
=========================================
  Files          14       14             
  Lines         378      321     -57     
  Branches       51       44      -7     
=========================================
- Hits          371      317     -54     
+ Misses          7        4      -3

Impacted Files	Coverage Δ
src/gitWorkflow.js	100% <100%> (+3.07%)	⬆️
src/runAll.js	100% <100%> (ø)	⬆️
src/index.js	95% <0%> (+2.5%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e332c80...2942e1f. Read the comment docs.

[iiroj]

Alright, so the tests should now be green. I renamed the tasks in runAll to better reflect the new behaviour:

1. Backing up local changes...
2. Running linters...
3. (if errors) Restoring local changes...
4. Clearing temporary backup...

I figure there are 8 different scenarios/combinations of behaviour that should be tested:

1. Stage entire file, run linter with no errors and no further modifications

   • From state A, should commit state A

2. Stage entire file, run linter with no errors but with further modifications

   • From state A, linter creates state B, should commit state B

3. Stage entire file, run linter with errors and no further modifications

   • From state A, should abort and return to state A

4. Stage entire file, run linter with errors and with further modifications

   • From state A, linter creates state B, should abort and return to state A

5. Stage partial file, run linter with no errors and no further modifications

   • From state A, should commit state A

6. Stage partial file, run linter with no errors but with further modifications

   • From state A, linter creates state B, should commit state B

7. Stage partial file, run linter with errors and no further modifications

   • From state A, should abort and return to state A

8. Stage partial file, run linter with errors and with further modifications

   • From state A, linter creates state B, should abort and return to state A

Maybe if I rewrite the gitStash.spec.js to handle all these cases?

[iiroj]

I've added all these 8 test cases in a new test at test/gitWorkflow.spec.js (moved old test to test/execGit.spec.js).

I think this nicely demonstrates that the refactor handles failure cases nicely, and even preserves old edits and staged/unstaged differences.

What are your thoughts, @okonet?

[iiroj]

I see now in case 6, that even though I have staged a partial file, since the command is [prettier --write, git add], it will add the entire file and commit it, including unstaged change.

Is this the same behaviour as currently?

[okonet]

No, and that’s exactly the trick here. We only want to add modifications for staged hunks and not everything.

I think my tests had all those scenarios handled, aren’t they?

Also, when you mean state A, for partial staged files we have to consider working copy changes as well.

I’m reading the code now so I can give you more feedback.

[okonet]

I checkout our the code and the change surface is just too big to grasp for me know. Would you consider going back to my test suite for now and leave copy changes out until we have a working solution, please? That would help me better understand your changes. Pretty please?

Edit: I was able to comprehend your changes but some tests were "fixed" improperly. I left a comment.

[okonet]

That's incorrect since this should be now "fixed"

[iiroj]

I'll revisit this with the logic of only linting stashes, but might be that this method does not work after all. 😅 In that case I'll close this PR.