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

[Junio C Hamano <gitster@pobox.com>]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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.