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

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

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

Show 16 quoted lines

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

Show 5 quoted lines

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

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