help wanted: switch to ignore crate for gitignore handling

[arxanas]

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added tests to cover my changes

[arxanas]

Tests are failing, e.g. https://github.com/martinvonz/jj/actions/runs/6727225092/job/18284735965?pr=2499, could use help on this

[martinvonz]

I spent some time on this. I wonder if it's simply a bug in the ignore crate. I think the same library is used by ripgrep, so I figured I could test the same case with ripgrep to see if it seemed like a bug there. However, I'm new to ripgrep and I'm confused about how it's supposed to work. I couldn't get it to respect the most basic .gitignore:

$ echo ignored > .gitignore
$ echo test > ignored
$ rg test
ignored
1:test

Does anyone else here have any experience with ripgrep?

[thoughtpolice]

Use .ignore if there's no .git repo; otherwise ripgrep won't respect .gitignore, but will always respect .ignore. You can just ln -s .gitignore .ignore (to cover all cases)

austin@GANON:~/tmp/rg-test$ echo test > ignored
austin@GANON:~/tmp/rg-test$ echo ignored > .ignore
austin@GANON:~/tmp/rg-test$ rg test
No files were searched, which means ripgrep probably applied a filter you didn't expect.
Running with --debug will show why files are being skipped.

[martinvonz]

Use .ignore if there's no .git repo

Thanks! It does seem to ignore the files correctly in the case that's failing in this PR. So I guess we're not using the API correctly somehow. I'll see if can find more time to spend on it later.

[necauqua]

Speaking of .ignore - creator of ignore and ripgrep pushes the idea of a universal .ignore standard (which is basically just .gitignore I think, but just respected in contexts it has any meaning).

It would be trivially easy to support both .ignore and .gitignore with the crate, and the idea imo is appealing.
And no need to even make a .jjignore ever :)

[daehyeok]

Ignore lib not support sibling ripgrep/issues/1909. So it might need to custom data structure to use multiple GitIgnore module.

[martinvonz]

Ignore lib not support sibling ripgrep/issues/1909. So it might need to custom data structure to use multiple GitIgnore module.

It sounds to me like we shouldn't be affected by that because we don't use the matcher on sibling directories. Or did you find a case where we do?

[daehyeok]

It sounds to me like we shouldn't be affected by that because we don't use the matcher on sibling directories. Or did you find a case where we do?

+match with parent path. If i understand correctly comment on linked issue(ripgrep/issues/1909).
We can't handling gitignore.rs test cases without multiple GitIgnoreFile object.

   fn test_gitignore_literal_with_prefix() {
        let file = GitIgnoreFile::empty().chain("dir/", b"foo\n");
        // I consider it undefined whether a file in a parent directory matches, but
        // let's test it anyway
        assert!(!file.matches("foo"));

Can we ignore this cases?

[martinvonz]

It sounds to me like we shouldn't be affected by that because we don't use the matcher on sibling directories. Or did you find a case where we do?

+match with parent path. If i understand correctly comment on linked issue(ripgrep/issues/1909).

Ah.

We can't handling gitignore.rs test cases without multiple GitIgnoreFile object.

   fn test_gitignore_literal_with_prefix() {
        let file = GitIgnoreFile::empty().chain("dir/", b"foo\n");
        // I consider it undefined whether a file in a parent directory matches, but
        // let's test it anyway
        assert!(!file.matches("foo"));

Can we ignore this cases?

I would say so. That comment (which I'm pretty sure I wrote once upon a time) seems quite clear in saying that it doesn't really matter what we do in that scenario. And I don't think we have any new cases where we have started depending on how it would behave.

Thanks for checking.

[daehyeok]

I would say so. That comment (which I'm pretty sure I wrote once upon a time) seems quite clear in saying that it doesn't really matter what we do in that scenario. And I don't think we have any new cases where we have started depending on how it would behave.

Thanks for checking.

Okay i found other regression on other ambiguous pattern. I will left comment later.
by the way, is it anyway to add co-auther through JJ? in other words, what is the best way to working/patch based on others's PR or unsubmitted git branch?

[martinvonz]

in other words, what is the best way to working/patch based on others's PR or unsubmitted git branch?

I don't personally care how you do it. I would simply say "Thanks to @arxanas who did most/some/whatever of the work on this patch.". Some people put footers like "Co-authored-by:" or something in the commit message. Maybe @arxanas has an opinion.

[PhilipMetzger]

by the way, is it anyway to add co-auther through JJ? in other words, what is the best way to working/patch based on others's PR or unsubmitted git branch?

There's no co-author support itself in jj but you can always add the footer by hand. Then it will be displayed on Github, see #2226 and #3005.

[martinvonz]

Then it will be displayed on Github, see #2226 and #3005.

Ah, since GitHub respects it, I agree that it's a good idea to use it. (I guess I should have remembered that because I saw that on some recent PR where the Google CLA bot failed.)

[arxanas]

I use Co-authored-by:. It doesn't really matter to me who is the primary author and who is the coauthor.

[daehyeok]

Created new PR #3071. and summarized changed test cases.