[PATCH 0/3] Implicitly use --no-index if git grep is used outside of repo

[PATCH 0/3] Implicitly use --no-index if git grep is used outside of repo

[Thomas Gummerer]

Often when looking for something in a downloaded source archive I find
myself using git grep --no-index after forgetting to use --no-index
the first time and git unhelpfully dies.

This patch series makes git grep implicitly run git grep --no-index
when run outside of a repository, analogous to how git diff works when
run outside of a repository.

Thomas Gummerer (3):
  t7810: correct --no-index test
  builtin/grep: rename use_index to no_index
  builtin/grep: allow implicit --no-index

 builtin/grep.c  | 40 ++++++++++++++++++++++++++++------------
 t/t7810-grep.sh | 45 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 68 insertions(+), 17 deletions(-)

-- 
2.6.3.387.g749a69c.dirty

[PATCH 1/3] t7810: correct --no-index test

[Thomas Gummerer]

GIT_CEILING_DIRECTORIES doesn't prevent chdir up into another directory
while looking for a repository directory if it is equal to the current
directory.  Because of this, the test which claims to test the git grep
--no-index command outside of a repository actually tests it inside of a
repository.  The test_must_fail assertions still pass because the git
grep only looks at untracked files and therefore no file matches, but
not because it's run outside of a repository as it was originally
intended.

Set the GIT_CEILING_DIRECTORIES environment variable to the parent
directory of the directory in which the git grep command is executed, to
make sure it is actually run outside of a git repository.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---

 t/t7810-grep.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 028ffe4..ab94716 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -791,7 +791,7 @@ test_expect_success 'outside of git repository' '
 	} >non/expect.full &&
 	echo file2:world >non/expect.sub &&
 	(
-		GIT_CEILING_DIRECTORIES="$(pwd)/non/git" &&
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
 		test_must_fail git grep o &&
@@ -805,7 +805,7 @@ test_expect_success 'outside of git repository' '
 
 	echo ".*o*" >non/git/.gitignore &&
 	(
-		GIT_CEILING_DIRECTORIES="$(pwd)/non/git" &&
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
 		test_must_fail git grep o &&
-- 
2.6.3.387.g749a69c.dirty

[PATCH 2/3] builtin/grep: rename use_index to no_index

[Thomas Gummerer]

Switch the OPT_NEGBIT argument to a OPT_BIT argument, and rename the
corresponding use_index variable to a no_index variable.  This will make
the following step easier to follow.  No functional changes intended.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/grep.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 4229cae..3a27bd5 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -625,14 +625,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	struct string_list path_list = STRING_LIST_INIT_NODUP;
 	int i;
 	int dummy;
-	int use_index = 1;
+	int no_index = 0;
 	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
 			N_("search in index instead of in the work tree")),
-		OPT_NEGBIT(0, "no-index", &use_index,
-			 N_("find in contents not managed by git"), 1),
+		OPT_BIT(0, "no-index", &no_index,
+			N_("find in contents not managed by git"), 1),
 		OPT_BOOL(0, "untracked", &untracked,
 			N_("search in both tracked and untracked files")),
 		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
@@ -755,7 +755,7 @@ 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)
+	if (!no_index && !startup_info->have_repository)
 		/* die the same way as if we did it at the beginning */
 		setup_git_directory();
 
@@ -873,11 +873,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!show_in_pager && !opt.status_only)
 		setup_pager();
 
-	if (!use_index && (untracked || cached))
+	if (no_index && (untracked || cached))
 		die(_("--cached or --untracked cannot be used with --no-index."));
 
