Re: [PATCH v13 04/12] ref-filter: implement an `align` atom

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Aug 25, 2015, 06:47 UTC
Message-ID: <vpqk2sj7tlb.fsf@anie.imag.fr>
In-Reply-To: <xmqqmvxg2v3u.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 13 quoted lines

> You can see that I expected that "if !state.stack->prev" check to be
> inside append_atom(), and I would imagine future readers would have
> the same expectation when reading this code.  I.e.
>
> 	append_atom(struct atom_value *v, struct ref_f_s *state)
>         {
> 		if (state->stack->prev)
> 			strbuf_addstr(&state->stack->output, v->s);
> 		else
>                 	quote_format(&state->stack->output, v->s, state->quote_style);
> 	}
>
> The end result may be the same,

There's another call to append_atom() when inserting the "reset color at end of line if needed", so moving this if inside append_atom means we would do the check also for the reset color. It would not change the behavior (by construction, we insert it only when the stack has only the initial element), so it's OK.

I agree that this is a good thing to do.

Show 5 quoted lines

> Moreover, notice that the function signature of append_atom() is
> exactly the same as atomv->handler's.  I wonder if it would be
> easier to understand if you made append_atom() the handler for a
> non-magic atoms, which would let you do the above without any if/else
> and just a single unconditional

I can't decide between "ah, very elegant" and "no, too much magic" ;-). I lean towards the former.

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