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

> 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).