-	if (!use_index || untracked) {
-		int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
+	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."));
 		hit = grep_directory(&opt, &pathspec, use_exclude);
-- 
2.6.3.387.g749a69c.dirty

Re: [PATCH 2/3] builtin/grep: rename use_index to no_index

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> Switch the OPT_NEGBIT argument to a OPT_BIT argument, and rename the
> corresponding use_index variable to a no_index variable.  This will make
> the following step easier to follow.  No functional changes intended.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/grep.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

This makes the resulting code full of double negations, I am
afraid.  Please do not do this.

I am open to turning "use_index" to a tristate, initialized to -1
(unknown), set to 0 with OPT_SET_INT("no-index"), and optionally set
to 1 with OPT_SET_INT("use-index").  Then after parsing the command
line argument, if use_index is still -1 (i.e. no command line option),
set it to 1 when we can use the index, or 0 (the new feature you
want to add with 3/3).

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 4229cae..3a27bd5 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -625,14 +625,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	struct string_list path_list = STRING_LIST_INIT_NODUP;
>  	int i;
>  	int dummy;
> -	int use_index = 1;
> +	int no_index = 0;
>  	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
>  
>  	struct option options[] = {
>  		OPT_BOOL(0, "cached", &cached,
>  			N_("search in index instead of in the work tree")),
> -		OPT_NEGBIT(0, "no-index", &use_index,
> -			 N_("find in contents not managed by git"), 1),
> +		OPT_BIT(0, "no-index", &no_index,
> +			N_("find in contents not managed by git"), 1),
>  		OPT_BOOL(0, "untracked", &untracked,
>  			N_("search in both tracked and untracked files")),
>  		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
> @@ -755,7 +755,7 @@ 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)
> +	if (!no_index && !startup_info->have_repository)
>  		/* die the same way as if we did it at the beginning */
>  		setup_git_directory();
>  
> @@ -873,11 +873,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	if (!show_in_pager && !opt.status_only)
>  		setup_pager();
>  
> -	if (!use_index && (untracked || cached))
> +	if (no_index && (untracked || cached))
>  		die(_("--cached or --untracked cannot be used with --no-index."));
>  
> -	if (!use_index || untracked) {
> -		int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
> +	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."));
>  		hit = grep_directory(&opt, &pathspec, use_exclude);

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

[Thomas Gummerer]

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
+
 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 &&
-- 
2.6.3.387.g749a69c.dirty

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

[Duy Nguyen]

On Sun, Jan 10, 2016 at 9:19 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> 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.

Should we have a line about this behavior in git-grep.txt, maybe the
description section? I wonder if anybody wants the old behavior (e.g.
non-zero exit code when running outside a repo). If there is such a
case (*), we may need an option to revert it back (--no-no-index seems
ridiculous, maybe --use-index). The safest way though, is introduce a
new option like --use-index=<always|optional|never> then you can make
an grep alias with --use-index=optional.

(*) I've been hitting really weird real-world use cases so I'm a bit paranoid..
-- 
Duy

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

[Thomas Gummerer]

On 01/11, Duy Nguyen wrote:
> On Sun, Jan 10, 2016 at 9:19 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > 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.
>
> Should we have a line about this behavior in git-grep.txt, maybe the
> description section?

Yes good point, the behavior change should definitely be documented.

> I wonder if anybody wants the old behavior (e.g.
> non-zero exit code when running outside a repo). If there is such a
> case (*), we may need an option to revert it back (--no-no-index seems
> ridiculous, maybe --use-index). The safest way though, is introduce a
> new option like --use-index=<always|optional|never> then you can make
> an grep alias with --use-index=optional.

You're right.  I couldn't think of a reason why someone would rely on
the old behavior, but maybe I missed something.  I like the idea of
introducing the --use-index=... option.

How should we handle priority between --no-index and --use-index,
should we just give --no-index priority if it is set and ignore the
new --use-index option, or is there some other way?

> (*) I've been hitting really weird real-world use cases so I'm a bit paranoid..
> --
> Duy

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

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> On 01/11, Duy Nguyen wrote:
>> On Sun, Jan 10, 2016 at 9:19 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> > 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.
>>
>> Should we have a line about this behavior in git-grep.txt, maybe the
>> description section?
>
> Yes good point, the behavior change should definitely be documented.
>
>> I wonder if anybody wants the old behavior (e.g.
>> non-zero exit code when running outside a repo). If there is such a
>> case (*), we may need an option to revert it back (--no-no-index seems
>> ridiculous, maybe --use-index). The safest way though, is introduce a
>> new option like --use-index=<always|optional|never> then you can make
>> an grep alias with --use-index=optional.
>
> You're right.  I couldn't think of a reason why someone would rely on
> the old behavior, but maybe I missed something.  I like the idea of
> introducing the --use-index=... option.

I don't like that, though ;-)

"We run 'git grep' in random places and relied on it to fail when
run in somewhere not under control of Git." feels so flawed at
multiple levels that I doubt it deserves to be kept working.  For
one thing, "git grep" is not the way to tell something is under
control of Git (rev-parse would be a better thing for scriptor to
use).  For another, how would such a script tell between "not a
git repository" and there was no hits?

So I do agree that automatic fallback needs to be documented and
advertised as a feature (or even a bugfix), I do not think we want
to add knobs to keep such a broken script working.

> How should we handle priority between --no-index and --use-index,
> should we just give --no-index priority if it is set and ignore the
> new --use-index option, or is there some other way?
>
>> (*) I've been hitting really weird real-world use cases so I'm a bit paranoid..
>> --
>> Duy

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

[Thomas Gummerer]

On 01/11, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > On 01/11, Duy Nguyen wrote:
> >> On Sun, Jan 10, 2016 at 9:19 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >> > 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.
> >>
> >> Should we have a line about this behavior in git-grep.txt, maybe the
> >> description section?
> >
> > Yes good point, the behavior change should definitely be documented.
> >
> >> I wonder if anybody wants the old behavior (e.g.
> >> non-zero exit code when running outside a repo). If there is such a
> >> case (*), we may need an option to revert it back (--no-no-index seems
> >> ridiculous, maybe --use-index). The safest way though, is introduce a
> >> new option like --use-index=<always|optional|never> then you can make
> >> an grep alias with --use-index=optional.
> >
> > You're right.  I couldn't think of a reason why someone would rely on
> > the old behavior, but maybe I missed something.  I like the idea of
> > introducing the --use-index=... option.
>
> I don't like that, though ;-)
>
> "We run 'git grep' in random places and relied on it to fail when
> run in somewhere not under control of Git." feels so flawed at
> multiple levels that I doubt it deserves to be kept working.  For
> one thing, "git grep" is not the way to tell something is under
> control of Git (rev-parse would be a better thing for scriptor to
> use).  For another, how would such a script tell between "not a
> git repository" and there was no hits?

I agree that scripts don't deserve to be kept working in that case.
What about a user though who accidentally runs git grep outside of a
repository, and is usually warned by git failing quickly, whereas with
the changed behavior some time might go by until the user realizes the
error.  Not sure if we want to support this use case or not?

> So I do agree that automatic fallback needs to be documented and
> advertised as a feature (or even a bugfix), I do not think we want
> to add knobs to keep such a broken script working.
>
> > How should we handle priority between --no-index and --use-index,
> > should we just give --no-index priority if it is set and ignore the
> > new --use-index option, or is there some other way?
> >
> >> (*) I've been hitting really weird real-world use cases so I'm a bit paranoid..
> >> --
> >> Duy

--
Thomas Gummerer

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

[Jeff King]

On Mon, Jan 11, 2016 at 06:48:17PM +0100, Thomas Gummerer wrote:

> > "We run 'git grep' in random places and relied on it to fail when
> > run in somewhere not under control of Git." feels so flawed at
> > multiple levels that I doubt it deserves to be kept working.  For
> > one thing, "git grep" is not the way to tell something is under
> > control of Git (rev-parse would be a better thing for scriptor to
> > use).  For another, how would such a script tell between "not a
> > git repository" and there was no hits?
> 
> I agree that scripts don't deserve to be kept working in that case.
> What about a user though who accidentally runs git grep outside of a
> repository, and is usually warned by git failing quickly, whereas with
> the changed behavior some time might go by until the user realizes the
> error.  Not sure if we want to support this use case or not?

Yeah, I don't think git would be _wrong_ here, but I could certainly see
it being annoying. Several times a week I probably run `git grep` in my
home directory, and after seeing its error, realize "oops, I meant to
`cd git`".

Having it spew nonsense results, and/or appear to hang while it
literally reads every file on my disk would be at least a minor
annoyance.

But I don't think any kind of command-line flag would help that; I'm not
going to start typing "git grep --use-index=never" for every invocation.
I think the only sensible mitigation would be a config option, so that
people who rarely use `--no-index` (and are OK with having to specify
it) do not get punished by false positives.

I dunno. Maybe I would find the new behavior so useful I would be OK
with the occasional false-positive. But when we make a release with the
new behavior and somebody _does_ complain, it sure would be nice not to
have to say "deal with it; it's the new behavior and there is no escape
hatch".

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 11, 2016 at 06:48:17PM +0100, Thomas Gummerer wrote:
>
>> What about a user though who accidentally runs git grep outside of a
>> repository, and is usually warned by git failing quickly, whereas with
>> the changed behavior some time might go by until the user realizes the
>> error.  Not sure if we want to support this use case or not?
>
> Yeah, I don't think git would be _wrong_ here, but I could certainly see
> it being annoying. Several times a week I probably run `git grep` in my
> home directory, and after seeing its error, realize "oops, I meant to
> `cd git`".
> ...
> ..., it sure would be nice not to
> have to say "deal with it; it's the new behavior and there is no escape
> hatch".

Ahh, you two are absolutely right.

I assumed people (after making their new year's resolutions) will be
perfect this year and from now on, and I totally forgot about the
case where somebody runs it by mistake.

"grep.fallbacktoNoindex = true/false" that is by default 'false'
would be our usual approach, then.

Thanks for pointing it out.

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

[Eric Sunshine]

On Sun, Jan 10, 2016 at 9:19 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> 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>
> ---
> diff --git a/builtin/grep.c b/builtin/grep.c
> @@ -19,6 +19,9 @@
> +#define GREP_NO_INDEX_EXPLICIT 1
> +#define GREP_NO_INDEX_IMPLICIT 2
> @@ -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)

Just below here when checking --untracked, you use a simple 'else'
rather than 'else if' to handle the other case of explicit vs
implicit. Why the inconsistency? Also, the ordering of 'if/else' arms
is opposite.

> +                       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."));
> +               }
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> @@ -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 &&

Isn't the above just a complicated way of saying:

    cat <<-\EOF >non/expect.full &&
    file:hello
    sub/file2:world
    EOF

?

> +       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

Broken &&-chain.

> +               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*"

Broken &&-chain.

> +                       cat ../expect.full
> +               } >../expect.with.ignored &&
> +               git grep --no-exclude o >../actual.full &&
> +               test_cmp ../expect.with.ignored ../actual.full
> +       )
> +'

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

