Add better support for partially staged files about lint-staged HOT 151 CLOSED

Why not simply temporarily doing git stash -k?

from lint-staged.

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!

from lint-staged.

I could fail the whole hook in that case and say that fixes can't be applied to a partially staged file and leave it up to you.
Does anyone think that would be acceptable?

@okonet I think it's acceptable, and actually desirable (to fail the whole hook).

DX: You've gone to all the trouble to git add --patch, when you commit an unexpected thing happens, any files with hunks staged are added in their entirety. I think a failure is more predictable than adding the whole file silently (well it appears in the diff, but it's easy to miss).

from lint-staged.

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.

from lint-staged.

I'd love to see something like this:

1. stash all changes that are not staged
2. do something that modifies files that have staged hunks (e.g. prettier, eslint)
3. stage any new modification of those files
4. pop all stashed changes

if 4. fails due to confllicts, abort commit and revert everything back to what it was before 1. and tell the user to fix things manually.

from lint-staged.

Who wouldn’t :) Any help us appreciated. See #75

from lint-staged.

@okonet @julien-f having some experience with that strategy, it does work well, you'll want to instead use the following commands (more robust):

from here

git diff-index --ignore-submodules --binary --exit-code --no-color --no-ext-diff $tree --

and for apply (from here)

git apply --whitespace=nowarn patch ||
    git -c core.autocrlf=false apply --whitespace=nowarn patch

If you want a testsuite for this, here's a pretty comprehensive one: https://github.com/pre-commit/pre-commit/blob/master/tests/staged_files_only_test.py

from lint-staged.

@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.

from lint-staged.

from lint-staged.

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).

from lint-staged.

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).

from lint-staged.

I'm currently actively working on the implementation for it in #75 and looks like my tests just got passed so there is some hope. I'll need to add more tests, though.

The solution I might end up will be a combination of all mentioned approaches.

from lint-staged.

pre-commit takes the stance that 3. above is unacceptable and surprising :)

from lint-staged.

pre-commit seems to handle the partially staged files pretty well. What exactly are they doing differently?

from lint-staged.

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: 

from lint-staged.

http://pre-commit.com handles this (and with fixers) - - you may find the strategy used there useful.

from lint-staged.

Hey @asottile! I'll definitely ping you for some help. I was referring to the problem that you'll remove the fixes but I see now why it is a better solution. Thanks for chiming in. Please take a look at the git workflow branch if you have time and let me know what you think.

from lint-staged.

I got it working with:

  scripts: {
    // ...
    "precommit": "lint-staged",
    "lint-staged-stash": "git stash save --keep-index 'lint-staged' && touch .didstash || rm .didstash || true",
    "lint-staged-stash-pop": "test -f .didstash && rm .didstash && git stash pop || true"
  },
  "lint-staged": {
    "*.js": ["lint-staged-stash", "eslint --fix", "git add", "lint-staged-stash-pop"]
  },

but I think I'm going simplify and disable auto-fixing, assuming people have it in their editor already - just throw an error on lint fail.

from lint-staged.

@okonet The workflow of git-exec-and-restage is stated here. I will explain my example in details. Now we use the fullystaged.js and partiallystaged.js as example. Say there is two hunk, namely 1 and 2 of partiallystaged.js.

So what will happen after we run

git add fullystaged.js
git add -p partiallystaged.js # y for first hunk and n for the next hunk
git commit

The pre-commit hook runs as:

1. git-exec-and-restage finds out that both fullystaged.js and partiallystaged.js is staged in this commit.
2. run linter/formatter on both fullystaged.js and partiallystaged.js
3. run git add fullystaged.js

After the hook runs, git commit will continue.

In this example, the final commit HEAD will include:

a. a formatted version of fullystaged.js
b. a non-formatted version of partiallystaged.js

The working directory will include:

a. a formatted version of partiallystaged.js

