Re: [PATCH 08/15] ref-filter: make "%(symref)" atom work with the ':short' modifier

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Mar 8, 2016 at 7:26 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>> On Mon, Mar 7, 2016 at 3:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>>
>>>>> The "%(symref)" atom doesn't work when used with the ':short' modifier
>>>>> because we strictly match only 'symref' for setting the 'need_symref'
>>>>> indicator. Fix this by using 'starts_with()' rather than 'strcmp()'.
>>>>
>>>> Does that mean you also accept %(symrefgarbage) without complaining?
>>>>
>>>>
>>>
>>> Looks like patch 9 fixes this by introducing symref_atom_parser.
>>>
>>
>> There are two ways this kinda errors can occur:
>> 1. %(symrefgarbage) : This is handled by parse_ref_filter_atom() which would
>> print a "fatal: unknown field name: symrefgarbage".
>> 2. %(symref:garbage): This is handled by populate_value() which would print
>> a "fatal: unknown symref: format garbage".
>>
>> Either ways we do not need to worry about this as existing code would handle
>> it. Also like Jacob mentioned Patch 09 would ensure this error checking would
>> happen within symref_atom_parser().
>
> You forgot to mention that there is 3., which is that you just
> closed the door for a new valid_atom[] that begins with substring
> "symref" which does not need to flip need_symref on, I think.
>
> You can check valid_atom[i].name with strcmp() to achieve what you
> are trying to do here, instead of checking used_atom[at].name, and I
> think that would be a cleaner way to avoid all three problems.