Re: [PATCH v5 11/12] Fix error-prone fill_directory() API; make it only return matches

[Elijah Newren <newren@gmail.com>]

Hi,

On Sun, Jul 19, 2020 at 5:39 AM Martin Ågren <martin.agren@gmail.com> wrote:
>
> On Sun, 19 Jul 2020 at 08:37, Andreas Schwab <schwab@linux-m68k.org> wrote:
> >
> > This breaks git status --ignored.
> >
> > $ ./git status --porcelain --ignored -- a
> > !! abspath.o
> > !! add-interactive.o
> ...
> > !! attr.o
>
> Thanks for bisecting. This is 95c11ecc73 ("Fix error-prone
> fill_directory() API; make it only return matches", 2020-04-01).
>
> I wonder if the below makes any sense. It seems to fix this usage and
> the tests pass, but I have no idea what else this might be breaking...
>
> Maybe Elijah has an idea whether this is roughly the right approach?
> Looking at the commit in question (95c11ecc73), there must have been
> some reason that it injected the pathspec check between the
> "path_excluded" and the "path_untracked" cases.  The diff below
> basically undoes that split, so I have a feeling I'm missing something.

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.  As a separate
optimization, we could maybe make simplify_away() a bit more involved
and have it exclude a few more paths so that fewer make it to this
final check, but that's just optimization work that is separate from
your fix here.

Reviewed-by: Elijah Newren <newren@gmail.com>

> Martin
>
> 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.7.g31f2d237fa
>