If we later fully stage partiallystaged.js, everything is good and well formatted after we commit the fully-staged partiallystaged.js.

However, if we run git checkout -- partiallystaged.js, the git repository will have partiallystaged.js non formatted and never format unless we later revise partiallystaged.js and fully stage.

The lump-sum result is devs can have a unformatted version of partiallystaged.js in some commit. But like I said before, they can also circumvent pre-commit hook by git commit -n.

As pre-commit is never 💩-proof since git commit -n, I think it is acceptable to align lint-staged with git-exec-and-restage workflow.

from lint-staged.

What I really want is something like eslint --fix=exit-nonzero where if any fixes occur it does them (and writes them out) but exits with non-zero exit code so that I then have to add the changes manually and commit again. Is anyone aware of something like this?

@benjie suggestion: run eslint first, if this fails run eslint --fix and exit with an error code :)

from lint-staged.

@julienw Thanks for the suggestion, but that'l double the lint time which is already too long :(

from lint-staged.

Here is my current approach, it works quite well except that the files are not unstashed on errors:

{
  "scripts": {
    "lint-staged-stash": "git diff > .git/lint-staged.diff && git checkout . && true",
    "lint-staged-unstash": "patch < .git/lint-staged.diff && rm -f .git/lint-staged.diff && true",
    "precommit": "lint-staged"
  },
  "lint-staged": {
    "*.js": [
      "lint-staged-stash",
      "eslint --fix",
      "jest --findRelatedTests",
      "git add",
      "lint-staged-unstash"
    ]
  }
}

from lint-staged.

@julien-f do you think if we would incorporate this approach to the lib and do unstash in case of error would solve it? Do you mind creating a PR so I can test it?

from lint-staged.

My colleagues and I had several times that unstaged files got committed, so I've decided to try out the solutions @julien-f and @asottile suggested.

Notes:

• The && true at the end is important, since lint-staged adds the staged files paths at the end of the given command (30 min wasted cause I was a smart-ass and removed those, bah).
• git-apply seems to be better than patch
  (ref https://www.lullabot.com/articles/git-best-practices-upgrading-the-patch-process)

  apply has two key differences from patch. First, it will not apply a patch if you have other uncommitted changes in your code. The other significant difference is that by default, git apply will not apply a patch that does not apply cleanly...

• If we have several glob patterns, we need to add "concurrent": false, and put the settings under "linters" (see example below)

Questions @asottile

• How do I use git diff-index? What do I put in place of $tree?
  I couldn't get it to work
• Why git diff-index vs git diff?
• Why --exit-code? It exits with 1 if there is a difference
• Do we really need --no-ext-diff?
• In what case we'd need git -c core.autocrlf=false before the apply?

Working setup example

  "scripts": {
    "lint-staged-stash": "git diff --ignore-submodules --binary --no-color > lint-staged.diff && git checkout . && true",
    "lint-staged-unstash": "git apply --whitespace=nowarn lint-staged.diff && rm -f lint-staged.diff && true",
    "precommit": "lint-staged"
  },
  "lint-staged": {
    "concurrent": false,
    "linters": {
      "*.js": [
        "lint-staged-stash",
        "eslint --fix",
        "prettier --write",
        "git add",
        "lint-staged-unstash"
      ],
      "*.scss": [
        "lint-staged-stash",
        "stylelint --syntax scss --fix",
        "git add",
        "lint-staged-unstash"
      ]
    }
  },

This worked, and already much much better than silently adding and committing unstaged files.
If lint-staged did this behind the scenes and just documented how to apply the patch on failure, it would be great. But as @okonet mentioned, it would be possible to catch the error and apply the patch any way.

from lint-staged.

The main difference is lint-staged runs git add without prompting. pre-commit takes a more timid stance on this (hooks after all are written by humans and therefore can have mistakes) and leaves it up to the user to ensure changes made by hooks are sane and committable. As a user, I'd be quite surprised if a tool was running git add on my behalf :)

EDIT: To elaborate on that, consider the worst case: a malicious hook which writes modifications and gets automatically added to the commit :)

