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