-
-
Notifications
You must be signed in to change notification settings - Fork 424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Git add fails after eslint --fix #225
Comments
I'm running into this issue as well. Both on 3.3.1 and the latest. And it started after rebuilding my node_modules, which makes me think that it's a dependency that changed, not the code in this repo |
Pinning execa to 0.6.0 appeared to fix the issue for me, although I shifted a bunch of other things around, so I can't be completely confident that was the problem |
We have the same issue. Instead of linting we use a script to strip accidentally added BOMs from files prior to committing changes. |
Hmm, interesting. Maybe this is because of running things in parallel? See #149 Could you please try setting |
@okonet, thanks for the hint. Probably I should have figured that out myself from the docs. thanks! |
I'm wondering if the parallel execution is a problematic thing in itself because of this and should be reverted/removed? |
I certainly did not expect parallel execution. I think it could be ok, if sync was the default |
cc @sudo-suhas |
Yes. Right now, it is set to 2 but can be changed to 1. However, I also have a couple of other suggestions:
I can make a PR to do one or more of these suggested changes. Also, multiple glob patterns are run in parallel as well. Can that also cause an issue? |
I'm not sure what is causing it yet but to stay on the safe side I'd disable the concurrent operations by default. |
I had the same issue when committing loads of files, and it was solved by setting concurrency to 1. The reason it failed was that two or more processes tried to do a git add at the same time. A feature request would be that the final step (git add) should done only when the previous steps have completed. |
@torarnek This is the current behavior already. However, each step like |
The fix must be released already. Please update to latest version. See https://github.com/okonet/lint-staged/releases/tag/v4.0.4 |
Awesome. Thank you. |
I was using:
But after updating this broke with this error:
I changed my config to:
This solved my issue. I think the new default basically renders the boolean option obsolete? |
@ndelangen |
Hi! I noticed that in the latest version of |
It’s not related to that option. As this thread shows it’s related to locks on files caused by editors (VsCode most probably). |
I'm not sure about that, I could consistently reproduce it with vs without concurrency and I don't even use VSCode.
Same thing works fine with |
Yeah sounds like a race condition when |
I get the same error after merging. Is there any way to fix it? And it arises on my windows. But everything is fine on the mac |
Sorry about the delay! Sure, something as simple as :
The idea is for both rules to be applicable. |
Ok. I think I fixed it via
I am testing this fix. |
From reading the source code, this is a problem because by default top level tasks are still executed concurrently: Whereas subtasks are not: It can be worked around by ensuring that only one top level task runs git commands. For example, I replaced this configuration: {
'*': (files) =>
files.reduce(
(commands, file) => [
...commands,
`prettier --write '${file}'`,
`git add '${file}'`,
],
[]
),
'*.{js,jsx,ts,tsx}': (files) =>
files.reduce(
(commands, file) => [
...commands,
`eslint --fix '${file}'`,
`git add '${file}'`,
],
[]
),
} with this one: {
'*': (files) =>
files.reduce(
(commands, file) => [
...commands,
micromatch.isMatch(file, eslintMatcher)
? `eslint --fix '${file}'`
: `prettier --write '${file}'`,
`git add '${file}'`,
],
[]
),
} |
Good point. I’m wondering if we should hide all git interactions in the lib and ensure the right order since we already doing a lot of heavy lifting anyways. |
@okonet it should doable after v10, because before running linters we have only staged files present, so if after running all linters (but before restoring original unstaged changes) there are any new unstaged changes, those could be automatically added. |
Yeah that’s what I mean. Do you think you could update the PR to make this change as well? That would be great so we don’t have to do v11 right after |
Sure, I can do it. What do you think would be the proper migration path? Should we skip running any |
I would say so, yes. Or to stay on the safer path, just warn about |
Here the output:
The text was updated successfully, but these errors were encountered: