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

> 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().