Re: [PATCH v3 1/9] ref-filter: add option to align atoms to the left

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

Junio C Hamano <gitster@pobox.com> writes:

> Your caller is iterating over the elements in a format string,
> e.g. 'A %(align:20)%(foo) B %(bar) C', and its caller is iterating
> over a list of refs, e.g. 'maint', 'master' branches.  With that
> format string, as long as %(foo) does not expand to something that
> exceeds 20 display places or so, I'd expect literal 'B' for all refs
> to align, but I do not think this code gives me that; what happens
> if '%(foo)' happens to be an empty string for 'maint' but is a
> string, say 'x', for 'master'?

Having looked at the caller once again, I have to say that the
interface to this function is poorly designed.  'info' might have
been a convenient place to keep the "formatting state" during this
loop (e.g. "was the previous atom tell us to format this atom in a
special way and if so how?"), but that state does not belong to the
'info' thing we are getting from our caller.  It is something we'd
want to clear before we come into the for() loop, and mutate and
utilize while in the loop.  For example, if the caller ever wants
to show the same ref twice by calling this function with the same
ref twice, and if the format string ended with %(align:N), you do
not want that leftover state to right-pad the first atom in the
second invocation.

Imagine that in the future you might want to affect how things are
formatted based on how much we have already output for the ref so
far (e.g. limiting the total line length).  Where would you implement
such a feature and hook it in this loop?

I'd imagine that a sensible way to organize and structure the
codeflow to support this "align" and related enhancement we may want
to have in the future cleanly would be to teach "print_value" about
the "formatting state" and share it with this loop.  Roughly...

>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>  {
>  	const char *cp, *sp, *ep;
>  

Insert something like this here:

	struct ref_formatting_state state;

	memset(&state, 0, sizeof(state));
        state.quote_style = quote_style;

>  	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>  		struct atom_value *atomv;
> +		int parsed_atom;
>  
>  		ep = strchr(sp, ')');
>  		if (cp < sp)
>  			emit(cp, sp);
> -		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> +		parsed_atom = parse_ref_filter_atom(sp + 2, ep);
> +		get_ref_atom_value(info, parsed_atom, &atomv);
> +		assign_formating(info, parsed_atom, atomv);
>  		print_value(atomv, quote_style);

and replace all of the above with something like this (a separate
variable parsed_atom may not be necessary):

		get_ref_atom_value(&state, info,
				parse_ref_filter_atom(sp + 2, ep), &atomv);
		print_value(&state, atomv);

Things like %(align:20) are not really atoms in the sense that they
are not used as placeholders for attributes that refs being printed
have, but they are there solely in order to affect the "formating
state".  Introduce a new field "struct atom_value.pseudo_atom" to
tell print_value() that fact from get_ref_atom_value(), e.g.

	static void print_value(struct ref_formatting_state *state,
        			struct atom_value *v)
	{
		struct strbuf value = STRBUF_INIT;
		struct strbuf formatted = STRBUF_INIT;

        	if (v->pseudo_atom)
                	return;
		if (state->pad_to_right) {
                	strbuf_addf(&value, "%.*s", state->pad_to_right, v->s);
			state->pad_to_right = 0;
		}
		switch (state->quote_style) {                
		case QUOTE_SHELL:
                	sq_quote_buf(&formatted, value.buf);
                        break;
                        ...
		}
                fputs(formatted.buf, stdout);
                strbuf_release(&value);
                strbuf_release(&formatted);
	}

or something like that.  As this print_value() knows everything that
happens to a single output line during that loop and is allowed to
keep track of what it sees in 'state', this would give a natural and
codeflow to add 'limit the total line length' and things like that
if desired.

We may want to further clean up to update %(color) thing to clarify
that it is a pseudo atom.  I suspect %(align:20)%(color:blue) would
do a wrong thing with the current code, and it would be a reasonable
thing to allow both of these interchangeably:

  %(align:20)%(color:blue)%(refname:short)%(color:reset)
  %(color:blue)%(align:20)%(refname:short)%(color:reset)

and implementation of that would become more obvious once you have a
more explicit "formatting state" that is known to and shared among
get_value(), the for() loop that walks the format string, and
print_value().