Re: [PATCH v12 03/13] ref-filter: introduce the ref_formatting_state stack machinery

[Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx>]

Karthik Nayak <karthik.188@xxxxxxxxx> writes:

> +static void pop_state(struct ref_formatting_state **stack)
> +{
> +	struct ref_formatting_state *current = *stack;
> +	struct ref_formatting_state *prev = current->prev;
> +
> +	if (prev)
> +		strbuf_addbuf(&prev->output, &current->output);

I find this "if (prev)" suspicious: if there's a previous element in the
stack, push to it, but otherwise, you're throwing away the content of
the stack top silently.

Given the rest of the patch, this is correct, since you're using
state->output before pop_state(), but I find it weird to have the same
function to actually pop a state, and to destroy the last element.

Just thinking out loudly, I don't have specific alternative to propose
here.

> @@ -1262,23 +1284,24 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>  {
>  	const char *cp, *sp, *ep;
> -	struct ref_formatting_state state;
> +	struct strbuf *final_buf;
> +	struct ref_formatting_state *state = NULL;
>  
> -	strbuf_init(&state.output, 0);
> -	state.quote_style = quote_style;
> +	push_new_state(&state);
> +	state->quote_style = quote_style;

I do not think that the quote_style should belong to the stack. At the
moment, only the bottom of the stack has it set, and as a result you're
getting weird results like:

$ ./git for-each-ref --shell --format '|%(align:80,left)<%(author)>%(end)|' | head -n 3 
|<Junio C Hamano <gitster@xxxxxxxxx> 1435173702 -0700>                           ''|
|<Junio C Hamano <gitster@xxxxxxxxx> 1435173701 -0700>                           ''|
|<Junio C Hamano <gitster@xxxxxxxxx> 1433277352 -0700>                           ''|

See, the '' are inserted where the %(end) was, but not around atoms as
one would expect.

One stupid fix would be to propagate the quote_style accross the stack,
like this:

--- a/ref-filter.c
+++ b/ref-filter.c
@@ -155,6 +155,8 @@ static void push_new_state(struct ref_formatting_state **stack)
 
        strbuf_init(&s->output, 0);
        s->prev = *stack;
+       if (*stack)
+               s->quote_style = (*stack)->quote_style;
        *stack = s;
 }
 

After applying this, I do get the '' around the author (= correct
behavior I think), but then one wonders even more why this is part of
the stack.

You replaced the quote_style argument with ref_formatting_state, and I
think you should have kept this argument and added ref_formatting_state.
The other option is to add an extra indirection like

struct ref_formatting_state {
	int quote_style;
	struct ref_formatting_stack *stack;
}

(ref_formatting_stack would be what you currently call
ref_formatting_state). But that's probably overkill.

Also, after applying my toy patch above, I get useless '' around
%(align) and %(end). I can get rid of them with

--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1499,7 +1501,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format,
                get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
                if (atomv->handler)
                        atomv->handler(atomv, &state);
-               append_atom(atomv, state);
+               else
+                       append_atom(atomv, state);
        }
        if (*cp) {
                sp = cp + strlen(cp);

Unless I missed something, this second patch is sensible anyway and
should be squashed into [PATCH v12 05/13]: you don't need to call
append_atom() when you have a handler, right?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html