Skip to content
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

Closed
ErBlack opened this issue Aug 11, 2017 · 29 comments · Fixed by opencomponents/base-templates#128
Closed

Git add fails after eslint --fix #225

ErBlack opened this issue Aug 11, 2017 · 29 comments · Fixed by opencomponents/base-templates#128

Comments

@ErBlack
Copy link

ErBlack commented Aug 11, 2017

Here the output:

[16:45:44] eslint --fix [started]
[16:45:47] eslint --fix [completed]
[16:45:47] git add [started]
[16:45:47] git add [failed]
[16:45:47] → 🚫 git add --ignore-errors found some errors. Please fix them and try committing again.

fatal: Unable to create '…/.git/index.lock': File exists.

Another git process seems to be running in this repository, e.g.
� git add found some errors. Please fix them and try committing again.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.

[16:45:47] Running tasks for *.js [failed]

fatal: Unable to create '…/.git/index.lock': File exists.
[16:45:47] → 🚫 git add found some errors. Please fix them and try committing again.

Another git process seems to be running in this repository, e.g.

an editor opened by 'git commit'. Please make sure all processes
fatal: Unable to create '…/.git/index.lock': File exists.
are terminated then try again. If it still fails, a git process

may have crashed in this repository earlier:
Another git process seems to be running in this repository, e.g.
remove the file manually to continue.
an editor opened by 'git commit'. Please make sure all processes

are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.
@benmccormick
Copy link

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

@benmccormick
Copy link

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

@baabgai
Copy link

baabgai commented Aug 18, 2017

We have the same issue. Instead of linting we use a script to strip accidentally added BOMs from files prior to committing changes.
For a typical commit everything works as expected, but for commits or merges including a lot of files (~40+) we regularly run into this issue.

@okonet
Copy link
Collaborator

okonet commented Aug 19, 2017

Hmm, interesting. Maybe this is because of running things in parallel? See #149

Could you please try setting subTaskConcurrency to 1 and see if that happens again?

@baabgai
Copy link

baabgai commented Aug 22, 2017

@okonet, thanks for the hint. Probably I should have figured that out myself from the docs.
Anyway, I can confirm that in my case setting subTaskConcurrency to 1 solves my issue. I tried running it on a commit with nearly 700 files and it works perfectly as expected now.

thanks!

@okonet
Copy link
Collaborator

okonet commented Aug 22, 2017

I'm wondering if the parallel execution is a problematic thing in itself because of this and should be reverted/removed?

@benmccormick
Copy link

I certainly did not expect parallel execution. I think it could be ok, if sync was the default

@okonet
Copy link
Collaborator

okonet commented Aug 22, 2017

cc @sudo-suhas

@sudo-suhas
Copy link
Collaborator

sudo-suhas commented Aug 23, 2017

I can't remember all the details but can the default subTaskConcurrency be set to 1 to prevent this?

Yes. Right now, it is set to 2 but can be changed to 1. However, I also have a couple of other suggestions:

  • On non-windows systems, keep the chunkSize as Number.MAX_SAFE_INTEGER as there is no need to split the command.
  • Override the concurrency as 1 for git add

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?

@okonet
Copy link
Collaborator

okonet commented Aug 23, 2017

I'm not sure what is causing it yet but to stay on the safe side I'd disable the concurrent operations by default.

@torarnek
Copy link

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.

@sudo-suhas
Copy link
Collaborator

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 eslint --fix can be run concurrently. Meaning that if you had multiple files to be run through the linter, the files would be split into chunks and executed in parallel. This was necessary for windows where there is limitation on the length of command being executed.

@okonet
Copy link
Collaborator

okonet commented Aug 24, 2017

The fix must be released already. Please update to latest version. See https://github.com/okonet/lint-staged/releases/tag/v4.0.4

@ErBlack
Copy link
Author

ErBlack commented Aug 25, 2017

Awesome. Thank you.

@ndelangen
Copy link

I was using:

  "lint-staged": {
    "linters": {
      "*.js": [
        "npm run lint:js -- --fix",
        "git add"
      ],
      "*.json": [
        "npm run lint:js -- --fix",
        "git add"
      ],
      "*.md": [
        "npm run lint:md -- -o",
        "git add"
      ]
    },
    "verbose": true,
    "concurrent": false
  },

