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.
> > @@ -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...
> > @@ -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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
