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

[Karthik Nayak <karthik.188@gmail.com>]

On Wed, Aug 19, 2015 at 8:26 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> 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.
>

Hmm, but destroying the last element is also pop'ing it off the stack in a way.
I can't think of a something else.

>> @@ -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@pobox.com> 1435173702 -0700>                           ''|
> |<Junio C Hamano <gitster@pobox.com> 1435173701 -0700>                           ''|
> |<Junio C Hamano <gitster@pobox.com> 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;
>  }
>
>

This seems about right, why do you think it's a stupid fix?

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

Yes, seems like an 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?

Yes, this I'll squash into 05/13.

-- 
Regards,
Karthik Nayak