grep: ignore --recurse-submodules if --no-index is given #540

phil-blain · 2020-01-25T23:07:00Z

Changes since v1:

set recurse_submodules to 0 early in cmd_grep if !use_index. This makes more sense, and eliminates an incompatibility with topic mt/threaded-grep-in-object-store.

v1:
Since grep learned to recurse into submodules in 0281e48
(grep: optionally recurse into submodules, 2016-12-16),
using --recurse-submodules along with --no-index makes Git
die().

This is unfortunate because if submodule.recurse is set in a user's
~/.gitconfig, invoking git grep --no-index either inside or outside
a Git repository results in

fatal: option not supported with --recurse-submodules

Let's allow using these options together, so that setting submodule.recurse
globally does not prevent using git grep --no-index.

Using --recurse-submodules should not have any effect if --no-index
is used inside a repository, as Git will recurse into the checked out
submodule directories just like into regular directories.

CC: Brandon Williams [email protected], SZEDER Gábor [email protected], Junio C Hamano [email protected], Matheus Tavares Bernardino [email protected]

phil-blain · 2020-01-26T00:57:19Z

/submit

gitgitgadget · 2020-01-26T00:58:41Z

Submitted as [email protected]

gitgitgadget · 2020-01-26T09:44:23Z

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Philippe,

On Sun, 26 Jan 2020, Philippe Blain via GitGitGadget wrote:

> From: Philippe Blain <[email protected]>
>
> Since grep learned to recurse into submodules in 0281e487fd
> (grep: optionally recurse into submodules, 2016-12-16),
> using --recurse-submodules along with --no-index makes Git
> die().
>
> This is unfortunate because if submodule.recurse is set in a user's
> ~/.gitconfig, invoking `git grep --no-index` either inside or outside
> a Git repository results in
>
>     fatal: option not supported with --recurse-submodules
>
> Let's allow using these options together, so that setting submodule.recurse
> globally does not prevent using `git grep --no-index`.
>
> Using `--recurse-submodules` should not have any effect if `--no-index`
> is used inside a repository, as Git will recurse into the checked out
> submodule directories just like into regular directories.
>
> Signed-off-by: Philippe Blain <[email protected]>

My initial reaction to this patch was: "but why would you try to combine
`--recurse-submodules` with `--no-index`?". The `submodule.recurse`
reference made me think again, though.

And then it hit me: by `--no-index`, what we _really_ mean to say is:
ignore that this _might_ be tracked in Git. And that obviously means that
there cannot be any submodules to recurse into, as far as `git diff
--no-index` is concerned.

So I think your patch makes a ton of sense.

Thanks,
Dscho

> ---
>     grep: ignore --recurse-submodules if --no-index is given
>
>     Since grep learned to recurse into submodules in 0281e487fd (grep:
>     optionally recurse into submodules, 2016-12-16), using
>     --recurse-submodules along with --no-index makes Git die().
>
>     This is unfortunate because if submodule.recurse is set in a user's
>     ~/.gitconfig, invoking git grep --no-index either inside or outside a
>     Git repository results in
>
>     fatal: option not supported with --recurse-submodules
>
>     Let's allow using these options together, so that setting
>     submodule.recurse globally does not prevent using git grep --no-index.
>
>     Using --recurse-submodules should not have any effect if --no-indexis
>     used inside a repository, as Git will recurse into the checked out
>     submodule directories just like into regular directories.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-540%2Fphil-blain%2Fgrep-no-index-ignore-recurse-submodule-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-540/phil-blain/grep-no-index-ignore-recurse-submodule-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/540
>
>  Documentation/git-grep.txt         |  3 ++-
>  builtin/grep.c                     |  4 ++--
>  t/t7814-grep-recurse-submodules.sh | 11 ++++++++++-
>  3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index c89fb569e3..ffc3a6efdc 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -96,7 +96,8 @@ OPTIONS
>  	Recursively search in each submodule that has been initialized and
>  	checked out in the repository.  When used in combination with the
>  	<tree> option the prefix of all submodule output will be the name of
> -	the parent project's <tree> object.
> +	the parent project's <tree> object. This option has no effect
> +	if `--no-index` is given.
>
>  -a::
>  --text::
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 50ce8d9461..d5f089dd41 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1115,8 +1115,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>
> -	if (recurse_submodules && (!use_index || untracked))
> -		die(_("option not supported with --recurse-submodules"));
> +	if (recurse_submodules && untracked)
> +		die(_("--untracked not supported with --recurse-submodules"));
>
>  	if (!show_in_pager && !opt.status_only)
>  		setup_pager();
> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> index 946f91fa57..828cb3ba58 100755
> --- a/t/t7814-grep-recurse-submodules.sh
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -345,7 +345,16 @@ test_incompatible_with_recurse_submodules ()
>  }
>
>  test_incompatible_with_recurse_submodules --untracked
> -test_incompatible_with_recurse_submodules --no-index
> +
> +test_expect_success 'grep --recurse-submodules --no-index ignores --recurse-submodules' '
> +	git grep --recurse-submodules --no-index -e "^(.|.)[\d]" >actual &&
> +	cat >expect <<-\EOF &&
> +	a:(1|2)d(3|4)
> +	submodule/a:(1|2)d(3|4)
> +	submodule/sub/a:(1|2)d(3|4)
> +	EOF
> +	test_cmp expect actual
> +'
>
>  test_expect_success 'grep --recurse-submodules should pass the pattern type along' '
>  	# Fixed
>
> base-commit: bc7a3d4dc04dd719e7c8c35ebd7a6e6651c5c5b6
> --
> gitgitgadget
>

