On Thu, Sep 10, 2015 at 10:26 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> Introduce match_atom_name() which helps in checking if a particular
>> atom is the atom we're looking for and if it has a value attached to
>> it or not.
>>
>> Use it instead of starts_with() for checking the value of %(color:...)
>> atom. Write a test for the same.
>>
>> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
>> Mentored-by: Matthieu Moy <matthieu.moy@xxxxxxxxxxxxxxx>
>> Thanks-to: Junio C Hamano <gitster@xxxxxxxxx>
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>> ---
>> ref-filter.c | 23 +++++++++++++++++++++--
>> t/t6302-for-each-ref-filter.sh | 4 ++++
>> 2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index a993216..70d36fe 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -189,6 +189,22 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
>> *stack = prev;
>> }
>>
>> +static int match_atom_name(const char *name, const char *atom_name, const char **val)
>> +{
>> + const char *body;
>> +
>> + if (!skip_prefix(name, atom_name, &body))
>> + return 0; /* doesn't even begin with "atom_name" */
>> + if (!body[0] || !body[1]) {
>> + *val = NULL; /* %(atom_name) and no customization */
>
> Why do we check body[1] here? I do not understand why you are not
> checking !body[0] alone nothing else in this if condition.
>
> For (atom_name="align", name="aligna"), should the function say that
> "%(aligna)" is an "%(align)" with no customization?
>
The check was for checking if there is anything after the colon,
Matthieu's modified version seems better though.
>
> 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.
>
Yes definitely, but it would work with only refname, whereas
the color atom had no check before this patch, I didn't want to introduce
those patches in this series.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html