On Tue, Jul 30, 2019 at 5:04 PM Junio C Hamano <[email protected]> wrote:
>
> Matheus Tavares <[email protected]> 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.
