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

> Junio C Hamano <gitster@pobox.com> 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.
>