Re: [GSoC][PATCH] grep: fix worktree case in submodules
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 30, 2019, 20:04 UTC
- Message-ID
- <xmqqtvb3s2zi.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <ba3d8a953a2cc5b4ff03fefa434ffd7bd6a78f15.1564505605.git.matheus.bernardino@usp.br>
Matheus Tavares <matheus.bernardino@usp.br> writes:
Show 7 quoted lines
> @@ -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.
Passing the 'cached' bit down from grep_cache() does look like a sensible way to go.
Show 17 quoted lines
> @@ -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.
Show 13 quoted lines
> 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")?