Re: [PATCH] dir: check pathspecs before returning `path_excluded`
On Mon, Jul 20, 2020 at 11:46 AM Martin Ågren <martin.agren@gmail.com> wrote:
Show 18 quoted lines
>
> 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.
Show 9 quoted lines
> 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"Show 74 quoted lines
> 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
>
Elijah Newren <newren@gmail.com>