Re: [PATCH v5 02/11] ref-filter: make `color` use `ref_formatting_state`

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 27, 2015, 14:28 UTC
Message-ID: <xmqq4mkpk73h.fsf@gitster.dls.corp.google.com>
In-Reply-To: <vpqa8uhaht3.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Show 20 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Make the `color` atom a pseudo atom and ensure that it uses
>> `ref_formatting_state`.
>
> Actually, I think this is an incorrect change.
>
> Previously, %(color:...) was taking effect immediately, and after this
> patch, it takes effect starting from the next atom.
> ...
> As a consequence of the remark above, I think the name and comment of
> the field are misleading. There are 3 kinds of atoms:
>
> * Placeholders for ref attributes
>
> * Atoms that take action immediately, but that are not ref attributes
>   like %(color)
>
> * Atoms that only act as modifiers for the next atom. Perhaps they could
>   be called "prefix atoms" or "modifier atoms".

My fault.

I briefly thought that it may be simpler to treat %(color) just as a different way to express a literal that cannot be typed from the keyboard, but your "different kind of atom" is a much better way to think about it.

What is necessary is that, just like the updated "print_value()" knows about the formatting state, "emit()" needs to be told about the same formatting state. Some of these "state affecting" atoms may take effect on what is output by "emit()" (e.g. "color" is obviously an example, the hypotethical one that counts the current output column and uses that knowledge to "align" the output to certain columns, instead of "right pad to make the next column 30-columns wide" one, which is in this series, is another).

Thanks for finding this, and Karthik, sorry for an incomplete suggestion based on a faulty thinking.