On Sun, Dec 13, 2015 at 8:40 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> 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@xxxxxxxxx>
>> ---
>> 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.
>
Like I said, an enum would cause problems here.
If you see the code flow when we check for contents we currently (as
per this patch)
use the 'all' bit to denote usage of contents, but when using an enum we would
set the enum to the first value when using memset.
But this should go away with the 'enum atom_type' implementation which
you suggested.
>> } 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.
>
Will change that.
>> + 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?
>
Will do.
>> +}
>> +
>> 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).
>
Actually I missed that out, now that you mention it, i'll see how I
can come around this.
>> 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) */
>
Will do thanks.
--
Regards,
Karthik Nayak
--
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