On Mon, Aug 31, 2015 at 11:32 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Mon, Aug 31, 2015 at 1:28 PM, Matthieu Moy
> <Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
>> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
>>>
>>> Warning about unrecognized atoms may indeed be a good idea, however,
>>> the current behavior isn't a huge problem since user discovers the
>>> error when the output fails to match his expectation.
>>
>> It's a bit worse than it seems: without this change, using --format
>> '%(align)%(end)' results in Git complaining about %(end) without
>> matching atom, which is really confusing since you do have a %(align) (I
>> got it for real while testing a preliminary version).
>>
>>> This behavior of ignoring unrecognized atoms predates your work,
>>> doesn't it? If so, it's probably not something you need to address in
>>> this series.
>>
>> I wouldn't insist in having it in the series, but now that it's here, I
>> think we can keep it (if only to shorten the interdiff for the next
>> iteration).
>
> The unstated subtext in my original question is that the approach
> taken by this patch of warning only about unrecognized %(align) is too
> special-case; if it's going to warn at all, it should do so
> generically for any unrecognized %(atom). Special-casing the warning
> about malformed %(align) sets a poor precedent; it's code which will
> eventually need to be removed anyhow when the generic warning
> mechanism is eventually implemented.
Probably, I could just have a check within the align block and maybe build a
general case later.
Like this:
else if (skip_prefix(name, "align", &valp)) {
struct align *align = &v->u.align;
struct strbuf **s;
if (valp[0] != ':')
die(_("format: usage %%(align:<width>,<position>)"));
else
valp++;
......
........
}
--
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