fsmonitor: fix empty path in dirfilter #601
Conversation
|
Is there a way to automatically test this fix?
What situation do you need to be in for the bug to occur? |
|
@strager I didn't see any tests for the fsmonitor functionality...but perhaps I missed them. Do they exist? Will gladly add a test if so. Re: what state we need to be in: Imagine a repo that has this in the .gitignore Anything in the root of the repo should be ignored, unless it's in the Without this fix, if you add some file under |
|
|
@strager thanks! Still making my way through the codebase. Will take a look. |
|
@skevy thanks for diving into the issue and fixing it. Since it's related with gitignore. Could you provide a minimal test to reproduce the issue in test-gitignore.t? I was wondering if we should fix it in GitIgnoreMatcher and/or its Python bindings gitignorematcher |
skevy
force-pushed
the
skevy--fix-empty-path-fsmonitor
branch
from
64bb509 to
c98fb13
Compare
|
@zzl0 I don't think it's actually a problem with the gitignore matcher. It's specifically a problem with the fact that an empty path is being passed to the matcher, and it kind of makes sense that it would want to ignore it when the gitignore contains If you see anything obvious here in the gitignore matcher itself, then sure, we can/should fix it. But given that it works as necessary when we give it valid input, it seems reasonable to make this fix. I suppose, fwiw, we could add some type of invariant check in the matcher to ensure it throws if receives bad input... |
skevy
force-pushed
the
skevy--fix-empty-path-fsmonitor
branch
2 times, most recently
from
463d3d6 to
9d0ad76
Compare
| @@ -0,0 +1,15 @@ | |||
| #require fsmonitor | |||
There was a problem hiding this comment.
nit: we need to add #debugruntest-compatible for new tests, so it will be run with new test runner, which has better performance
| $ touch root-file foobar/foo # adds files to root and to foobar | ||
| $ hg status | ||
| ? foobar/foo | ||
| $ hg status # run it a second time to ensure that we didn't accidentally exclude the file |
There was a problem hiding this comment.
This is interesting, maybe there is another bug that impacts the result of the second run.
Yeah, I understand your point. The matchers in Sapling are kind of specific to Sapling usage and we have added that special check to other matchers: e.g. tree_matcher and regex_matcher. So Rust and Python implementation will have consistent behavior. With that said, I am good with current solution. Thanks again for diving into the bug and fixing it. |
## Summary Running the ignore matcher on an empty path produces incorrect results. In the rust implementation, this isn't an issue, because there are directory matchers that run on the directory result. In the Python treestate bindings, we don't have that functionality, so it returns an empty string. This makes this logic match the code in `dirstate.py` (line ~925) where we check for path being an empty string. This fixes `sl status` (and related functionality) when fsmonitor is enabled and there's a complex gitignore file that ignores files in the root directory. ## Test Plan Added Mercurial integration test
skevy
force-pushed
the
skevy--fix-empty-path-fsmonitor
branch
from
9d0ad76 to
5ccebe4
Compare
|
@zzl0 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary
Fixes the bug discussed in Discord here: https://discord.com/channels/1042527964224557107/1042527965256364157/1095764831480590467
Running the ignore matcher on an empty path produces incorrect results. In the rust implementation, this isn't an issue, because there are directory matchers that run on the directory result. In the Python treestate bindings, we don't have that functionality, so it returns an empty string.
This makes this logic match the code in
dirstate.py(line ~925) where we check for path being an empty string.This fixes
sl status(and related functionality) when fsmonitor is enabled and there's a complex gitignore file that ignores files in the root directory.Test Plan
Added Mercurial integration test and manually tested locally