Hi Phillip, On Fri, 13 Nov 2020, Phillip Wood wrote: > On 13/11/2020 13:53, Johannes Schindelin wrote: > > > > On Fri, 13 Nov 2020, Phillip Wood wrote: > > > > > On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote: > > > > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > > > > > > > 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. > > 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