[Thomas Gummerer]

On 01/10, Eric Sunshine wrote:
> On Sun, Jan 10, 2016 at 9:19 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > 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>
> > ---
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > @@ -19,6 +19,9 @@
> > +#define GREP_NO_INDEX_EXPLICIT 1
> > +#define GREP_NO_INDEX_IMPLICIT 2
> > @@ -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)
>
> Just below here when checking --untracked, you use a simple 'else'
> rather than 'else if' to handle the other case of explicit vs
> implicit. Why the inconsistency? Also, the ordering of 'if/else' arms
> is opposite.

The reason is that I removed the no_index check from the surrounding
if block, and grep should only die if no_index is set, while in the
block below it should also die if only untracked is set, but no_index
isn't.  I agree however that it's a bit confusing, so I'll just leave
the no_index check in the surrounding block and use the simple else
here, so the block ends up like this:

	if (no_index && (untracked || cached)) {
		if (no_index == GREP_NO_INDEX_IMPLICIT)
			die(_("--cached or --untracked cannot be used outside a git repository."));
		else
			die(_("--cached or --untracked cannot be used with --no-index."));
	}

> > +                       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."));
> > +               }
> > diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> > @@ -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 &&
>
> Isn't the above just a complicated way of saying:
>
>     cat <<-\EOF >non/expect.full &&
>     file:hello
>     sub/file2:world
>     EOF
>
> ?

Yes, the test case is mostly copy paste from the test case above it
where it is written like this.  Looking around in t7810, both the more
complicated way as I used it, as well as the way you've written it are
used, so I'll use the simpler way in the re-roll.  Thanks.

> > +       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
>
> Broken &&-chain.
>
> > +               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*"
>
> Broken &&-chain.

Thanks for catching these two, again the test is mostly a copy paste,
so the chain is broken in the test before this one as well, will add
the fix to my first cleanup patch for the other tests and fix these in
the re-roll.

Thanks for your review!

> > +                       cat ../expect.full
> > +               } >../expect.with.ignored &&
> > +               git grep --no-exclude o >../actual.full &&
> > +               test_cmp ../expect.with.ignored ../actual.full
> > +       )
> > +'

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

[Junio C Hamano]

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

>  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 &&

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

[Thomas Gummerer]

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

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

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

>> > +#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?

I can understand that you need three states (--no-index given
explicitly from the command line, we fall back to --no-index when we
found we are in a diretory not under control by Git, and we do want
to use the index) to be able to give different messages between the
first two cases.

The usual way we do that is by making the variable tristate (which I
outlined in the previous message, initialize use_index to -1
"unspecified" and then fix it up when it is left unspecified after
you check the config and the command line option).

I however fail to see why that necessitates to change use_index to
no_index, making the code harder to follow by introducing double
negation.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I however fail to see why that necessitates to change use_index to
> no_index, making the code harder to follow by introducing double
> negation.

