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

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

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.

>>>> +             } else if (skip_prefix(name, "align", &valp)) {
>>>
>>> This looked as if you are willing to take %(align) in addition to
>>> %(align:...), but...
>>>
>>>> +                     struct align *align = &v->align;
>>>> +                     struct strbuf **s;
>>>> +
>>>> +                     if (valp[0] != ':')
>>>> +                             die(_("format: usage %%(align:<width>,<position>)"));
>>>
>>> ... apparently that is not what is happening.  Why not skip "align:"
>>> with colon as the prefix, then?
>>
>> 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?

No, since "align" is defined as an atom, in the valid_atom struct.
Changing it to "align:" would work, but that seems a little inconsistent
with the other atoms.

Hence --format="%(align)foo%(end)" would just result in the
"format: `end` atom used without a supporting atom" error being
displayed, hence confusing the user even more.

-- 
Regards,
Karthik Nayak