From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 13 Nov 2020 14:08:22 GMT
Subject: Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain
Message-ID: <4ad92124-de1f-df66-093b-5089ad111cb7@gmail.com>
In-Reply-To: <48d8e0badfec5f0e576868f7a406ed7ede6c7200.1605097704.git.gitgitgadget@gmail.com>

Hi Dscho

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). 
Having said that I think this patch is fine and the commit message does 
a good job of explaining the situation.

Thanks for working on this series, it's great to see a test at the end

Best Wishes

Phillip


> The best we can do here is to look for `.context` and if none is found,
> fall back to looking for `.plain`, and if still not found, fall back to
> the hard-coded default (which in this case is simply the empty string,
> as context lines are typically rendered without colored).
> 
> This still needs to inconsistencies when both config names are used: the
> initial diff will be colored by the diff machinery. Once edited by a
> user, a hunk has to be re-colored by `git add -p`, though, which would
> then use the other setting to color the context lines.
> 
> In practice, this is not _all_ that bad. The `git config` manual says
> this in the `color.diff.<slot>`:
> 
> 	`context` (context text - `plain` is a historical synonym)
> 
> We should therefore assume that users use either one or the other, but
> not both names. Besides, it is relatively uncommon to look at a hunk
> after editing it because it is immediately staged by default.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-interactive.c         | 6 ++++--
>   git-add--interactive.perl | 6 +++---
>   2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/add-interactive.c b/add-interactive.c
> index c298a8b80f..54dfdc56f5 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -49,8 +49,10 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
>   
>   	init_color(r, s, "diff.frag", s->fraginfo_color,
>   		   diff_get_color(s->use_color, DIFF_FRAGINFO));
> -	init_color(r, s, "diff.context", s->context_color,
> -		diff_get_color(s->use_color, DIFF_CONTEXT));
> +	init_color(r, s, "diff.context", s->context_color, "fall back");
> +	if (!strcmp(s->context_color, "fall back"))
> +		init_color(r, s, "diff.plain", s->context_color,
> +			   diff_get_color(s->use_color, DIFF_CONTEXT));
>   	init_color(r, s, "diff.old", s->file_old_color,
>   		diff_get_color(s->use_color, DIFF_FILE_OLD));
>   	init_color(r, s, "diff.new", s->file_new_color,
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index adbac2bc6d..bc3a1e8eff 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -30,9 +30,9 @@
>   	$diff_use_color ? (
>   		$repo->get_color('color.diff.frag', 'cyan'),
>   	) : ();
> -my ($diff_plain_color) =
> +my ($diff_context_color) =
>   	$diff_use_color ? (
> -		$repo->get_color('color.diff.plain', ''),
> +		$repo->get_color($repo->config('color.diff.context') ? 'color.diff.context' : 'color.diff.plain', ''),
>   	) : ();
>   my ($diff_old_color) =
>   	$diff_use_color ? (
> @@ -1046,7 +1046,7 @@ sub color_diff {
>   		colored((/^@/  ? $fraginfo_color :
>   			 /^\+/ ? $diff_new_color :
>   			 /^-/  ? $diff_old_color :
> -			 $diff_plain_color),
> +			 $diff_context_color),
>   			$_);
>   	} @_;
>   }
>