Oh, perhaps your thinking is that there are multiple ways that
use_index can become 0 (i.e. it could come from the config, could
come from an explicit --no-index, or it could come from the new
default behaviour), and the error messages deep in the callchain
(long after option parsing is done) want to react to these
differences.

To that I am somewhat sympathetic, but then use_index can become 1
(rather, no_index can become 0) in multiple ways (i.e. it can be
because the user is just using the command as designed for its
primary use case, or the user explicitly said --no-no-index), so I
am not sure.

In either case, I do not have a strong objection.  Avoiding double
negation is merely a moderately strong general preference.

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

[Thomas Gummerer]

On 01/11, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I however fail to see why that necessitates to change use_index to
> > no_index, making the code harder to follow by introducing double
> > negation.
>
> Oh, perhaps your thinking is that there are multiple ways that
> use_index can become 0 (i.e. it could come from the config, could
> come from an explicit --no-index, or it could come from the new
> default behaviour), and the error messages deep in the callchain
> (long after option parsing is done) want to react to these
> differences.

Yes, that's what I was thinking, sorry if I wasn't clear before.
Though I think the a bit more generic error messages are just fine, so
we can avoid the double negation.

> To that I am somewhat sympathetic, but then use_index can become 1
> (rather, no_index can become 0) in multiple ways (i.e. it can be
> because the user is just using the command as designed for its
> primary use case, or the user explicitly said --no-no-index), so I
> am not sure.
>
> In either case, I do not have a strong objection.  Avoiding double
> negation is merely a moderately strong general preference.

[PATCH v2 0/3] Introduce a --use-index command line argument in git grep

[Thomas Gummerer]

The first round is at $gmane/283619.  Thanks to Duy and Eric for the reviews.

I decided to go with Duy's suggestion, introducing a new --use-index
option, instead of modifying the behaviour of git grep outside of a
git repository, as it seems to be the safer way to go.

Changes since the last version:
01/03: Fix the && chain in this test as well.
03/03:
 - Introduced the new --no-index argument
 - fixed the test thanks to the suggestion of Eric
 - modified the test to use --use-index=optional and added a test that
   uses --use-index=never.
 - Fixed the if/else statements for the error messages to be more
   consistent.
 - Added documentation for the new option

Interdiff below:

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4a44d6d..90ff643 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -25,6 +25,7 @@ SYNOPSIS
 	   [-W | --function-context]
 	   [-f <file>] [-e] <pattern>
 	   [--and|--or|--not|(|)|-e <pattern>...]
+	   [--use-index=<always|optional|never>]
 	   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
 	   [--] [<pathspec>...]
 
@@ -66,6 +67,18 @@ OPTIONS
 --no-index::
 	Search files in the current directory that is not managed by Git.
 
+--use-index::
+	Set the behavior of git grep when used in- or outside of a git
+	repository.  If set to always, the command can only be used
+	inside of a git repository, and will fail if it is used
+	outside of a git repository.  If set to never, it will work
+	both inside and outside of a git repository, but will not use
+	any information in the index if used inside of a repository.
+	If set to optional, it will work both inside and outside of a
+	git repository, using the information available in the index
+	if used inside a repository.  Always and optional can be
+	overridden by `--no-index`.
+
 --untracked::
 	In addition to searching in the tracked files in the working
 	tree, search also in untracked files.
diff --git a/builtin/grep.c b/builtin/grep.c
index a886af1..bb2add9 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -629,6 +629,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int i;
 	int dummy;
 	int no_index = 0;
+	const char *use_index = NULL;
 	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
 
 	struct option options[] = {
@@ -637,6 +638,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-index", &no_index,
 			N_("find in contents not managed by git"),
 			GREP_NO_INDEX_EXPLICIT),
+		OPT_STRING(0, "use-index", &use_index, N_("always|optional|never"),
+			   N_("options for using the index")),
 		OPT_BOOL(0, "untracked", &untracked,
 			N_("search in both tracked and untracked files")),
 		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
