Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> On Mon, Aug 31, 2015 at 5:55 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>>>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>>>>>> + } else if (!strcmp(name, "align"))
>>>>>> + die(_("format: incomplete use of the `align` atom"));
>>>>>
>>>>> Why does %(align) get flagged as a malformation of %(align:), whereas
>>>>> %(color) does not get flagged as a malformation of %(color:)? Why does
>>>>> one deserve special treatment but not the other?
>>>>
>>>> Didn't see that, I think its needed to add a check for both like :
>>>>
>>>> else if (!strcmp(name, "align") || !strcmp(name, "color"))
>>>> die(_("format: improper usage of %s atom"), name);
>>>>
>>>> I had a look if any other atoms need a subvalue to operate, couldn't
>>>> find any.
>>>
>>> Hmm, I'm not convinced that either %(align) or %(color) need to be
>>> called out specially. What is the current behavior when these
>>> "malformations" or any other misspelled atoms are used? Does it error
>>> out? Does it simply ignore them and pass them through to the output
>>> unmolested?
>>
>> It just simply ignores them currently, which is kinda bad, as the user
>> is given no warning, and the atom is ineffective.
>
> 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).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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