Re: [PATCH] grep: do not do external grep on skip-worktree entries

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

On 1/3/10, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>  > On Wed, Dec 30, 2009 at 11:09:52PM -0800, Junio C Hamano wrote:
>  >> Junio C Hamano <gitster@pobox.com> writes:
>  >>
>  >> > This looks a bit wrong for a couple of reasons:
>  >> >
>  >> >  - external_grep() is designed to return negative without running external
>  >> >    grep when it shouldn't be used (see the beginning of the function for
>  >> >    how it refuses to run when opt->extended is set and other conditions).
>  >> >    The new logic seems to belong there, i.e. "in addition to the existing
>  >> >    case we decline, if ce_skip_worktree() entry exists in the cache, we
>  >> >    decline";
>  >>
>  >> IOW, something like this instead of your patch.  You would want to tests
>  >> to demonstrate the original breakage, perhaps?
>  >
>  > Your patch works great. By the way I think we should move "cached"
>  > check from grep_cache() into external_grep() too, for consistency.
>
>
> I think what I gave you is more consistent.
>
>  "cached" is about "are we searching in the index or in the work tree?" and
>  "external_grep()" is about "it often is faster to run external grep when
>  we are searching in the work tree, so do that if the other constraints
>  allow us to".  An example of such a constraint is that we must be able to
>  express the operation on the command line of a traditional grep, and use
>  of git extended grep synatx makes it unfeasible, hence the function says
>  "I can't" in such a case.
>
>  You can change the definition of "external_grep()" to "try to use external
>  grep somewhere, if the other constraints allow us to", and have the caller
>  and the function do an "this time, please grep for this pattern in the
>  index---I can't" exchange, but I don't see much point.  We _know_ no
>  external grep will ever be able to read from the index.
>
>  We should simply drop "cached" argument from external_grep().

OK.

>  > diff --git a/t/t7002-grep.sh b/t/t7002-grep.sh
>  > index abd14bf..f77970c 100755
>  > --- a/t/t7002-grep.sh
>  > +++ b/t/t7002-grep.sh
>  > @@ -8,6 +8,14 @@ test_description='git grep various.
>  >
>  >  . ./test-lib.sh
>  >
>  > +support_external_grep() {
>  > +     case "$(git grep -h 2>&1 >/dev/null|grep -e --ext-grep)" in
>  > +     *"(default)"*)  return 0;;
>  > +     *"(ignored by this build)"*) return 1;;
>  > +     *) test_expect_success 'External grep check is broken' 'false';;
>  > +     esac
>  > +}
>
>
> Heh, clever.
>
>         git grep -h 2>&1 | grep 'allow calling of grep.*default' >/dev/null
>
>  may be sufficient, though.
>

Yes, until somebody changes help text in builtin-grep.c and all
external grep tests become disable. I wanted to catch that case too.

Or maybe just add another option --show-features to "git rev-parse"
(or extend git version string, is the version format fixed?) to list
all built-time features that a git build supports. Some comes to mind:
external-grep, iconv, ipv6, http (unlikely), threading... Much less
tricky to test.
-- 
Duy