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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Aug 19, 2015, 15:44 UTC
Message-ID: <vpqa8tn2sgy.fsf@anie.imag.fr>
In-Reply-To: <CAOLa=ZTy8QO=H9g9g3SKU4HaG=qg71GidGuXKLXNgu7Su3FQ9A@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 14 quoted lines

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

If you have a stack of N elemments, why replicate a field N times if all the N instances always have the same value?

There's nothing to be pushed or poped with quote_style, so having it in the stack is confusing to the reader (one has to infer the property "all instances have the same value" by reading the code instead of having just one variable), and error-prone for the author: you already got it wrong once.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/