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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 2, 2015, 15:05 UTC
Message-ID: <CAOLa=ZQLG_HB0trodXPCb9n4x2FuH7h7-+KJYLEdNZSpd0PHsg@mail.gmail.com>
In-Reply-To: <xmqqd1y0q30g.fsf@gitster.mtv.corp.google.com>

On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> 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.

Show 19 quoted lines

>>>> +             } 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