git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Nov 13, 2020, 14:08 UTC
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:
Show 15 quoted lines
> 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
Show 68 quoted lines
> 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),
>   			$_);
>   	} @_;
>   }
> 
Previous: Johannes SchindelinNext: Johannes Schindelin
Message 30 of 66 in “Fix color handling in git add -i”
  1. 0/9 Fix color handling in git add -iJohannes Schindelin via GitGitGadget, Nov 10, 2020
  2. 1/9 add -i (built-in): do show an error message for incorrect inputsJohannes Schindelin via GitGitGadget, Nov 10, 2020
  3. 2/9 add -i (built-in): send error messages to stderrJohannes Schindelin via GitGitGadget, Nov 10, 2020
  4. 7/9 add -i (built-in): use the same indentation as the Perl versionJohannes Schindelin via GitGitGadget, Nov 10, 2020
  5. 8/9 add -i (Perl version): include indentation in the colored headerJohannes Schindelin via GitGitGadget, Nov 10, 2020
  6. 9/9 add -i: verify in the tests that colors can be overriddenJohannes Schindelin via GitGitGadget, Nov 10, 2020
  7. Jeff KingNov 11, 2020
  8. Johannes SchindelinNov 11, 2020
  9. Jeff KingNov 11, 2020
  10. Johannes SchindelinNov 12, 2020
  11. Jeff KingNov 12, 2020
  12. Johannes SchindelinNov 15, 2020
  13. Jeff KingNov 17, 2020
  14. 5/9 add -i (built-in): use correct names to load color.diff.* configJohannes Schindelin via GitGitGadget, Nov 10, 2020
  15. 3/9 add -i: use `reset_color` consistentlyJohannes Schindelin via GitGitGadget, Nov 10, 2020
  16. 6/9 add -p (built-in): do not color the progress indicator separatelyJohannes Schindelin via GitGitGadget, Nov 10, 2020
  17. 4/9 add -i (built-in): prevent the `reset` "color" from being configuredJohannes Schindelin via GitGitGadget, Nov 10, 2020
  18. Jeff KingNov 11, 2020
  19. 00/11 Fix color handling in git add -iJohannes Schindelin via GitGitGadget, Nov 11, 2020
  20. 01/11 add -i (built-in): do show an error message for incorrect inputsJohannes Schindelin via GitGitGadget, Nov 11, 2020
  21. 07/11 add -p (built-in): do not color the progress indicator separatelyJohannes Schindelin via GitGitGadget, Nov 11, 2020
  22. Phillip WoodNov 13, 2020
  23. Johannes SchindelinNov 13, 2020
  24. Phillip WoodNov 13, 2020
  25. Johannes SchindelinNov 15, 2020
  26. 08/11 add -i (built-in): use the same indentation as the Perl versionJohannes Schindelin via GitGitGadget, Nov 11, 2020
  27. 10/11 add -p: prefer color.diff.context over color.diff.plainJohannes Schindelin via GitGitGadget, Nov 11, 2020
  28. Philippe BlainNov 13, 2020
  29. Johannes SchindelinNov 15, 2020
  30. Phillip WoodNov 13, 2020
  31. Johannes SchindelinNov 16, 2020
  32. 09/11 add -i (Perl version): include indentation in the colored headerJohannes Schindelin via GitGitGadget, Nov 11, 2020
  33. Phillip WoodNov 13, 2020
  34. Johannes SchindelinNov 13, 2020
  35. Phillip WoodNov 13, 2020
  36. Johannes SchindelinNov 15, 2020
  37. 06/11 add -i (built-in): use correct names to load color.diff.* configJohannes Schindelin via GitGitGadget, Nov 11, 2020
  38. 11/11 add -i: verify in the tests that colors can be overriddenJohannes Schindelin via GitGitGadget, Nov 11, 2020
  39. 05/11 add -i (built-in): prevent the `reset` "color" from being configuredJohannes Schindelin via GitGitGadget, Nov 11, 2020
  40. 02/11 add -i (built-in): send error messages to stderrJohannes Schindelin via GitGitGadget, Nov 11, 2020
  41. 03/11 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headersJohannes Schindelin via GitGitGadget, Nov 11, 2020
  42. 04/11 add -i: use `reset_color` consistentlyJohannes Schindelin via GitGitGadget, Nov 11, 2020
  43. Phillip WoodNov 13, 2020
  44. Johannes SchindelinNov 13, 2020
  45. Phillip WoodNov 13, 2020
  46. Johannes SchindelinNov 15, 2020
  47. Jeff KingNov 11, 2020
  48. Johannes SchindelinNov 12, 2020
  49. Jeff KingNov 12, 2020
  50. Johannes SchindelinNov 15, 2020
  51. Jeff KingNov 17, 2020
  52. 00/11 Fix color handling in git add -iJohannes Schindelin via GitGitGadget, Nov 16, 2020
  53. 01/11 add -i (built-in): do show an error message for incorrect inputsJohannes Schindelin via GitGitGadget, Nov 16, 2020
  54. 02/11 add -i (built-in): send error messages to stderrJohannes Schindelin via GitGitGadget, Nov 16, 2020
  55. 03/11 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headersJohannes Schindelin via GitGitGadget, Nov 16, 2020
  56. 04/11 add -i: use `reset_color` consistentlyJohannes Schindelin via GitGitGadget, Nov 16, 2020
  57. 05/11 add -i (built-in): prevent the `reset` "color" from being configuredJohannes Schindelin via GitGitGadget, Nov 16, 2020
  58. 06/11 add -i (built-in): use correct names to load color.diff.* configJohannes Schindelin via GitGitGadget, Nov 16, 2020
  59. 08/11 add -i (built-in): use the same indentation as the Perl versionJohannes Schindelin via GitGitGadget, Nov 16, 2020
  60. 07/11 add -p (built-in): do not color the progress indicator separatelyJohannes Schindelin via GitGitGadget, Nov 16, 2020
  61. 11/11 add -i: verify in the tests that colors can be overriddenJohannes Schindelin via GitGitGadget, Nov 16, 2020
  62. 09/11 add -i (Perl version): color header to match the C versionJohannes Schindelin via GitGitGadget, Nov 16, 2020
  63. 10/11 add -p: prefer color.diff.context over color.diff.plainJohannes Schindelin via GitGitGadget, Nov 16, 2020
  64. Jeff KingNov 17, 2020
  65. Jeff KingNov 17, 2020
  66. Junio C HamanoNov 25, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.