Re: [PATCH] grep: do not do external grep on skip-worktree entries
- From
Nguyen Thai Ngoc Duy <pclouds@gmail.com>
- Date
- Jan 2, 2010, 11:50 UTC
- Message-ID
- <20100102115041.GA32381@do>
- In-Reply-To
- <7vzl4zy5z3.fsf@alter.siamese.dyndns.org>
On Wed, Dec 30, 2009 at 11:09:52PM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> 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 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