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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html