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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]