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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 11, 2016, 01:54 UTC
Message-ID: <CAPig+cRXy=CBZBTC7fU5wHA3d9Acqh_WYD54DmY1sHj7rob9Pw@mail.gmail.com>
In-Reply-To: <1452435597-12099-4-git-send-email-t.gummerer@gmail.com>

On Sun, Jan 10, 2016 at 9:19 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:

Show 21 quoted lines

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

Show 24 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

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

Show 15 quoted lines

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

Show 6 quoted lines

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