gitgitgadget · 2020-01-29T07:19:36Z

This patch series was integrated into pu via git@60f8b41.

gitgitgadget · 2020-01-29T18:55:36Z

This branch is now known as pb/do-not-recurse-grep-no-index.

gitgitgadget · 2020-01-29T18:55:37Z

This patch series was integrated into pu via git@9a6c201.

Since grep learned to recurse into submodules in 0281e48 (grep: optionally recurse into submodules, 2016-12-16), using --recurse-submodules along with --no-index makes Git die(). This is unfortunate because if submodule.recurse is set in a user's ~/.gitconfig, invoking `git grep --no-index` either inside or outside a Git repository results in fatal: option not supported with --recurse-submodules Let's allow using these options together, so that setting submodule.recurse globally does not prevent using `git grep --no-index`. Using `--recurse-submodules` should not have any effect if `--no-index` is used inside a repository, as Git will recurse into the checked out submodule directories just like into regular directories. Helped-by: Junio C Hamano <[email protected]> Signed-off-by: Philippe Blain <[email protected]>

phil-blain · 2020-01-30T13:36:42Z

/submit

gitgitgadget · 2020-01-30T13:37:37Z

Submitted as [email protected]

gitgitgadget · 2020-01-30T18:18:33Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Philippe Blain via GitGitGadget" <[email protected]> writes:

> Using `--recurse-submodules` should not have any effect if `--no-index`
> is used inside a repository, as Git will recurse into the checked out
> submodule directories just like into regular directories.
> ...
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 50ce8d9461..ae2d5bbafc 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -958,6 +958,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  			/* die the same way as if we did it at the beginning */
>  			setup_git_directory();
>  	}
> +	/* Ignore --recurse-submodules if --no-index is given or implied */
> +	if (!use_index)
> +		recurse_submodules = 0;

This is done quite early in the execution flow.  That makes any and
all existing checks that says "if recurse-submodules is set and we
are in no-index mode, do this" unneeded.