from lint-staged.

@asottile the case I mentioning is a different one. What I'm trying to do is this

1. stash local changes
2. run hooks with fixes
3. if all hooks return 0, add fixes to index
4. apply the local changes back

This is covered by this test: https://github.com/okonet/lint-staged/pull/75/files#diff-f5002743e0b16c70d5bc43040f1d3847R257

Now I'd like to do the same but with partially staged file. This is where it gets more complicated and it seems to be a no reliable way to implement it. Which makes me think if a compromise solution should be accepted in this case: for example, I could fail the whole hook in that case and say that fixes can't be applied to a partially staged file and leave it up to you.

Does anyone think that would be acceptable?

from lint-staged.

@karlhorky not really. It might help with Prettier but afaik no other tool support this and lint-staged is being used with eslint and stylelint (both support --fix). Also there is #366 which is exactly about that.

from lint-staged.

My current approach: https://github.com/vatesfr/xen-orchestra/blob/master/scripts/lint-staged

1. compute the list of files with unstaged changes
2. format all files
3. save in memory the files with unstaged changes and restore them
4. reformat them
5. run the tests and add the files if no errors
6. restore the files with unstaged changed to their saved state

from lint-staged.

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

from lint-staged.

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.

from lint-staged.

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

from lint-staged.

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?

from lint-staged.

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.

from lint-staged.

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"]
}

from lint-staged.

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?

from lint-staged.

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.

from lint-staged.

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

from lint-staged.

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

from lint-staged.

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

from lint-staged.

@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)?

from lint-staged.

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.

from lint-staged.

How would they if we only lint staged files?

from lint-staged.

@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.

from lint-staged.

As a heads up, I've been using this technique for a few days now, and for some reason the read-tree command fails after a merge with conflicts with the following error:

fatal: could not open '.git/MERGE_HEAD' for reading: No such file or directory

Running git commit a second time makes the commit work, not sure why yet.

from lint-staged.

@leonaves thanks for the heads up! I'm playing with it in the https://github.com/okonet/lint-staged/tree/git-worflow branch so feel free to check it out and help.

from lint-staged.

Interesting! Will check it out but it seems they also doesn't manage conflicts from autofixes.

https://github.com/pre-commit/pre-commit/blob/master/pre_commit/staged_files_only.py

from lint-staged.

It does: https://github.com/pre-commit/pre-commit/blob/1be4e4f82e31336fa5fca096c962c72ac0041537/pre_commit/staged_files_only.py#L50-L58 -- on conflict, the autofixes are rolled back and the unstaged changes are re-applied

from lint-staged.

Disclaimer: I wrote and maintain pre-commit/pre-commit -- if you have any questions I'd be happy to answer :)

from lint-staged.

@okonet I fixed some problems in @leonaves 's code in my project. I put a PR in react-starter-kit.

Problems

Problems fixed:

• when there's nothing to commit and your stash list is not clear, pos-stash would pop a stash you don't want
• when you press Ctrl+C during commit, pop-stash would not pop for you, you can only manually do that
• when there's nothing to commit after lint-staged, commit should be stopped

