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

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

>  struct atom_value{

Obviously not a problem with this step, but you need a SP before the
open brace.

> @@ -692,6 +704,26 @@ static void populate_value(struct ref_array_item *ref)
>  			else
>  				v->s = " ";
>  			continue;
> +		} else if (skip_prefix(name, "align:", &valp)) {
> +			struct align *align = xmalloc(sizeof(struct align));
> +
> +			if (skip_prefix(valp, "left,", &valp))
> +				align->position = ALIGN_LEFT;
> +			else if (skip_prefix(valp, "right,", &valp))
> +				align->position = ALIGN_RIGHT;
> +			else if (skip_prefix(valp, "middle,", &valp))
> +				align->position = ALIGN_MIDDLE;
> +			else
> +				die(_("improper format entered align:%s"), valp);
> +			if (strtoul_ui(valp, 10, &align->width))
> +				die(_("positive width expected align:%s"), valp);

Minor nits on the design.  %(align:<width>[,<position>]) would let
us write %(align:16)...%(end) and use the "default position", which
may be beneficial if one kind of alignment is prevalent (I guess all
the internal users left-align?)  %(align:<position>,<width>) forces
users to spell both out all the time.

> @@ -1198,7 +1230,9 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>  
>  struct ref_formatting_state {
>  	struct strbuf output;
> +	struct align *align;
>  	int quote_style;
> +	unsigned int end : 1;
>  };

Mental note: it is not clear why you need 'end' field in the state.
Perhaps it is an indication that the division of labor is poorly
designed between the helper that updates the formatting state and
the other helper that reflects the formatting state to the final
string.

> @@ -1262,12 +1296,31 @@ static void append_non_atom(const char *cp, const char *ep, struct ref_formattin
>  
>  static void set_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)
>  {
> -	/* Based on the atomv values, the formatting state is set */
> +	if (atomv->align) {
> +		state->align = atomv->align;
> +		atomv->align = NULL;
> +	}
> +	if (atomv->end)
> +		state->end = 1;
> +}
> +
> +static int align_ref_strbuf(struct ref_formatting_state *state, struct strbuf *final)
> +{
> +	if (state->align && state->end) {

... and I think that is what I see.  If this function knows that we
are processing %(end), i.e. perform-state-formatting is called for
each atom and receives atomv, there wouldn't have to be a code like
this.

> +		struct align *align = state->align;
> +		strbuf_utf8_align(final, align->position, align->width, state->output.buf);
> +		strbuf_reset(&state->output);
> +		state->align = NULL;
> +		return 1;
> +	} else if (state->align)
> +		return 1;
> +	return 0;
>  }
>  
>  static void perform_state_formatting(struct ref_formatting_state *state, struct strbuf *final)
>  {
> -	/* More formatting options to be eventually added */
> +	if (align_ref_strbuf(state, final))
> +		return;

At the design level, I have a strong suspicion that it is a wrong
way to go.  It piles more "if (this state bit was left by the
previous atom) then do this" on this function and will make an
unmanageable mess.

You have a dictionary of all possible atoms somewhere.  Why not hook
a pointer to the "handler" function (or two) to each element in it,
instead of duplicating "this one is special" information down to
individual atom instantiations (i.e. atomv) as atomv.modifier_atom
bit, an dstructure the caller more like this?

	get_ref_atom_value(info, parse_ref_filter_atom, &atomv);
        if (atomv->pre_handler)
        	atomv->pre_handler(atomv, &state);
	format_quote_value(atomv, &state);
        if (atomv->post_handler)
        	atomv->post_handler(atomv, &state);

Actually, each atom could just have a single handler; an atom like
%(refname:short) whose sole effect is to append atomv->s to the
state buffer can point a function to do so in its handler.

On the other hand, align atom's handler would push a new state on
the stack, marking that it is the one to handle diverted output.

	align_atom_handler(atomv, state)
        {
        	struct format_state *new = push_new_state(state);
		strbuf_init(&new->output);
                new->atend = align_handler;
                new->return_to = atomv; /* or whatever that holds width,pos */
	}

Then end atom's handler would pop the state from the stack, and the
processing to be done

	end_atom_handler(atomv, state)
	{
                state->atend(state);
                pop_state(state);
	}                

and the called align_handler would be something like:

	align_handler(state)
	{
                struct strbuf aligned = STRBUF_INIT;
		struct format_state *return_to = state->prev;
                struct atom_value *atomv = state->return_to;

                strbuf_utf8_align(&aligned,
                        atomv->align.pos, atomv->align.width,
                        state->output.buf);
		strbuf_addbuf(&return_to->output, &aligned);
                strbuf_release(&aligned);
	}

With an arrangement like that, the body of the loop in
show_ref_array_item() could be as simple and regular as:

	get_ref_atom_value(info, parse_ref_filter_atom, &atomv);
       	atomv->handler(atomv, &state);

without any new "ah, this %(end) is special so we need a new
mechanism to pass information between set_formatting_state and
perform_formatting" logic introduced every time you add new things.