On 1/3/10, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Nguyen Thai Ngoc Duy <pclouds@xxxxxxxxx> writes:
>
> > On Wed, Dec 30, 2009 at 11:09:52PM -0800, Junio C Hamano wrote:
> >> Junio C Hamano <gitster@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html