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

Use a character class to ignore Icon\r directories #4596

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

markjaquith
Copy link

Reasons for making this change:
Years ago, Icon^M^M was added as a macOS rule. This gets parsed as Icon^M, which ignores macOS Icon\r / Icon^M directories.

A strange byproduct of this rule is that when Ripgrep's commonly used ignore crate parses gitinore rules, it strips trailing whitespace that is not a literal space character.

This means that if you have the current Icon^M^M rule in your global gitignore tool, Ripgrep, and any Rust-based directory scanning tool that uses the ignore crate, will be unable to see any directory named Icon — which happens to occur frequently in React codebases.

This change switches to using a character class like [^M] (literal CR surrounded by square brackets).

This prevents the rule from ending in whitespace, and thus makes Icon directories visible to Ripgrep and Rust-based directory scanning tools again.

Why not change Ripgrep

This is old code in Ripgrep, and is widely deployed. It also seems reasonable to see a carriage return at the end of a gitignore rule as not part of the rule. What Ripgrep/ignore is doing is fairly reasonable. Having a .gitignore line that ends in two \r\r is pretty odd. I think the character class solution with brackets is a reasonable solution.

This prevents the rule from ending in whitespace, which will get
stripped by tools like Ripgrep, causing any directory named "Icon" to be
invisible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant