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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Karthik Nayak <karthik.188@gmail.com> 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@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.

> +		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/