jj ignoring trailing slash in .gitignore

[dragonmaus]

Description

jj does not seem to correctly handle .gitignore lines that end with a slash.
Instead of only ignoring directories that match the part preceding the trailing slash, it ignores all files that match.

Steps to Reproduce the Problem

# Set up test directory
mkdir test
cd test
echo '/*/' >.gitignore # this should only ignore subdirectories and files in them
mkdir foo ping
for f in foo/bar.txt baz.txt ping/pong.txt woot.txt; do echo $f >$f; done

jj git init
jj status

git init
git status

Expected Behavior

Both jj status and git status should show the same list of files, namely .gitignore, baz.txt, and woot.txt. The subdirectories and the files in them should be ignored.

Actual Behavior

Only git status shows the expected behaviour. jj status instead ignores all of the files (including .gitignore itself).

Correct output of git status:

On branch master

No commits yet

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        .gitignore
        baz.txt
        woot.txt

nothing added to commit but untracked files present (use "git add" to track)

Incorrect output of jj status:

The working copy is clean
Working copy : rwrzskoo 8681d9ad (empty) (no description set)
Parent commit: zzzzzzzz 00000000 (empty) (no description set)

Specifications

• Platform: Void Linux
• Version: jj 0.22.0

(Possibly) Related issues/pull requests:

• jj tracks ignored files #2051 / Fix .gitignore handling of ignored directories #2109

[daehyeok]

#3071

JJ internally use ignore ripgrep/ignore crate.
i think it's crate bug.

❯ cat .gitignore
/*/ # this should only ignore subdirectories and files in them

❯ rg ba
foo/bar.txt
1:foo/bar.txt

baz.txt
1:baz.txt

[emilazy]

FWIW, the ignore crate has several known deficiencies and its author has said that it is in need of a rewrite; we already work around at least one bug in Jujutsu. Months ago, before we moved to it, I had a work‐in‐progress stack to move to gix-ignore, which seemed much more compatible. If it would be useful, I can try to revive that work.

[neutralinsomniac]

That would be amazing. We just ran into an issue with trying to ignore all files except one in a given directory.

[daehyeok]

FWIW, the ignore crate has several known deficiencies and its author has said that it is in need of a rewrite; we already work around at least one bug in Jujutsu. Months ago, before we moved to it, I had a work‐in‐progress stack to move to gix-ignore, which seemed much more compatible. If it would be useful, I can try to revive that work.

Are you working on it? if not i can try.

[stouset]

I've started working on porting to gix-ignore, but there are some API differences that seemingly make this a bit challenging.

As an example, gix-ignore's implementations won't match the path foo/bar/baz against the pattern foo/bar. As far as I can tell, the expectation is that the parent directory foo/bar/ would have already matched and so children of that directory won't be tested against. But the tests for our GitIgnore implementation check that those paths will be filtered.

let mut ignore = gix_ignore::Search::default();
ignore.add_patterns_buffer(b"/dir1/dir2/dir3\n", PathBuf::default(), None);

// passes
assert!(ignore.pattern_matching_relative_path(b"dir1/dir2/dir3".into(), Some(true), pattern::Case::Sensitive).is_some());

// fails
assert!(ignore.pattern_matching_relative_path(b"dir1/dir2/dir3/foo".into(), Some(false), pattern::Case::Sensitive).is_some());

But in our tests:

#[test]
fn test_gitignore_deep_dir() {
    let file = GitIgnoreFile::empty()
        .chain("", b"/dir1/dir2/dir3\n")
        .unwrap();
    assert!(!file.matches("foo"));
    assert!(!file.matches("dir1/foo"));
    assert!(!file.matches("dir1/dir2/foo"));
    assert!(file.matches("dir1/dir2/dir3/foo"));
    assert!(file.matches("dir1/dir2/dir3/dir4/foo"));
}

I'm diving in now to see if consumers of this API are dependent upon this detail.

[stouset]

If I ignore the test failures in the GitIgnore library itself, we do still see 5 failures in the rest in jj-lib and 83 more in jj-cli. That's not promising.

I'll dig a little further but it does look like a port to gix-ignore isn't going to be as simple as I'd hoped.

[arxanas]

As an example, gix-ignore's implementations won't match the path foo/bar/baz against the pattern foo/bar. As far as I can tell, the expectation is that the parent directory foo/bar/ would have already matched and so children of that directory won't be tested against. But the tests for our GitIgnore implementation check that those paths will be filtered.

There's an API like that in ignore as well, referenced in this comment:

https://github.com/martinvonz/jj/blob/05fc85518586d6a3cb2dd26faf4a148512477ffc/lib/src/gitignore.rs#L110-L113

If you wanted to make this change, it might make sense to first switch our code to using that API, then cut over to gitoxide. Alternatively, you could try to wrap the logic to explicitly check all parent paths as well, like this code.

Make sure to profile the working copy snapshot time before and after your change. Switching to ignore had a significant performance improvement (for some reason), and it would be unfortunate to lose that: #3071 (comment)

[stouset]

Thanks for the heads up. That's probably a better path forward.

It might even be good to expose a second function that explicitly matches without parents, then gradually convert over to that.

I'll make sure to profile/benchmark.