Support ignore files with custom names #706

ptzz · 2017-12-04T09:07:26Z

This allows for application specific ignorefile names, e.g. using
.fdignore for fd.

Moved the hardcoded .rgignore from the ignore crate to ripgrep
.ignore is always respected and has the highest precedence

This allows for application specific ignorefile names, e.g. using `.fdignore` for `fd`. - Moved the hardcoded `.rgignore` from the ignore crate to ripgrep - `.ignore` is always respected and has the highest precedence

ptzz · 2017-12-04T09:12:11Z

See discussion in sharkdp/fd#156, specifically sharkdp/fd#156 (comment)

ptzz · 2017-12-04T09:15:20Z

ignore/src/dir.rs

+    /// Enables reading of ignore files in addition to `.ignore`
+    ///
+    /// Earlier names have lower precedence
+    pub fn ignorefile(&mut self, filename: String) -> &mut IgnoreBuilder {


ignorefile() vs existing add_ignore() (for global ignorefiles) might be a bit confusing. Happy to change the name into something better. Or maybe change add_ignore to add_global_ignore?

This method's type signature should be:

pub fn ignorefile<S: AsRef<OsStr>>(&mut self, file_name: S) -> &mut IgnoreBuilder {

And yes, this means your ignore files should not be Vec<String> but rather Vec<OsString>.

I think this should be named add_custom_ignore_filename.

ptzz · 2017-12-04T11:09:44Z

Should probably add tests that uses multiple ignore files and validates precedence.

BurntSushi

Thanks for working on this! Unfortunately, I think there was a miscommunication or I didn't make myself sufficiently clear, and as a result, this PR isn't quite what I was expecting.

First and foremost, these docs need to be our reference point: https://docs.rs/ignore/0.3.1/ignore/struct.WalkBuilder.html#ignore-rules --- I'll speak in terms of those.

A critical aspect of ignore file checking is that there are layers of ignore rules. Notice that in the second bullet item, each class of ignore file has its own precedence rules. This is necessary not only for predictable UX, but it also matches how git does things. (git has its own layers: .gitignore, .git/info/exclude and global gitignores.) In particular, any .gitignore file---regardless of where it lives---will override all .git/info/exclude files.

This precedence hierarchy continues with .ignore. This is best shown with an example:

$ mkdir /tmp/demorules
$ cd /tmp/demorules
$ git init
$ mkdir foo
$ echo test > baz
$ echo test > foo/quux
$ echo test > foo/bar
$ echo -e 'baz\nquux' > .gitignore
$ echo '!quux' > foo/.gitignore

If you stop here, you'll notice that git won't ignore quux since the .gitignore inside the foo directory overrides the rules found in the .gitignore in the root of the repository. If you run rg --files, then ripgrep will come up with the same results.

However, if you run

$ echo quux > .ignore

in the root of the repository and run rg --files, you'll notice that quux is actually ignored, even though a .gitignore file exists that is "closer" to quux in the foo directory. This is because any .ignore overrides all .gitignore files.

So where am I going with this? Well, when I said this:

I think the way to solve your problem is to add an additional layer of ignore files that has higher precedence than .ignore but uses a custom (probably application specific) name.

I meant that the application specific ignore files should actually be another layer, in addition to .ignore. That means any application specific ignore file (like .fdignore) will override all .ignore rules, just like any .ignore file will override all .gitignore rules.

In this PR, you've implemented such that .ignore is inter-mingled with the application specific ignores. That is, a foo/.fdignore won't override a foo/bar/.ignore, even though I think it should in order to be consistent with how the ignore rules are processed today.

Doing this is a little harder, but you should still mostly be able to work off of what's there. Please ping me with questions!

BurntSushi · 2017-12-04T11:36:27Z

ignore/src/dir.rs

    explicit_ignores: Arc<Vec<Gitignore>>,
+    /// Ignore files used in addition to `.ignore`
+    ignorefiles: Vec<String>,


The names here could be better I think. I'd probably go with custom_ignore_filenames here.

BurntSushi · 2017-12-04T11:38:33Z

ignore/src/dir.rs

+    /// Enables reading of ignore files in addition to `.ignore`
+    ///
+    /// Earlier names have lower precedence
+    pub fn ignorefile(&mut self, filename: String) -> &mut IgnoreBuilder {


This method's type signature should be:

pub fn ignorefile<S: AsRef<OsStr>>(&mut self, file_name: S) -> &mut IgnoreBuilder {

And yes, this means your ignore files should not be Vec<String> but rather Vec<OsString>.

BurntSushi · 2017-12-04T11:39:34Z

ignore/src/dir.rs

+    /// Enables reading of ignore files in addition to `.ignore`
+    ///
+    /// Earlier names have lower precedence
+    pub fn ignorefile(&mut self, filename: String) -> &mut IgnoreBuilder {


I think this should be named add_custom_ignore_filename.

ptzz · 2017-12-04T13:01:13Z

@BurntSushi Thanks for the comments!

I think the way to solve your problem is to add an additional layer of ignore files that has higher precedence than .ignore but uses a custom (probably application specific) name.

Indeed I didn't read this carefully and assumed .ignore would have the highest precedence. I understand that just correcting the order won't solve the hierarchy issues that you talk about though.

I will try to find time to fix this properly.

This layer has the highest precedence.

ptzz · 2017-12-06T20:36:26Z

@BurntSushi I think it should be working as expected now. Also addressed your comments on naming and method signatures.

BurntSushi

Thanks for continuing to work on this! I took another review and this is looking much better. There are still a few fixes I'd like to see though.

One major thing missing from this PR is tests, and we really need those before merging this. The tests should cover at least the following:

Basic functionality. i.e., Can I specify a custom ignore file and have it be respected?
Precedence. Precedence should be checked between different ignore file types and also between different custom ignore names.
Integration tests for ripgrep itself. These tests don't need to have as much coverage as 1+2, but something that checks that it actually works is fine.

I think 1+2 should be in the ignore crate while 3 should be in tests/tests.rs.

BurntSushi · 2017-12-30T21:12:08Z

ignore/src/dir.rs

    explicit_ignores: Arc<Vec<Gitignore>>,
+    /// Ignore files used in addition to `.ignore`
+    custom_ignore_filenames: Vec<OsString>,
+    custom_ignore_matcher: Gitignore,


This should have a doc string.

BurntSushi · 2017-12-30T21:14:20Z

ignore/src/dir.rs

        let mut errs = PartialErrorBuilder::default();
+        let custom_ig_matcher =
+            if !self.0.opts.ignore {


I suspect we don't want this. The caller needs to supply an application specific name, I assume, in order for a matcher to be created at all. So by passing a name, they are implicitly enabling it.

BurntSushi · 2017-12-30T21:15:12Z

ignore/src/dir.rs

    explicit_ignores: Arc<Vec<Gitignore>>,
+    /// Ignore files used in addition to `.ignore`
+    custom_ignore_filenames: Vec<OsString>,


This should be in a Arc<Vec<OsString>>. Otherwise you're going to wind up copying it everywhere.

ptzz · 2018-01-01T20:55:23Z

@BurntSushi Tests have been added for 1+2 above. As for precedence tests, I added one that verifies that a custom ignore file have higher precedence than .ignore, and one that verifies that earlier custom ignore files have lower precedence than later.

Let me know if you want to see more extensive tests. I wasn’t sure if it would make sense to add precedence tests between e.g. custom ignore and .gitignore, as there already tests for .ignore over .gitignore and precedence should apply transitively.

As for 3, there are already tests that uses .rgignore, maybe that's sufficient? It's not hardcoded in the ignore crate anymore, but passed as a (hardcoded) parameter from ripgrep.

ptzz · 2018-01-27T21:31:23Z

@BurntSushi Do you have concerns about merging this and/or want more time for review? Let me know if there is anything I can address.

BurntSushi

This looks great! Thanks for pinging me. Apologies for the tardiness. Also, thanks for pointing out existing tests for .rgignore, I forgot about those. :-) I will get this merged once CI passes (I just kicked it).

ptzz · 2018-01-29T22:52:26Z

Thanks, it was a great learning experience doing a bit of Rust!

Support ignore files with custom names

a9fd2d4

This allows for application specific ignorefile names, e.g. using `.fdignore` for `fd`. - Moved the hardcoded `.rgignore` from the ignore crate to ripgrep - `.ignore` is always respected and has the highest precedence

ptzz commented Dec 4, 2017

View reviewed changes

BurntSushi requested changes Dec 4, 2017

View reviewed changes

Use a separate ignore layer for custom ignore files

3d690ab

This layer has the highest precedence.

Clean up and improve documentation

a308036

BurntSushi requested changes Dec 30, 2017

View reviewed changes

Address PR comments and add tests

7b65c57

BurntSushi approved these changes Jan 29, 2018

View reviewed changes

BurntSushi merged commit 3cb4d13 into BurntSushi:master Jan 29, 2018

BurntSushi mentioned this pull request Jan 29, 2018

Add --ignore-file flag sharkdp/fd#156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ignore files with custom names #706

Support ignore files with custom names #706

ptzz commented Dec 4, 2017

ptzz commented Dec 4, 2017

ptzz Dec 4, 2017

BurntSushi Dec 4, 2017

BurntSushi Dec 4, 2017

ptzz commented Dec 4, 2017

BurntSushi left a comment

BurntSushi Dec 4, 2017

BurntSushi Dec 4, 2017

BurntSushi Dec 4, 2017

ptzz commented Dec 4, 2017 •

edited

Loading

ptzz commented Dec 6, 2017

BurntSushi left a comment

BurntSushi Dec 30, 2017

BurntSushi Dec 30, 2017

BurntSushi Dec 30, 2017

ptzz commented Jan 1, 2018 •

edited

Loading

ptzz commented Jan 27, 2018

BurntSushi left a comment

ptzz commented Jan 29, 2018

Support ignore files with custom names #706

Support ignore files with custom names #706

Conversation

ptzz commented Dec 4, 2017

ptzz commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptzz commented Dec 4, 2017

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptzz commented Dec 4, 2017 • edited Loading

ptzz commented Dec 6, 2017

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptzz commented Jan 1, 2018 • edited Loading

ptzz commented Jan 27, 2018

BurntSushi left a comment

Choose a reason for hiding this comment

ptzz commented Jan 29, 2018

ptzz commented Dec 4, 2017 •

edited

Loading

ptzz commented Jan 1, 2018 •

edited

Loading