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

From: Junio C Hamano <[email-masked]>
Date: Wed, 02 Sep 2015 08:45:59 -0700

Karthik Nayak <[email-masked]> writes:

> On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <[email-masked]> wrote:
>> Karthik Nayak <[email-masked]> 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.

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