Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

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

On Wed, Sep 2, 2015 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>>>> +             die(_("format: `end` atom used without a supporting atom"));
>>>>>
>>>>> Not a show-stopper, but we may need some wordsmithing for "a
>>>>> supporting atom" here; an end-user would not know what it is.
>>>>
>>>> Probably something like "format: `end` atom should only be
>>>> used with modifier atoms".
>>>
>>> Between "supporting" and "modifier" I do not see much difference,
>>> though.
>>
>> I don't see how we could provide a better message, as %(end) atom
>> would be common to various atoms eventually.
>
> I said "not a show-stopper" without giving a suggestion exactly
> because I didn't (and I still don't) think either you or I can come
> up with a good wording ;-).  That is why the message was Cc'ed to
> the list for others to comment.

Haha okay.

>
>>>> Cause we wanted to provide an error for usage of "%(ailgn)" without any
>>>> subvalues as such.
>>>
>>> Wouldn't it be something that would be caught in the same codepath
>>> as what catches %(unrecognized) in the format string?
>
> One potential issue you have with prefix matching with "align" is
> that you can never have a different atom whose name happens to begin
> with that substring.  I think the best behaviour is for the higher
> level parser to recognize %(ATOM) and %(ATOM:anything) and nothing
> else for all ATOM in the valid atoms registry.  That would prevent
> %(ATOMfoo) from being handled as part of handling ATOM.
>
> And make the code consider %(ATOM) form as a short-hand for %(ATOM:)
> that does not have customizations.  Conceptually %(refname) is a
> %(refname:default).
>
> For an atom like 'align' that does not have a reasonable default,
> the parser for 'align' can notice that a required customization is
> missing and give an error that is specific to 'align'.
>
> So all calls in your code of the from
>
>         ... else if (skip_prefix(name, "align", &val)) {
>                 ...
>
> should become a call to helper that does more than skip_prefix().
>

This is what Eric suggested and we agreed on working on a general
solution to this problem eventually, seems like you beat me to it :)

Will incorporate this :)

> int match_atom_name(const char *name, const char *atom_name, char **val)
> {
>         char *body
>
>         if (!skip_prefix(name, atom_name, &body))
>                 return 0; /* doesn't even begin with "align" */
>         if (!body[0]) {
>                 *val = NULL; /* %(align) and no customization */
>                 return 1;
>         }
>         if (body[0] != ':')
>                 return 0; /* "alignfoo" is not "align" or "align:..." */
>         *val = body + 1; /* "align:val" */
>         return 1;
> }

Thanks.

-- 
Regards,
Karthik Nayak