jj tracks ignored files

[0xdeafbeef]

Steps to Reproduce the Problem

repro.zip

1. cd $(mktemp -d) && wget https://github.com/martinvonz/jj/files/12327348/repro.zip && unzip repro.zip && cd repro
2. jj st
   3.observe that files in target are not actually ignored

Expected Behavior

Files are ignored

Specifications

• Platform: fedora 38
• Version: jj 0.8.0-1abd36725a47ca1b25b815b684fab2eb1b71fc0d

[qfel]

Reproducing test:

#[test]
fn test_status_gitignore() {
    let test_env = TestEnvironment::default();
    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
    let repo_path = test_env.env_root().join("repo");
    std::fs::write(repo_path.join("test_file"), "bar").unwrap();
    // Commenting out the line below makes the test pass.
    test_env.jj_cmd_success(&repo_path, &["status"]);
    std::fs::write(repo_path.join(".gitignore"), "test_file\n").unwrap();
    let stdout = test_env.jj_cmd_success(&repo_path, &["status"]);
    assert!(!stdout.contains("test_file"));
}

[qfel]

Hmm though I suspect the example above is actually WAI (never mind whether unintuitive). But then you should be able to fix it with jj untrack, except in your specific case it doesn't seem to recognize those files as ignored.

[martinvonz]

We talked a bit about this on Discord (feel free to join at https://discord.com/invite/dkmfj3aGQN) before @0xdeafbeef filed this bug report. It's something about incorrectly respecting a .gitignore inside a .gitignore'd directory. Sorry we didn't provide that context here.

[martinvonz]

Let me give some more detail. The .gitignore in the root directory in the repro above looks like this:

/target
.idea/
db/
config.yaml
requests.log
ton-global.config.json
queue/
.env
!/contrib/config.yaml
!/contrib/ton-global.config.json

The relevant bits there are /target/ and the two negative patterns at the end. Note that, per Git's spec, later patterns override earlier ones, so the negative patterns override the /target/ pattern. That means that we can't skip walking target/ even though it's ignored, because there might be a /contrib/config.yaml file in there. Well, since that expression is rooted, it can't, so that's just a bug. Wait, I just found the more important bug. From https://git-scm.com/docs/gitignore#_pattern_format:

An optional prefix "!" which negates the pattern; any matching file excluded by a previous pattern will become included again. It is not possible to re-include a file if a parent directory of that file is excluded. Git doesn’t list excluded directories for performance reasons, so any patterns on contained files have no effect, no matter where they are defined.

I'm pretty sure I had missed the second sentence there before. So that's saying that e.g. !/contrib/config.yaml should be ignored inside target/ even if it had not been rooted.

@qfel, do you feel like fixing that? The code is in https://github.com/martinvonz/jj/blob/main/lib/src/gitignore.rs

[qfel]

I don't quite follow.

That means that we can't skip walking

That sounds like performance, but the problem is correctness.

From what you're saying, it sounds like the minimal reproduction could be:

/target
config.yaml
!/contrib/config.yaml

and then put something inside target. But if I do that, things look fine (files under /target are ignored).

Also in the provided repo:

$ touch target/debug/build/tikv-jemalloc-sys-e2bcbb41c8161192/out/build/test/unit/x
$ jj untrack target/debug/build/tikv-jemalloc-sys-e2bcbb41c8161192/out/build/test/unit/x
$ touch target/debug/build/tikv-jemalloc-sys-e2bcbb41c8161192/out/build/test/unit/x.c
$ jj untrack target/debug/build/tikv-jemalloc-sys-e2bcbb41c8161192/out/build/test/unit/x.c
Error: 'target/debug/build/tikv-jemalloc-sys-e2bcbb41c8161192/out/build/test/unit/x.c' is not ignored.
Hint: Files that are not ignored will be added back by the next command.
Make sure they're ignored, then try again.

So it seems there's some problem regarding files with extensions.

[martinvonz]

I don't quite follow.

That means that we can't skip walking

That sounds like performance, but the problem is correctness.

Hmm, sorry, I stopped my explanation halfway when I discovered the part of the spec I had missed :(

The part I missed was that since the negative pattern means - I thought! - that we need to walk the target/ directory, we would do that like any non-ignored directory. The problem was if target/some/subdir/.gitignore contains patterns like !*, then we'd read that and start not ignoring anything in that directory.

I hope that clarifies. The fix should be to not let negative patterns override earlier patterns matching a directory.

[Dr-Emann]

Is there a possibility to use burntsushi's ignore crate for gitignore processing?

[martinvonz]

Is there a possibility to use burntsushi's ignore crate for gitignore processing?

Maybe. We discussed that briefly in #2109 (comment). It sounds like @qfel is going to look into it. When I took a quick look earlier, it seemed like it was more targeted at walking a directory while respecting the .gitignores. I suspect we won't want to reuse its directory-walking mechanism, but we maybe we can use just the pattern-matching bits. Another thing to keep in mind is that we currently read .gitignores in subdirectories and apply them on top of an instance from the parent directories we already had. I don't know if the ignore crate supports that, but I wouldn't be surprised if that's an unnecessary optimization we get rid of anyway.

[martinvonz]

This was fixed by #2109

[qfel]

I suspect we won't want to reuse its directory-walking mechanism

Yeah, that library heavily splits deciding which directories to walk/filter out and the processing logic. It would be hard to fit he current logic into it while taking advantage of its directory walking code.

In general, it looks like the visit_directory code grew quite complicated and it's hard to do anything with it. It also has no unit tests, and somebody made it even more complicated to optimize for extra concurrency - but added on benchmark, so keeping whatever performance boosts were there is tricky.

If we're going to change it, I think a sensible (but expensive) approach could be:

• Add unit tests
• Add some benchmarks good enough to distinguish the parallelized version vs regular
• Factor out pieces of existing code into multiple shorter functions, so it's easier to understand what's happening
• Think on potentially replacing the gitignore code with some library. This will likely require a refactor, but now we'll be in a good position to get there without correctness and performance hit.

If this sounds sensible I can slowly [try to] do those things, but it's a lot of work and I don't have that much free time nowadays, so not sure how long it would take me.

[martinvonz]

I think this code is going to change a bit more in the near future. We're going to add a custom working copy implementation at Google for our distributed file system. @hooper is going to work on that. That will probably involve a fair amount of refactoring here to be able to reuse code. It might not involve much refactoring of visit_directory() since our distributed file system will instead tell us which paths have changed so we shouldn't have to call that function. My guess is that it's still not worth changing the code until that's done.