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