On Mon, Aug 31, 2015 at 3:44 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> On Mon, Aug 31, 2015 at 4:26 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Sun, Aug 30, 2015 at 1:27 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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(¤t->output);
>> strbuf_addbuf(¤t->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.
>
Also I think you mean `!state->stack->prev->prev` as we push a new
element on the stack when an atom such as %(align) is encountered. And
this quoting done at the end should be only for such atoms. Hence it should be
`!state->stack->prev->prev` and not `!state->stack->prev`.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html