On Wed, Sep 2, 2015 at 9:15 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>> Karthik Nayak <karthik.188@xxxxxxxxx> 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.
Haha okay.
>
>>>> 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().
>
This is what Eric suggested and we agreed on working on a general
solution to this problem eventually, seems like you beat me to it :)
Will incorporate this :)
> 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;
> }
Thanks.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html