Re: [PATCH/RFC v2] grep: Add the option '--exclude'

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 29, 2012, 23:02 UTC
Message-ID: <7vvcnudsda.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1327876934-61526-1-git-send-email-surfingalbert@gmail.com>

Albert Yale <surfingalbert@gmail.com> writes:

> Feedback would again be appreciated,

Hmm.

>  	   [-f <file>] [-e] <pattern>
> +	   [-x<pattern>|--exclude<pattern>]

Compare the above two lines and notice some funny in the added one?

Show 10 quoted lines

> @@ -124,6 +125,12 @@ OPTIONS
>  	Use fixed strings for patterns (don't interpret pattern
>  	as a regex).
>  
> +-x<pattern>::
> +--exclude<pattern>::
> +	In addition to those found in .gitignore (per directory) and
> +	$GIT_DIR/info/exclude, also consider these patterns to be in the
> +	set of the ignore rules in effect.
> +

Likewise.

Show 12 quoted lines

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 9ce064a..9772fa4 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -530,6 +530,10 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
>  			continue;
>  		if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))
>  			continue;
> +		if (pathspec->exclude &&
> +			pathspec->exclude->nr &&
> +			match_pathspec_depth(pathspec->exclude, ce->name, ce_namelen(ce), 0, NULL))
> +			continue;

Why isn't this just

	if (!match_pathspec_depth(pathspec, ce->name, ce_namelen(ce), 0, NULL))

that is, *no change whatsoever* on the existing codepaths that call match_pathspec_depth()? Can't the "even if one of the positive pathspec matched, if one of the excluded one matches, declare that the path does *not* match" logic live in match_pathspec_depth() itself?

Exactly the same comment applies to all the other additions that calls match_pathspec_depth() with pathspec->exclude as its first parameter in this patch.

Show 10 quoted lines

> @@ -1053,6 +1084,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	pathspec.max_depth = opt.max_depth;
>  	pathspec.recursive = 1;
>  
> +	if( exclude_list.nr ) {
> +		create_pathspec_from_string_list(&pathspec.exclude, &exclude_list);
> +		pathspec.exclude->max_depth = opt.max_depth;
> +		pathspec.exclude->recursive = 1;
> +	}
> +

Style. Notice where SPs should be near parentheses on "if" statement in our codebase.