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

Commit Message

Johannes Schindelin Nov. 11, 2020, 12:28 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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()`.

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 <[e-mail]>
---
 add-interactive.c         | 6 ++++--
 git-add--interactive.perl | 6 +++---
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Philippe Blain Nov. 13, 2020, 2:04 p.m. UTC | #1

Hi Dscho, 


> Le 11 nov. 2020 à 07:28, Johannes Schindelin via GitGitGadget <[e-mail]> a écrit :
> 
> From: Johannes Schindelin <[e-mail]>
> 
> This still needs

s/needs/leads/

> 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.

Philippe.

Phillip Wood Nov. 13, 2020, 2:08 p.m. UTC | #2

Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   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),
>   			$_);
>   	} @_;
>   }
>

Johannes Schindelin Nov. 15, 2020, 11:57 p.m. UTC | #3

Hi Philippe,

[to make me completely confused about which spelling for
"Phillip"/"Philippe" to use, Philip Oakley needs to chime in.]

On Fri, 13 Nov 2020, Philippe Blain wrote:

> > Le 11 nov. 2020 à 07:28, Johannes Schindelin via GitGitGadget <[e-mail]> a écrit :
> >
> > From: Johannes Schindelin <[e-mail]>
> >
> > This still needs
>
> s/needs/leads/

Of course!

Thanks,
Dscho

> > 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.

Johannes Schindelin Nov. 16, 2020, 12:02 a.m. UTC | #4

Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[e-mail]>
> >
> > 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

Patch

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),
 			$_);
 	} @_;
 }