But after updating this broke with this error:

Running lint-staged with the following config:
{
	linters: {
		'*.js': [
			'npm run lint:js -- --fix',
			'git add'
		],
		'*.json': [
			'npm run lint:js -- --fix',
			'git add'
		],
		'*.md': [
			'npm run lint:md -- -o',
			'git add'
		]
	},
	verbose: true,
	concurrent: false,
	chunkSize: 9007199254740991,
	gitDir: '.',
	globOptions: {
		matchBase: true,
		dot: true
	},
	subTaskConcurrency: 1,
	renderer: 'verbose'
}

Could not parse lint-staged config.
Make sure you have created it. See https://github.com/okonet/lint-staged#readme.

Error: Invalid config provided to runAll! Use getConfig instead.


husky > pre-commit hook failed (add --no-verify to bypass)

I changed my config to:

  "lint-staged": {
    "linters": {
      "*.js": [
        "npm run lint:js -- --fix",
        "git add"
      ],
      "*.json": [
        "npm run lint:js -- --fix",
        "git add"
      ],
      "*.md": [
        "npm run lint:md -- -o",
        "git add"
      ]
    },
    "verbose": true
  },

This solved my issue.

I think the new default basically renders the boolean option obsolete?

ndelangen added a commit to thomasbertet/storybook that referenced this issue Sep 4, 2017
@okonet
Copy link
Collaborator

okonet commented Sep 5, 2017

@ndelangen can you please create a separate issue for that? Thanks! #263

@mihneadb
Copy link

Hi! I noticed that in the latest version of lint-staged you cannot specify concurrent: false anymore. How would one handle the situation from this issue with the latest version?

@okonet
Copy link
Collaborator

okonet commented Jul 19, 2019

It’s not related to that option. As this thread shows it’s related to locks on files caused by editors (VsCode most probably).

@mihneadb
Copy link

I'm not sure about that, I could consistently reproduce it with vs without concurrency and I don't even use VSCode.
Just had a config file with two rules (both with git add at the end). Only happens for commits after merge with conflicts. Couldn't figure out why, but this is the scenario:

  1. Make branch1 from master. Edit a line in a file. Commit it.
  2. Make branch2 from master. Edit the same line in the same file. Commit it.
  3. Merge branch1 into branch2. You'll get the conflict message.
  4. git add THE_FILE
  5. git commit. Hooks run, you get lock error.

Same thing works fine with concurrent: false.

@okonet
Copy link
Collaborator

okonet commented Jul 22, 2019

Yeah sounds like a race condition when git add would be executed simultaneously for the same file. Mind sharing your config as well?

@eavam
Copy link

eavam commented Aug 13, 2019

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

@mihneadb
Copy link

Yeah sounds like a race condition when git add would be executed simultaneously for the same file. Mind sharing your config as well?

Sorry about the delay! Sure, something as simple as :

"linters": {
      "*": ["touch", "git add"],
      "THE_FILE": ["touch", "git add"]
    },

The idea is for both rules to be applicable.

@eavam
Copy link

eavam commented Aug 13, 2019

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

Ok. I think I fixed it via

lint-staged -r

I am testing this fix.

@hedgepigdaniel
Copy link

From reading the source code, this is a problem because by default top level tasks are still executed concurrently:

https://github.com/okonet/lint-staged/blob/403f80b74468dd5682f27bfbeabdd779d492f0e8/src/runAll.js#L123-L123

Whereas subtasks are not:

https://github.com/okonet/lint-staged/blob/403f80b74468dd5682f27bfbeabdd779d492f0e8/src/runAll.js#L76-L83

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}'`,
      ],
      []
    ),
}

@okonet
Copy link
Collaborator

okonet commented Oct 18, 2019

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.

@iiroj
Copy link
Member

iiroj commented Oct 18, 2019

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

@okonet
Copy link
Collaborator

okonet commented Oct 18, 2019

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

@iiroj
Copy link
Member

iiroj commented Oct 20, 2019

Sure, I can do it. What do you think would be the proper migration path? Should we skip running any git add in the config, or show a warning that it is automatic?

@okonet
Copy link
Collaborator

okonet commented Oct 20, 2019

I would say so, yes. Or to stay on the safer path, just warn about git add And suggest removing it. I assume some might have other hit commands there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment