Re: [PATCH v14 04/13] ref-filter: implement an `align` atom

> On Mon, Aug 31, 2015 at 4:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sun, Aug 30, 2015 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>> With the disclaimer that I wasn't following the quoting discussion
>>>> closely: Is this condition going to be sufficient for all cases, such
>>>> as an %(if:) atom? That is, if you have:
>>>>
>>>>     %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)
>>>>
>>>> isn't the intention that, %(bloop) within the %(then) section should
>>>> be quoted but not the literal "--option="?
>>>
>>> I think you'll see that the intention of the above is to quote the
>>> entirty of the result of %(if...)...%(end) if you read the previous
>>> discussion.  The "quoting" is used when you say you are making --format
>>> write a script in specified programming language, e.g.
>>>
>>>         for-each-ref --shell --format='
>>>                 a=%(atom) b=%(if...)...%(end)
>>>                 do interesting things using $a and $b here
>>>         ' | sh
>>>
>>> You are correct to point out in the earlier part of your message I
>>> am responding to that %(align) is not special and any nested thing
>>> including %(if) will uniformly trigger the same "usually each atom
>>> is quoted separately, but with this opening atom, everything up to
>>> the matching end atom is evaluated first and then the result is
>>> quoted" logic.
>>
>> So, if I'm understanding correctly, the semantic behavior of the
>> current patch seems to be more or less correct, but the implementation
>> (and commit message) place perhaps too much emphasis on specializing
>> quoting suppression only for %(align:), whereas it could/should be
>> generalized?
>>
>> I am a bit concerned about this code from end_atom_handler():
>>
>>     /*
>>      * Whenever we have more than one stack element that means we
>>      * are using a certain modifier atom. In that case we need to
>>      * perform quote formatting.
>>      */
>>     if (state->stack->prev) {
>>         quote_formatting(&s, current->output.buf, state->quote_style);
>>         strbuf_reset(&current->output);
>>         strbuf_addbuf(&current->output, &s);
>>     }
>>
>> Aren't both the comment and the condition backward? Shouldn't quoting
>> be done only for the top-most state on the stack rather than every
>> state other than the top-most? That is, shouldn't the condition be
>> `!state->stack->prev' as it is in append_atom()?
>
> After seeing the example of quote usage given by Junio, yes you're right.
> `!state->stack->prev` is the way to go.
>