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

[Junio C Hamano <gitster@pobox.com>]

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

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?

> +		return 1;
> +	}
> +	if (body[0] != ':')
> +		return 0; /* "atom_namefoo" is not "atom_name" or "atom_name:..." */
> +	*val = body + 1; /* "atom_name:val" */
> +	return 1;
> +}
> +
>  /*
>   * In a format string, find the next occurrence of %(atom).
>   */
> @@ -687,6 +703,7 @@ static void populate_value(struct ref_array_item *ref)
>  		int deref = 0;
>  		const char *refname;
>  		const char *formatp;
> +		const char *valp;
>  		struct branch *branch = NULL;
>  
>  		v->handler = append_atom;
> @@ -721,10 +738,12 @@ static void populate_value(struct ref_array_item *ref)
>  			refname = branch_get_push(branch, NULL);
>  			if (!refname)
>  				continue;
> -		} 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.

>  			char color[COLOR_MAXLEN] = "";
>  
> -			if (color_parse(name + 6, color) < 0)
> +			if (!valp)
> +				die(_("expected format: %%(color:<color>)"));
> +			if (color_parse(valp, color) < 0)
>  				die(_("unable to parse format"));
>  			v->s = xstrdup(color);
>  			continue;
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..c4f0378 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -81,4 +81,8 @@ test_expect_success 'filtering with --contains' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '%(color) must fail' '
> +	test_must_fail git for-each-ref --format="%(color)%(refname)"
> +'
> +
>  test_done