Solution with npm scripts and bash scripts

  "scripts": {
    "stash-unstaged": "git stash save -k --include-untracked 'unstaged-stash' >> /dev/null",
    "pop-if-gst-clean": "./tools/pop-if-gst-clean.sh",
    "lint-staged": "lint-staged || (npm run pop-stash >> /dev/null && exit 1)",
    "pop-if-gd-clean": "./tools/pop-if-gd-clean.sh",
    "stash-linted": "git stash save 'linted-stash' >> /dev/null",
    "stash-pop-unstaged": "git stash pop stash@{1}",
    "stash-pop-linted": "git read-tree stash && git stash drop",
    "clean-lint-staged": "lint-staged >> /dev/null"
  },
  "//pre-commit": {
    "//reference-links": {
      "github": "https://github.com/okonet/lint-staged/issues/62#issuecomment-286830310",
      "stackoverflow": "http://stackoverflow.com/questions/13889242/26685296#26685296"
    },
    "//stash-pop-linted": "use `git read-tree` to resolve conflicts",
    "//clean-lint-staged": [
      "Re-run `lint-staged` for fixable style errors mistakenly recovered to be unstaged by `read-tree`",
      "NOTICE: Although they are fixed, you will see them when input a commit-message",
      "Side effect is it will auto-fix unstaged part in these staged files",
      "Feel free to remove this cmd if you don't want the side effect or just feel adventurous",
      "If so, don't be surprised when you see unstaged&fixable style errors after commit"
    ]
  },
  "pre-commit": [
    "stash-unstaged",
    "pop-if-gst-clean",
    "lint-staged",
    "pop-if-gd-clean",
    "stash-linted",
    "stash-pop-unstaged",
    "stash-pop-linted",
    "clean-lint-staged"
  ]

Help wanted to make it into lint-staged

But like they said,

the solution is too complex to be used in multiple projects and also contains cross-platform issues like does not work on windows

I'm not familiar with cross-platform issues. Can you fulfill this in this repo?

from lint-staged.

@asottile I didn't find any info loss in this solution(not mean git stash) until now. Can you name it more specificly?
Considering it's just partially stash and stash pop, I think it's quite safe.

from lint-staged.

Notably:

• git stash + conflicts
• git stash + ^C
• git stash + ^\

popping a stash which conflicts causes the stash to be dropped and the changes evaporating to the aether

from lint-staged.

@asottile I think we should consider it in this lint partially stage situation

• git stash + lint + pop => conflicts -- not gona happen using git read-tree
• git stash + lint + ^C -- use must run git stash pop by hand
• git stash + ^\ -- I don't understand this one...

from lint-staged.

