Fix negated globs passed to CLI are no longer worked.

[Tapchicoma]

Description

Migrate method to get all mathing globs files to use internally PathMatcher and FileVisitor instead of using klob dependency.

Fixes #999

Checklist

• tests are added
• CHANGELOG.md is updated

[Tapchicoma]

@thecoden could you take a look on this PR and check it?

[thecoden]

@thecoden could you take a look on this PR and check it?

I've taken a look through the code and so far it looks good to me. I'm going to try to find some time to check it out and run it as well. Thank you for your attention to this issue.

[thecoden]

Hi @Tapchicoma,

I've pulled and done some comparison with our branch and cleaned up our branch a bit to make the differences easier to see. I can submit a pull request if that makes it easier for you to see and pull what you want from it of course.

See the comparison here

The main differences in our solution are:

• Corrected the expandTilde() method to work for negated patterns
• Separated the glob support into a FileMatcher class that supports both regex and glob patterns. This greatly simplified the fileSequence() method as well
• Maintained the order of the patterns so you can include, exclude, and include again based on different patterns. Not something we would use likely, but left the adaptability open for others.
• We used a triple in the FileMatcher class that I'm not proud of and would prefer abstract more clearly. I apologize for that.

Otherwise yours appears to function per your spec as well from check out and I hope we've been helpful. On future improvements, we will be more efficient as it took quite a bit to get the project set up and testing on our end.

Oh, and Happy New Year!

[Tapchicoma]

@thecoden thank you for your review 👏

Corrected the expandTilde() method to work for negated patterns

Yeah, this is the thing I want to add to this PR before merging it.

Maintained the order of the patterns so you can include, exclude, and include again based on different patterns. Not something we would use likely, but left the adaptability open for others.

Interesting, now sure that this scenario should be supported 🤔 But on the second though may happen 🤔

I will take another look on your implementation 👍

[thecoden]

Thank you @Tapchicoma, I'm sorry for the delay. For some reason I'm not getting my github alerts for any of this conversation or repo so I missed it.

One more difference I just caught as well, is that we added support for the glob: and regex: prefixes and if detected, we do not prefix with glob: allowing the use of the regex matching. You can see this in the internal FileMatcher class in the compare, line 117.

Something we also should have done is converted the expandTilde method to use a compiled regex instead of creating it on every call. I can add a commit for that if you like after.

[Tapchicoma]

One more difference I just caught as well, is that we added support for the glob: and regex: prefixes and if detected, we do not prefix with glob: allowing the use of the regex matching. You can see this in the internal FileMatcher class in the compare, line 117.

Probably I will not add regex: support, don't see the point of supporting both 🤔

Something we also should have done is converted the expandTilde method to use a compiled regex instead of creating it on every call. I can add a commit for that if you like after.

That is a nice improvement - will add it as well 👍

[thecoden]

One more difference I just caught as well, is that we added support for the glob: and regex: prefixes and if detected, we do not prefix with glob: allowing the use of the regex matching. You can see this in the internal FileMatcher class in the compare, line 117.

Probably I will not add regex: support, don't see the point of supporting both thinking

With a developer tool, and it being directly supported in the PathMatcher, I figured leave regex because it is essentially more complete / flexible than glob, but the decision is yours sir and can always be enabled later easily.

Something we also should have done is converted the expandTilde method to use a compiled regex instead of creating it on every call. I can add a commit for that if you like after.

That is a nice improvement - will add it as well +1

👍 Thank you and I agree. Little things like compiled regexes do make a big performance difference over time. I'm not into pre-optimization per se, but really creating the regex once should be default practice anyway since it is immutable anyway.

[girish3]

@Tapchicoma #1084 looks related. It would be great if you can resolve the conflicts or let me know if I can be of any help. Thanks.

[Tapchicoma]

@girish3 yes, related.

This PR is not yet ready to be merged and needs some more improvements that was pointed by @thecoden.

[bjdodson]

This is very problematic for large codebases. Our pre-commit hook attempts to pass an explicit list of files to ktlint. Unfortunately, ktlint now walks over our entire codebase to filter those files in, and so our pre-commit hook now takes about 40 seconds rather than completing in under 1 second.

Is there some way we can pass an explicit list of files and bypass this directory crawl?

[cesar1000]

Same – we used to pass changed files to ktlint in 0.40.0 to get out results in a snap, and have to wait 15-20 seconds with 0.41.0. It seems to me that the addressing negated blobs by doing a full crawl doesn't make sense in the general case. Also, I don't think this implementation respects the order of the globs (e.g. a/** !a/b/** a/b/c/**), which is likely to cause confusion. Why not processing each at a time, adding or removing files from the selected set as you process each additional glob?

[kenyee]

walking an entire huge project from the root to see what matches the globs is hugely inefficient.
Each glob should be expanded instead via Files.newDirectoryStream()

[kenyee]

looked over the Klob code and it has a lot of optimizations that should be used and was ironically extracted out of ktlint 🤔
I'll put up a PR that pulls in the Klob code since that handles negated globs as well.

[kenyee]

hmm..this was added after this PR to handle the case of passing in a list of files w/o any filetree walking:
https://github.com/pinterest/ktlint/blob/master/ktlint/src/main/kotlin/com/pinterest/ktlint/internal/FileUtils.kt#L42