git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / 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
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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git