Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 16, 2020, 00:02 UTC
Message-ID: <nycvar.QRO.7.76.6.2011160057560.18437@tvgsbejvaqbjf.bet>
In-Reply-To: <4ad92124-de1f-df66-093b-5089ad111cb7@gmail.com>

Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

Show 21 quoted lines

> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Git's diff machinery allows users to override the colors to use in
> > diffs, even the plain-colored context lines. As of 8dbf3eb6850 (diff.h:
> > rename DIFF_PLAIN color slot to DIFF_CONTEXT, 2015-05-27), the preferred
> > name of the config setting is `color.diff.context`, although Git still
> > allows `color.diff.plain`.
> >
> > In the context of `git add -p`, this logic is a bit hard to replicate:
> > `git_diff_basic_config()` reads all config values sequentially and if it
> > sees _any_ `color.diff.context` or `color.diff.plain`, it accepts the
> > new color. The Perl version of `git add -p` needs to go through `git
> > config --get-color`, though, which allows only one key to be specified.
> > The same goes for the built-in version of `git add -p`, which has to go
> > through `repo_config_get_value()`.
>
> One nit pick: the built-in version does not have to go through
> `repo_config_get_value()`, it could get the values in a callback using
> `git_config()` which would match the behavior of diff but chooses not to
> (presumably it is more convenient to just look up the config values).

Oh, but that would require _all_ callers of the interactive patch functionality to be audited, to ensure that the correct `git_config()` call back is used.

That's positively not what I intended.

Instead, using `repo_config_get_value()` does not require that care, and does not open us up to future callers that may forget to include the required callback in _their_ config parsing.

In addition, using `repo_config_get_value()` already prepares this part of Git's code for a future where the `struct repository *` pointer is passed into every code path that needs it, so that we no longer have to rely on global state at all.

Besides, since this patch is really about fixing the discrepancy between the Perl version's use of `.plain` and the built-in's use of `.context`, changing something as unrelated as how the config is accessed would be inappropriate (even elsewhere in this patch series, which really is about fixing interactive `add`'s color handling).

Ciao, Dscho