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 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. Perhaps
this on top of yours? The way to check for external grep support isn't
nice, but it works.
diff --git a/builtin-grep.c b/builtin-grep.c
index f093b60..bc4500a 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -222,6 +222,7 @@ static int exec_grep(int argc, const char **argv)
int status;
argv[argc] = NULL;
+ trace_argv_printf(argv, "trace: grep:");
pid = fork();
if (pid < 0)
return pid;
@@ -371,7 +372,7 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)
struct grep_pat *p;
if (opt->extended || (opt->relative && opt->prefix_length)
- || has_skip_worktree_entry(opt, paths))
+ || cached || has_skip_worktree_entry(opt, paths))
return -1;
len = nr = 0;
push_arg("grep");
@@ -509,7 +510,7 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached,
* we grep through the checked-out files. It tends to
* be a lot more optimized
*/
- if (!cached && external_grep_allowed) {
+ if (external_grep_allowed) {
hit = external_grep(opt, paths, cached);
if (hit >= 0)
return hit;
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
+}
+
cat >hello.c <<EOF
#include <stdio.h>
int main(int argc, const char **argv)
@@ -426,4 +434,20 @@ test_expect_success 'grep -Fi' '
test_cmp expected actual
'
+if support_external_grep; then
+
+test_expect_success 'external grep' '
+ GIT_TRACE=2 git grep foo >/dev/null 2>actual &&
+ grep "trace: grep:.*foo" actual >/dev/null
+'
+test_expect_success 'no external grep when skip-worktree entries exist' '
+ git update-index --skip-worktree file &&
+ GIT_TRACE=2 git grep foo >/dev/null 2>actual &&
+ ! grep "trace: grep:" actual >/dev/null &&
+ git update-index --no-skip-worktree file
+'
+
+else
+ say "External grep tests skipped"
+fi
test_done
--
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