^\ sends SIGQUIT (it's like ^C but with moar power)

SIGINT (^C) is catchable so you should really try to undo things when it happens
SIGQUIT (^) is unavoidable in either case so it's really the user's fault if they use this heavy of a sledge hammer (but at least it won't clobber their private state)

pre-commit avoids this by maintaining its own patch file separate from the (full of spiders) git stash stack

from lint-staged.

There's one low-hanging fruit I can see here, in the form of automatically restaging fully-staged files after --fixing them. Assuming most files in most commits are fully staged, this is a significant win - one only ever needs to manually amend a commit if there's a lint fix in a partially staged file.

With this in mind I created git-exec-and-restage, which can be configured in package.json as seen below. However, if there's any interest in building it directly into lint-staged (behind a flag?) I'd love to put together a PR.

{
  "scripts": {
    "precommit": "lint-staged"
  },
  "lint-staged": {
    "*.js": ["git-exec-and-restage eslint --fix --"]
  }
}

from lint-staged.

@motiz88 I think this is amazing! I'd love to integrate this into the core to make the setup even simpler. I think this could be the default behavior. Thoughts?

from lint-staged.

@okonet Still can't do partially commit and fix well

@motiz88 what's the equivalent cmds of your cmd git-exec-and-restage eslint --fix --?

from lint-staged.

@Stupidism

git-exec-and-restage eslint --fix -- fullystaged.js partiallystaged.js would run the following:

1. git diff --name-only --diff-filter=ACDMRTUXB fullystaged.js partiallystaged.js (any files returned by this command are not fully staged, so will be excluded in step 3)
2. eslint --fix fullystaged.js partiallystaged.js (run the requested command on all files)
3. git add fullystaged.js (add only the files that started out as fully staged)

Have a look at the test suite - The corresponding snapshots show exactly which commands are run in each case.

from lint-staged.

Thanks for this solution @motiz88, it helps a lot!

from lint-staged.

@benjie I think conflicts would occur if file changed by eslint --fix

from lint-staged.

@Stupidism Yes, they do in rare cases though they're generally easy to resolve from my testing. Nonetheless I use patch adding enough that I'm just going to use lint-staged as a warning rather than automatically adding the fixes - I like to know exactly what's going into my git history!

What I really want is something like eslint --fix=exit-nonzero where if any fixes occur it does them (and writes them out) but exits with non-zero exit code so that I then have to add the changes manually and commit again. Is anyone aware of something like this?

from lint-staged.

One idea I had in my hard is to have a postcommit hook with fixing and creating a fixup commit. But I don't know how to do this in a reliable way. Does anyone want to explore this?

from lint-staged.

@benjie http://pre-commit.com does exactly that :)

from lint-staged.

Inspired from @motiz88 's git-exec-and-restage, could we add a fullyStagedOnly: boolean flag to the configuration so that

1. when fullyStagedOnly is true, run lint only on fully staged files specified by linters
2. when fullyStagedOnly is false, no extra behavior is introduced. This is the default behavior.

As prettier does not (and will not ever I think) support linting chunks, partially staged files will always be a headache for lint-staged on a prettier + git add workflow. If fullyStagedOnly is supported, prettier + git add will become secure since git add would never add unstaged changes.

The drawback of fullStagedOnly will be the case when a dev can commit partially staged files and then discard local changes to circumvent lint-staged. Anyway the dev could pass any git hooks by git commit -n so I don't think it will hinder this fullStagedOnly workflow.

Further discussion is definitely welcomed.

from lint-staged.

As prettier does not (and will not ever I think) support linting chunks

It supports ranges today.

https://github.com/prettier/prettier/blob/3f6a232cea56f60f1ba50cbae2d56f78b36a7699/README.md#range

from lint-staged.

It supports ranges today.
https://github.com/prettier/prettier/blob/3f6a232cea56f60f1ba50cbae2d56f78b36a7699/README.md#range

Good catch!

It seems that the range option is mostly suited for editor integration. If one uses range to lint partially staged files, prettier is expected to return the formatted line numbers for git partially staging. Otherwise git add will still add unstaged hunks.

from lint-staged.

@JLHwung I'm wondering if I could just integrate git-exec-and-restage and is it by default, what would be the drawbacks?

from lint-staged.

@okonet If git-exec-and-restage is enabled by default. There will be a case where devs will NOT have a staged file linted and they might be even unaware of that.

For example, say we have a.js and b.js revised. Say b.js is revised between line 118-128 and line 138-148. The first time we stage a.js and line 118 - 128 of b.js and the linter will run on a.js only. Now the second time we find the line 138 - 148 is error-prone so we decide simply discard the changes between line 138-148. In this case, b.js is never linted and the only way to trigger the linter is revised b.js again.

Other than this case where a partially staged file become actually fully staged via the succeeding operations, it seems git-exec-and-restage can be a safe default. Anyway it is a breaking change and should be well documented if enabled.

from lint-staged.

@JLHwung I'm not sure I completely understood your example, TBH. I believe git-exec-and-restage will run on both files but if you stage just a hunk of b.js, the second change will be discarded during the run of linter, so you'll lint/process only this hunk. After it's done, git-exec-and-restage will revert the second change in b.js so when you run pre-commit on it, it will be linted again. What am I missing?

It will be a breaking change for sure!

from lint-staged.

Thanks to lint-staged, it's not so long, but yeah I quite missed this would double the time.

from lint-staged.

I'm considering using eslint_d or something to make it faster but I've not got around to it yet. Anyway this is off-topic so I'll shut up :)

from lint-staged.

suggestion: run eslint first, if this fails run eslint --fix and exit with an error code :)

Enabling caching with the --cache option may improve performance but probably not adequately.

from lint-staged.

To recap, the issue here is when we automatically run git add on files that the developer only partially staged, the direct result being that the full file is committed.

I got an idea: make it possible to configure different commands for partially staged vs fully staged files.

For example, partially staged would have only eslint being run while fully staged would have eslint --fix, git add.

Thoughts ?

from lint-staged.

@julienw this is feasible but not the best UX IMO. Ideally, developers should not think about commands at all and this should be implementation detail of lint-staged. But I still don't have a solution to the problem that would cover all edge cases.

My concern about your idea is how the config will look like. Ideas?

from lint-staged.

I'm not entirely sure, but I think the config format will change in #273 anyway so this could be included in the format overhaul. I haven't studied how the proposed format looks like yet.

It's because there are a lot of edge cases that I think this idea is good: it's much more conservative, doesn't try to be clever, but still provides a service to the developer by deciding whether a file is partially or fully staged.

from lint-staged.

One option is to check out the staged files into a separate folder and lint fix them and add back into the index then commit.

from lint-staged.

@benjie yes, this is what I'd like to try next TBH. If you have time to work on it, you're welcome to draft a PR!

from lint-staged.

I'm new to this but I'm wondering if this may be related to something I've been seeing.

I work in pycharm and have lint-staged piping to prettier. What will happen to me, is I will commit and push and that workd - and then my committed files will show up as uncommitted but have no changes.

I wonder if git-exec-and-restage would help with that?

from lint-staged.

One option is to check out the staged files into a separate folder and lint fix them and add back into the index then commit.

too magical for my taste... but maybe if this works perfectly I won't even see it. ;)

@userUnderC this issue is only about partially staged files (using git add -p). I'd say your problem is something else unrelated to this issue.

from lint-staged.

@userUnderC you're probably hitting this #151

from lint-staged.

How do I use git diff-index? What do I put in place of $tree?
I couldn't get it to work

here's where I set up tree

Why git diff-index vs git diff?

git diff is a porcelain command whereas git diff-index is a plumbing command. Generally tools should only use plumbing commands. more on this

Why --exit-code? It exits with 1 if there is a difference

This was changed in a more recent version of git, previous versions did not do this without --exit-code :( -- then again, if you don't care about the exit code then this doesn't matter

Do we really need --no-ext-diff?

Say a user is using vimdiff, git apply isn't going to necessarily understand their patch output. A simpler case, consider their external diff provider is just diff --color boom

In what case we'd need git -c core.autocrlf=false before the apply?

the code comment covers this. The specific issue in question: pre-commit/pre-commit#570

from lint-staged.

@asottile Thanks!

So that worked:

git diff-index --ignore-submodules --binary --exit-code --no-color --no-ext-diff $(git write-tree) > lint-staged.diff

Patch output is the same, but I guess it has it's benefits over git diff.

Interesting that every flag has a story :-).

Issue with empty patch file

If there are no unstaged files, the patch will be empty, is there an easy way to not run the apply if the file is empty? I found [ -s lint-staged.diff ] returns 1 or 0 but this didn't work:

[ -s lint-staged.diff ] && git apply --whitespace=nowarn lint-staged.diff && rm -f lint-staged.diff && true

This is getting real messy and complex, definitely should be handled internally.

from lint-staged.

lol, at this point it's getting close to "derivative work"

fwiw, here's how pre-commit handles the empty patch: https://github.com/pre-commit/pre-commit/blob/41d998f1c46f371c9977dcc9d31d7b42387ed74c/pre_commit/staged_files_only.py#L39

(and yes, there are cases where git has a returncode indicating there's a diff but also produces an empty patch)

from lint-staged.

I'm not sure there's a way I can see this working with --fix unless the fix is applied before the stashing so that the stash is a diff against what will be committed rather than something that was changed before committing.

from lint-staged.

@asottile Hmm, I'm trying to solve this as a shell command for now.
I'm just missing the proper conditional with && or || or something.
Any idea?

@tilgovi The goal is to stash unstaged files so that git add would not stage them before commit.
This means that --fix can't run before the stash, since then we'll have mixed pre-fix unstaged files and post-fix unstaged files.

from lint-staged.

My impression is that this issue is about partially staged files, where the stash patch fails to apply after the fix because the partially staged files have changed.

It seems fine to me to stash unstaged files to avoid making changes to them with a linter, but if a file is partially staged I have little hope that it can have its unstaged hunks stashed before the linting and still there be an expectation that unstashing will apply cleanly.

How much of this issue is solved if we accepted that even unstaged hunks of partially staged files will be linted?

from lint-staged.

@tilgovi I don't find a way your suggestion could work. I don't see how you would add the fixes for only the staged hunks, which is a goal. Then the staged and commited hunks won't have the fixes, as they're already in the index and we're not adding them again. And because --fix doesn't error if it can change the errors, you wouldn't warned about it.

I still think we shouldn't be too clever and not try to fix in case of partially staged hunks. But we need some support from lint-staged so that we can define separate commands for completely staged and partially staged files.

from lint-staged.

As @julienw mentioned

I don't see how you would add the fixes for only the staged hunks, which is a goal

I'm running into this problem in my tests. It doesn't look like it's solvable by using files, which we're limited to.

@asottile I'm borrowing your code ATM and it seems like you not even trying to solve this case. Is this for the reason I mentioned above?

from lint-staged.

yeah, pre-commit handles both of the issues listed above, the entire implementation of this is here

I mentioned more on this back in april in this thread

from lint-staged.

So I'm back to the point there I need to meet a decision of what the expected behavior in case of partially staged files would be. Suggestions?

from lint-staged.

@asottile I'm confused. How do you handle the case there I have a partially staged file and I'd like to only apply fixes to staged hunks? The code you're referencing saying you just rolling back to the initial patch: https://github.com/pre-commit/pre-commit/blob/41d998f1c46f371c9977dcc9d31d7b42387ed74c/pre_commit/staged_files_only.py#L64-L66

from lint-staged.

pre-commit does:

• stash changes
• run hooks
• apply stashed changes
  • if this fails (conflicts): roll back hook modifications and retry applying stashed changes

from lint-staged.

I've just written a test for what I'd like to achieve: fc9a2ae could you please take a look a confirm that pre-commit handles that?

from lint-staged.

@asottile I've borrowed this logic and expanded it to use @Stupidism ideas. Here is the test suite I'm running it against: https://github.com/okonet/lint-staged/pull/75/files#diff-f5002743e0b16c70d5bc43040f1d3847

from lint-staged.

@asottile from your answer I still missing if pre-commit is handling the case I mentioned. With or without user input — that doesn't matter in this case, does it?

from lint-staged.

Yes it handles the case, if stashed changes conflict with hook fixes, the stashed changes win and the repository state is put back to where it was when the commit started.

from lint-staged.

Would really love to see this closed.

from lint-staged.

They say:

Running hooks on unstaged changes can lead to both false-positives and false-negatives during
committing. pre-commit only runs on the staged contents of files by temporarily saving the contents of
your files at commit time and stashing the unstaged changes while running hooks.

from lint-staged.

@amit1911 they only run on the staged hunks which I also have done in #75 but the problem I'm facing is with the case when we modify those hunks during the command (i.e. prettier) and we need to "merge" back. Please take a look at the branch and tests to get a better sense of what I'm trying to do.

from lint-staged.

Does the approach taken by precise commits (formatting character ranges instead of full files) provide anything of interest?

from lint-staged.

Currently, precise commits is the only pre-commit hook offering this functionality:

"This is currently the only tool that will format only staged lines rather than the entire file"

Thus, it is of interest to me, for the projects where I don't use Prettier

from lint-staged.