Re: [PATCH] grep: do not do external grep on skip-worktree entries

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 31, 2009, 07:01 UTC
Message-ID: <7v637nzky0.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1262182304-19911-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 45 quoted lines

> 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@gmail.com>
> ---
>  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.

Show 7 quoted lines

> +	else
> +		trace_printf("grep: external grep not used\n");
>  #endif
>  
>  	for (nr = 0; nr < active_nr; nr++) {
> -- 
> 1.6.6.315.g1a406