fix 13773 - 'cargo build' fails when list_files() with gix is triggered

[Byron]

Fixes #13773.

Tasks

• reproduce issue with new test-case
• update fixed gix-dir in Cargo.lock to turn test green

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[weihanglo]

Thanks for the fix! Always amazed how quick you respond to an issue :)

Would you like to follow the pattern as #13770 that first commit a test showing the problematic behavior, and the following commit fixes both test and bug? I am fine with it if you don't have time.

Also, I am too dumb to understand the change in git-dir. Not a blocker but would love to learn more:

assure worktree-roots aren't pruned with pathspecs that are never meant for them.
Previously, when pathspecs were defined, the classification of the worktree-root
would also be using them. This means that depending on the pathspec, worktree-roots would
be pruned, which in turn makes it impossible to recurse into them.

Now pathspecs are disabled when classifying the worktree-root directory.

What are worktree-roots, and what does classification of worktree-root do?

[Byron]

Thanks for the fix! Always amazed how quick you respond to an issue :)

Thanks :)!

Would you like to follow the pattern as #13770 that first commit a test showing the problematic behavior, and the following commit fixes both test and bug? I am fine with it if you don't have time.

Actually I think I did. The first commit 07d2bd7 adds a test and fails exactly like it should, the second commit includes the fix. Is there anything I am missing?

Also, I am too dumb to understand the change in git-dir. Not a blocker but would love to learn more:

assure worktree-roots aren't pruned with pathspecs that are never meant for them.
Previously, when pathspecs were defined, the classification of the worktree-root
would also be using them. This means that depending on the pathspec, worktree-roots would
be pruned, which in turn makes it impossible to recurse into them.
Now pathspecs are disabled when classifying the worktree-root directory.

What are worktree-roots, and what does classification of worktree-root do?

The worktree root is basically invisible, or Path::new("") if you will, what matters to pathspec checks are worktree-relative paths (which are never empty, unless it's the worktree root itself). Previously, it would classify the worktree-root, an empty relative path, and even though usually that would mean… I don't even know how useful that is, but most importantly, that could lead to the empty path being excluded as 'pruned', which led to the traversal root itself being rejected as directory to traverse into, which in turn led to our error here.

Now that I write this, the question really is if empty paths should ever be excluded by pathspecs, and if the pathspec-check makes a mistake. Right now it's fixed by having an empty pathspec check the empty path, which indeed considers it to be included. In fact that should also work for any other pathspec (I suppose). Something to look into again, maybe the bug is fixed in the wrong location, and the trick would be to simply always incude the empty path.

[Byron]

Thanks so much for asking that question :) - it made me think again, less rushed this time.

After revisiting this I realized that the underlying problem was indeed related to handling empty input paths with pathspecs. These, I think I have fixed and tested there now, while having cleaned up gix-dir which now is entirely unchanged. This also means I don't have a direct test for this in the gix-dir codebase, only an indirect one related to the gix-pathspec handling of empty paths. Interestingly, that might even be the better test of the two, so I will probably stop the investigation.

An immediate change to what's released isn't necessary, I think, so these improvements will come in with the next natural release in a month or so.

Thanks again!

[weihanglo]

Actually I think I did. The first commit 07d2bd7 adds a test and fails exactly like it should, the second commit includes the fix. Is there anything I am missing?

Yes, you did it pretty good. For myself, I also match the failure output in the test in the first commit, so the next commit we shows both fix and test change.

Should we wait for Byron/gitoxide@5dea6f1 and update to the new version?

[weihanglo]

Ok okay I missed this:

An immediate change to what's released isn't necessary, I think, so these improvements will come in with the next natural release in a month or so.

Let's merge this. Thanks!

@bors r+

[bors]

📌 Commit 3fd120b has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 3fd120b with merge 80d5b60...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 80d5b60 to master...

[Byron]

Yes, you did it pretty good. For myself, I also match the failure output in the test in the first commit, so the next commit we shows both fix and test change.

Oh, right! I don't know how I could miss this opportunity - I just visually inspected the output. With a test-assertion on stderr, it would have been far clearer though, as the test would still have failed. Next time, even though I hope there won't be, who needs bugs ;).