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

> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>>> +{
>>> +       /*
>>> +        * Quote formatting is only done when the stack has a single
>>> +        * element. Otherwise quote formatting is done on the
>>> +        * element's entire output strbuf when the %(end) atom is
>>> +        * encountered.
>>> +        */
>>> +       if (!state->stack->prev)
>>
>> 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="?
>>
>> The condition `!state->stack->prev' would be insufficient to handle
>> this if %(if:) pushes one or more states onto the stack, no? This
>> implies that you might want an explicit flag for enabling/disabling
>> quoting rather than relying upon the state of the stack, and that
>> individual atom handlers would control that flag.
>
>> Or, am I misunderstanding due to not having followed the discussion closely?
>
> Yes this wont be work for what you've said.
>
> To sum up the discussion:
> We didn't want atoms within the %(align)....%(end) to be quoted separately
> rather the whole aligned buffer to be quoted at the end.
>
> But this does conflict with %(if)...%(end). So it makes sense to change it to
> only have checks for %(align) atom being used.

> So probably:
>
> static void append_atom(struct atom_value *v, struct
> ref_formatting_state *state)
> {
>     if (state->stack->at_end == align_handler)

>         strbuf_addstr(&state->stack->output, v->s);
>     else
>         quote_formatting(&state->stack->output, v->s, state->quote_style);
> }
>
> and
>
> static void end_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> {
>     struct ref_formatting_stack *current = state->stack;
>     struct strbuf s = STRBUF_INIT;
>
>     if (!current->at_end)
>         die(_("format: `end` atom used without a supporting atom"));
>     current->at_end(current);
>     /*
>      * Whenever we have more than one stack element that means we
>      * are using a align modifier atom. In that case we need to
>      * perform quote formatting.
>      */
>     if (current->at_end == align_handler) {

>         quote_formatting(&s, current->output.buf, state->quote_style);
>         strbuf_reset(&current->output);
>         strbuf_addbuf(&current->output, &s);
>     }
>     strbuf_release(&s);
>     pop_stack_element(&state->stack);
> }
>>> @@ -725,6 +818,37 @@ static void populate_value(struct ref_array_item *ref)
>>>                         else
>>>                                 v->s = " ";
>>>                         continue;
>>> +               } else if (!strcmp(name, "align"))
>>> +                       die(_("format: incomplete use of the `align` atom"));
>>
>> Why does %(align) get flagged as a malformation of %(align:), whereas
>> %(color) does not get flagged as a malformation of %(color:)? Why does
>> one deserve special treatment but not the other?
>
> Didn't see that, I think its needed to add a check for both like :
>
> else if (!strcmp(name, "align") || !strcmp(name, "color"))
>             die(_("format: improper usage of %s atom"), name);
>
> I had a look if any other atoms need a subvalue to operate, couldn't
> find any.