From: Junio C Hamano Date: Tue, 08 Mar 2016 20:19:01 GMT Subject: Re: [PATCH 08/15] ref-filter: make "%(symref)" atom work with the ':short' modifier Message-ID: In-Reply-To: Karthik Nayak writes: > On Tue, Mar 8, 2016 at 7:26 AM, Jacob Keller wrote: >> On Mon, Mar 7, 2016 at 3:08 PM, Junio C Hamano wrote: >>> Karthik Nayak 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.