Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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.
> 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/
--
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