>  
>  	/*
>  	 * skip a -- separator; we know it cannot be

> @@ -1115,8 +1118,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> -	if (recurse_submodules && (!use_index || untracked))
> -		die(_("option not supported with --recurse-submodules"));
> +	if (recurse_submodules && untracked)
> +		die(_("--untracked not supported with --recurse-submodules"));

And this is an example of a change to remove a check for such a
redundant condition.  If recurse_submodules is true (which is the
only time the RHS of &&- is evaluated), we know use_index cannot be
false, so the final outcome solely depends on the value of untracked.

Looks good.  And my quick reading of the current builtin/grep.c code
suggests that this is the only such combination that can be simplified.

Thanks.

>  	if (!show_in_pager && !opt.status_only)
>  		setup_pager();
> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> index 946f91fa57..828cb3ba58 100755
> --- a/t/t7814-grep-recurse-submodules.sh
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -345,7 +345,16 @@ test_incompatible_with_recurse_submodules ()
>  }
>  
>  test_incompatible_with_recurse_submodules --untracked
> -test_incompatible_with_recurse_submodules --no-index
> +
> +test_expect_success 'grep --recurse-submodules --no-index ignores --recurse-submodules' '
> +	git grep --recurse-submodules --no-index -e "^(.|.)[\d]" >actual &&
> +	cat >expect <<-\EOF &&
> +	a:(1|2)d(3|4)
> +	submodule/a:(1|2)d(3|4)
> +	submodule/sub/a:(1|2)d(3|4)
> +	EOF
> +	test_cmp expect actual
> +'
>  
>  test_expect_success 'grep --recurse-submodules should pass the pattern type along' '
>  	# Fixed
>
> base-commit: bc7a3d4dc04dd719e7c8c35ebd7a6e6651c5c5b6

gitgitgadget · 2020-01-30T23:29:58Z

This patch series was integrated into pu via git@a112d46.

gitgitgadget · 2020-02-05T23:39:53Z

This patch series was integrated into pu via git@517e1e0.

gitgitgadget · 2020-02-05T23:39:55Z

This patch series was integrated into next via git@227bbe1.

gitgitgadget · 2020-02-07T21:54:56Z

This patch series was integrated into pu via git@18c3f5e.

gitgitgadget · 2020-02-11T22:01:54Z

This patch series was integrated into pu via git@109f6d9.

gitgitgadget · 2020-02-12T22:06:26Z

This patch series was integrated into pu via git@556ccd4.

gitgitgadget · 2020-02-12T22:06:27Z

This patch series was integrated into next via git@556ccd4.

gitgitgadget · 2020-02-12T22:06:28Z

This patch series was integrated into master via git@556ccd4.

gitgitgadget · 2020-02-12T22:06:30Z

Closed via 556ccd4.

gitgitgadget bot added the pu label Jan 29, 2020

phil-blain force-pushed the grep-no-index-ignore-recurse-submodule branch from 6634266 to 215e9ce Compare January 30, 2020 13:27

phil-blain force-pushed the grep-no-index-ignore-recurse-submodule branch from 215e9ce to 6fc8bf7 Compare January 30, 2020 13:30

weekly-digest bot mentioned this pull request Feb 2, 2020

Weekly Digest (26 January, 2020 - 2 February, 2020) #543

Closed

gitgitgadget bot added the next label Feb 5, 2020

weekly-digest bot mentioned this pull request Feb 9, 2020

Weekly Digest (2 February, 2020 - 9 February, 2020) #547

Closed

gitgitgadget bot added the master label Feb 12, 2020

gitgitgadget bot closed this Feb 12, 2020

phil-blain deleted the grep-no-index-ignore-recurse-submodule branch August 17, 2021 21:53

grep: ignore --recurse-submodules if --no-index is given #540

grep: ignore --recurse-submodules if --no-index is given #540

Uh oh!

Conversation

phil-blain commented Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phil-blain commented Jan 26, 2020

Uh oh!

gitgitgadget bot commented Jan 26, 2020

Uh oh!

gitgitgadget bot commented Jan 26, 2020

Uh oh!

gitgitgadget bot commented Jan 29, 2020

Uh oh!

gitgitgadget bot commented Jan 29, 2020

Uh oh!

gitgitgadget bot commented Jan 29, 2020

Uh oh!

phil-blain commented Jan 30, 2020

Uh oh!

gitgitgadget bot commented Jan 30, 2020

Uh oh!

gitgitgadget bot commented Jan 30, 2020

Uh oh!

gitgitgadget bot commented Jan 30, 2020

Uh oh!

gitgitgadget bot commented Feb 5, 2020

Uh oh!

gitgitgadget bot commented Feb 5, 2020

Uh oh!

gitgitgadget bot commented Feb 7, 2020

Uh oh!

gitgitgadget bot commented Feb 11, 2020

Uh oh!

gitgitgadget bot commented Feb 12, 2020

Uh oh!

gitgitgadget bot commented Feb 12, 2020

Uh oh!

gitgitgadget bot commented Feb 12, 2020

Uh oh!

gitgitgadget bot commented Feb 12, 2020

Uh oh!

Uh oh!

phil-blain commented Jan 25, 2020 •

edited

Loading