git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org,
	"Christian Couder" <christian.couder@gmail.com>,
	"Olga Telezhnaya" <olyatelezhnaya@gmail.com>,
	kernel-usp@googlegroups.com, jackdanielz@eyomi.org,
	"Antonio Ospite" <ao2@ao2.it>,
	"Stefan Beller" <stefanbeller@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"Brandon Williams" <bwilliams.eng@gmail.com>
Subject: Re: [GSoC][PATCH] grep: fix worktree case in submodules
Date: Tue, 30 Jul 2019 13:04:49 -0700	[thread overview]
Message-ID: <xmqqtvb3s2zi.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <ba3d8a953a2cc5b4ff03fefa434ffd7bd6a78f15.1564505605.git.matheus.bernardino@usp.br> (Matheus Tavares's message of "Tue, 30 Jul 2019 13:53:27 -0300")

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.

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.

> 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")?


  reply	other threads:[~2019-07-30 20:04 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-08  8:14 Weird behavior with git grep --recurse-submodules Daniel Zaoui
2019-07-10  6:43 ` Matheus Tavares Bernardino
2019-07-10 11:14   ` Johannes Schindelin
2019-07-16 18:10     ` Daniel Zaoui
2019-07-29 20:27       ` Matheus Tavares Bernardino
2019-07-30 16:53         ` [GSoC][PATCH] grep: fix worktree case in submodules Matheus Tavares
2019-07-30 20:04           ` Junio C Hamano [this message]
2019-07-30 22:02             ` Christian Couder
2019-07-31 15:57               ` Junio C Hamano
2019-08-01  3:08                 ` Matheus Tavares Bernardino
2019-07-30 23:40             ` Matheus Tavares Bernardino
2019-07-31 15:35           ` [GSoC][PATCH v2] " Matheus Tavares
2019-08-01  3:13             ` [GSoC][PATCH v3] " Matheus Tavares
2019-08-03 23:39         ` Weird behavior with git grep --recurse-submodules Brandon Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqtvb3s2zi.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=ao2@ao2.it \
    --cc=bwilliams.eng@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jackdanielz@eyomi.org \
    --cc=jonathantanmy@google.com \
    --cc=kernel-usp@googlegroups.com \
    --cc=matheus.bernardino@usp.br \
    --cc=olyatelezhnaya@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=stefanbeller@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).