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

Stuck if gitignore: true (probable globby gitignore performance issue) #365

Closed
LastDragon-ru opened this issue Jul 3, 2024 · 12 comments
Closed
Labels
enhancement New feature or request fixed in next Fixed in the "next" branch for the next release

Comments

@LastDragon-ru
Copy link

LastDragon-ru commented Jul 3, 2024

I'm trying to migrate to cli2, but seems .gitignore processing somehow different from in cli: cli2 just stuck if I set gitignore: true in .markdownlint-cli2.yaml :( The npx markdownlint-cli --ignore-path=.gitignore it used now, and it is very fast.

My test command is:

npx markdownlint-cli2 ./README.md

This .markdownlint-cli2.yaml will work

config:
  default: true

This is not (waited for ~5min):

gitignore: true
config:
  default: true

Am I doing something wrong or this is a bug?

PS: markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)

@DavidAnson
Copy link
Owner

I can't think why this should take so much longer unless maybe globby is getting hung on something in your .gitignore? Can you point to a repository that demonstrates the problem, please?

@DavidAnson DavidAnson added the question Further information is requested label Jul 3, 2024
@LastDragon-ru
Copy link
Author

Can you point to a repository that demonstrates the problem, please?

Sure, https://github.com/LastDragon-ru/lara-asp I'm running it on my dev machine (inside docker) where all deps are installed (npm install + composer install)

@LastDragon-ru
Copy link
Author

Seems .gitignore processed incorrectly by cli2. For example, if .gitignore contains

# General
/node_modules
/vendor
/vendor-bin/**/vendor
/tmp
/.phpunit
/.vagrant

git (and cli) will ignore all these directories and their nested dirs/files (as it should be). But cli2 will not. These directories will be ignored only if glob contains **.

So, until the fix, the workaround is

ignores:
  - dev/**
  - tmp/**
  - vendor/**
  - vendor-bin/**
  - node_modules/**
  - .vagrant/**

LastDragon-ru added a commit to LastDragon-ru/lara-asp that referenced this issue Jul 4, 2024
@DavidAnson
Copy link
Owner

Most of the implementation or .gitignore lives in globby, but I do not see any issues there that sound exactly like this. I won't be able to debug this for a while, but I wonder if the problem goes away if you remove the leading '/' character from those paths in .gitignore?

@LastDragon-ru
Copy link
Author

if the problem goes away if you remove the leading '/' character from those paths in .gitignore?

Yep, seems also helps.

@DavidAnson
Copy link
Owner

Per specification, .gitignore treats paths beginning with slash as being relative to the same directory, but that is unusual and unexpected in my opinion - and also unnecessary because the leading slash can be omitted without changing the meaning. Leaving off the leading slash is what I would recommend, though I will investigate and fix or open a bug against globby if that is appropriate.

@LastDragon-ru
Copy link
Author

and also unnecessary because the leading slash can be omitted without changing the meaning.

Nope. The /vendor will ignore only <repo>/vendor, but not <repo>/a/vendor. Without the / both will be ignored. It may be important in some situations.

@DavidAnson
Copy link
Owner

I had a bit of time to try this. Your scenario seems to behave as expected:

@DavidAnson ➜ /workspaces/markdownlint-cli2/issue365 (next) $ cat .gitignore 
/beta
@DavidAnson ➜ /workspaces/markdownlint-cli2/issue365 (next) $ cat .markdownlint-cli2.yaml 
@DavidAnson ➜ /workspaces/markdownlint-cli2/issue365 (next) $ ../node_modules/.bin/markdownlint-cli2 **/*.md
markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)
Finding: alpha/file.md beta/file.md
Linting: 2 file(s)
Summary: 2 error(s)
alpha/file.md:1:9 MD047/single-trailing-newline Files should end with a single newline character
beta/file.md:1:9 MD047/single-trailing-newline Files should end with a single newline character

[EDIT .markdownlint-cli2.yaml]

@DavidAnson ➜ /workspaces/markdownlint-cli2/issue365 (next) $ cat .markdownlint-cli2.yaml 
gitignore: true
@DavidAnson ➜ /workspaces/markdownlint-cli2/issue365 (next) $ ../node_modules/.bin/markdownlint-cli2 **/*.md
markdownlint-cli2 v0.13.0 (markdownlint v0.34.0)
Finding: alpha/file.md beta/file.md
Linting: 1 file(s)
Summary: 1 error(s)
alpha/file.md:1:9 MD047/single-trailing-newline Files should end with a single newline character
@DavidAnson ➜ /workspaces/markdownlint-cli2/issue365 (next) $ 

I think you may be encountering a performance issue with the globby implementation where enabling the gitignore option causes it to enumerate the entire directory tree looking for ignore files. Here is the relevant file/pattern in that code:
https://github.com/sindresorhus/globby/blob/c000568bd20c97d94397c71ec22df4e1c5f41d47/ignore.js#L22

As above, I will open an issue against that project once I produce a standalone demonstration of the problem.

For your purposes, you could try to verify this is only a performance issue by clearing out most of the ignored directories and running the original configuration to see if it completes faster.

@DavidAnson
Copy link
Owner

Haha, I forgot I had already looked into this some. You should be able to confirm you are seeing very bad performance (but correct behavior) by waiting for the original scenario to complete Your “ignores” workaround may be necessary for now.

Relevant thread and especially my comments here: sindresorhus/globby#50

@DavidAnson DavidAnson changed the title Stuck if gitignore: true Stuck if gitignore: true (probable globby gitignore performance issue) Jul 8, 2024
@LastDragon-ru
Copy link
Author

You should be able to confirm you are seeing very bad performance (but correct behavior) by waiting for the original scenario to complete

It finished after ~40min... and results are fine. So seems this is really related to the sindresorhus/globby#50

@DavidAnson
Copy link
Owner

Thanks! I think I will use this issue as a reminder to myself to add an option to use only the root ignore file because that should not cause this problem.

@DavidAnson DavidAnson added enhancement New feature or request and removed question Further information is requested labels Jul 17, 2024
LastDragon-ru added a commit to LastDragon-ru/lara-asp that referenced this issue Aug 16, 2024
@DavidAnson DavidAnson added the fixed in next Fixed in the "next" branch for the next release label Aug 31, 2024
@DavidAnson
Copy link
Owner

0.14.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed in next Fixed in the "next" branch for the next release
Projects
None yet
Development

No branches or pull requests

2 participants