DavidAnson / markdownlint-cli2

A fast, flexible, configuration-based command-line interface for linting Markdown/CommonMark files with the markdownlint library
MIT License
343 stars 46 forks source link

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

Closed LastDragon-ru closed 4 weeks ago

LastDragon-ru commented 3 months ago

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 commented 3 months ago

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?

LastDragon-ru commented 3 months ago

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 commented 3 months ago

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/**
DavidAnson commented 3 months ago

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 commented 3 months ago

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

Yep, seems also helps.

DavidAnson commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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: https://github.com/sindresorhus/globby/issues/50

LastDragon-ru commented 2 months ago

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 https://github.com/sindresorhus/globby/issues/50

DavidAnson commented 2 months ago

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 commented 4 weeks ago

0.14.0