Re: [PATCH v3 2/2] builtin/grep: add grep.fallbackToNoIndex config

[Jeff King <peff@peff.net>]

On Mon, Jan 11, 2016 at 10:26:20PM +0100, Thomas Gummerer wrote:

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 4229cae..5efe9bb 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -755,9 +755,20 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  			     PARSE_OPT_STOP_AT_NON_OPTION);
>  	grep_commit_pattern_type(pattern_type_arg, &opt);
>  
> -	if (use_index && !startup_info->have_repository)
> -		/* die the same way as if we did it at the beginning */
> -		setup_git_directory();
> +	if (use_index && !startup_info->have_repository) {
> +		int fallback = 0;
> +		git_config_get_bool("grep.fallbacktonoindex", &fallback);
> +		if (fallback) {
> +			int nongit = 0;
> +
> +			setup_git_directory_gently(&nongit);
> +			if (nongit)
> +				use_index = 0;
> +		} else {
> +			/* die the same way as if we did it at the beginning */
> +			setup_git_directory();
> +		}
> +	}

Hmm. We used to have problems accessing config before calling
setup_git_directory(). I am not sure if that is still the case, though.

I guess the startup sequence is muddied here, though. cmd_grep() is
marked as RUN_SETUP_GENTLY, so we would have already run setup, and here
we are following the "we are not in a repository" code path (i.e., we
saw "!startup_info->have_repository").

And the existing setup_git_directory() is just there to die(), as the
comment explains. So what is the new setup_git_directory_gently() doing?
We know we've already done setup, and that we're not in a git repo,
right? Shouldn't we just be able to set use_index to 0 and keep going?
Under what circumstances would it _not_ return "nongit == 1"?

> @@ -874,12 +885,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		setup_pager();
>  
>  	if (!use_index && (untracked || cached))
> -		die(_("--cached or --untracked cannot be used with --no-index."));
> +		die(_("--cached or --untracked cannot be used with --no-index "
> +		      "or outside of a git repository"));

I'm lukewarm on this (and the other) change. What you've written is
technically correct, but it's getting rather verbose. We've presented
the option already as "turn on --no-index by default outside a
repository", so I'm not sure we need to clarify it here. Since it's a
feature people must turn on manually, presumably they would know that.

I don't know, maybe it would help somebody. I'm not strongly opposed.

-Peff