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

From: Sven Verdoolaege <skimo@kotnet.org>
Date: Jul 7, 2007, 20:22 UTC
Message-ID: <20070707202200.GF1528MdfPADPa@greensroom.kotnet.org>
In-Reply-To: <Pine.LNX.4.64.0707071957370.4093@racer.site>

On Sat, Jul 07, 2007 at 08:35:35PM +0100, Johannes Schindelin wrote:

Show 6 quoted lines

> 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.

[..]

Show 10 quoted lines

> 
> 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.

I can do these things, but they don't exactly improve readability, IMHO.

Show 10 quoted lines

> > @@ -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 "}".

I may still need them for doing something with all_match...

Show 19 quoted lines

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

You prefer

	if (opt->grep_filter && !grep_buffer(opt->grep_filter,
			   NULL, /* we say nothing, not even filename */
			   commit->buffer, strlen(commit->buffer)))
	       return 0;
	if (opt->grep_neg_filter && grep_buffer(opt->grep_neg_filter,
			   NULL, /* we say nothing, not even filename */
			   commit->buffer, strlen(commit->buffer)));
	       return 0;
       return 1;

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

So what are the semantics of all_match without negated matches? It doesn't seem to be documented in git-rev-list.txt.

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

Could you be a bit more specific? If you're talking about the GREP_NOT thing, then AFAICS that is line based and I want these things to be commit based. That is I want to select commits with either a or no lines that match a given pattern and not commits that have a line that matches some patterns and not some others.

skimo