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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 2, 2015, 15:01 UTC
Message-ID: <xmqqd1y0q30g.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAOLa=ZQ0nyaQmvE3suT6hh0jO3s_iYxFLjSuLUY4STneMFJf9Q@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 7 quoted lines

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

Show 16 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?