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

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Phillip Wood <phillip.wood123@gmail.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain
Date: Mon, 16 Nov 2020 01:02:34 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2011160057560.18437@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <4ad92124-de1f-df66-093b-5089ad111cb7@gmail.com>

Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

> 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