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

On Mon, Aug 17, 2015 at 10:28 AM, Karthik Nayak [off-list ref] wrote:

On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak [off-list ref] wrote:

quoted

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