Re: [PATCH v6 01/10] ref-filter: introduce 'ref_formatting_state'

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2015, 19:50 UTC
Message-ID: <xmqqh9om920w.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAPig+cS+w8ECma--ncJDoN1fEgrFZMvBC8GBgU6+tLYm_oGkaw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 17 quoted lines

>> @@ -1254,9 +1273,26 @@ static void emit(const char *cp, const char *ep)
>> +static void reset_formatting_state(struct ref_formatting_state *state)
>> +{
>> +       int quote_style = state->quote_style;
>> +       memset(state, 0, sizeof(*state));
>> +       state->quote_style = quote_style;
>
> I wonder if this sledge-hammer approach of saving one or two values
> before clearing the entire 'ref_formatting_state' and then restoring
> the saved values will scale well. Would it be better for this to just
> individually reset the fields which need resetting and not touch those
> that don't?
>
> Also, the fact that quote_style has to be handled specially may be an
> indication that it doesn't belong in this structure grouped with the
> other modifiers or that you need better classification within the
> structure.

Actually, I think it is wrong to have this function in the first place. It is a sign that the caller is doing too little before calling this function.

If the act of printing an atom uses the formatting state that says "next one needs X", then it is responsible to clear that "next one needs X" part of the state, as it is the one who consumed that state. E.g. if it used to say "next one needs to be padded to the right" before entering print_value(), then the function did that "padded output", then the "next one needs to be padded to the right" should be cleared inside print_value().