On Mon, Jul 20, 2015 at 10:59 PM, Junio C Hamano <[email protected]> wrote:
> Junio C Hamano <[email protected]> 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.
You do have a point, my current implementation won't work with the
scenario you just mentioned.
>
> 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.
This implementation looks good. Will include this, thanks a bunch.
>
> 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().
Yup! ill put in a fix for %(color:<color>) with this patch.
Thanks a lot.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
