On Mon, Aug 31, 2015 at 1:28 PM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> 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).
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.
--
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