From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 10 Sep 2015 18:25:09 GMT
Subject: Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()
Message-ID: <xmqq4mj23zei.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <vpqio7inrc5.fsf@scolette.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> -		} else if (starts_with(name, "color:")) {
>>> +		} else if (match_atom_name(name, "color", &valp)) {
>>
>> Why use the helper only for this one?  Aren't existing calls to
>> starts_with() in the same if/else if/... cascade all potential bugs
>> that the new helper function is meant to help fixing?  For example,
>> the very fist one in the cascade:
>>
>> 	if (starts_with(name, "refname"))
>>         	refname = ref->refname;
>>
>> is correct *ONLY* when name is "refname" or "refname:" followed by
>> something, and it should skip "refnamex" when such a new atom is
>> added to valid_atom[] list, i.e. a bug waiting to happen.  I think
>> the new helper is designed to prevent such a bug from happening.
>
> I fully agree, but I also think that this should be a separate topic.

Yeah, it can be a separate topic.  I am neutral (i.e. I certainly
would not insist that the existing one should be fixed with the
helper in the series, but I cannot quite say that I prefer the fix
to be made outside this topic, either).

Thanks.