Add better support for partially staged files

[sedubois]

See react-boilerplate/react-boilerplate#1064 (comment) for description

[okonet]

Partial staging bugging me a lot but I don't know how to solve this. If you know how to solve that issue, you're welcome to re-open the issue and file a PR. Otherwise I'll close the issue as won't fix.

[sedubois]

Sure 😊 I'm not familiar with the mechanism, so I don't have a solution to propose right now.

[sedubois]

Why not simply temporarily doing git stash -k?

[okonet]

Can you elaborate on ↑ since I'm not familiar with how it works.

This sounds interesting and is worth trying. Thanks for pointing it out.

[okonet]

@sedubois I'm a bit stuck with #75. Do you have ideas how to solve it probably?

[sedubois]

Hmm. Maybe first concentrate on the simple case when linter doesn't modify the working copy, and address the case of eslint --fix separately?

Also you proposed in PR to git stash pop after the commit, but conflicts would also happen in that case, no? Sounds like the conflict is unavoidable and the user should then be given the opportunity to resolve it?

[okonet]

Also you proposed in PR to git stash pop after the commit, but conflicts would also happen in that case, no?

I don't think it will. The conflict will only happen if the actual content has diverged from stashed one. Otherwise, it will successfully merge them. So this use case can be solved with the post-command.

address the case of eslint --fix separately

I really don't want to loose this feature. So if there is a way of not losing it, I'd take this effort. So for now this is the only issue that prevents me from releasing it (besides of some code that needs to be written and the cleanup).

[okonet]

For now, the second case doesn't seem to be solvable without a user interaction which sucks :(

If someone has a better solution for that, any ideas are appreciated!

See #73 for some more context.

[sedubois]

Well, if you stash then allow linters to perform changes, conflicts might happen. Either don't stash, or disable the linter --fix option. Maybe you can let the user choose what they prefer.

Personally I would just disable the --fix option, so that I can use stash -k. Otherwise, lint-staged's behavior is simply incorrect (it lints code which isn't meant to be committed).

[okonet]

These aren't solutions but workaround of the problem :(

To add git stash you don't need to change anything in the library itself. You can just add those to the config:

{
  "*.js": ["git stash -k", "eslint", "git stash pop"]
}

[sedubois]

Oh that's nice, I'll give that a try. Should it be documented?

But how is that a problem? Isn't it normal to ask for user input, if a merge conflict occurs?

[okonet]

Well, technically, yes :) But in this case, I don't think so since the changes are coming from eslint and this will result in annoying user prompts.

BTW: I agree this should be backed into the library but the --fix case is just too important to me.

[sedubois]

Actually, didn't manage to get this working:

 ❯ Running tasks for *.js
   →    or: git stash clear
   ✖ git stash -k
     →    or: git stash clear
     eslint
     git stash pop

🚫  git stash -k found some errors. Please fix them and try committing again.

usage: git stash list [<options>]
   or: git stash show [<stash>]
   or: git stash drop [-q|--quiet] [<stash>]
   or: git stash ( pop | apply ) [--index] [-q|--quiet] [<stash>]
   or: git stash branch <branchname> [<stash>]
   or: git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
                       [-u|--include-untracked] [-a|--all] [<message>]]
   or: git stash clear



pre-commit: 
pre-commit: We've failed to pass the specified git pre-commit hooks as the `lint-staged`
pre-commit: hook returned an exit code (1). If you're feeling adventurous you can
pre-commit: skip the git pre-commit hooks by adding the following flags to your commit:
pre-commit: 
pre-commit:   git commit -n (or --no-verify)
pre-commit: 
pre-commit: This is ill-advised since the commit is broken.
pre-commit: 

[okonet]

Hmm, it's probably since I'm always passing paths to the command 🤔

[kmiyashiro]

The readme mentions that as of 3.1, lint-staged stashes un-staged changes. This is apparently not the case?

[okonet]

@kmiyashiro yes, it seems it slipped into the Readme. I'll remove it.

[leonaves]

So, while I don't know what the approach would be for getting this working in lint-staged, I think conflicts are always going to be an issue, i managed to get this working using pre-commit and some git trickery, with a lot owed to this answer on stack overflow.

My partial package.json:

  "lint-staged": {
    "*.js": [
      "eslint",
      "jest --bail --findRelatedTests"
    ],
    "*.scss": "stylelint --syntax scss",
    "*.css": "stylelint"
  },
  "scripts": {
    "lint-staged": "lint-staged || (yarn run pop-stash >> /dev/null && exit 1)",
    "stash-unstaged": "git stash save -k 'pre-linting-stash' >> /dev/null",
    "pop-stash": "git stash && git stash pop stash@{1} && git read-tree stash && git stash drop",
  },
  "pre-commit": [
    "stash-unstaged",
    "lint-staged",
    "pop-stash"
  ],

