Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()

[Karthik Nayak <karthik.188@gmail.com>]

On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce objectname_atom_parser() which will parse the
>> '%(objectname)' atom and store information into the 'used_atom'
>> structure based on the modifiers used along with the atom.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -43,6 +43,7 @@ static struct used_atom {
>>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>                         unsigned int no_lines;
>>                 } contents;
>> +               enum { O_FULL, O_SHORT } objectname;
>>         } u;
>>  } *used_atom;
>> @@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom)
>> +static void objectname_atom_parser(struct used_atom *atom)
>> +{
>> +       const char * buf;
>> +
>> +       if (match_atom_name(atom->str, "objectname", &buf))
>> +               atom->u.objectname = O_FULL;
>> +       if (!buf)
>> +               return;
>
> Let me make sure that I understand this correctly.
>
> make_atom_name("objectname") will return true only for "objectname" or
> "objectname:", and will return false for anything else, such as
> "objectnamely" or "schmorf". Furthermore, the only way
> objectname_atom_parser() can be called is when %(objectname) or
> %(objectname:...) is seen, thus match_atom_name() *must* return true
> here, which means the above conditional is misleading, suggesting that
> it could somehow return false.
>
> And, if match_atom_name() did return false here, then that indicates a
> programming error: objectname_atom_parser() somehow got called for
> something other than %(objectname) or %(objectname:...). This implies
> that the code should instead be structured like this:
>
>     if (!match_atom_name(..., "objectname", &buf)
>         die("BUG: parsing non-'objectname'")
>     if (!buf)
>         atom->u.objectname = O_FULL;
>     else if (!strcmp(buf, "short"))
>         atom->u.objectname = O_SHORT;
>     else
>         die(_("unrecognized %%(objectname) argument: %s"), buf);
>
> However, this can be simplified further by recognizing that, following
> this patch series, match_atom_name() is *only* called by these new
> parse functions[1], which means that, as a convenience,
> match_atom_name() itself could become a void rather than boolean
> function and die() if the expected atom name is not found. Thus, the
> code would become:
>
>     match_atom_name(...);
>     if (!buf)
>         ...
>     else if (!strcmp(...))
>         ...
>     ...
>
> By the way, the above commentary isn't specific to this patch and
> %(objectname), but is in fact also relevant for all of the preceding
> patches which introduce parse functions calling match_atom_name().
>

Ah! Thats some good observation, makes sense, match_atom_name()
is only called after the atom name, so making it return a indication of
success or failure doesn't make sense.

I think this change would go nicely with the introduction of 'enum atom_type'
which you mentioned[0] in the previous series.

[0]: http://article.gmane.org/gmane.comp.version-control.git/282320

> More below...
>
> [1]: ...assuming you replace the unnecessary match_atom_name() in
> populate_value() with starts_with() as suggested in my patch 7/11
> review addendum[2].
>
> [2]: http://article.gmane.org/gmane.comp.version-control.git/282700
>
>> +
>> +       if (!strcmp(buf, "short"))
>> +               atom->u.objectname = O_SHORT;
>> +       else
>> +               die(_("unrecognized %%(objectname) argument: %s"), buf);
>> +}
>> +
>> @@ -461,15 +477,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo
>>  static int grab_objectname(const char *name, const unsigned char *sha1,
>> -                           struct atom_value *v)
>> +                          struct atom_value *v, struct used_atom *atom)
>>  {
>> -       if (!strcmp(name, "objectname")) {
>> -               v->s = xstrdup(sha1_to_hex(sha1));
>> -               return 1;
>> -       }
>> -       if (!strcmp(name, "objectname:short")) {
>> -               v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>> -               return 1;
>> +       if (starts_with(name, "objectname")) {
>> +               if (atom->u.objectname == O_SHORT) {
>> +                       v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>> +                       return 1;
>> +               } else if (atom->u.objectname == O_FULL) {
>> +                       v->s = xstrdup(sha1_to_hex(sha1));
>> +                       return 1;
>> +               }
>
> Since 'objectname' can only ever be O_SHORT or O_FULL wouldn't it be a
> programming error if it ever falls through to this point after the
> closing brace? Perhaps a die("BUG:...") would be appropriate?
>

Yeah, will add that in.

>>         }
>>         return 0;
>>  }
>> @@ -493,7 +510,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
>>                         v->s = xstrfmt("%lu", sz);
>>                 }
>>                 else if (deref)
>> -                       grab_objectname(name, obj->sha1, v);
>> +                       grab_objectname(name, obj->sha1, v, &used_atom[i]);
>
> This patch hunk is somehow corrupt and doesn't apply. Was there some
> hand-editing involved, or were some earlier patches regenerated after
> this patch was made or something?
>

Nothing of that sort, weird.

>>         }
>>  }
>>
>> @@ -999,7 +1016,7 @@ static void populate_value(struct ref_array_item *ref)
>>                                 v->s = xstrdup(buf + 1);
>>                         }
>>                         continue;
>> -               } else if (!deref && grab_objectname(name, ref->objectname, v)) {
>> +               } else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {
>>                         continue;
>>                 } else if (!strcmp(name, "HEAD")) {
>>                         const char *head;
>> --
>> 2.6.4



-- 
Regards,
Karthik Nayak