| Message ID | 20200720184529.22449-1-martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dir: check pathspecs before returning `path_excluded` | expand |
On Mon, Jul 20, 2020 at 11:46 AM Martin Ågren <martin.agren@gmail.com> wrote: > > On Mon, 20 Jul 2020 at 17:25, Elijah Newren <newren@gmail.com> wrote: > > > > Awesome, thanks Andreas for the bisected report and Martin for finding > > and fixing the bug. As for the reason that the old patch injected the > > pathspec check between the path_excluded and the path_untracked cases, > > that appears to me to just be "I'm good at making boneheaded > > mistakes". Your changes here are the right fix. > > Ok, here it is as a proper patch. > > > Reviewed-by: Elijah Newren <newren@gmail.com> > > Thanks. I've included your reviewed-by below. The log message is > obviously new, but the diff is identical to what I posted earlier. > > BTW, this bug first appeared in v2.27.0, so this is not a regression > during the v2.28.0 cycle. Looks good other than a minor typo in the new commit message. > Martin > > -- >8 -- > In 95c11ecc73 ("Fix error-prone fill_directory() API; make it only > return matches", 2020-04-01), we taught `fill_directory()`, or more > specifically `treat_path()`, to check against any pathspecs so that we > could simplify the callers. > > But in doing so, we added a slightly-to-early return for the "excluded" s/to/too/ > case. We end up not checking the pathspecs, meaning we return > `path_excluded` when maybe we should return `path_none`. As a result, > `git status --ignored -- pathspec` might show paths that don't actually > match "pathspec". > > Move the "excluded" check down to after we've checked any pathspecs. > > Reported-by: Andreas Schwab <schwab@linux-m68k.org> > Reviewed-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > dir.c | 4 ++-- > t/t7061-wtstatus-ignore.sh | 25 +++++++++++++++++++++++++ > 2 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/dir.c b/dir.c > index 1045cc9c6f..fe64be30ed 100644 > --- a/dir.c > +++ b/dir.c > @@ -2209,13 +2209,13 @@ static enum path_treatment treat_path(struct dir_struct *dir, > baselen, excluded, pathspec); > case DT_REG: > case DT_LNK: > - if (excluded) > - return path_excluded; > if (pathspec && > !match_pathspec(istate, pathspec, path->buf, path->len, > 0 /* prefix */, NULL /* seen */, > 0 /* is_dir */)) > return path_none; > + if (excluded) > + return path_excluded; > return path_untracked; > } > } > diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh > index e4cf5484f9..2f9bea9793 100755 > --- a/t/t7061-wtstatus-ignore.sh > +++ b/t/t7061-wtstatus-ignore.sh > @@ -30,6 +30,31 @@ test_expect_success 'same with gitignore starting with BOM' ' > test_cmp expected actual > ' > > +test_expect_success 'status untracked files --ignored with pathspec (no match)' ' > + git status --porcelain --ignored -- untracked/i >actual && > + test_must_be_empty actual && > + git status --porcelain --ignored -- untracked/u >actual && > + test_must_be_empty actual > +' > + > +test_expect_success 'status untracked files --ignored with pathspec (literal match)' ' > + git status --porcelain --ignored -- untracked/ignored >actual && > + echo "!! untracked/ignored" >expected && > + test_cmp expected actual && > + git status --porcelain --ignored -- untracked/uncommitted >actual && > + echo "?? untracked/uncommitted" >expected && > + test_cmp expected actual > +' > + > +test_expect_success 'status untracked files --ignored with pathspec (glob match)' ' > + git status --porcelain --ignored -- untracked/i\* >actual && > + echo "!! untracked/ignored" >expected && > + test_cmp expected actual && > + git status --porcelain --ignored -- untracked/u\* >actual && > + echo "?? untracked/uncommitted" >expected && > + test_cmp expected actual > +' > + > cat >expected <<\EOF > ?? .gitignore > ?? actual > -- > 2.28.0.rc1 >
On Mon, 20 Jul 2020 at 20:49, Elijah Newren <newren@gmail.com> wrote: > > On Mon, Jul 20, 2020 at 11:46 AM Martin Ågren <martin.agren@gmail.com> wrote: > > Looks good other than a minor typo in the new commit message. > > > But in doing so, we added a slightly-to-early return for the "excluded" > > s/to/too/ Thanks! Martin
Martin Ågren <martin.agren@gmail.com> writes: > In 95c11ecc73 ("Fix error-prone fill_directory() API; make it only > return matches", 2020-04-01), we taught `fill_directory()`, or more > specifically `treat_path()`, to check against any pathspecs so that we > could simplify the callers. > > But in doing so, we added a slightly-to-early return for the "excluded" > case. We end up not checking the pathspecs, meaning we return > `path_excluded` when maybe we should return `path_none`. As a result, > `git status --ignored -- pathspec` might show paths that don't actually > match "pathspec". > > Move the "excluded" check down to after we've checked any pathspecs. > > Reported-by: Andreas Schwab <schwab@linux-m68k.org> > Reviewed-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- Makes sense. Thanks. Will queue.
diff --git a/dir.c b/dir.c index 1045cc9c6f..fe64be30ed 100644 --- a/dir.c +++ b/dir.c @@ -2209,13 +2209,13 @@ static enum path_treatment treat_path(struct dir_struct *dir, baselen, excluded, pathspec); case DT_REG: case DT_LNK: - if (excluded) - return path_excluded; if (pathspec && !match_pathspec(istate, pathspec, path->buf, path->len, 0 /* prefix */, NULL /* seen */, 0 /* is_dir */)) return path_none; + if (excluded) + return path_excluded; return path_untracked; } } diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh index e4cf5484f9..2f9bea9793 100755 --- a/t/t7061-wtstatus-ignore.sh +++ b/t/t7061-wtstatus-ignore.sh @@ -30,6 +30,31 @@ test_expect_success 'same with gitignore starting with BOM' ' test_cmp expected actual ' +test_expect_success 'status untracked files --ignored with pathspec (no match)' ' + git status --porcelain --ignored -- untracked/i >actual && + test_must_be_empty actual && + git status --porcelain --ignored -- untracked/u >actual && + test_must_be_empty actual +' + +test_expect_success 'status untracked files --ignored with pathspec (literal match)' ' + git status --porcelain --ignored -- untracked/ignored >actual && + echo "!! untracked/ignored" >expected && + test_cmp expected actual && + git status --porcelain --ignored -- untracked/uncommitted >actual && + echo "?? untracked/uncommitted" >expected && + test_cmp expected actual +' + +test_expect_success 'status untracked files --ignored with pathspec (glob match)' ' + git status --porcelain --ignored -- untracked/i\* >actual && + echo "!! untracked/ignored" >expected && + test_cmp expected actual && + git status --porcelain --ignored -- untracked/u\* >actual && + echo "?? untracked/uncommitted" >expected && + test_cmp expected actual +' + cat >expected <<\EOF ?? .gitignore ?? actual