Re: [PATCH] revision: allow selection of commits that do not match a pattern

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sat, 7 Jul 2007, Sven Verdoolaege wrote:

> We do this by maintaining two lists of patterns, one for
> those that should match and one for those that should not match.

I would at least give one example in the commit message

> diff --git a/revision.c b/revision.c
> index 5184716..4b00ada 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -821,40 +821,65 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
>  	return 0;
>  }
>  
> -static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
> +static void add_grep_to_filter(struct grep_opt **filter, const char *ptn,
> +				enum grep_pat_token what)
>  {
> -	if (!revs->grep_filter) {
> +	if (!*filter) {

Why not keep it "add_grep", and do a

	struct grep_opt **filter = negated ? 
		&revs->grep_neg_filter : &revs->grep_filter;

Hm? You avoid an extra function that way.

> +static void add_header_grep(struct rev_info *revs, const char *field,
> +			    const char *pattern)
>  {
>  	char *pat;
>  	const char *prefix;
>  	int patlen, fldlen;
> +	int negated = 0;
>  
>  	fldlen = strlen(field);
>  	patlen = strlen(pattern);
>  	pat = xmalloc(patlen + fldlen + 10);
>  	prefix = ".*";
> +	if (*pattern == '!') {
> +		negated = 1;
> +		pattern++;
> +	}
> +	if (pattern[0] == '\\' && pattern[1] == '!')
> +		pattern++;
>  	if (*pattern == '^') {
>  		prefix = "";
>  		pattern++;
>  	}
>  	sprintf(pat, "^%s %s%s", field, prefix, pattern);
> -	add_grep(revs, pat, GREP_PATTERN_HEAD);
> +	add_grep(revs, pat, GREP_PATTERN_HEAD, negated);
>  }

The parsing for "!" is again duplicated in add_message_grep(). Why not put 
it into add_grep(), and do

	negated = *pattern == '!';
	sprintf(pat, "%s^%s %s%s", negated ? "!" : "", field, prefix, 
		pattern + negated);

instead? No need to change the signature of add_grep(), and all callers 
get the '!' feature for free.

> @@ -1249,6 +1277,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
>  		compile_grep_patterns(revs->grep_filter);
>  	}
>  
> +	if (revs->grep_neg_filter) {
> +		compile_grep_patterns(revs->grep_neg_filter);
> +	}
> +

Please lose the "{" and "}".

> @@ -1329,11 +1361,14 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
>  
>  static int commit_match(struct commit *commit, struct rev_info *opt)
>  {
> -	if (!opt->grep_filter)
> -		return 1;
> -	return grep_buffer(opt->grep_filter,
> +	return (!opt->grep_filter ||
> +		grep_buffer(opt->grep_filter,
> +			   NULL, /* we say nothing, not even filename */
> +			   commit->buffer, strlen(commit->buffer))) &&
> +	       (!opt->grep_neg_filter ||
> +		!grep_buffer(opt->grep_neg_filter,
>  			   NULL, /* we say nothing, not even filename */
> -			   commit->buffer, strlen(commit->buffer));
> +			   commit->buffer, strlen(commit->buffer)));
>  }

Urgh! That's not nice on my eyes.

Also, I suspect that the semantics are not yet clear, what should happen 
if all_match is unset.

BTW I suspect that a better way than having two filter lists is 
demonstrated in builtin-grep.c.

Ciao,
Dscho