@@ -759,14 +762,29 @@ 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) {
-		int nongit = 0;
+	if (use_index) {
+		if (!strcmp(use_index, "always")) {
+			/* same as git grep without --use-index */
+		} else if (!strcmp(use_index, "optional")) {
+			if (!no_index && !startup_info->have_repository) {
+				int nongit = 0;
 
-		setup_git_directory_gently(&nongit);
-		if (nongit)
-			no_index = GREP_NO_INDEX_IMPLICIT;
+				setup_git_directory_gently(&nongit);
+				if (nongit)
+					no_index = GREP_NO_INDEX_IMPLICIT;
+			}
+		} else if (!strcmp(use_index, "never")) {
+			no_index = GREP_NO_INDEX_EXPLICIT;
+		} else {
+			die(_("invalid option \"%s\" for --use-index, expecting"
+			      " always, optional or never"), use_index);
+		}
 	}
 
+	if (!no_index && !startup_info->have_repository)
+		/* die the same way as if we did it at the beginning */
+		setup_git_directory();
+
 	/*
 	 * skip a -- separator; we know it cannot be
 	 * separating revisions from pathnames if
@@ -881,11 +899,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!show_in_pager && !opt.status_only)
 		setup_pager();
 
-	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 || cached)) {
+		if (no_index == GREP_NO_INDEX_IMPLICIT)
+			die(_("--cached or --untracked cannot be used outside "
+			      "of a git repository."));
+		else
+			die(_("--cached or --untracked cannot be used with "
+			      "--no-index or --no-index=never."));
 	}
 
 	if (no_index || untracked) {
@@ -894,7 +914,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			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."));
+				die(_("--no-index, --use-index=never or "
+				      "--untracked cannot be used with revs."));
 		}
 		hit = grep_directory(&opt, &pathspec, use_exclude);
 	} else if (0 <= opt_exclude) {
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 4ba955d..9776f15 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -794,9 +794,11 @@ 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
+		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
 	) &&
@@ -806,11 +808,12 @@ 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 &&
 
 		{
-			echo ".gitignore:.*o*"
+			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
 		git grep --no-index --no-exclude o >../actual.full &&
@@ -818,24 +821,67 @@ test_expect_success 'outside of git repository' '
 	)
 '
 
-test_expect_success 'outside of git repository without --no-index' '
+test_expect_success 'outside of git repository with --use-index=optional' '
 	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 &&
+	cat <<-\EOF >non/expect.full &&
+	file1:hello
+	sub/file2:world
+	EOF
+	echo file2:world >non/expect.sub &&
+	(
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non/git &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=optional o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+		cd sub &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=optional 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 &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=optional --exclude-standard o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+
+		{
+			echo ".gitignore:.*o*" &&
+			cat ../expect.full
+		} >../expect.with.ignored &&
+		git grep --use-index=optional --no-exclude o >../actual.full &&
+		test_cmp ../expect.with.ignored ../actual.full
+	)
+'
+
+test_expect_success 'outside of git repository with --use-index=never' '
+	rm -fr non &&
+	mkdir -p non/git/sub &&
+	echo hello >non/git/file1 &&
+	echo world >non/git/sub/file2 &&
+	cat <<-\EOF >non/expect.full &&
+	file1:hello
+	sub/file2:world
+	EOF
 	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
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=never o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
 		cd sub &&
-		git grep o >../../actual.sub &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=never o >../../actual.sub &&
 		test_cmp ../../expect.sub ../../actual.sub
 	) &&
 
@@ -844,14 +890,15 @@ test_expect_success 'outside of git repository without --no-index' '
 		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
-		git grep --exclude-standard o >../actual.full &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=never --exclude-standard o >../actual.full &&
 		test_cmp ../expect.full ../actual.full &&
 
 		{
-			echo ".gitignore:.*o*"
+			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git grep --no-exclude o >../actual.full &&
+		git grep --use-index=never --no-exclude o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '

Thomas Gummerer (3):
  t7810: correct --no-index test
  builtin/grep: rename use_index to no_index
  builtin/grep: introduce --use-index argument

 Documentation/git-grep.txt | 13 +++++++
 builtin/grep.c             | 57 +++++++++++++++++++++++------
 t/t7810-grep.sh            | 90 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 146 insertions(+), 14 deletions(-)

-- 
2.7.0.3.g214d8d0.dirty

[PATCH v2 1/3] t7810: correct --no-index test

[Thomas Gummerer]

GIT_CEILING_DIRECTORIES doesn't prevent chdir up into another directory
while looking for a repository directory if it is equal to the current
directory.  Because of this, the test which claims to test the git grep
--no-index command outside of a repository actually tests it inside of a
repository.  The test_must_fail assertions still pass because the git
grep only looks at untracked files and therefore no file matches, but
not because it's run outside of a repository as it was originally
intended.

Set the GIT_CEILING_DIRECTORIES environment variable to the parent
directory of the directory in which the git grep command is executed, to
make sure it is actually run outside of a git repository.

In addition the && chain was broken in a couple of places, fix that.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 t/t7810-grep.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 028ffe4..cc4b97d 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -791,12 +791,12 @@ test_expect_success 'outside of git repository' '
 	} >non/expect.full &&
 	echo file2:world >non/expect.sub &&
 	(
-		GIT_CEILING_DIRECTORIES="$(pwd)/non/git" &&
+		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
+		test_cmp ../expect.full ../actual.full &&
 		cd sub &&
 		test_must_fail git grep o &&
 		git grep --no-index o >../../actual.sub &&
@@ -805,7 +805,7 @@ test_expect_success 'outside of git repository' '
 
 	echo ".*o*" >non/git/.gitignore &&
 	(
-		GIT_CEILING_DIRECTORIES="$(pwd)/non/git" &&
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
 		test_must_fail git grep o &&
@@ -813,7 +813,7 @@ test_expect_success 'outside of git repository' '
 		test_cmp ../expect.full ../actual.full &&
 
 		{
-			echo ".gitignore:.*o*"
+			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
 		git grep --no-index --no-exclude o >../actual.full &&
-- 
2.7.0.3.g214d8d0.dirty

[PATCH v2 2/3] builtin/grep: rename use_index to no_index

[Thomas Gummerer]

Switch the OPT_NEGBIT argument to a OPT_BIT argument, and rename the
corresponding use_index variable to a no_index variable.  This will make
the following step easier to follow.  No functional changes intended.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/grep.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 4229cae..3a27bd5 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -625,14 +625,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	struct string_list path_list = STRING_LIST_INIT_NODUP;
 	int i;
 	int dummy;
-	int use_index = 1;
+	int no_index = 0;
 	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
 			N_("search in index instead of in the work tree")),
-		OPT_NEGBIT(0, "no-index", &use_index,
-			 N_("find in contents not managed by git"), 1),
+		OPT_BIT(0, "no-index", &no_index,
+			N_("find in contents not managed by git"), 1),
 		OPT_BOOL(0, "untracked", &untracked,
 			N_("search in both tracked and untracked files")),
 		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
@@ -755,7 +755,7 @@ 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)
+	if (!no_index && !startup_info->have_repository)
 		/* die the same way as if we did it at the beginning */
 		setup_git_directory();
 
@@ -873,11 +873,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	if (!show_in_pager && !opt.status_only)
 		setup_pager();
 
-	if (!use_index && (untracked || cached))
+	if (no_index && (untracked || cached))
 		die(_("--cached or --untracked cannot be used with --no-index."));
 
-	if (!use_index || untracked) {
-		int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
+	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."));
 		hit = grep_directory(&opt, &pathspec, use_exclude);
-- 
2.7.0.3.g214d8d0.dirty

[PATCH v2 3/3] builtin/grep: introduce --use-index argument

[Thomas Gummerer]

Currently when git grep is used outside of a git repository without the
--no-index option git simply dies.  Introduce a new --use-index command line
argument, which can be set to always, optional, or never.  If it is set
to always, git grep continues to behave as it currently does.  If set to
optional, git grep will work normally if executed inside of a git
repository, and behave like git grep --no-index if used outside of a git
repository.  If set to never, git grep will behave like git grep
--no-index regardless of whether it is used inside or outside of a git
repository.

Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/git-grep.txt | 13 ++++++++
 builtin/grep.c             | 47 +++++++++++++++++++++++---
 t/t7810-grep.sh            | 82 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4a44d6d..90ff643 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -25,6 +25,7 @@ SYNOPSIS
 	   [-W | --function-context]
 	   [-f <file>] [-e] <pattern>
 	   [--and|--or|--not|(|)|-e <pattern>...]
+	   [--use-index=<always|optional|never>]
 	   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
 	   [--] [<pathspec>...]
 
@@ -66,6 +67,18 @@ OPTIONS
 --no-index::
 	Search files in the current directory that is not managed by Git.
 
+--use-index::
+	Set the behavior of git grep when used in- or outside of a git
+	repository.  If set to always, the command can only be used
+	inside of a git repository, and will fail if it is used
+	outside of a git repository.  If set to never, it will work
+	both inside and outside of a git repository, but will not use
+	any information in the index if used inside of a repository.
+	If set to optional, it will work both inside and outside of a
+	git repository, using the information available in the index
+	if used inside a repository.  Always and optional can be
+	overridden by `--no-index`.
+
 --untracked::
 	In addition to searching in the tracked files in the working
 	tree, search also in untracked files.
diff --git a/builtin/grep.c b/builtin/grep.c
index 3a27bd5..bb2add9 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
+
 static char const * const grep_usage[] = {
 	N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
 	NULL
@@ -626,13 +629,17 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	int i;
 	int dummy;
 	int no_index = 0;
+	const char *use_index = NULL;
 	int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED;
 
 	struct option options[] = {
 		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_STRING(0, "use-index", &use_index, N_("always|optional|never"),
+			   N_("options for using the index")),
 		OPT_BOOL(0, "untracked", &untracked,
 			N_("search in both tracked and untracked files")),
 		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
@@ -755,6 +762,25 @@ 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) {
+		if (!strcmp(use_index, "always")) {
+			/* same as git grep without --use-index */
+		} else if (!strcmp(use_index, "optional")) {
+			if (!no_index && !startup_info->have_repository) {
+				int nongit = 0;
+
+				setup_git_directory_gently(&nongit);
+				if (nongit)
+					no_index = GREP_NO_INDEX_IMPLICIT;
+			}
+		} else if (!strcmp(use_index, "never")) {
+			no_index = GREP_NO_INDEX_EXPLICIT;
+		} else {
+			die(_("invalid option \"%s\" for --use-index, expecting"
+			      " always, optional or never"), use_index);
+		}
+	}
+
 	if (!no_index && !startup_info->have_repository)
 		/* die the same way as if we did it at the beginning */
 		setup_git_directory();
@@ -873,13 +899,24 @@ 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 (no_index && (untracked || cached)) {
+		if (no_index == GREP_NO_INDEX_IMPLICIT)
+			die(_("--cached or --untracked cannot be used outside "
+			      "of a git repository."));
+		else
+			die(_("--cached or --untracked cannot be used with "
+			      "--no-index or --no-index=never."));
+	}
 
 	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, --use-index=never 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 cc4b97d..9776f15 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -821,6 +821,88 @@ test_expect_success 'outside of git repository' '
 	)
 '
 
+test_expect_success 'outside of git repository with --use-index=optional' '
+	rm -fr non &&
+	mkdir -p non/git/sub &&
+	echo hello >non/git/file1 &&
+	echo world >non/git/sub/file2 &&
+	cat <<-\EOF >non/expect.full &&
+	file1:hello
+	sub/file2:world
+	EOF
+	echo file2:world >non/expect.sub &&
+	(
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non/git &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=optional o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+		cd sub &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=optional 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 &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=optional --exclude-standard o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+
+		{
+			echo ".gitignore:.*o*" &&
+			cat ../expect.full
+		} >../expect.with.ignored &&
+		git grep --use-index=optional --no-exclude o >../actual.full &&
+		test_cmp ../expect.with.ignored ../actual.full
+	)
+'
+
+test_expect_success 'outside of git repository with --use-index=never' '
+	rm -fr non &&
+	mkdir -p non/git/sub &&
+	echo hello >non/git/file1 &&
+	echo world >non/git/sub/file2 &&
+	cat <<-\EOF >non/expect.full &&
+	file1:hello
+	sub/file2:world
+	EOF
+	echo file2:world >non/expect.sub &&
+	(
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non/git &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=never o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+		cd sub &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=never 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 &&
+		test_must_fail git grep --use-index=always o &&
+		git grep --use-index=never --exclude-standard o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+
+		{
+			echo ".gitignore:.*o*" &&
+			cat ../expect.full
+		} >../expect.with.ignored &&
+		git grep --use-index=never --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 &&
-- 
2.7.0.3.g214d8d0.dirty

[PATCH v3 0/2] grep: add fallbackToNoIndex config option

[Thomas Gummerer]

Previous rounds are at $gmane/283619 and $gmane/283670.  Thanks to
Junio and Peff for the input after the last round.

After the discussion I decided to drop the --use-index command line
argument, and use a fallbackToNoIndex config option instead.  In
addition I dropped the second patch, keeping the use_index name for
the variable used to keep track which mode should be used.

Thomas Gummerer (2):
  t7810: correct --no-index test
  builtin/grep: add grep.fallbackToNoIndex config

 Documentation/config.txt   |  4 ++++
 Documentation/git-grep.txt |  4 ++++
 builtin/grep.c             | 23 +++++++++++++++++-----
 t/t7810-grep.sh            | 49 ++++++++++++++++++++++++++++++++++++++++++----
 4 files changed, 71 insertions(+), 9 deletions(-)

-- 
2.7.0.2.gafd9bce.dirty

[PATCH v3 1/2] t7810: correct --no-index test

[Thomas Gummerer]

GIT_CEILING_DIRECTORIES doesn't prevent chdir up into another directory
while looking for a repository directory if it is equal to the current
directory.  Because of this, the test which claims to test the git grep
--no-index command outside of a repository actually tests it inside of a
repository.  The test_must_fail assertions still pass because the git
grep only looks at untracked files and therefore no file matches, but
not because it's run outside of a repository as it was originally
intended.

Set the GIT_CEILING_DIRECTORIES environment variable to the parent
directory of the directory in which the git grep command is executed, to
make sure it is actually run outside of a git repository.

In addition, the && chain was broken in a couple of places in the same
test, fix that.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 t/t7810-grep.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 028ffe4..cc4b97d 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -791,12 +791,12 @@ test_expect_success 'outside of git repository' '
 	} >non/expect.full &&
 	echo file2:world >non/expect.sub &&
 	(
-		GIT_CEILING_DIRECTORIES="$(pwd)/non/git" &&
+		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
+		test_cmp ../expect.full ../actual.full &&
 		cd sub &&
 		test_must_fail git grep o &&
 		git grep --no-index o >../../actual.sub &&
@@ -805,7 +805,7 @@ test_expect_success 'outside of git repository' '
 
 	echo ".*o*" >non/git/.gitignore &&
 	(
-		GIT_CEILING_DIRECTORIES="$(pwd)/non/git" &&
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
 		test_must_fail git grep o &&
@@ -813,7 +813,7 @@ test_expect_success 'outside of git repository' '
 		test_cmp ../expect.full ../actual.full &&
 
 		{
-			echo ".gitignore:.*o*"
+			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
 		git grep --no-index --no-exclude o >../actual.full &&
-- 
2.7.0.2.gafd9bce.dirty

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

[Thomas Gummerer]

Currently when git grep is used outside of a git repository without the
--no-index option git simply dies.  For convenience, add a
grep.fallbackToNoIndex configuration variable.  If set to true, git grep
behaves like git grep --no-index if it is run outside of a git
repository.  It defaults to false, preserving the current behavior.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/config.txt   |  4 ++++
 Documentation/git-grep.txt |  4 ++++
 builtin/grep.c             | 23 ++++++++++++++++++-----
 t/t7810-grep.sh            | 41 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f617886..8d51f80 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1450,6 +1450,10 @@ grep.extendedRegexp::
 	option is ignored when the 'grep.patternType' option is set to a value
 	other than 'default'.
 
+grep.fallbackToNoIndex::
+	If set to true, fall back to git grep --no-index if git grep
+	is executed outside of a git repository.  Defaults to false.
+
 gpg.program::
 	Use this custom program instead of "gpg" found on $PATH when
 	making or verifying a PGP signature. The program must support the
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4a44d6d..15b9033 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -56,6 +56,10 @@ grep.extendedRegexp::
 grep.fullName::
 	If set to true, enable '--full-name' option by default.
 
+grep.fallbackToNoIndex::
+	If set to true, fall back to git grep --no-index if git grep
+	is executed outside of a git repository.  Defaults to false.
+
 
 OPTIONS
 -------
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();
+		}
+	}
 
 	/*
 	 * skip a -- separator; we know it cannot be
@@ -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"));
 
 	if (!use_index || untracked) {
 		int use_exclude = (opt_exclude < 0) ? use_index : !!opt_exclude;
 		if (list.nr)
-			die(_("--no-index or --untracked cannot be used with revs."));
+			die(_("git grep outside of a repository, --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 cc4b97d..b540944 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -821,6 +821,47 @@ test_expect_success 'outside of git repository' '
 	)
 '
 
+test_expect_success 'outside of git repository with fallbackToNoIndex' '
+	rm -fr non &&
+	mkdir -p non/git/sub &&
+	echo hello >non/git/file1 &&
+	echo world >non/git/sub/file2 &&
+	cat <<-\EOF >non/expect.full &&
+	file1:hello
+	sub/file2:world
+	EOF
+	echo file2:world >non/expect.sub &&
+	(
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non/git &&
+		test_must_fail git -c grep.fallbackToNoIndex=false grep o &&
+		git -c grep.fallbackToNoIndex=true grep o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+		cd sub &&
+		test_must_fail git -c grep.fallbackToNoIndex=false grep o &&
+		git -c grep.fallbackToNoIndex=true 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 &&
+		test_must_fail git -c grep.fallbackToNoIndex=false grep o &&
+		git -c grep.fallbackToNoIndex=true grep --exclude-standard o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+
+		{
+			echo ".gitignore:.*o*" &&
+			cat ../expect.full
+		} >../expect.with.ignored &&
+		git -c grep.fallbackToNoIndex 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 &&
-- 
2.7.0.2.gafd9bce.dirty

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

[Jeff King]

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

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

[Thomas Gummerer]

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

[PATCH v4 0/2] grep: add fallbackToNoIndex config option

[Thomas Gummerer]

Previous rounds are at $gmane/283619 and $gmane/283670. (Gmane seems
to lag behind, I can't seem to find v3 there). Thanks to Peff for the
review of the previous round.

Changes since the previous round:
  - removed unnecessary call to setup_git_directory_gently()
  - reverted the change to the error messages, as the originals should
    provide enough information to the user.

Thomas Gummerer (2):
  t7810: correct --no-index test
  builtin/grep: add grep.fallbackToNoIndex config

 Documentation/config.txt   |  4 ++++
 Documentation/git-grep.txt |  4 ++++
 builtin/grep.c             | 12 +++++++++---
 t/t7810-grep.sh            | 49 ++++++++++++++++++++++++++++++++++++++++++----
 4 files changed, 62 insertions(+), 7 deletions(-)

-- 
2.7.0.2.gcdcca30.dirty

[PATCH v4 1/2] t7810: correct --no-index test

[Thomas Gummerer]

GIT_CEILING_DIRECTORIES doesn't prevent chdir up into another directory
while looking for a repository directory if it is equal to the current
directory.  Because of this, the test which claims to test the git grep
--no-index command outside of a repository actually tests it inside of a
repository.  The test_must_fail assertions still pass because the git
grep only looks at untracked files and therefore no file matches, but
not because it's run outside of a repository as it was originally
intended.

Set the GIT_CEILING_DIRECTORIES environment variable to the parent
directory of the directory in which the git grep command is executed, to
make sure it is actually run outside of a git repository.

In addition, the && chain was broken in a couple of places in the same
test, fix that.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 t/t7810-grep.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 028ffe4..cc4b97d 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -791,12 +791,12 @@ test_expect_success 'outside of git repository' '
 	} >non/expect.full &&
 	echo file2:world >non/expect.sub &&
 	(
-		GIT_CEILING_DIRECTORIES="$(pwd)/non/git" &&
+		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
+		test_cmp ../expect.full ../actual.full &&
 		cd sub &&
 		test_must_fail git grep o &&
 		git grep --no-index o >../../actual.sub &&
@@ -805,7 +805,7 @@ test_expect_success 'outside of git repository' '
 
 	echo ".*o*" >non/git/.gitignore &&
 	(
-		GIT_CEILING_DIRECTORIES="$(pwd)/non/git" &&
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non/git &&
 		test_must_fail git grep o &&
@@ -813,7 +813,7 @@ test_expect_success 'outside of git repository' '
 		test_cmp ../expect.full ../actual.full &&
 
 		{
-			echo ".gitignore:.*o*"
+			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
 		git grep --no-index --no-exclude o >../actual.full &&
-- 
2.7.0.2.gcdcca30.dirty

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

[Thomas Gummerer]

Currently when git grep is used outside of a git repository without the
--no-index option git simply dies.  For convenience, add a
grep.fallbackToNoIndex configuration variable.  If set to true, git grep
behaves like git grep --no-index if it is run outside of a git
repository.  It defaults to false, preserving the current behavior.

Helped-by: Jeff King <peff@peff.net>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 Documentation/config.txt   |  4 ++++
 Documentation/git-grep.txt |  4 ++++
 builtin/grep.c             | 12 +++++++++---
 t/t7810-grep.sh            | 41 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f617886..8d51f80 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1450,6 +1450,10 @@ grep.extendedRegexp::
 	option is ignored when the 'grep.patternType' option is set to a value
 	other than 'default'.
 
+grep.fallbackToNoIndex::
+	If set to true, fall back to git grep --no-index if git grep
+	is executed outside of a git repository.  Defaults to false.
+
 gpg.program::
 	Use this custom program instead of "gpg" found on $PATH when
 	making or verifying a PGP signature. The program must support the
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4a44d6d..15b9033 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -56,6 +56,10 @@ grep.extendedRegexp::
 grep.fullName::
 	If set to true, enable '--full-name' option by default.
 
+grep.fallbackToNoIndex::
+	If set to true, fall back to git grep --no-index if git grep
+	is executed outside of a git repository.  Defaults to false.
+
 
 OPTIONS
 -------
diff --git a/builtin/grep.c b/builtin/grep.c
index 4229cae..6b7add6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -755,9 +755,15 @@ 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)
+			use_index = 0;
+		else
+			/* die the same way as if we did it at the beginning */
+			setup_git_directory();
+	}
 
 	/*
 	 * skip a -- separator; we know it cannot be
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index cc4b97d..b540944 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -821,6 +821,47 @@ test_expect_success 'outside of git repository' '
 	)
 '
 
+test_expect_success 'outside of git repository with fallbackToNoIndex' '
+	rm -fr non &&
+	mkdir -p non/git/sub &&
+	echo hello >non/git/file1 &&
+	echo world >non/git/sub/file2 &&
+	cat <<-\EOF >non/expect.full &&
+	file1:hello
+	sub/file2:world
+	EOF
+	echo file2:world >non/expect.sub &&
+	(
+		GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd non/git &&
+		test_must_fail git -c grep.fallbackToNoIndex=false grep o &&
+		git -c grep.fallbackToNoIndex=true grep o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+		cd sub &&
+		test_must_fail git -c grep.fallbackToNoIndex=false grep o &&
+		git -c grep.fallbackToNoIndex=true 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 &&
+		test_must_fail git -c grep.fallbackToNoIndex=false grep o &&
+		git -c grep.fallbackToNoIndex=true grep --exclude-standard o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+
+		{
+			echo ".gitignore:.*o*" &&
+			cat ../expect.full
+		} >../expect.with.ignored &&
+		git -c grep.fallbackToNoIndex 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 &&
-- 
2.7.0.2.gcdcca30.dirty

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

[Jeff King]

On Tue, Jan 12, 2016 at 11:40:26AM +0100, Thomas Gummerer wrote:

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 4229cae..6b7add6 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -755,9 +755,15 @@ 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)
> +			use_index = 0;
> +		else
> +			/* die the same way as if we did it at the beginning */
> +			setup_git_directory();
> +	}
>  
>  	/*
>  	 * skip a -- separator; we know it cannot be

Nice. This turned out delightfully simple.

-Peff

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

[Thomas Gummerer]

On 01/12, Jeff King wrote:
> On Tue, Jan 12, 2016 at 11:40:26AM +0100, Thomas Gummerer wrote:
>
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 4229cae..6b7add6 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -755,9 +755,15 @@ 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)
> > +			use_index = 0;
> > +		else
> > +			/* die the same way as if we did it at the beginning */
> > +			setup_git_directory();
> > +	}
> >
> >  	/*
> >  	 * skip a -- separator; we know it cannot be
>
> Nice. This turned out delightfully simple.

Indeed, much simpler than I expected.  Thanks for the help getting it there.

> -Peff