Re: [PATCH] grep: do not do external grep on skip-worktree entries
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:57
Nguyễn Thái Ngọc Duy [off-list ref] writes:
quoted hunk
Skip-worktree entries are not on disk. There is no reason to call external grep in such cases. A trace message is also added to aid debugging external grep cases. Signed-off-by: Nguyễn Thái Ngọc Duy <redacted> --- builtin-grep.c | 19 ++++++++++++++++++- 1 files changed, 18 insertions(+), 1 deletions(-)diff --git a/builtin-grep.c b/builtin-grep.c index d582232..d49c637 100644 --- a/builtin-grep.c +++ b/builtin-grep.c@@ -488,17 +488,34 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached, read_cache(); #if !NO_EXTERNAL_GREP + if (cached) + external_grep_allowed = 0; + if (external_grep_allowed) { + for (nr = 0; nr < active_nr; nr++) { + struct cache_entry *ce = active_cache[nr]; + if (!S_ISREG(ce->ce_mode)) + continue; + if (!pathspec_matches(paths, ce->name, opt->max_depth)) + continue; + if (ce_skip_worktree(ce)) { + external_grep_allowed = 0; + break; + } + } + } /* * Use the external "grep" command for the case where * 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; hit = 0; }
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"; - It should be a separate helper function, has_skip_worktree_entry(paths); I wouldn't be surprised if there are some other codepaths that want to check for the same condition and do things differently, not just grep. Originally I thought S_ISREG() check and pathspec_matches() check were also questionable, but they actually are good things to have, as we skip symbolic links (or submodules) and we do want to use external grep if the subtree we are grepping in do not have skip_worktree entries even when some other parts of the index are marked as skip_worktree.
+ else
+ trace_printf("grep: external grep not used\n");
#endif
for (nr = 0; nr < active_nr; nr++) {
--
1.6.6.315.g1a406