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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Jan 11, 2016, 11:29 UTC
Message-ID: <20160111112944.GB10612@hank>
In-Reply-To: <CAPig+cRXy=CBZBTC7fU5wHA3d9Acqh_WYD54DmY1sHj7rob9Pw@mail.gmail.com>

On 01/10, Eric Sunshine wrote:

Show 27 quoted lines

> 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."));
	}

Show 33 quoted lines

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

Show 27 quoted lines

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

Show 6 quoted lines

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