This works by first stashing the unstaged hunks, as above, then running the lint-staged script, which will pop the stash if it fails, and return a non-zero exit code to ensure it fails the overall pre-commit hook. Lastly it uses the trick in the aforelinked stackoverflow answer to safely return the stashed changes.

Hope this helps someone!

[okonet]

@leonaves that's awesome and I'm gonna try it out. Did you see @75? This is pretty much the same what I was trying to implement but I could not come up with the right command to stash pop.

Also, how does this work in case of aborted commit?

Does it also works with updating the code (eslint --fix)?

[leonaves]

@okonet Ah cool, yeah that stack overflow answer really helped me, it's a clever sequence of git commands, I don't think I would got there myself in the end.

With regards to the fix flag, I've been playing with it this morning, and it's actually a really tough problem, that I'm not sure will be possible cleanly or without some user interaction.

Say I have the following code:

export default function() {
  return "My hello world module";
}

I stage this and then add the following line:

export default function() {
  console.log("I don't want this committed just yet");
  return "My hello world module";
}

If I run the steps from my previous comment with eslint --fix and git add . as well, what happens is this:

• The unstaged hunk is stashed, leaving us with this again:

export default function() {
  return "My hello world module";
}

• eslint --fix automaticaly fixes the double string issue, leaving us with this:

export default function() {
  return 'My hello world module';
}

• git add . then stages that change as well, so that the above fixed file in its entirety is in the index.
• We stash the current index, leaving our working tree clean.
• We apply the previously unstaged changes from the stash stack, giving us this unstaged:

export default function() {
  console.log("I don't want this committed just yet");
  return "My hello world module";
}

• We read from the top of the stash into the index, giving us this in the index:

export default function() {
  return 'My hello world module';
}

• That will be committed, which is correct, however once it's committed, we will have a dirty working tree with the following:

export default function() {
  console.log("I don't want this committed just yet");
  return "My hello world module"; // This fix has been undone.
}

I don't think the end result is achievable, without also applying a linting fix to the console.log line, which to be honest, would probably be acceptable to pretty much everyone, as long as that line is not added to the index. But you would have to do some inefficient hoop jumping to do so:

• Before stashing anything, apply eslint --fix and swallow the exit code (we don't want this aborting the commit if it fails as unstaged hunks might have unfixable linting errors that aren't the problem of the current commit).
• Stash the unstaged changes (this would stash all the fixes just applied to the working tree).
• Run eslint --fix again, this time not swallowing the exit code (unfixable errors in the index should abort the commit).
• Stash the entire working tree + index.
• Apply the first stash onto the working tree with git stash apply @{1}.
• Read the second stash into the index with git read-tree stash.
• Drop the last stash on the stash stack.

Now your working tree should contain all of the same fixes as your index, as we applied eslint --fix before stashing the first time, and it will also have some fixes of it's own, albeit unstaged. These can then be either checked out or applied to the next commit. The main downside I see to this is that we are running eslint twice, but so far I can't see another way to do it, again, unless you apply the stashes to the working tree and have the user deal with conflicts.

Anyway, sorry this turned out so long, but hope it helps.

[AndersDJohnson]

Also consider adding the the -u flag to git stash save -k so we also stash any untracked files such that they don't cause any lint failures unrelated to the staged files. Refer to: https://git-scm.com/docs/git-stash#git-stash-save-p--patch-k--no-keep-index-u--include-untracked-a--all-q--quietltmessagegt.

[okonet]

How would they if we only lint staged files?

[AndersDJohnson]

@okonet True, in most cases untracked files wouldn't be picked up, unless the scripts being run don't support accepting a list of files as arguments, instead running on the entire directory anyway.

[alexilyaev]

This works really well, really appreciate the hard work!
Tested partial commits (hunks) and also when a linter fails everything is restored properly and not lost.

I'm curious if the note in the README about "Automatically fix code" is still relevant:

Automatically fix code style with --fix and add to commit

{
  "*.js": ["eslint --fix", "git add"]
}

This will run eslint --fix and automatically add changes to the commit. Please note, that it doesn’t work well with committing hunks (git add -p).

[okonet]

@alexilyaev good catch! Mind PRing the fix?

[alexilyaev]

@okonet #553

[asottile]

just a heads up since it came up in pre-commit -- I suspect you'll have issues using git stash with git add --intent-to-add files: https://marc.info/?l=git&m=154654843113132&w=2 (pre-commit doesn't use git stash)

not sure if it breaks lint-staged, I didn't try

[Mouvedia]

@asottile thanks for the heads-up. @okonet could you try it out?

[okonet]

We are also not using git stash. I can’t try since I lack time. If someone can create a test case that would be great.