Re: [PATCH v2 04/11] add -i: use `reset_color` consistently

Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

On 13/11/2020 13:53, Johannes Schindelin wrote:

quoted

On Fri, 13 Nov 2020, Phillip Wood wrote:

quoted

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

We already maintain a list of colors in the `add_i_state`, therefore we
should use them.

Playing devil's advocate for a moment - why do we have a reset entry
in that list? The next patch makes sure it cannot be customized
which is a good thing so why not drop the `reset` member from
`add_i_state` entirely? The perl version needed to get the reset
string from `git config` and store it somewhere but in the C version
we have a perfectly good constant we can use instead.

Right.

On the other hand, does it hurt to keep that field for consistency with
the rest of the coloring stuff?

It creates the illusion that `reset` is similar to the others but it isn't as
we don't want it to be customizable.

In a certain sense, however, it *is* customizable. Namely when the color
is turned _off_. Then `reset_color` is set to the empty string, and
`GIT_RESET_COLOR` still isn't.

quoted

It would probably cost more to make the change you suggested than it
would benefit us.

At the moment there is one use of `s->s.color_reset` and two of
`GIT_COLOR_RESET` in add-patch.c

I did spend the time to look at those call sites, and they seem to be
guarded by "if colored" conditions.

But as I said, I don't think that it will buy us a ton to treat resetting
color different from setting color.

I mean, who is to say that we won't add some sort of low-level mode of
`git add -p` at some stage that can be used by 3rd-party applications? We
would then have to switch back to the current code. And even if not, I
think that there is a benefit to the current code, where no caller has to
wonder whether the reset sequence has been overridden (e.g. by the empty
string) or not, it just says semantically that this is where we switch
back to regular color. And that's that.

Ciao,
Dscho