Re: [GSoC][PATCH] grep: fix worktree case in submodules

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

On Tue, Jul 30, 2019 at 5:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > @@ -475,7 +475,7 @@ static int grep_submodule(struct grep_opt *opt,
> >               strbuf_release(&base);
> >               free(data);
> >       } else {
> > -             hit = grep_cache(&subopt, pathspec, 1);
> > +             hit = grep_cache(&subopt, pathspec, cached);
> >       }
>
> Interesting.  It appears to me that this has always searched in
> submodule index and never working tree.  I am not sure if there was
> any specific reason to avoid looking into the working tree files.

It seems that git-grep was taking the worktree into account before
f9ee2fc ("grep: recurse in-process using 'struct repository'",
02-08-2017). So maybe it was just a mistake during the in-process
conversion.

> Passing the 'cached' bit down from grep_cache() does look like a
> sensible way to go.
>
> > @@ -523,7 +523,8 @@ static int grep_cache(struct grep_opt *opt,
> >                       }
> >               } else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
> >                          submodule_path_match(repo->index, pathspec, name.buf, NULL)) {
> > -                     hit |= grep_submodule(opt, pathspec, NULL, ce->name, ce->name);
> > +                     hit |= grep_submodule(opt, pathspec, NULL, ce->name,
> > +                                           ce->name, cached);
> >               } else {
> >                       continue;
> >               }
> > @@ -598,7 +599,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> >                       free(data);
> >               } else if (recurse_submodules && S_ISGITLINK(entry.mode)) {
> >                       hit |= grep_submodule(opt, pathspec, &entry.oid,
> > -                                           base->buf, base->buf + tn_len);
> > +                                           base->buf, base->buf + tn_len,
> > +                                           1); /* ignored */
>
> The trailing comment is misleading.  In the context of reviewing
> this patch, we can probably tell it applies only to that "1", but
> if you read only the postimage, the "ignored" comment looks as if
> the call itself is somehow ignored by somebody unspecified.  It is
> not clear at all that it is only about the final parameter.
>
> If you must...
>
>                 hit |= grep_submodule(opt, pathspec, &entry.oid,
>                                       base->buf, base->buf + tn_len,
>                                       1 /* ignored */);
>
> ... is a reasonable way to write it.

Right, thanks.

> > diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> > index a11366b4ce..946f91fa57 100755
> > --- a/t/t7814-grep-recurse-submodules.sh
> > +++ b/t/t7814-grep-recurse-submodules.sh
> > @@ -408,4 +408,25 @@ test_expect_success 'grep --recurse-submodules with submodules without .gitmodul
> >       test_cmp expect actual
> >  '
> >
> > +reset_and_clean () {
> > +     git reset --hard &&
> > +     git clean -fd &&
> > +     git submodule foreach --recursive 'git reset --hard' &&
> > +     git submodule foreach --recursive 'git clean -fd'
>
> Do we need two separate foreach, instread of a single one that does
> both reset and clean (i.e. "git reset --hard && git clean -f -d")?

Indeed! Thanks. I'll send a v2 addressing both comments.