Re: [PATCH/RFC 09/10] ref-filter: introduce contents_atom_parser()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 13, 2015, 03:10 UTC
- Message-ID
- <CAPig+cTbT60cWk+=6EARte68mmQ2xkS5xFgsgh0FbpoueFcBbg@mail.gmail.com>
- In-Reply-To
- <1447271075-15364-10-git-send-email-Karthik.188@gmail.com>
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 22 quoted lines
> 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 <Karthik.188@gmail.com>
> ---
> 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.
Show 13 quoted lines
> } 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.
Show 12 quoted lines
> + 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?
Show 36 quoted lines
> +}
> +
> 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).
Show 23 quoted lines
> 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) */
Show 6 quoted lines
> + v->s = xstrdup(subpos); > } > } > > -- > 2.6.2