Re: [PATCH v14 04/13] ref-filter: implement an `align` atom

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Aug 31, 2015 at 5:55 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak <karthik.188@gmail.com> 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/