Re: [PATCH 3/3] builtin/grep: allow implicit --no-index

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

On 01/11, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > Currently when git grep is used outside of a git repository without the
> > --no-index option git simply dies.  For convenience, implicitly make git
> > grep behave like git grep --no-index when it is called outside of a git
> > repository.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >  builtin/grep.c  | 32 ++++++++++++++++++++++++--------
> >  t/t7810-grep.sh | 41 ++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 62 insertions(+), 11 deletions(-)
> >
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 3a27bd5..a886af1 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -19,6 +19,9 @@
> >  #include "dir.h"
> >  #include "pathspec.h"
> >
> > +#define GREP_NO_INDEX_EXPLICIT 1
> > +#define GREP_NO_INDEX_IMPLICIT 2
>
> I am not sure this is the best way to do this.  For things like
> this, the usual pattern is to initialize "no_index" to an "unknown"
> value, allow "--no-index" to toggle it to true (by the way, I think
> we should reject "--no-no-index", but that is a separate topic), and
> then after command line parsing finishes, tweak the no_index if it
> is still "unknown".

The reason for this (and the change in 02/03) is so we can distinguish
whether there is an explicit no-index or not for the error messages.
I think it would be okay to have more generic error messages
("--cached or --untracked cannot be used without index" and
"--untracked or no index mode cannot be used with revs").  What do you
think?

> >  static char const * const grep_usage[] = {
> >  	N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
> >  	NULL
> > @@ -632,7 +635,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >  		OPT_BOOL(0, "cached", &cached,
> >  			N_("search in index instead of in the work tree")),
> >  		OPT_BIT(0, "no-index", &no_index,
> > -			N_("find in contents not managed by git"), 1),
> > +			N_("find in contents not managed by git"),
> > +			GREP_NO_INDEX_EXPLICIT),
> >  		OPT_BOOL(0, "untracked", &untracked,
> >  			N_("search in both tracked and untracked files")),
> >  		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
> > @@ -755,9 +759,13 @@ 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 (!no_index && !startup_info->have_repository)
> > -		/* die the same way as if we did it at the beginning */
> > -		setup_git_directory();
> > +	if (!no_index && !startup_info->have_repository) {
> > +		int nongit = 0;
> > +
> > +		setup_git_directory_gently(&nongit);
> > +		if (nongit)
> > +			no_index = GREP_NO_INDEX_IMPLICIT;
> > +	}
> >
> >  	/*
> >  	 * skip a -- separator; we know it cannot be
> > @@ -873,13 +881,21 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >  	if (!show_in_pager && !opt.status_only)
> >  		setup_pager();
> >
> > -	if (no_index && (untracked || cached))
> > -		die(_("--cached or --untracked cannot be used with --no-index."));
> > +	if (untracked || cached) {
> > +		if (no_index == GREP_NO_INDEX_EXPLICIT)
> > +			die(_("--cached or --untracked cannot be used with --no-index."));
> > +		else if (no_index == GREP_NO_INDEX_IMPLICIT)
> > +			die(_("--cached or --untracked cannot be used outside a git repository."));
> > +	}
> >
> >  	if (no_index || untracked) {
> >  		int use_exclude = (opt_exclude < 0) ? !no_index : !!opt_exclude;
> > -		if (list.nr)
> > -			die(_("--no-index or --untracked cannot be used with revs."));
> > +		if (list.nr) {
> > +			if (no_index == GREP_NO_INDEX_IMPLICIT)
> > +				die(_("cannot use revs outside of a git repository."));
> > +			else
> > +				die(_("--no-index or --untracked cannot be used with revs."));
> > +		}
> >  		hit = grep_directory(&opt, &pathspec, use_exclude);
> >  	} else if (0 <= opt_exclude) {
> >  		die(_("--[no-]exclude-standard cannot be used for tracked contents."));
> > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> > index ab94716..4ba955d 100755
> > --- a/t/t7810-grep.sh
> > +++ b/t/t7810-grep.sh
> > @@ -794,11 +794,9 @@ test_expect_success 'outside of git repository' '
> >  		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> >  		export GIT_CEILING_DIRECTORIES &&
> >  		cd non/git &&
> > -		test_must_fail git grep o &&
> >  		git grep --no-index o >../actual.full &&
> >  		test_cmp ../expect.full ../actual.full
> >  		cd sub &&
> > -		test_must_fail git grep o &&
> >  		git grep --no-index o >../../actual.sub &&
> >  		test_cmp ../../expect.sub ../../actual.sub
> >  	) &&
> > @@ -808,7 +806,6 @@ test_expect_success 'outside of git repository' '
> >  		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> >  		export GIT_CEILING_DIRECTORIES &&
> >  		cd non/git &&
> > -		test_must_fail git grep o &&
> >  		git grep --no-index --exclude-standard o >../actual.full &&
> >  		test_cmp ../expect.full ../actual.full &&
> >
> > @@ -821,6 +818,44 @@ test_expect_success 'outside of git repository' '
> >  	)
> >  '
> >
> > +test_expect_success 'outside of git repository without --no-index' '
> > +	rm -fr non &&
> > +	mkdir -p non/git/sub &&
> > +	echo hello >non/git/file1 &&
> > +	echo world >non/git/sub/file2 &&
> > +	{
> > +		echo file1:hello &&
> > +		echo sub/file2:world
> > +	} >non/expect.full &&
> > +	echo file2:world >non/expect.sub &&
> > +	(
> > +		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> > +		export GIT_CEILING_DIRECTORIES &&
> > +		cd non/git &&
> > +		git grep o >../actual.full &&
> > +		test_cmp ../expect.full ../actual.full
> > +		cd sub &&
> > +		git grep o >../../actual.sub &&
> > +		test_cmp ../../expect.sub ../../actual.sub
> > +	) &&
> > +
> > +	echo ".*o*" >non/git/.gitignore &&
> > +	(
> > +		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
> > +		export GIT_CEILING_DIRECTORIES &&
> > +		cd non/git &&
> > +		git grep --exclude-standard o >../actual.full &&
> > +		test_cmp ../expect.full ../actual.full &&
> > +
> > +		{
> > +			echo ".gitignore:.*o*"
> > +			cat ../expect.full
> > +		} >../expect.with.ignored &&
> > +		git grep --no-exclude o >../actual.full &&
> > +		test_cmp ../expect.with.ignored ../actual.full
> > +	)
> > +'
> > +
> >  test_expect_success 'inside git repository but with --no-index' '
> >  	rm -fr is &&
> >  	mkdir -p is/git/sub &&

--
Thomas Gummerer