Re: [PATCH v4 6/6] for-each-ref: avoid color leakage

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Nov 19, 2013, 17:32 UTC
Message-ID: <CALkWK0nKGp4XdJX1Dds-Y+4o2eRXp8XH3qpqU8MNfTG+UdwACA@mail.gmail.com>
In-Reply-To: <xmqqiovo9x2r.fsf@gitster.dls.corp.google.com>

Junio C Hamano wrote:

Show 6 quoted lines

> My knee-jerk "adding it to struct refinfo" is not correct, either,
> because what we want to know, i.e. "do we need an extra reset?" is
> not a property for each ref.  It is similar to "what is the set of
> atoms the format string is using?" and "do we need to peel tags in
> order to show all information asked by the format string?"
> (i.e. used_atom[] and need_tagged, respectively).

It's mostly cruft carried over from my reset-color-after-each-token implementation. My severe distaste for global variables prevented me from looking for the simpler solution.

Show 6 quoted lines

> Unlike need_tagged which asks "is there any *refname that asks us to
> peel tags?", however, "is the _last_ color:anything in the format
> string not 'reset'?" depends on the order of appearance of atoms in
> the format string, so this needs to be done in a loop that scans the
> format string from left to right once at the very beginning, and we
> have a perfect place to do so in verify_format().

I should be shot for my laziness and lack of ingenuity. Yeah, verify_format() is a much better place to put the logic than populate_value().

Show 9 quoted lines

> So perhaps like this one on top?
>
>  builtin/for-each-ref.c | 45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 04e35ba..5ff51d1 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c

Thanks for taking the time to do this. And apologies for the laziness.