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

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

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.

[..]

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.

quoted

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

quoted

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