Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> 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 <pclouds@xxxxxxxxx>
> ---
> 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
--
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