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