Re: [PATCH v3 2/2] builtin/grep: add grep.fallbackToNoIndex config
- From
Jeff King <peff@peff.net>
- Date
- Jan 11, 2016, 21:48 UTC
- Message-ID
- <20160111214846.GC21131@sigill.intra.peff.net>
- In-Reply-To
- <1452547580-30687-3-git-send-email-t.gummerer@gmail.com>
On Mon, Jan 11, 2016 at 10:26:20PM +0100, Thomas Gummerer wrote:
Show 25 quoted lines
> 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"?
Show 7 quoted lines
> @@ -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