Re: [PATCH/RFC 09/10] ref-filter: introduce contents_atom_parser()
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:07:26
On Wed, Nov 11, 2015 at 2:44 PM, 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. Signed-off-by: Karthik Nayak <redacted> ---diff --git a/ref-filter.c b/ref-filter.c index 802629b..117bbbb 100644 --- a/ref-filter.c +++ b/ref-filter.c@@ -42,6 +42,14 @@ static struct used_atom { track : 1, trackshort : 1; } remote_ref; + struct { + unsigned int subject : 1, + body : 1, + signature : 1, + all : 1, + lines : 1, + no_lines; + } contents;
Same question as patch 8/10: With the exception of non-bitfield 'no_lines', are these 'contents' flags mutually exclusive? If so, an enum would be a more natural representation than bitfields.
quoted hunk ↗ jump to hunk
} u; } *used_atom; static int used_atom_cnt, need_tagged, need_symref;@@ -92,6 +100,29 @@ void remote_ref_atom_parser(struct used_atom *atom) +void contents_atom_parser(struct used_atom *atom) +{ + const char * buf; + + if (match_atom_name(atom->str, "contents", &buf)) + atom->u.contents.all = 1; + + if (!buf) + return;
Hmm, I'd have placed the blank line after the 'if (!buf) return;' rather than before it.
+ if (!strcmp(buf, "body"))
+ atom->u.contents.body = 1;
+ else if (!strcmp(buf, "signature"))
+ atom->u.contents.signature = 1;
+ else if (!strcmp(buf, "subject"))
+ atom->u.contents.subject = 1;
+ else if (skip_prefix(buf, "lines=", &buf)) {
+ atom->u.contents.lines = 1;
+ if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))
+ die(_("positive value expected contents:lines=%s"), buf);
+ } else
+ die(_("improper format entered contents:%s"), buf);Perhaps a more grammatically-friendly error message?
quoted hunk ↗ jump to hunk
+} + static align_type get_align_position(const char *type) { if (!strcmp(type, "right"))@@ -761,20 +786,16 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0; for (i = 0; i < used_atom_cnt; i++) { - const char *name = used_atom[i].str; + struct used_atom *atom = &used_atom[i]; + const char *name = atom->str; struct atom_value *v = &val[i]; - const char *valp = NULL; if (!!deref != (*name == '*')) continue; if (deref) name++; if (strcmp(name, "subject") && strcmp(name, "body") && - strcmp(name, "contents") && - strcmp(name, "contents:subject") && - strcmp(name, "contents:body") && - strcmp(name, "contents:signature") && - !starts_with(name, "contents:lines=")) + !atom->u.contents.all) continue; if (!subpos) find_subpos(buf, sz,@@ -784,26 +805,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj if (!strcmp(name, "subject")) v->s = copy_subject(subpos, sublen); - else if (!strcmp(name, "contents:subject")) + else if (atom->u.contents.subject) v->s = copy_subject(subpos, sublen);
With the disclaimer that I haven't fully digested the existing logic, is there a reason that you don't also preprocess bare "subject" as you preprocess "contents:subject"? Isn't "subject" just historic aliases for "contents:subject"? A similar observation may be made about "body" and "contents:body", although I see they mean slightly different things (for, I suppose, historical reasons).
else if (!strcmp(name, "body"))
v->s = xmemdupz(bodypos, bodylen);
- else if (!strcmp(name, "contents:body"))
+ else if (atom->u.contents.body)
v->s = xmemdupz(bodypos, nonsiglen);
- else if (!strcmp(name, "contents:signature"))
+ else if (atom->u.contents.signature)
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.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 /* For %(contents) without modifiers */
Too many blanks following '/*'; downcase 'for' or drop it altogether:
/* bare %(contents) */
+ v->s = xstrdup(subpos);
}
}
--
2.6.2