Re: [PATCH v16 00/14] port tag.c to use ref-filter APIs

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

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

> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index d039f40..c5154bb 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -128,13 +128,14 @@ color::
>  	are described in `color.branch.*`.
>  
>  align::
> -	Left-, middle-, or right-align the content between %(align:..)
> +	Left-, middle-, or right-align the content between %(align:...)
>  	and %(end). Followed by `:<width>,<position>`, where the
>  	`<position>` is either left, right or middle and `<width>` is
>  	the total length of the content with alignment. If the
>  	contents length is more than the width then no alignment is
>  	performed. If used with '--quote' everything in between
> -	%(align:..)  and %(end) is quoted.
> +	%(align:...) and %(end) is quoted, but if nested then only the
> +	topmost level performs quoting.

I am not sure if the description of quote belongs to "align".  Isn't
the general rule at the endgame when there are more opening things
that would buffer its effect up to the corresponding %(end):

 - Some atoms like %(align) and %(if) always require a matching
   %(end).  We call them "opening atoms" and sometimes denote them
   as %($open).

 - When a scripting language specific quoting is in effect, except
   for opening atoms, replacement from every %(atom) is quoted when
   and only when it appears at the top-level (that is, when it
   appears outside %($open)...%(end)).

 - When a scripting language specific quoting is in effect,
   everything between a top-level opening atom and its matching
   %(end) is evaluated according to the semantics of the opening
   atom and its result is quoted.

To put the third item above in a different way, a matching
%($open)...  %(end) pair behaves as if it is a single normal atom,
from the point of view of the quoting rule.  All top-level atoms are
invidivually quoted, whether they are normal atoms or open/end pairs.
And that rule is not limited to %(align).

I am flexible with the terminology, but the point is that I think
the quoting rules are better be specified _outside_ the description
of a particular atom, but as a general rule.

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9fa1400..f55dfda 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -43,8 +43,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
>  
>  	if (!format) {
>  		if (filter->lines)
> -			format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
> -						   filter->lines);
> +			format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) "
> +						   "%%(contents:lines=%d)", filter->lines);

This line still looks overlong.  Would it help to stop spelling this
as a double "a = b = overlong expression" assignment?

>  	/*
>  	 * Perform quote formatting when the stack element is that of
> -	 * a modifier atom and right above the first stack element.
> +	 * a supporting atom. If nested then perform quote formatting
> +	 * only on the topmost supporting atom.
>  	 */
>  	if (!state->stack->prev->prev) {
>  		quote_formatting(&s, current->output.buf, state->quote_style);
> @@ -261,6 +262,22 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
>  	pop_stack_element(&state->stack);
>  }
>  
> +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 */
> +		return 1;
> +	}

This if condition is puzzling.  !body[0] would mean the name was
exactly atom_name, which is what you want to catch.

But the other part does not make any sense to me.  what does
(body[0] != '\0' && !body[1]) mean?  atom_name asked for was "align"
and name was "aligna"?  That is not "%(align) and no customization".

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

This one makes sense to me.

> +	*val = body + 1; /* "atomname:val" */

Spell that "atom_name:val" perhaps?