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

> 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?