Re: [PATCH v17 05/14] ref-filter: introduce match_atom_name()

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 10, 2015, 16:49 UTC
Message-ID: <vpqbndap6d1.fsf@scolette.imag.fr>
In-Reply-To: <1441900110-4015-6-git-send-email-Karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 32 quoted lines

> 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@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Thanks-to: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  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 */

The logic is still hard to follow. If I read correctly, this function accepts "%(colorX)" the same ways as "%(color)" here. I first thought it was a bug, but it doesn't seem to be since %(colorX) would have been rejected earlier.

It would be a bug if colorX was actually a valid atom name though: you would be returning 1 for match_atom_name(name, "color") when name=="colorX". So, I would say this "we can accept one extra character because some earlier code rejected it before" is too hard to follow for reviwers and too fragile.

OTOH, you are now accepting %(atom:) as a synonym to %(atom), and it's not clear whether this is a deliberate decition.

Show 7 quoted lines

> +		return 1;
> +	}
> +	if (body[0] != ':')
> +		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
> +	*val = body + 1; /* "atom_name:val" */
> +	return 1;
> +}

Reversing the logic like this would be better IMHO:

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)) {
		/* doesn't even begin with "atom_name" */
		return 0;
	}
	if (!body[0]) {
		/* "atom_name" and no customization */
                *val = NULL;
                return 1;
	}
	if (body[0] != ':')
		/* "atom_namefoo" is not "atom_name" or "atom_name:..." */
		return 0;
	if (!body[1]) {
		/* "atom_name:" */
		*val = NULL;
		return 1;
	}
	/* "atom_name:... */
	*val = body + 1;
	return 1;
}

=> each case appears very clearly, and we check body[0] != ':' before testing !body[1], so %(colorX) is rejected before noticing the '\0' after the 'X'.

"atom_name:" appears explicitly. If we want to reject it, we know which code to change.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/