Re: [PATCH v2 10/11] ref-filter: introduce contents_atom_parser()
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:07:30
On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Introduce contents_atom_parser() which will parse the '%(contents)' atom and store information into the 'used_atom' structure based on the modifiers used along with the atom. Helped-by: Ramsay Jones [off-list ref] Signed-off-by: Karthik Nayak <redacted> ---diff --git a/ref-filter.c b/ref-filter.c@@ -39,6 +39,10 @@ static struct used_atom { struct align align; enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL } remote_ref; + struct { + enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option; + unsigned int no_lines;
'no_lines' sounds like "no lines!". How about 'nlines' instead?
quoted hunk ↗ jump to hunk
+ } contents; } u; } *used_atom; static int used_atom_cnt, need_tagged, need_symref;@@ -90,6 +94,36 @@ static void remote_ref_atom_parser(struct used_atom *atom) +static void contents_atom_parser(struct used_atom *atom) +{ + const char * buf; + + if (match_atom_name(atom->str, "contents", &buf)) + atom->u.contents.option = C_BARE; + else if (match_atom_name(atom->str, "subject", &buf)) {
The original code used strcmp() and matched only "subject", however the new code will incorrectly match both "subject" and "subject:whatever". Therefore, you should be using strcmp() here rather than match_atom_name(). Ditto for "body".
+ atom->u.contents.option = C_SUB;
+ return;
+ } else if (match_atom_name(atom->str, "body", &buf)) {
+ atom->u.contents.option = C_BODY_DEP;
+ return;
+ }
+ if (!buf)
+ return;
It's not easy to see that this 'if (!buf)' check relates to the
"contents" check at the very top of the if/else if/ chain since there
are entirely unrelated checks in between. Reorganizing it can improve
clarity:
if (!strcmp("subject")) {
...
return;
} else if (!strcmp("body")) {
...
return;
} else if (!match_atom_name(...,"contents", &buf))
die("BUG: expected 'contents' or 'contents:'");
if (!buf) {
atom->u.contents.option = C_BARE;
return;
}
quoted hunk ↗ jump to hunk
+ if (!strcmp(buf, "body")) + atom->u.contents.option = C_BODY; + else if (!strcmp(buf, "signature")) + atom->u.contents.option = C_SIG; + else if (!strcmp(buf, "subject")) + atom->u.contents.option = C_SUB; + else if (skip_prefix(buf, "lines=", &buf)) { + atom->u.contents.option = C_LINES; + if (strtoul_ui(buf, 10, &atom->u.contents.no_lines)) + die(_("positive value expected contents:lines=%s"), buf); + } else + die(_("unrecognized %%(contents) argument: %s"), buf); +}@@ -777,28 +801,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj &bodypos, &bodylen, &nonsiglen, &sigpos, &siglen); - if (!strcmp(name, "subject")) + if (atom->u.contents.option == C_SUB) v->s = copy_subject(subpos, sublen); - else if (!strcmp(name, "contents:subject")) - v->s = copy_subject(subpos, sublen); - else if (!strcmp(name, "body")) + else if (atom->u.contents.option == C_BODY_DEP) v->s = xmemdupz(bodypos, bodylen); - else if (!strcmp(name, "contents:body")) + else if (atom->u.contents.option == C_BODY) v->s = xmemdupz(bodypos, nonsiglen); - else if (!strcmp(name, "contents:signature")) + else if (atom->u.contents.option == C_SIG) v->s = xmemdupz(sigpos, siglen); - else if (!strcmp(name, "contents")) - v->s = xstrdup(subpos); - else if (skip_prefix(name, "contents:lines=", &valp)) { + else if (atom->u.contents.option == C_LINES) { struct strbuf s = STRBUF_INIT; const char *contents_end = bodylen + bodypos - siglen; - if (strtoul_ui(valp, 10, &v->u.contents.lines)) - die(_("positive value expected contents:lines=%s"), valp); /* Size is the length of the message after removing the signature */ - append_lines(&s, subpos, contents_end - subpos, v->u.contents.lines); + append_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines); v->s = strbuf_detach(&s, NULL); - } + } else if (atom->u.contents.option == C_BARE) + v->s = xstrdup(subpos); } } --2.6.4