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

[Junio C Hamano <gitster@pobox.com>]

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

> I thought of tests when I wrote the patch, but it was hard to find a
> reliable way to detect if a git build supports external grep.

Ah, you are right.  I forgot about that issue.

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