Re: [PATCH v11 05/13] ref-filter: implement an `align` atom

> On Mon, Aug 17, 2015 at 10:28 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> +{
>>>> +       struct strbuf aligned = STRBUF_INIT;
>>>> +       struct ref_formatting_state *return_to = state->prev;
>>>> +       struct align *align = (struct align *)state->cb_data;
>>>> +
>>>> +       strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);
>>>> +       strbuf_addbuf(&return_to->output, &aligned);
>>>
>>> Second, I realize that Junio suggested the 'return_to' idea, but it
>>> seems like it could become overly painful since each handler of this
>>> sort is going to have to perform the same manipulation to append its
>>> collected output to its parent state's output. What if you instead
>>> make it the responsibility of pop_state() to append the 'output' from
>>> the state being popped to the "prev" state's 'output'? This way, it
>>> happens automatically, thus reducing code in each individual handler,
>>> and reducing the burden of having to keep writing the same code.
>>
>> Good question, what if we don't want to append to strbuf at all?
>> For e.g., We were discussing an "%(if).....%(then)......%(end)"
>> atom structure, here if the if condition isn't met we wouldn't want to
>> append to the prev strbuf, hence I thought it's better if the handler
>> decided whether or not to append to prev strbuf.
>
> An %(if)/%(then)/%(end) construct should not present a problem. As
> long as the processing of the conditional ensures that the current
> state's 'output' contains no content, when pop_state() appends that
> (empty) content to the parent state's 'output', then the net result is
> exactly the desired behavior.
>
> The question of "how" the conditional processing ensures that the
> current state's 'output' is empty when the %(if) condition is false is
> unimportant (for this discussion). It might be the case that it just
> doesn't collect any content at all for a false condition, or it
> collects it but throws it away before the state is popped. Either way,
> that's an implementation detail which needn't impact the decision to
> retire 'return_to' and instead make pop_state() responsible for
> appending the current state's output to the parent state's.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> ...
>>> Second, I realize that Junio suggested the 'return_to' idea, but it
>>> seems like it could become overly painful since each handler of this
>>> sort is going to have to perform the same manipulation to append its
>>> collected output to its parent state's output. What if you instead
>>> make it the responsibility of pop_state() to append the 'output' from
>>> the state being popped to the "prev" state's 'output'? This way, it
>>> happens automatically, thus reducing code in each individual handler,
>>> and reducing the burden of having to keep writing the same code.
>>
>> Good question, what if we don't want to append to strbuf at all?
>> For e.g., We were discussing an "%(if).....%(then)......%(end)"
>> atom structure, here if the if condition isn't met we wouldn't want to
>> append to the prev strbuf, hence I thought it's better if the handler
>> decided whether or not to append to prev strbuf.
>
> I'd imagine the implementation of these to be along the lines of
> (thinking aloud):
>
>  - "%(if:[nonempty|empty|...])" pushes a new stack, and sets its
>    attend/at_end/end_scope function to signal a syntax error.  It
>    also records what condition (e.g. "nonempty") to use in the new
>    stack.
>
>  - "%(then)" inspects the top-of-stack output and uses the condition
>    recorded by the %(if) that created the stack to decide true or
>    false.  The stack element pushed by %(if) is then removed.
>    Notice that the end_scope function prepared by %(if) is never
>    called.
>
>    Then (no pun intended):
>
>    - If true, that means we would want the (expansion of) text up to
>      "%(end)" or "%(else)", whichever comes first, appended to the
>      surrounding output.  Push a new stack and set its end_scope
>      function to the one that appends the top-of-stack output to the
>      surrounding output, expecting %(end) will follow without
>      %(else).
>
>    - If false, that means we would want the (expansion of) text up
>      to "%(end)" or "%(else)", whichever comes first, discarded.
>      Push a new stack and set its end_scope function to the one that
>      discards the top-of-stack output, expecting %(end) will follow
>      without %(else).
>
>  - "%(else)" inspects the top of the stack, and if it is not left by
>    "%(then)", signal a syntax error.
>
>    Else (no pun intended), it runs the end_scope function left by
>    "%(then)" on the top-of-stack output (e.g. if "%(then)" found
>    that the condition holds true, the accumulated output at this
>    point should be appended to the surrounding output, and it was
>    expected to be done by "%(end)" if this "%(else)" weren't
>    present.  We do it here before waiting for "%(end)").
>
>    Truncate the top-of-stack output, flip the end_scope function to
>    the one opposite from the one left by "%(then)".  And let "%(end)"
>    take care of it.
>
>  - "%(end)" just unconditionally runs the end_scope function on the
>    top of the stack output.
>

> Eric's suggestion is let the caller of the end_scope function to
> always append the output of the top-of-stack, and I think it makes
> sense.  It makes a common "%(atom)" implementation simpler.  Things
> like %(then) and %(else) above need to make sure that they reset the
> top-of-stack output to empty as necessary, but that is not such a
> huge implementation burden---their operation is already unusual and
> needs to be more complex than the plain-vanilla %(atom)s anyway.