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

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

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