Skip to content

Respect global .gitignore when excludesFile path uses surrounding double quotes#2629

Closed
Kentokamoto wants to merge 2 commits intoBurntSushi:masterfrom
Kentokamoto:kentokamoto/2392-global-gitignore-bug
Closed

Respect global .gitignore when excludesFile path uses surrounding double quotes#2629
Kentokamoto wants to merge 2 commits intoBurntSushi:masterfrom
Kentokamoto:kentokamoto/2392-global-gitignore-bug

Conversation

@Kentokamoto
Copy link

@Kentokamoto Kentokamoto commented Oct 14, 2023

Related Issue: #2392

Summary of problem:

In a .gitconfig file, a user may specify additional file patterns they may want to ignore when committing changes by using the core.excludesFile setting. RipGrep respects these file patterns as well.

Git also handles file paths that are surrounded by double quotes like

[core]
    excludesFile = "~/foo/bar"

RipGrep's logic does not handle for double quotes.

Changes

The regex used to parse the excludesFile section of the .gitconfig adds double quotes (\x22) as a part of the parsing pattern.

Tests

Wrote parse_excludes_file4 for unit testing

Manually tested from issue's example:

$ mkdir rg_global_gitignore
$ cd rg_global_gitignore
$ git init

# adding the config this way will strip the quotation marks
# git config --global core.excludesFile "~/.gitignore"

# open ~/.gitconfig and add:
#
# [core]
# 	excludesFile = "~/.gitignore"

$ echo "*.sql" > ~/.gitignore

$ touch backup backup.sql backup123.sql db_backup.sql
$ <path>/ripgrep/target/release/rg --files --debug
DEBUG|grep_regex::config|crates/regex/src/config.rs:173: assembling HIR from 0 fixed string literals
DEBUG|rg::args|crates/core/args.rs:555: running with 8 threads for parallelism
DEBUG|rg::args|crates/core/args.rs:1148: hyperlink format: "file://{host}{path}"
[126, 47, 46, 103, 105, 116, 105, 103, 110, 111, 114, 101]
DEBUG|globset|crates/globset/src/lib.rs:445: built glob set; 0 literals, 0 basenames, 1 extensions, 0 prefixes, 0 suffixes, 0 required extensions, 0 regexes
DEBUG|globset|crates/globset/src/lib.rs:445: built glob set; 0 literals, 0 basenames, 1 extensions, 0 prefixes, 0 suffixes, 0 required extensions, 0 regexes
DEBUG|ignore::walk|crates/ignore/src/walk.rs:1804: ignoring ./db_backup.sql: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("/Users/kento/.gitignore"), original: "*.sql", actual: "**/*.sql", is_whitelist: false, is_only_dir: false })))
DEBUG|ignore::walk|crates/ignore/src/walk.rs:1804: ignoring ./backup123.sql: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("/Users/kento/.gitignore"), original: "*.sql", actual: "**/*.sql", is_whitelist: false, is_only_dir: false })))
DEBUG|ignore::walk|crates/ignore/src/walk.rs:1804: ignoring ./.gitignore: Ignore(IgnoreMatch(Hidden))
DEBUG|ignore::walk|crates/ignore/src/walk.rs:1804: ignoring ./.git: Ignore(IgnoreMatch(Hidden))
DEBUG|ignore::walk|crates/ignore/src/walk.rs:1804: ignoring ./backup.sql: Ignore(IgnoreMatch(Gitignore(Glob { from: Some("/Users/kento/.gitignore"), original: "*.sql", actual: "**/*.sql", is_whitelist: false, is_only_dir: false })))
backup

BurntSushi
BurntSushi requested changes Oct 14, 2023
View reviewed changes
Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! But the regex here looks too permissive. It accepts any sequence of whitespace and quotes. It probably should be \s*"?\s* or something.

And please use a literal quote instead of its hex escape. You'll want to use r#"..."# for the string syntax.

@Kentokamoto
Copy link
Author

Kentokamoto commented Oct 14, 2023

@BurntSushi , Thanks for the feedback. I've gone ahead and made the changes you suggested. I added an extra test to check if more than one pair of double quotes surrounding the path is valid or not. I can remove it if it's overkill but let me know.

BurntSushi
BurntSushi approved these changes Oct 15, 2023
View reviewed changes
Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does allow a quote on one side but not the other, but we're never going to get this exactly right with using a regex to parse this file anyway.

@BurntSushi
Copy link
Owner

BurntSushi commented Oct 15, 2023

Also, in the future, PRs like this should just be one commit. So changes should be fixed up into one commit and then force pushed.

@BurntSushi BurntSushi added the rollup A PR that has been merged with many others in a rollup. label Oct 15, 2023
BurntSushi
BurntSushi reviewed Oct 15, 2023
View reviewed changes
crates/ignore/src/gitignore.rs
super::expand_tilde("~/foo/bar")
);
}
#[test]
Copy link
Owner

@BurntSushi BurntSushi Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also, code should follow the style of the surrounding code. In this case, there should be a blank
line separating test functions.

(You don't need to do this now. I've done it for you and this PR will get rolled up into another bigger one.)

BurntSushi pushed a commit that referenced this pull request Oct 16, 2023
@BurntSushi
ignore: improve 'excludesFile' parsing
004747b 
This permits the value to be surrounded in double quotes. It's still not
perfect, but probably better than it was. Getting this to be more
correct will likely require writing (or using) a real parser, which I'm
not particularly incliend to do at present.

Fixes #2392, Closes #2629
BurntSushi pushed a commit that referenced this pull request Oct 16, 2023
@BurntSushi
ignore: improve 'excludesFile' parsing
c949107 
This permits the value to be surrounded in double quotes. It's still not
perfect, but probably better than it was. Getting this to be more
correct will likely require writing (or using) a real parser, which I'm
not particularly incliend to do at present.

Fixes #2392, Closes #2629
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BurntSushi BurntSushi BurntSushi approved these changes

Assignees

No one assigned

Labels

rollup A PR that has been merged with many others in a rollup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Kentokamoto @BurntSushi