Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Sep 10, 2015, 18:58 UTC
- Message-ID
- <vpq1te6aype.fsf@scolette.imag.fr>
- In-Reply-To
- <xmqqpp1q428y.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's
>> not clear whether this is a deliberate decition.
>
> I would say so. When the caller wants to reject %(atom:), the
> caller can tell it by checking val[0] == '\0' and reject that.
>
> So it is better if you did not do this:
>
>> if (!body[1]) {
>> /* "atom_name:" */
>> *val = NULL;
>> return 1;
>> }
>
> which robs that information from the caller.OK. Just dropping this part lets the code fall back to
/* "atom_name:... */ *val = body + 1; return 1;
right below in my version. It also accepts it (return 1) but lets val point to an empty string. Makes sense.
And indeed, without this, my code looks a lot like Karthik's one, just dropping the "|| !body[1]" part in a condition.
In any case, I'd like to see "atom_name:" explicitly mentionned somewhere in a comment, if only to make it clear that what is done with it is deliberate (e.g. avoid having someone not following this conversation later considering this %(atom:) thing to be a bug and try to fix it).
-- Matthieu Moy http://www-verimag.imag.fr/~moy/