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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 19, 2013, 17:28 UTC
Message-ID: <xmqqwqk48fn0.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CALkWK0mnYXBVW-agV_v6=mPxA=MoJAfHQaPKarwKU=x2SE+tnQ@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 17 quoted lines

> Junio C Hamano wrote:
>>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>>> index 2ff4e54..04e35ba 100644
>>> --- a/builtin/for-each-ref.c
>>> +++ b/builtin/for-each-ref.c
>>> @@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>>  struct atom_value {
>>>       const char *s;
>>>       unsigned long ul; /* used for sorting when not FIELD_STR */
>>> +     int color : 2; /* 1 indicates color, 2 indicates color-reset */
>>>  };
>>
>> Hmph.  It looks wasteful to have this information in atom_value.
>
> I wanted to avoid an ugly global. On the other end of the spectrum,
> modifying the various functions to take an extra reset_color_leakage
> parameter seems much too intrusive. Do you have any suggestions?

We already represent information that is for the format string as existing globals. It means that, if we ever want to make the program accept and use more than one format string, we can't. We need one set of them for each such format string before we can use more than one.

If you want to solve that problem, complaining by using a subjective word "ugly" does not help us much. The right approach to the solution would be to first think what each global really means and decide which ones are per-format properties. Then turn them into a proper abstraction by defining a structure to hold the currently considered format string and these various "per format string" properties.

Once you do that, you can optionally make the code pass that single structure around, and that will remove the global, but I think that step can wait until we actually find a need for it.