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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 01/11, Jeff King wrote:
> 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.

A few lines earlier, git_config() is called, so I guess we're fine
here.

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

I don't think it ever does return nongit == 1.  I'm not sure why I
thought that could be the case, will fix in the re-roll.

> > @@ -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.

Yes, I think I agree with you.  The original error message should give
enough information for users.  Will revert the change in the re-roll.

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