On Wed, Aug 19, 2015 at 8:26 PM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> 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, ¤t->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@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;
> }
>
>
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
--
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