All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/9] Fix color handling in git add -i
@ 2020-11-10 23:42 Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 1/9] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
                   ` (10 more replies)
  0 siblings, 11 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

This patch series started out as a tiny fix for a bug reported in 
https://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.
And then I only wanted to add a regression test to make sure that this does
not regress. And then I just wanted to ensure that it passes both with the
Perl version of git add -i as well as with the built-in version.

And in no time I was looking at a real patch series.

Johannes Schindelin (9):
  add -i (built-in): do show an error message for incorrect inputs
  add -i (built-in): send error messages to stderr
  add -i: use `reset_color` consistently
  add -i (built-in): prevent the `reset` "color" from being configured
  add -i (built-in): use correct names to load color.diff.* config
  add -p (built-in): do not color the progress indicator separately
  add -i (built-in): use the same indentation as the Perl version
  add -i (Perl version): include indentation in the colored header
  add -i: verify in the tests that colors can be overridden

 add-interactive.c          | 34 ++++++++++++++----------
 add-patch.c                | 14 +++++-----
 git-add--interactive.perl  |  6 ++---
 t/t3701-add-interactive.sh | 53 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 25 deletions(-)


base-commit: e4d83eee9239207622e2b1cc43967da5051c189c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/785
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH 1/9] add -i (built-in): do show an error message for incorrect inputs
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 2/9] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

There is a neat feature in `git add -i` where it allows users to select
items via unique prefixes.

In the built-in version of `git add -i`, we specifically sort the items
(unless they are already sorted) and then perform a binary search to
figure out whether the input constitutes a unique prefix. Unfortunately,
by mistake this code misidentifies matches even if the input string is
not actually a prefix of any item.

For example, in the initial menu, where there is a `status` and an
`update` command, the input `tadaa` was mistaken as a prefix of
`update`.

Let's fix this by looking a bit closer whether the input is actually a
prefix of the item at the found insert index.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 555c4abf32..8ca503d803 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
 	else if (index + 1 < list->sorted.nr &&
 		 starts_with(list->sorted.items[index + 1].string, string))
 		return -1;
-	else if (index < list->sorted.nr)
+	else if (index < list->sorted.nr &&
+		 starts_with(list->sorted.items[index].string, string))
 		item = list->sorted.items[index].util;
 	else
 		return -1;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH 2/9] add -i (built-in): send error messages to stderr
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 1/9] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 3/9] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of that command already does that since a301973641f
(add -p: print errors in separate color, 2009-02-05). The built-in
version's development started by reimplementing the initial version from
5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though,
which still printed error messages to stdout.

Let's fix that by imitating the Perl version's behavior in the built-in
version of that command.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 8ca503d803..0f24992ca4 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
 
 			if (from < 0 || from >= items->items.nr ||
 			    (singleton && from + 1 != to)) {
-				color_fprintf_ln(stdout, s->error_color,
+				color_fprintf_ln(stderr, s->error_color,
 						 _("Huh (%s)?"), p);
 				break;
 			} else if (singleton) {
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH 3/9] add -i: use `reset_color` consistently
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 1/9] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 2/9] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 4/9] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We already maintain a list of colors in the `add_i_state`, therefore we
should use them.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index be4cf6e9e5..33a24f58fe 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -667,7 +667,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)
-			strbuf_addf(out, "%s\n", GIT_COLOR_RESET);
+			strbuf_addf(out, "%s\n", s->s.reset_color);
 		else
 			strbuf_addch(out, '\n');
 	}
@@ -1060,7 +1060,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
 			      s->s.file_new_color :
 			      s->s.context_color);
 		strbuf_add(&s->colored, plain + current, eol - current);
-		strbuf_addstr(&s->colored, GIT_COLOR_RESET);
+		strbuf_addstr(&s->colored, s->s.reset_color);
 		if (next > eol)
 			strbuf_add(&s->colored, plain + eol, next - eol);
 		current = next;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH 4/9] add -i (built-in): prevent the `reset` "color" from being configured
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
                   ` (2 preceding siblings ...)
  2020-11-10 23:42 ` [PATCH 3/9] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 5/9] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of that command sneakily uses `git config --get-color`
to figure out the ANSI sequence to reset the color, but passes the empty
string and therefore cannot actually match any config entry.

This was missed when re-implementing the command as a built-in command.
Let's fix this, preventing the `reset` sequence from being overridden
via the config.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 0f24992ca4..f3a1d7456e 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
 	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
 	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
 	init_color(r, s, "fraginfo", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
 	init_color(r, s, "context", s->context_color,
@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
+	strlcpy(s->reset_color,
+		s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN);
+
 	FREE_AND_NULL(s->interactive_diff_filter);
 	git_config_get_string("interactive.difffilter",
 			      &s->interactive_diff_filter);
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH 5/9] add -i (built-in): use correct names to load color.diff.* config
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
                   ` (3 preceding siblings ...)
  2020-11-10 23:42 ` [PATCH 4/9] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 6/9] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The builtin version of add-interactive mistakenly loads diff colors from
color.interactive.* instead of color.diff.*. It also accidentally spells
`frag` as `fraginfo`.

Let's fix that.

Note also that we don't respect the historical `diff.color.*`. The perl
version never did, and those have been deprecated since 2007.

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index f3a1d7456e..9126684348 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -12,10 +12,10 @@
 #include "prompt.h"
 
 static void init_color(struct repository *r, struct add_i_state *s,
-		       const char *slot_name, char *dst,
+		       const char *section_and_slot, char *dst,
 		       const char *default_color)
 {
-	char *key = xstrfmt("color.interactive.%s", slot_name);
+	char *key = xstrfmt("color.%s", section_and_slot);
 	const char *value;
 
 	if (!s->use_color)
@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 			git_config_colorbool("color.interactive", value);
 	s->use_color = want_color(s->use_color);
 
-	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
-	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
-	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "fraginfo", s->fraginfo_color,
+	init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD);
+	init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED);
+	init_color(r, s, "interactive.prompt", s->prompt_color,
+		   GIT_COLOR_BOLD_BLUE);
+	init_color(r, s, "interactive.error", s->error_color,
+		   GIT_COLOR_BOLD_RED);
+
+	init_color(r, s, "diff.frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "context", s->context_color,
+	init_color(r, s, "diff.context", s->context_color,
 		diff_get_color(s->use_color, DIFF_CONTEXT));
-	init_color(r, s, "old", s->file_old_color,
+	init_color(r, s, "diff.old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
-	init_color(r, s, "new", s->file_new_color,
+	init_color(r, s, "diff.new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
 	strlcpy(s->reset_color,
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH 6/9] add -p (built-in): do not color the progress indicator separately
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
                   ` (4 preceding siblings ...)
  2020-11-10 23:42 ` [PATCH 5/9] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 7/9] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of this command colors the progress indicator and the
prompt message in one go, let's do the same in the built-in version.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 33a24f58fe..6f4c187238 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1456,15 +1456,15 @@ static int patch_update_file(struct add_p_state *s,
 		else
 			prompt_mode_type = PROMPT_HUNK;
 
-		color_fprintf(stdout, s->s.prompt_color,
-			      "(%"PRIuMAX"/%"PRIuMAX") ",
+		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
 			      (uintmax_t)hunk_index + 1,
 			      (uintmax_t)(file_diff->hunk_nr
 						? file_diff->hunk_nr
 						: 1));
-		color_fprintf(stdout, s->s.prompt_color,
-			      _(s->mode->prompt_mode[prompt_mode_type]),
-			      s->buf.buf);
+		printf(_(s->mode->prompt_mode[prompt_mode_type]),
+		       s->buf.buf);
+		if (*s->s.reset_color)
+			fputs(s->s.reset_color, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF)
 			break;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH 7/9] add -i (built-in): use the same indentation as the Perl version
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
                   ` (5 preceding siblings ...)
  2020-11-10 23:42 ` [PATCH 6/9] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 8/9] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When copying the spaces used to indent non-flat lists in `git add -i`,
one space was appended by mistake. This makes the output of the built-in
version of `git add -i` inconsistent with the Perl version. Let's adjust
the built-in version to produce the same output as the Perl version.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 9126684348..c298a8b80f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
 	print_file_item_data.color = data.color;
 	print_file_item_data.reset = data.reset;
 
-	strbuf_addstr(&header, "      ");
+	strbuf_addstr(&header, "     ");
 	strbuf_addf(&header, print_file_item_data.modified_fmt,
 		    _("staged"), _("unstaged"), _("path"));
 	opts.list_opts.header = header.buf;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH 8/9] add -i (Perl version): include indentation in the colored header
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
                   ` (6 preceding siblings ...)
  2020-11-10 23:42 ` [PATCH 7/9] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-10 23:42 ` [PATCH 9/9] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The header is formatted by padding each column heading with spaces up to
the length of 12 characters. These padding spaces are naturally included
when coloring the entire header.

However, the preceding five spaces indentation for non-flat lists were
_not_ included in the Perl version, but _were_ included in the built-in
version. Let's adjust the former to align with the latter's behavior.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e713fe3d02..adbac2bc6d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -483,10 +483,8 @@ sub list_and_choose {
 		my $last_lf = 0;
 
 		if ($opts->{HEADER}) {
-			if (!$opts->{LIST_FLAT}) {
-				print "     ";
-			}
-			print colored $header_color, "$opts->{HEADER}\n";
+			my $indent = $opts->{LIST_FLAT} ? "" : "     ";
+			print colored $header_color, "$indent$opts->{HEADER}\n";
 		}
 		for ($i = 0; $i < @stuff; $i++) {
 			my $chosen = $chosen[$i] ? '*' : ' ';
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH 9/9] add -i: verify in the tests that colors can be overridden
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
                   ` (7 preceding siblings ...)
  2020-11-10 23:42 ` [PATCH 8/9] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
@ 2020-11-10 23:42 ` Johannes Schindelin via GitGitGadget
  2020-11-11  2:35   ` Jeff King
  2020-11-11  2:36 ` [PATCH 0/9] Fix color handling in git add -i Jeff King
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
  10 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-10 23:42 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Now that the Perl version produces the same output as the built-in
version (mostly fixing bugs in the latter), let's add a regression test
to verify that it stays this way.

Note that we only `grep` for the colored error message instead of
verifying that the entire `stderr` consists of just this one line: when
running the test script via `dash`, using the `-x` option to trace the
commands, the sub-shell in `force_color` causes those commands to be
traced into `err.raw` because we set, but do not export
`BASH_XTRACEFD`).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index ca04fac417..28549a41a2 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -589,6 +589,59 @@ test_expect_success 'diffs can be colorized' '
 	grep "$(printf "\\033")" output
 '
 
+test_expect_success 'colors can be overridden' '
+	test_config color.interactive.header red &&
+	test_config color.interactive.help green &&
+	test_config color.interactive.prompt yellow &&
+	test_config color.interactive.error blue &&
+	test_config color.diff.frag magenta &&
+	test_config color.diff.context cyan &&
+	test_config color.diff.old bold &&
+	test_config color.diff.new "bold red" &&
+
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context >color-test &&
+
+	echo trigger an error message >input &&
+	force_color git add -i 2>err.raw <input &&
+	test_decode_color <err.raw >err &&
+	grep "<BLUE>Huh (trigger)?<RESET>" err &&
+
+	test_write_lines patch color-test "" y quit >input &&
+	force_color git add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual.decoded &&
+	sed "/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/d" <actual.decoded >actual &&
+	cat >expect <<-\EOF &&
+	<RED>           staged     unstaged path<RESET>
+	  1:        +3/-0        +1/-1 color-test
+
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> <RED>           staged     unstaged path<RESET>
+	  1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
+	<YELLOW>Patch update<RESET>>> <RED>           staged     unstaged path<RESET>
+	* 1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
+	<YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET>
+	<BOLD>--- a/color-test<RESET>
+	<BOLD>+++ b/color-test<RESET>
+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BOLD;RED>+<RESET><BOLD;RED>new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,e,?]? <RESET>
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> Bye.
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 66+ messages in thread

* Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
  2020-11-10 23:42 ` [PATCH 9/9] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
@ 2020-11-11  2:35   ` Jeff King
  2020-11-11 15:53     ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Jeff King @ 2020-11-11  2:35 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin

On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Now that the Perl version produces the same output as the built-in
> version (mostly fixing bugs in the latter), let's add a regression test
> to verify that it stays this way.
> 
> Note that we only `grep` for the colored error message instead of
> verifying that the entire `stderr` consists of just this one line: when
> running the test script via `dash`, using the `-x` option to trace the
> commands, the sub-shell in `force_color` causes those commands to be
> traced into `err.raw` because we set, but do not export
> `BASH_XTRACEFD`).

Your reasoning here confuses me. If we are using dash, then surely
BASH_XTRACEFD does not matter either way? Perhaps the subshell is
important if we are running bash, though.

Either way, being forgiving to the use of "-x" is a nice convenience and
I approve.

> +test_expect_success 'colors can be overridden' '
> +	test_config color.interactive.header red &&
> +	test_config color.interactive.help green &&
> +	test_config color.interactive.prompt yellow &&
> +	test_config color.interactive.error blue &&
> +	test_config color.diff.frag magenta &&
> +	test_config color.diff.context cyan &&
> +	test_config color.diff.old bold &&
> +	test_config color.diff.new "bold red" &&

Since you are being so thorough, and since it is already in the output,
maybe color color.diff.meta, too? Like:

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 28549a41a2..cca866c8f4 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -594,6 +594,7 @@ test_expect_success 'colors can be overridden' '
 	test_config color.interactive.help green &&
 	test_config color.interactive.prompt yellow &&
 	test_config color.interactive.error blue &&
+	test_config color.diff.meta italic &&
 	test_config color.diff.frag magenta &&
 	test_config color.diff.context cyan &&
 	test_config color.diff.old bold &&
@@ -625,9 +626,9 @@ test_expect_success 'colors can be overridden' '
 	  1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
 	<YELLOW>Patch update<RESET>>> <RED>           staged     unstaged path<RESET>
 	* 1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
-	<YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET>
-	<BOLD>--- a/color-test<RESET>
-	<BOLD>+++ b/color-test<RESET>
+	<YELLOW>Patch update<RESET>>> <ITALIC>diff --git a/color-test b/color-test<RESET>
+	<ITALIC>--- a/color-test<RESET>
+	<ITALIC>+++ b/color-test<RESET>
 	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>

-Peff

^ permalink raw reply related	[flat|nested] 66+ messages in thread

* Re: [PATCH 0/9] Fix color handling in git add -i
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
                   ` (8 preceding siblings ...)
  2020-11-10 23:42 ` [PATCH 9/9] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
@ 2020-11-11  2:36 ` Jeff King
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
  10 siblings, 0 replies; 66+ messages in thread
From: Jeff King @ 2020-11-11  2:36 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin

On Tue, Nov 10, 2020 at 11:42:10PM +0000, Johannes Schindelin via GitGitGadget wrote:

> This patch series started out as a tiny fix for a bug reported in 
> https://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.
> And then I only wanted to add a regression test to make sure that this does
> not regress. And then I just wanted to ensure that it passes both with the
> Perl version of git add -i as well as with the built-in version.
> 
> And in no time I was looking at a real patch series.

OK, I see now why you wanted to roll my work in. There was quite a bit
more here than I expected. :)

> Johannes Schindelin (9):
>   add -i (built-in): do show an error message for incorrect inputs
>   add -i (built-in): send error messages to stderr
>   add -i: use `reset_color` consistently
>   add -i (built-in): prevent the `reset` "color" from being configured
>   add -i (built-in): use correct names to load color.diff.* config
>   add -p (built-in): do not color the progress indicator separately
>   add -i (built-in): use the same indentation as the Perl version
>   add -i (Perl version): include indentation in the colored header
>   add -i: verify in the tests that colors can be overridden

They all look good to me. I left some minor comments on the final patch.

-Peff

^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 00/11] Fix color handling in git add -i
  2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
                   ` (9 preceding siblings ...)
  2020-11-11  2:36 ` [PATCH 0/9] Fix color handling in git add -i Jeff King
@ 2020-11-11 12:28 ` Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
                     ` (12 more replies)
  10 siblings, 13 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin

This patch series started out as a tiny fix for a bug reported by Philippe
Blain in 
https://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.
And then I only wanted to add a regression test to make sure that this does
not regress. And then I just wanted to ensure that it passes both with the
Perl version of git add -i as well as with the built-in version.

And in no time I was looking at a real patch series.

Changes since v1:

 * The regression test now actually exercises the re-coloring (that is the
   primary purpose of git add -p looking at the color.diff.* variables).
 * The way the built-in git add -p renders hunk headers of split hunks was
   aligned with how the Perl version does things.
 * We now consistently prefer color.diff.context over color.diff.plain, no
   matter whether using the Perl or the built-in version of git add -p.
 * The commit message for the regression test no longer confuses readers by
   mentioning dash.
 * The regression test was structured a bit better, first testing error
   message coloring, then the menu in git add -i and then the diff coloring
   in git add -p.

Johannes Schindelin (11):
  add -i (built-in): do show an error message for incorrect inputs
  add -i (built-in): send error messages to stderr
  add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk
    headers
  add -i: use `reset_color` consistently
  add -i (built-in): prevent the `reset` "color" from being configured
  add -i (built-in): use correct names to load color.diff.* config
  add -p (built-in): do not color the progress indicator separately
  add -i (built-in): use the same indentation as the Perl version
  add -i (Perl version): include indentation in the colored header
  add -p: prefer color.diff.context over color.diff.plain
  add -i: verify in the tests that colors can be overridden

 add-interactive.c          | 38 ++++++++++-------
 add-patch.c                | 25 +++++++-----
 git-add--interactive.perl  | 12 +++---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 127 insertions(+), 32 deletions(-)


base-commit: e4d83eee9239207622e2b1cc43967da5051c189c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/785

Range-diff vs v1:

  1:  6152122c04 =  1:  6152122c04 add -i (built-in): do show an error message for incorrect inputs
  2:  068813912b =  2:  068813912b add -i (built-in): send error messages to stderr
  -:  ---------- >  3:  98deb538d9 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers
  3:  9a1ba71977 =  4:  c857c44932 add -i: use `reset_color` consistently
  4:  b48c878fad =  5:  337b45cad8 add -i (built-in): prevent the `reset` "color" from being configured
  5:  85b0d27d76 =  6:  dcd2ffc458 add -i (built-in): use correct names to load color.diff.* config
  6:  059344bfaf =  7:  73b6d60a80 add -p (built-in): do not color the progress indicator separately
  7:  8df87e6395 =  8:  91ded2fbbe add -i (built-in): use the same indentation as the Perl version
  8:  42113e20dd =  9:  304614751e add -i (Perl version): include indentation in the colored header
  -:  ---------- > 10:  48d8e0badf add -p: prefer color.diff.context over color.diff.plain
  9:  38355ec98f ! 11:  c94abff72f add -i: verify in the tests that colors can be overridden
     @@ Commit message
      
          Note that we only `grep` for the colored error message instead of
          verifying that the entire `stderr` consists of just this one line: when
     -    running the test script via `dash`, using the `-x` option to trace the
     +    running the test script using the `-x` option to trace the
          commands, the sub-shell in `force_color` causes those commands to be
     -    traced into `err.raw` because we set, but do not export
     -    `BASH_XTRACEFD`).
     +    traced into `err.raw` (unless running in Bash where we set the
     +    `BASH_XTRACEFD` variable to avoid that).
      
     +    Also note that the color reset in the `<BLUE>+<RESET><BLUE>new<RESET>`
     +    line might look funny and unnecessary, as the corresponding `old` line
     +    does not reset the color after the diff marker only to turn the color
     +    back on right away.
     +
     +    However, this is a (necessary) side effect of the white-space check: in
     +    `emit_line_ws_markup()`, we first emit the diff marker via
     +    `emit_line_0()` and then the rest of the line via `ws_check_emit()`. To
     +    leave them somewhat decoupled, the color has to be reset after the diff
     +    marker to allow for the rest of the line to start with another color (or
     +    inverted, in case of white-space issues).
     +
     +    Finally, we have to simulate hunk editing: the `git add -p` command
     +    cannot rely on the internal diff machinery for coloring after letting
     +    the user edit a hunk; It has to "re-color" the edited hunk. This is the
     +    primary reason why that command is interested in the exact values of the
     +    `color.diff.*` settings in the first place. To test this re-coloring, we
     +    therefore have to pretend to edit a hunk and then show that hunk in the
     +    regression test.
     +
     +    Co-authored-by: Jeff King <peff@peff.net>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## t/t3701-add-interactive.sh ##
     @@ t/t3701-add-interactive.sh: test_expect_success 'diffs can be colorized' '
       '
       
      +test_expect_success 'colors can be overridden' '
     -+	test_config color.interactive.header red &&
     -+	test_config color.interactive.help green &&
     -+	test_config color.interactive.prompt yellow &&
     -+	test_config color.interactive.error blue &&
     -+	test_config color.diff.frag magenta &&
     -+	test_config color.diff.context cyan &&
     -+	test_config color.diff.old bold &&
     -+	test_config color.diff.new "bold red" &&
     -+
      +	git reset --hard &&
      +	test_when_finished "git rm -f color-test" &&
      +	test_write_lines context old more-context >color-test &&
      +	git add color-test &&
     -+	test_write_lines context new more-context >color-test &&
     ++	test_write_lines context new more-context another-one >color-test &&
      +
      +	echo trigger an error message >input &&
     -+	force_color git add -i 2>err.raw <input &&
     ++	force_color git \
     ++		-c color.interactive.error=blue \
     ++		add -i 2>err.raw <input &&
      +	test_decode_color <err.raw >err &&
      +	grep "<BLUE>Huh (trigger)?<RESET>" err &&
      +
     -+	test_write_lines patch color-test "" y quit >input &&
     -+	force_color git add -i >actual.raw <input &&
     -+	test_decode_color <actual.raw >actual.decoded &&
     -+	sed "/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/d" <actual.decoded >actual &&
     ++	test_write_lines help quit >input &&
     ++	force_color git \
     ++		-c color.interactive.header=red \
     ++		-c color.interactive.help=green \
     ++		-c color.interactive.prompt=yellow \
     ++		add -i >actual.raw <input &&
     ++	test_decode_color <actual.raw >actual &&
      +	cat >expect <<-\EOF &&
      +	<RED>           staged     unstaged path<RESET>
     -+	  1:        +3/-0        +1/-1 color-test
     ++	  1:        +3/-0        +2/-1 color-test
      +
      +	<RED>*** Commands ***<RESET>
      +	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
      +	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
     -+	<YELLOW>What now<RESET>> <RED>           staged     unstaged path<RESET>
     -+	  1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
     -+	<YELLOW>Patch update<RESET>>> <RED>           staged     unstaged path<RESET>
     -+	* 1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
     -+	<YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET>
     -+	<BOLD>--- a/color-test<RESET>
     -+	<BOLD>+++ b/color-test<RESET>
     -+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
     -+	<CYAN> context<RESET>
     -+	<BOLD>-old<RESET>
     -+	<BOLD;RED>+<RESET><BOLD;RED>new<RESET>
     -+	<CYAN> more-context<RESET>
     -+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,e,?]? <RESET>
     ++	<YELLOW>What now<RESET>> <GREEN>status        - show paths with changes<RESET>
     ++	<GREEN>update        - add working tree state to the staged set of changes<RESET>
     ++	<GREEN>revert        - revert staged set of changes back to the HEAD version<RESET>
     ++	<GREEN>patch         - pick hunks and update selectively<RESET>
     ++	<GREEN>diff          - view diff between HEAD and index<RESET>
     ++	<GREEN>add untracked - add contents of untracked files to the staged set of changes<RESET>
      +	<RED>*** Commands ***<RESET>
      +	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
      +	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
      +	<YELLOW>What now<RESET>> Bye.
      +	EOF
     ++	test_cmp expect actual &&
     ++
     ++	: exercise recolor_hunk by editing and then look at the hunk again &&
     ++	test_write_lines s e K q >input &&
     ++	force_color git \
     ++		-c color.interactive.prompt=yellow \
     ++		-c color.diff.meta=italic \
     ++		-c color.diff.frag=magenta \
     ++		-c color.diff.context=cyan \
     ++		-c color.diff.old=bold \
     ++		-c color.diff.new=blue \
     ++		-c core.editor=touch \
     ++		add -p >actual.raw <input &&
     ++	test_decode_color <actual.raw >actual.decoded &&
     ++	sed "s/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/<INDEX-LINE>/" <actual.decoded >actual &&
     ++	cat >expect <<-\EOF &&
     ++	<ITALIC>diff --git a/color-test b/color-test<RESET>
     ++	<ITALIC><INDEX-LINE><RESET>
     ++	<ITALIC>--- a/color-test<RESET>
     ++	<ITALIC>+++ b/color-test<RESET>
     ++	<MAGENTA>@@ -1,3 +1,4 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+<RESET><BLUE>new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<BLUE>+<RESET><BLUE>another-one<RESET>
     ++	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
     ++	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+<RESET><BLUE>new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<BLUE>+<RESET><BLUE>another-one<RESET>
     ++	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
     ++	<CYAN> context<RESET>
     ++	<BOLD>-old<RESET>
     ++	<BLUE>+new<RESET>
     ++	<CYAN> more-context<RESET>
     ++	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET>
     ++	EOF
      +	test_cmp expect actual
      +'
      +

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v2 01/11] add -i (built-in): do show an error message for incorrect inputs
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 02/11] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
                     ` (11 subsequent siblings)
  12 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

There is a neat feature in `git add -i` where it allows users to select
items via unique prefixes.

In the built-in version of `git add -i`, we specifically sort the items
(unless they are already sorted) and then perform a binary search to
figure out whether the input constitutes a unique prefix. Unfortunately,
by mistake this code misidentifies matches even if the input string is
not actually a prefix of any item.

For example, in the initial menu, where there is a `status` and an
`update` command, the input `tadaa` was mistaken as a prefix of
`update`.

Let's fix this by looking a bit closer whether the input is actually a
prefix of the item at the found insert index.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 555c4abf32..8ca503d803 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
 	else if (index + 1 < list->sorted.nr &&
 		 starts_with(list->sorted.items[index + 1].string, string))
 		return -1;
-	else if (index < list->sorted.nr)
+	else if (index < list->sorted.nr &&
+		 starts_with(list->sorted.items[index].string, string))
 		item = list->sorted.items[index].util;
 	else
 		return -1;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 02/11] add -i (built-in): send error messages to stderr
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers Johannes Schindelin via GitGitGadget
                     ` (10 subsequent siblings)
  12 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of that command already does that since a301973641f
(add -p: print errors in separate color, 2009-02-05). The built-in
version's development started by reimplementing the initial version from
5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though,
which still printed error messages to stdout.

Let's fix that by imitating the Perl version's behavior in the built-in
version of that command.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 8ca503d803..0f24992ca4 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
 
 			if (from < 0 || from >= items->items.nr ||
 			    (singleton && from + 1 != to)) {
-				color_fprintf_ln(stdout, s->error_color,
+				color_fprintf_ln(stderr, s->error_color,
 						 _("Huh (%s)?"), p);
 				break;
 			} else if (singleton) {
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 02/11] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
                     ` (9 subsequent siblings)
  12 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In libxdiff, imitating GNU diff, the hunk headers only show the line
count if it is different from 1. When splitting hunks, the Perl version
of `git add -p` already imitates this. Let's do the same in the built-in
version of said command.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index be4cf6e9e5..b6d53229bb 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -661,9 +661,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		else
 			new_offset += delta;
 
-		strbuf_addf(out, "@@ -%lu,%lu +%lu,%lu @@",
-			    old_offset, header->old_count,
-			    new_offset, header->new_count);
+		strbuf_addf(out, "@@ -%lu", old_offset);
+		if (header->old_count != 1)
+			strbuf_addf(out, ",%lu", header->old_count);
+		strbuf_addf(out, " +%lu", new_offset);
+		if (header->new_count != 1)
+			strbuf_addf(out, ",%lu", header->new_count);
+		strbuf_addstr(out, " @@");
+
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 04/11] add -i: use `reset_color` consistently
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (2 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-13 12:03     ` Phillip Wood
  2020-11-11 12:28   ` [PATCH v2 05/11] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
                     ` (8 subsequent siblings)
  12 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We already maintain a list of colors in the `add_i_state`, therefore we
should use them.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index b6d53229bb..bf89c43145 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -672,7 +672,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)
-			strbuf_addf(out, "%s\n", GIT_COLOR_RESET);
+			strbuf_addf(out, "%s\n", s->s.reset_color);
 		else
 			strbuf_addch(out, '\n');
 	}
@@ -1065,7 +1065,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
 			      s->s.file_new_color :
 			      s->s.context_color);
 		strbuf_add(&s->colored, plain + current, eol - current);
-		strbuf_addstr(&s->colored, GIT_COLOR_RESET);
+		strbuf_addstr(&s->colored, s->s.reset_color);
 		if (next > eol)
 			strbuf_add(&s->colored, plain + eol, next - eol);
 		current = next;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 05/11] add -i (built-in): prevent the `reset` "color" from being configured
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (3 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 06/11] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
                     ` (7 subsequent siblings)
  12 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of that command sneakily uses `git config --get-color`
to figure out the ANSI sequence to reset the color, but passes the empty
string and therefore cannot actually match any config entry.

This was missed when re-implementing the command as a built-in command.
Let's fix this, preventing the `reset` sequence from being overridden
via the config.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 0f24992ca4..f3a1d7456e 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
 	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
 	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
 	init_color(r, s, "fraginfo", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
 	init_color(r, s, "context", s->context_color,
@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
+	strlcpy(s->reset_color,
+		s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN);
+
 	FREE_AND_NULL(s->interactive_diff_filter);
 	git_config_get_string("interactive.difffilter",
 			      &s->interactive_diff_filter);
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 06/11] add -i (built-in): use correct names to load color.diff.* config
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (4 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 05/11] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
                     ` (6 subsequent siblings)
  12 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The builtin version of add-interactive mistakenly loads diff colors from
color.interactive.* instead of color.diff.*. It also accidentally spells
`frag` as `fraginfo`.

Let's fix that.

Note also that we don't respect the historical `diff.color.*`. The perl
version never did, and those have been deprecated since 2007.

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index f3a1d7456e..9126684348 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -12,10 +12,10 @@
 #include "prompt.h"
 
 static void init_color(struct repository *r, struct add_i_state *s,
-		       const char *slot_name, char *dst,
+		       const char *section_and_slot, char *dst,
 		       const char *default_color)
 {
-	char *key = xstrfmt("color.interactive.%s", slot_name);
+	char *key = xstrfmt("color.%s", section_and_slot);
 	const char *value;
 
 	if (!s->use_color)
@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 			git_config_colorbool("color.interactive", value);
 	s->use_color = want_color(s->use_color);
 
-	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
-	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
-	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "fraginfo", s->fraginfo_color,
+	init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD);
+	init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED);
+	init_color(r, s, "interactive.prompt", s->prompt_color,
+		   GIT_COLOR_BOLD_BLUE);
+	init_color(r, s, "interactive.error", s->error_color,
+		   GIT_COLOR_BOLD_RED);
+
+	init_color(r, s, "diff.frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "context", s->context_color,
+	init_color(r, s, "diff.context", s->context_color,
 		diff_get_color(s->use_color, DIFF_CONTEXT));
-	init_color(r, s, "old", s->file_old_color,
+	init_color(r, s, "diff.old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
-	init_color(r, s, "new", s->file_new_color,
+	init_color(r, s, "diff.new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
 	strlcpy(s->reset_color,
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (5 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 06/11] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-13 12:07     ` Phillip Wood
  2020-11-11 12:28   ` [PATCH v2 08/11] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
                     ` (5 subsequent siblings)
  12 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of this command colors the progress indicator and the
prompt message in one go, let's do the same in the built-in version.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index bf89c43145..2fad92ca37 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,
 		else
 			prompt_mode_type = PROMPT_HUNK;
 
-		color_fprintf(stdout, s->s.prompt_color,
-			      "(%"PRIuMAX"/%"PRIuMAX") ",
+		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
 			      (uintmax_t)hunk_index + 1,
 			      (uintmax_t)(file_diff->hunk_nr
 						? file_diff->hunk_nr
 						: 1));
-		color_fprintf(stdout, s->s.prompt_color,
-			      _(s->mode->prompt_mode[prompt_mode_type]),
-			      s->buf.buf);
+		printf(_(s->mode->prompt_mode[prompt_mode_type]),
+		       s->buf.buf);
+		if (*s->s.reset_color)
+			fputs(s->s.reset_color, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF)
 			break;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 08/11] add -i (built-in): use the same indentation as the Perl version
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (6 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-11 12:28   ` [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
                     ` (4 subsequent siblings)
  12 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When copying the spaces used to indent non-flat lists in `git add -i`,
one space was appended by mistake. This makes the output of the built-in
version of `git add -i` inconsistent with the Perl version. Let's adjust
the built-in version to produce the same output as the Perl version.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 9126684348..c298a8b80f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
 	print_file_item_data.color = data.color;
 	print_file_item_data.reset = data.reset;
 
-	strbuf_addstr(&header, "      ");
+	strbuf_addstr(&header, "     ");
 	strbuf_addf(&header, print_file_item_data.modified_fmt,
 		    _("staged"), _("unstaged"), _("path"));
 	opts.list_opts.header = header.buf;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (7 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 08/11] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-13 12:11     ` Phillip Wood
  2020-11-11 12:28   ` [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
                     ` (3 subsequent siblings)
  12 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The header is formatted by padding each column heading with spaces up to
the length of 12 characters. These padding spaces are naturally included
when coloring the entire header.

However, the preceding five spaces indentation for non-flat lists were
_not_ included in the Perl version, but _were_ included in the built-in
version. Let's adjust the former to align with the latter's behavior.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e713fe3d02..adbac2bc6d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -483,10 +483,8 @@ sub list_and_choose {
 		my $last_lf = 0;
 
 		if ($opts->{HEADER}) {
-			if (!$opts->{LIST_FLAT}) {
-				print "     ";
-			}
-			print colored $header_color, "$opts->{HEADER}\n";
+			my $indent = $opts->{LIST_FLAT} ? "" : "     ";
+			print colored $header_color, "$indent$opts->{HEADER}\n";
 		}
 		for ($i = 0; $i < @stuff; $i++) {
 			my $chosen = $chosen[$i] ? '*' : ' ';
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (8 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-13 14:04     ` Philippe Blain
  2020-11-13 14:08     ` Phillip Wood
  2020-11-11 12:28   ` [PATCH v2 11/11] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
                     ` (2 subsequent siblings)
  12 siblings, 2 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

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

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


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v2 11/11] add -i: verify in the tests that colors can be overridden
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (9 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
@ 2020-11-11 12:28   ` Johannes Schindelin via GitGitGadget
  2020-11-11 18:45   ` [PATCH v2 00/11] Fix color handling in git add -i Jeff King
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
  12 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-11 12:28 UTC (permalink / raw)
  To: git; +Cc: Philippe Blain, Jeff King, Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Now that the Perl version produces the same output as the built-in
version (mostly fixing bugs in the latter), let's add a regression test
to verify that it stays this way.

Note that we only `grep` for the colored error message instead of
verifying that the entire `stderr` consists of just this one line: when
running the test script using the `-x` option to trace the
commands, the sub-shell in `force_color` causes those commands to be
traced into `err.raw` (unless running in Bash where we set the
`BASH_XTRACEFD` variable to avoid that).

Also note that the color reset in the `<BLUE>+<RESET><BLUE>new<RESET>`
line might look funny and unnecessary, as the corresponding `old` line
does not reset the color after the diff marker only to turn the color
back on right away.

However, this is a (necessary) side effect of the white-space check: in
`emit_line_ws_markup()`, we first emit the diff marker via
`emit_line_0()` and then the rest of the line via `ws_check_emit()`. To
leave them somewhat decoupled, the color has to be reset after the diff
marker to allow for the rest of the line to start with another color (or
inverted, in case of white-space issues).

Finally, we have to simulate hunk editing: the `git add -p` command
cannot rely on the internal diff machinery for coloring after letting
the user edit a hunk; It has to "re-color" the edited hunk. This is the
primary reason why that command is interested in the exact values of the
`color.diff.*` settings in the first place. To test this re-coloring, we
therefore have to pretend to edit a hunk and then show that hunk in the
regression test.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index ca04fac417..cc3f434a97 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -589,6 +589,90 @@ test_expect_success 'diffs can be colorized' '
 	grep "$(printf "\\033")" output
 '
 
+test_expect_success 'colors can be overridden' '
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context another-one >color-test &&
+
+	echo trigger an error message >input &&
+	force_color git \
+		-c color.interactive.error=blue \
+		add -i 2>err.raw <input &&
+	test_decode_color <err.raw >err &&
+	grep "<BLUE>Huh (trigger)?<RESET>" err &&
+
+	test_write_lines help quit >input &&
+	force_color git \
+		-c color.interactive.header=red \
+		-c color.interactive.help=green \
+		-c color.interactive.prompt=yellow \
+		add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual &&
+	cat >expect <<-\EOF &&
+	<RED>           staged     unstaged path<RESET>
+	  1:        +3/-0        +2/-1 color-test
+
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> <GREEN>status        - show paths with changes<RESET>
+	<GREEN>update        - add working tree state to the staged set of changes<RESET>
+	<GREEN>revert        - revert staged set of changes back to the HEAD version<RESET>
+	<GREEN>patch         - pick hunks and update selectively<RESET>
+	<GREEN>diff          - view diff between HEAD and index<RESET>
+	<GREEN>add untracked - add contents of untracked files to the staged set of changes<RESET>
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> Bye.
+	EOF
+	test_cmp expect actual &&
+
+	: exercise recolor_hunk by editing and then look at the hunk again &&
+	test_write_lines s e K q >input &&
+	force_color git \
+		-c color.interactive.prompt=yellow \
+		-c color.diff.meta=italic \
+		-c color.diff.frag=magenta \
+		-c color.diff.context=cyan \
+		-c color.diff.old=bold \
+		-c color.diff.new=blue \
+		-c core.editor=touch \
+		add -p >actual.raw <input &&
+	test_decode_color <actual.raw >actual.decoded &&
+	sed "s/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/<INDEX-LINE>/" <actual.decoded >actual &&
+	cat >expect <<-\EOF &&
+	<ITALIC>diff --git a/color-test b/color-test<RESET>
+	<ITALIC><INDEX-LINE><RESET>
+	<ITALIC>--- a/color-test<RESET>
+	<ITALIC>+++ b/color-test<RESET>
+	<MAGENTA>@@ -1,3 +1,4 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+<RESET><BLUE>new<RESET>
+	<CYAN> more-context<RESET>
+	<BLUE>+<RESET><BLUE>another-one<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+<RESET><BLUE>new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<CYAN> more-context<RESET>
+	<BLUE>+<RESET><BLUE>another-one<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET>
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 66+ messages in thread

* Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
  2020-11-11  2:35   ` Jeff King
@ 2020-11-11 15:53     ` Johannes Schindelin
  2020-11-11 18:07       ` Jeff King
  0 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-11 15:53 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git

Hi Peff,

On Tue, 10 Nov 2020, Jeff King wrote:

> On Tue, Nov 10, 2020 at 11:42:19PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Now that the Perl version produces the same output as the built-in
> > version (mostly fixing bugs in the latter), let's add a regression test
> > to verify that it stays this way.
> >
> > Note that we only `grep` for the colored error message instead of
> > verifying that the entire `stderr` consists of just this one line: when
> > running the test script via `dash`, using the `-x` option to trace the
> > commands, the sub-shell in `force_color` causes those commands to be
> > traced into `err.raw` because we set, but do not export
> > `BASH_XTRACEFD`).
>
> Your reasoning here confuses me.

Sorry!

> If we are using dash, then surely BASH_XTRACEFD does not matter either
> way?

It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable,
funnily enough, but does not hand the settings through to sub-shells,
whereas Bash does.

> Perhaps the subshell is important if we are running bash, though.

The most important thing, which I of course _failed_ to describe most
prominently, is that _in general_ `-x` will mess up the `err` file, i.e.
when running with anything but Bash. Therefoer we need the `grep` instead
of a `test_cmp`, and that's what I tried to convey in v2.

> Either way, being forgiving to the use of "-x" is a nice convenience and
> I approve.
>
> > +test_expect_success 'colors can be overridden' '
> > +	test_config color.interactive.header red &&
> > +	test_config color.interactive.help green &&
> > +	test_config color.interactive.prompt yellow &&
> > +	test_config color.interactive.error blue &&
> > +	test_config color.diff.frag magenta &&
> > +	test_config color.diff.context cyan &&
> > +	test_config color.diff.old bold &&
> > +	test_config color.diff.new "bold red" &&
>
> Since you are being so thorough, and since it is already in the output,
> maybe color color.diff.meta, too? Like:
>
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 28549a41a2..cca866c8f4 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -594,6 +594,7 @@ test_expect_success 'colors can be overridden' '
>  	test_config color.interactive.help green &&
>  	test_config color.interactive.prompt yellow &&
>  	test_config color.interactive.error blue &&
> +	test_config color.diff.meta italic &&
>  	test_config color.diff.frag magenta &&
>  	test_config color.diff.context cyan &&
>  	test_config color.diff.old bold &&
> @@ -625,9 +626,9 @@ test_expect_success 'colors can be overridden' '
>  	  1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
>  	<YELLOW>Patch update<RESET>>> <RED>           staged     unstaged path<RESET>
>  	* 1:        +3/-0        +1/-1 <YELLOW>c<RESET>olor-test
> -	<YELLOW>Patch update<RESET>>> <BOLD>diff --git a/color-test b/color-test<RESET>
> -	<BOLD>--- a/color-test<RESET>
> -	<BOLD>+++ b/color-test<RESET>
> +	<YELLOW>Patch update<RESET>>> <ITALIC>diff --git a/color-test b/color-test<RESET>
> +	<ITALIC>--- a/color-test<RESET>
> +	<ITALIC>+++ b/color-test<RESET>
>  	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
>  	<CYAN> context<RESET>
>  	<BOLD>-old<RESET>

Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting
is not even read by the interactive add command:

	$ git grep -w meta git-add--interactive.perl add-interactive.c \
		add-patch.c

comes up empty.

The reason is that we actually let the diff machinery do all the coloring.
So why, then, you might ask, do we bother to read those values in the
first place?

Well, you know, in `git add -p`, you can split hunks. That's still fine,
we basically just have to color the hunk header that we insert, and can
reuse the original coloring of the remaining lines, because they are
unchanged.

But. But you can also `edit` hunks.

And after that `edit` step, those hunks have to be re-colored. And _that_
is why `git-add--interactive` bothers to read those `color.diff.*` values
to begin with.

Now, how do you see this re-coloring in action?

I am almost glad that you asked. By simulating that `edit` in the
regression test. But that is not enough because the hunk is staged
immediately after editing. But you _can_ go back to that hunk, even if it
already has been changed, and make up your mind to _not_ stage it, but
that only works if `git add -p` hasn't quit already, which it would do
with a single hunk, so we have to have _two_ hunks.

This not only complicates the regression test, but _of course_ (because
_why would I already be done with this patch series???_) it shows further
differences between the Perl version and the built-in version. It's almost
as if `git add -i` said "Johannes, you think those 500 hours were all you
would spend on converting me to a built-in? Think again!".

One of the issues I found (and fixed in v2) was that the built-in version
tried to be practical and _not_ special-case a count of one in the hunk
header (`@@ -3 +3 @@` is equivalent to `@@ -3,1 +3,1 @@`). Well, now it
does, just like libxdiff and the Perl version.

The other issue was that the Perl version still used `color.diff.plain`
instead of `color.diff.context`. I hope I found a good-enough solution to
the problem that we simply cannot call `git config --get-color` or
`repo_get_config()` with _two_ keys to look for (and the last one wins).

For those reasons, v2 brings more changes than I had hoped for. In the
end, it is a better patch series, obviously. So even if I was reluctant to
work on all this: thank you for prodding me.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
  2020-11-11 15:53     ` Johannes Schindelin
@ 2020-11-11 18:07       ` Jeff King
  2020-11-12 14:04         ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Jeff King @ 2020-11-11 18:07 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git

On Wed, Nov 11, 2020 at 04:53:13PM +0100, Johannes Schindelin wrote:

> > If we are using dash, then surely BASH_XTRACEFD does not matter either
> > way?
> 
> It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable,
> funnily enough, but does not hand the settings through to sub-shells,
> whereas Bash does.

Really? That's news to me, and doesn't seem to trigger:

  [bash uses it]
  $ bash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace
  + BASH_XTRACEFD=3
  foo
  $ cat trace
  + echo foo

  [dash does not]
  $ dash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace
  + BASH_XTRACEFD=3
  + echo foo
  foo
  $ cat trace

> > Perhaps the subshell is important if we are running bash, though.
> 
> The most important thing, which I of course _failed_ to describe most
> prominently, is that _in general_ `-x` will mess up the `err` file, i.e.
> when running with anything but Bash. Therefoer we need the `grep` instead
> of a `test_cmp`, and that's what I tried to convey in v2.

Yeah. I understood that part (because it has bit me so many times ;) ),
but I agree it's good to make it clear.

> Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting
> is not even read by the interactive add command:
> 
> 	$ git grep -w meta git-add--interactive.perl add-interactive.c \
> 		add-patch.c
> 
> comes up empty.
> [how and why add--interactive.perl reads color config]

Hmm. Right, I knew about that weirdness. But I assumed that the builtin
add-interactive was doing the diffs in-core. Otherwise, why would we
have seen the failure to load diff.color.frag in the first place?
Philippe's simple example just did "git add -p". So now I'm doubly
confused.

The answer seems to be that render_hunk() always _replaces_ the colors
we got from running the external diff. Whereas the perl version only
applied coloring when reading back in the results of an edit operation
(and likewise used the frag color when generating a split hunk header).

I'm not sure that what the builtin version is doing is wrong, but it
seems like it's putting a lot of extra effort into parsing colors off of
the colorized version. Whereas the perl version just assumes the lines
match up. I do wonder if there are corner cases we might hit around
filters here, though. The lines we get from a filter might bear no
resemblance at all to diff lines. The only thing we require in the perl
version is that they have correspond 1-to-1 with the unfiltered diff
lines in meaning.

> For those reasons, v2 brings more changes than I had hoped for. In the
> end, it is a better patch series, obviously. So even if I was reluctant to
> work on all this: thank you for prodding me.

Heh. Sorry and thanks, I guess? :) I'll try to read over v2 carefully.

-Peff

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 00/11] Fix color handling in git add -i
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (10 preceding siblings ...)
  2020-11-11 12:28   ` [PATCH v2 11/11] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
@ 2020-11-11 18:45   ` Jeff King
  2020-11-12 14:20     ` Johannes Schindelin
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
  12 siblings, 1 reply; 66+ messages in thread
From: Jeff King @ 2020-11-11 18:45 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Johannes Schindelin

On Wed, Nov 11, 2020 at 12:28:13PM +0000, Johannes Schindelin via GitGitGadget wrote:

> Changes since v1:
> 
>  * The regression test now actually exercises the re-coloring (that is the
>    primary purpose of git add -p looking at the color.diff.* variables).
>  * The way the built-in git add -p renders hunk headers of split hunks was
>    aligned with how the Perl version does things.
>  * We now consistently prefer color.diff.context over color.diff.plain, no
>    matter whether using the Perl or the built-in version of git add -p.
>  * The commit message for the regression test no longer confuses readers by
>    mentioning dash.
>  * The regression test was structured a bit better, first testing error
>    message coloring, then the menu in git add -i and then the diff coloring
>    in git add -p.

The changes were less scary than I was led to believe after reading your
earlier response. :)

Everything here looks sensible. As I said elsewhere, I do worry there
may be a lingering issue with how parse_diff() looks at the filtered
diffs. Let me see if I can get diff-so-fancy working...

Aha, yes. I think using diff-so-fancy here isn't entirely robust,
because it has some cases where it fails at the 1-to-1 line
correspondence, but they're aware of the issue. But it does work in
simples cases now (there's some coloring which makes the output more
meaningful, but I obviously won't paste it here):

  $ git -c interactive.difffilter='diff-so-fancy' add -p
  ──────────────────────────────────────────────────────────────────────
  modified: file
  ──────────────────────────────────────────────────────────────────────
  @ file:1 @
  old
  new
  (1/1) Stage this hunk [y,n,q,a,d,e,?]? 

But with the builtin:

  $ git -c interactive.difffilter='diff-so-fancy' \
        -c add.interactive.usebuiltin=true \
	add -p
  error: could not parse colored hunk header '?[1m?[38;5;1m?[31mold?[m'

I don't use it myself, and as I said, I think they have some fixes to
make for it to be robust as a diff-filter. But I suspect this may be a
problem once the builtin version becomes the default.

I don't think it should be dealt with in this patch series, though.
While it may touch on some of the coloring code, it's otherwise
orthogonal to the fixes here. And while the fix is likely to be to make
render_hunk() stop re-coloring in the non-edit cases, your more-robust
test here will still be checking what you expect (because it really is
triggering the edit case, not relying on the extra coloring to see the
effect of in-process color config).

-Peff

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
  2020-11-11 18:07       ` Jeff King
@ 2020-11-12 14:04         ` Johannes Schindelin
  2020-11-12 18:29           ` Jeff King
  0 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-12 14:04 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git

Hi Peff,

On Wed, 11 Nov 2020, Jeff King wrote:

> On Wed, Nov 11, 2020 at 04:53:13PM +0100, Johannes Schindelin wrote:
>
> > > If we are using dash, then surely BASH_XTRACEFD does not matter either
> > > way?
> >
> > It kinda does, though. Dash _does_ respect the `BASH_XTRACEFD` variable,
> > funnily enough, but does not hand the settings through to sub-shells,
> > whereas Bash does.
>
> Really? That's news to me, and doesn't seem to trigger:
>
>   [bash uses it]
>   $ bash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace
>   + BASH_XTRACEFD=3
>   foo
>   $ cat trace
>   + echo foo
>
>   [dash does not]
>   $ dash -xc 'BASH_XTRACEFD=3; echo foo' 3>trace
>   + BASH_XTRACEFD=3
>   + echo foo
>   foo
>   $ cat trace

Gaaah! I totally forgot that `BASH_XTRACEFD` is only heeded by BusyBox'
ash (and only when built with `ENABLE_ASH_BASH_COMPAT`), not by `dash`.

Sorry for the noise.

> > Oh my. I really had tried to avoid going _this_ deep. The `.meta` setting
> > is not even read by the interactive add command:
> >
> > 	$ git grep -w meta git-add--interactive.perl add-interactive.c \
> > 		add-patch.c
> >
> > comes up empty.
> > [how and why add--interactive.perl reads color config]
>
> Hmm. Right, I knew about that weirdness. But I assumed that the builtin
> add-interactive was doing the diffs in-core. Otherwise, why would we
> have seen the failure to load diff.color.frag in the first place?

Oh, that's easy to explain: as you can verify reading
https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898
the Perl version of `git add -p` insists on (re-)constructing the hunk
headers manually, and obviously it needs to color them manually, too. And
https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows
that the built-in version of `git add -p` slavishly follows that practice.

> Philippe's simple example just did "git add -p". So now I'm doubly
> confused.

Right. I should have been more precise in what parts are used of the diff
that is colored via the internal diff machinery. The hunk headers are not
used. The hunks themselves are, unless edited. The file header is, too:
https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L683-L714

> The answer seems to be that render_hunk() always _replaces_ the colors
> we got from running the external diff. Whereas the perl version only
> applied coloring when reading back in the results of an edit operation
> (and likewise used the frag color when generating a split hunk header).

No, the Perl version also insists on applying `fraginfo_color`, see
https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898

> I'm not sure that what the builtin version is doing is wrong, but it
> seems like it's putting a lot of extra effort into parsing colors off of
> the colorized version. Whereas the perl version just assumes the lines
> match up. I do wonder if there are corner cases we might hit around
> filters here, though. The lines we get from a filter might bear no
> resemblance at all to diff lines. The only thing we require in the perl
> version is that they have correspond 1-to-1 with the unfiltered diff
> lines in meaning.

They do have to correspond 1-to-1 because the assumption is that the
individual lines will then correspond one-to-one, too. This does not need
to be true, of course, but then the filter is probably less useful than
the user wants it to be.

> > For those reasons, v2 brings more changes than I had hoped for. In the
> > end, it is a better patch series, obviously. So even if I was reluctant to
> > work on all this: thank you for prodding me.
>
> Heh. Sorry and thanks, I guess? :) I'll try to read over v2 carefully.

No need to be sorry on your side. _I_ am sorry that I did not add that
test case during my re-implementation efforts in the first place.

Thanks,
DScho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 00/11] Fix color handling in git add -i
  2020-11-11 18:45   ` [PATCH v2 00/11] Fix color handling in git add -i Jeff King
@ 2020-11-12 14:20     ` Johannes Schindelin
  2020-11-12 18:40       ` Jeff King
  0 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-12 14:20 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain

[-- Attachment #1: Type: text/plain, Size: 5622 bytes --]

Hi Peff,

On Wed, 11 Nov 2020, Jeff King wrote:

> On Wed, Nov 11, 2020 at 12:28:13PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > Changes since v1:
> >
> >  * The regression test now actually exercises the re-coloring (that is the
> >    primary purpose of git add -p looking at the color.diff.* variables).
> >  * The way the built-in git add -p renders hunk headers of split hunks was
> >    aligned with how the Perl version does things.
> >  * We now consistently prefer color.diff.context over color.diff.plain, no
> >    matter whether using the Perl or the built-in version of git add -p.
> >  * The commit message for the regression test no longer confuses readers by
> >    mentioning dash.
> >  * The regression test was structured a bit better, first testing error
> >    message coloring, then the menu in git add -i and then the diff coloring
> >    in git add -p.
>
> The changes were less scary than I was led to believe after reading your
> earlier response. :)
>
> Everything here looks sensible. As I said elsewhere, I do worry there
> may be a lingering issue with how parse_diff() looks at the filtered
> diffs. Let me see if I can get diff-so-fancy working...
>
> Aha, yes. I think using diff-so-fancy here isn't entirely robust,
> because it has some cases where it fails at the 1-to-1 line
> correspondence, but they're aware of the issue. But it does work in
> simples cases now (there's some coloring which makes the output more
> meaningful, but I obviously won't paste it here):
>
>   $ git -c interactive.difffilter='diff-so-fancy' add -p
>   ──────────────────────────────────────────────────────────────────────
>   modified: file
>   ──────────────────────────────────────────────────────────────────────
>   @ file:1 @
>   old
>   new
>   (1/1) Stage this hunk [y,n,q,a,d,e,?]?

It might _seem_ that it works. But try to split a hunk. I did this with
the test case (interrupting it just before running `add -p`):

-- snip --
$ git -C ./t/trash\ directory.t3701-add-interactive/ -c
interactive.difffilter='diff-so-fancy' -c add.interactive.usebuiltin=false add -p
<BOLD>────────────────────────────────────────────────────────────────────────────────</BOLD>
<BOLD>modified: color-test</BOLD>
<BOLD>────────────────────────────────────────────────────────────────────────────────</BOLD>
<MAGENTA>@ color-test:1 @</MAGENTA>
context
<RED>old</RED>
<GREEN>new</GREEN>
more-context
<GREEN>another-one</GREEN>
<BLUE>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]?</BLUE> s
<BOLD>Split into 2 hunks.</BOLD>
<MAGENTA>@@ -1,3 +1,3 @@</MAGENTA>
<RED>old</RED>
<GREEN>new</GREEN>
more-context
<GREEN>another-one</GREEN>
<BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?</BLUE>
-- snap --

Obviously, I marked it up so you can see what parts were colored.

See how it first _looks_ as if it works? But then you split the hunk, but
instead of showing only the old/new part, it shows the old/new/another-one
part!

In other words, it does not work at all, and the fact that it does not
even warn you about it is misleading, and that's pretty much all I will
say about it.

> But with the builtin:
>
>   $ git -c interactive.difffilter='diff-so-fancy' \
>         -c add.interactive.usebuiltin=true \
> 	add -p
>   error: could not parse colored hunk header '?[1m?[38;5;1m?[31mold?[m'

Granted, this is not quite helpful. I _think_ what is happening is that
the number of lines is different, and `add -p` goes like: noooope! But it
could probably be better at describing what the issue is. Or it could
cater to `diff-so-fancy`, if that is what people use these days, and
special-case it by falling back to detecting the hunk boundaries in a
manner that is compatible with `diff-so-fancy`.

Or we might be able to come up with a strategy that is not so limited and
that works for more than just `diff-so-fancy`.

> I don't use it myself, and as I said, I think they have some fixes to
> make for it to be robust as a diff-filter. But I suspect this may be a
> problem once the builtin version becomes the default.
>
> I don't think it should be dealt with in this patch series, though.

Oh, _that_ I wholeheartedly agree with.

> While it may touch on some of the coloring code, it's otherwise
> orthogonal to the fixes here. And while the fix is likely to be to make
> render_hunk() stop re-coloring in the non-edit cases, your more-robust
> test here will still be checking what you expect (because it really is
> triggering the edit case, not relying on the extra coloring to see the
> effect of in-process color config).

I don't actually think that we _can_ stop re-coloring the hunk header in
the general case because we need to keep this working even for split
hunks. It would be a very bad idea to make it work for non-split cases and
then something like `die()` only when the hunk is split. Better re-color
all of them, then, to not even risk that situation.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
  2020-11-12 14:04         ` Johannes Schindelin
@ 2020-11-12 18:29           ` Jeff King
  2020-11-15 23:35             ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Jeff King @ 2020-11-12 18:29 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git

On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote:

> Gaaah! I totally forgot that `BASH_XTRACEFD` is only heeded by BusyBox'
> ash (and only when built with `ENABLE_ASH_BASH_COMPAT`), not by `dash`.
> 
> Sorry for the noise.

Ah, that makes more sense. And I learned something new about busybox. :)

> > Hmm. Right, I knew about that weirdness. But I assumed that the builtin
> > add-interactive was doing the diffs in-core. Otherwise, why would we
> > have seen the failure to load diff.color.frag in the first place?
> 
> Oh, that's easy to explain: as you can verify reading
> https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898
> the Perl version of `git add -p` insists on (re-)constructing the hunk
> headers manually, and obviously it needs to color them manually, too. And
> https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows
> that the built-in version of `git add -p` slavishly follows that practice.

But that is only when we split hunks (your link to the perl script is in
split_hunks()). I agree we must color manually there when creating our
own hunk header. But outside of that and patch-editing, the perl script
does not otherwise recolor or rewrite (nor even really parse beyond
line-splitting) what it gets from the colorized version.

Whereas add-patch parses the colors off of the version and then
re-colors every hunk header. Which seems doubly weird to me. Even if we
were going to re-color every hunk (e.g., because we don't want to store
the original hunk line, but instead a parsed version of it), why bother
parsing the color version at all, and not just the machine-readable
version?

> > The answer seems to be that render_hunk() always _replaces_ the colors
> > we got from running the external diff. Whereas the perl version only
> > applied coloring when reading back in the results of an edit operation
> > (and likewise used the frag color when generating a split hunk header).
> 
> No, the Perl version also insists on applying `fraginfo_color`, see
> https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898

Only when we split. Try this to give different colors between the
interactive script and diff:

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e713fe3d02..862a21ff1f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -28,8 +28,9 @@
 my $diff_use_color = $repo->get_colorbool('color.diff');
 my ($fraginfo_color) =
 	$diff_use_color ? (
-		$repo->get_color('color.diff.frag', 'cyan'),
+		$repo->get_color('color.diff.nonsense', 'yellow'),
 	) : ();
+# noop to create split hunk
 my ($diff_plain_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.plain', ''),

Running "git add -p" does not result in yellow hunk headers. But issuing
a split command does.

The distinction is mostly academic, because diff-tree and the
interactive patch code should be using the same colors, so the result
should look the same. It could matter if the diff-filter chooses
different colors, though then the split headers will not match the
originals in style. We _could_ run the newly created hunk header
individually through the diff-filter, though I'm not sure how various
filters would handle that.

That's true of the perl version as well as the builtin one, but I think
the builtin one's insistence on parsing the colored output is taking us
in the wrong direction to eventually fix that.

> > I'm not sure that what the builtin version is doing is wrong, but it
> > seems like it's putting a lot of extra effort into parsing colors off of
> > the colorized version. Whereas the perl version just assumes the lines
> > match up. I do wonder if there are corner cases we might hit around
> > filters here, though. The lines we get from a filter might bear no
> > resemblance at all to diff lines. The only thing we require in the perl
> > version is that they have correspond 1-to-1 with the unfiltered diff
> > lines in meaning.
> 
> They do have to correspond 1-to-1 because the assumption is that the
> individual lines will then correspond one-to-one, too. This does not need
> to be true, of course, but then the filter is probably less useful than
> the user wants it to be.

Right, I'm not disputing the 1-to-1 thing (I was after all the one who
implemented interactive.diffilter, and added the "complain if the counts
don't line up" check). But in the perl script they only need to
correspond _semantically_, not syntactically.

-Peff

^ permalink raw reply related	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 00/11] Fix color handling in git add -i
  2020-11-12 14:20     ` Johannes Schindelin
@ 2020-11-12 18:40       ` Jeff King
  2020-11-15 23:40         ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Jeff King @ 2020-11-12 18:40 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain

On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote:

> >   $ git -c interactive.difffilter='diff-so-fancy' add -p
> >   ──────────────────────────────────────────────────────────────────────
> >   modified: file
> >   ──────────────────────────────────────────────────────────────────────
> >   @ file:1 @
> >   old
> >   new
> >   (1/1) Stage this hunk [y,n,q,a,d,e,?]?
> 
> It might _seem_ that it works. But try to split a hunk. I did this with
> the test case (interrupting it just before running `add -p`):

Yes, there are definitely problems with how diff-so-fancy represents
things (because they don't maintain a 1-to-1 correspondence). You should
generally get a complaint like:

  $ git -c interactive.difffilter='diff-so-fancy' add -p
  fatal: mismatched output from interactive.diffFilter
  hint: Your filter must maintain a one-to-one correspondence
  hint: between its input and output lines.

The diff-so-fancy folks have asked me about this, and I've made clear
the problem to them, and that the solution is to have a mode which
maintains the line correspondence. AFAIK there's no fix yet. I don't
know how many people are actually using it in practice in its current
buggy state.

There's a big thread here:

  https://github.com/so-fancy/diff-so-fancy/issues/35

I don't really recommend reading the whole thing. Beyond what I just
said, the interesting bits are:

  - people recommend using the "delta" tool; it has a "--color-only"
    option which does just diff-highlight-style coloring, but doesn't
    break line correspondence

  - somebody suggested instead of using interactive.difffilter, to add
    an option to show each hunk in an individual pager command. So
    add-interactive would never see the "fancy" version at all, but it
    would be generated on the fly when the user sees it (and that filter
    would have to be able to handle seeing individual hunks without the
    diff header).

All of which is to say that the current state is a bit of a mess, and I
don't consider it completely necessary to fix it before the builtin
version becomes the default. But:

  - I think we should expect some possible complaints / bug reports
    from fringe users of the already-somewhat-broken state

  - IMHO the parsing of the filtered version done by the builtin is
    going in the wrong direction (see my other mail for an elaboration)

> > While it may touch on some of the coloring code, it's otherwise
> > orthogonal to the fixes here. And while the fix is likely to be to make
> > render_hunk() stop re-coloring in the non-edit cases, your more-robust
> > test here will still be checking what you expect (because it really is
> > triggering the edit case, not relying on the extra coloring to see the
> > effect of in-process color config).
> 
> I don't actually think that we _can_ stop re-coloring the hunk header in
> the general case because we need to keep this working even for split
> hunks. It would be a very bad idea to make it work for non-split cases and
> then something like `die()` only when the hunk is split. Better re-color
> all of them, then, to not even risk that situation.

Yeah, obviously calling die() in the split case is bad. And the fact
that newly-created split hunk headers are not filtered the same way as
the original hunk headers isn't ideal. But it's a pretty easy fix in the
perl version, and not in the builtin version.

-Peff

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 04/11] add -i: use `reset_color` consistently
  2020-11-11 12:28   ` [PATCH v2 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
@ 2020-11-13 12:03     ` Phillip Wood
  2020-11-13 13:53       ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2020-11-13 12:03 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> We already maintain a list of colors in the `add_i_state`, therefore we
> should use them.

Playing devil's advocate for a moment - why do we have a reset entry in 
that list? The next patch makes sure it cannot be customized which is a 
good thing so why not drop the `reset` member from `add_i_state` 
entirely? The perl version needed to get the reset string from `git 
config` and store it somewhere but in the C version we have a perfectly 
good constant we can use instead.

Best Wishes

Phillip


> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-patch.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index b6d53229bb..bf89c43145 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -672,7 +672,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
>   		if (len)
>   			strbuf_add(out, p, len);
>   		else if (colored)
> -			strbuf_addf(out, "%s\n", GIT_COLOR_RESET);
> +			strbuf_addf(out, "%s\n", s->s.reset_color);
>   		else
>   			strbuf_addch(out, '\n');
>   	}
> @@ -1065,7 +1065,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
>   			      s->s.file_new_color :
>   			      s->s.context_color);
>   		strbuf_add(&s->colored, plain + current, eol - current);
> -		strbuf_addstr(&s->colored, GIT_COLOR_RESET);
> +		strbuf_addstr(&s->colored, s->s.reset_color);
>   		if (next > eol)
>   			strbuf_add(&s->colored, plain + eol, next - eol);
>   		current = next;
> 


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately
  2020-11-11 12:28   ` [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
@ 2020-11-13 12:07     ` Phillip Wood
  2020-11-13 13:57       ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2020-11-13 12:07 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The Perl version of this command colors the progress indicator and the
> prompt message in one go, let's do the same in the built-in version.

Why? the C version has access to an api that means we don't have to 
remember to print the reset string each time so why move away from that? 
I don't think it matters to the user that there are some extra escape 
codes in the prompt of the C version. The answer is probably "so we can 
use the same test as the perl version" which might be a good reason - if 
it is I think it would be helpful to say so in the commit message.

Best Wishes

Phillip

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   add-patch.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index bf89c43145..2fad92ca37 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,
>   		else
>   			prompt_mode_type = PROMPT_HUNK;
>   
> -		color_fprintf(stdout, s->s.prompt_color,
> -			      "(%"PRIuMAX"/%"PRIuMAX") ",
> +		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
>   			      (uintmax_t)hunk_index + 1,
>   			      (uintmax_t)(file_diff->hunk_nr
>   						? file_diff->hunk_nr
>   						: 1));
> -		color_fprintf(stdout, s->s.prompt_color,
> -			      _(s->mode->prompt_mode[prompt_mode_type]),
> -			      s->buf.buf);
> +		printf(_(s->mode->prompt_mode[prompt_mode_type]),
> +		       s->buf.buf);
> +		if (*s->s.reset_color)
> +			fputs(s->s.reset_color, stdout);
>   		fflush(stdout);
>   		if (read_single_character(s) == EOF)
>   			break;
> 


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header
  2020-11-11 12:28   ` [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
@ 2020-11-13 12:11     ` Phillip Wood
  2020-11-13 13:58       ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2020-11-13 12:11 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

Hi Dscho

On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The header is formatted by padding each column heading with spaces up to
> the length of 12 characters. These padding spaces are naturally included
> when coloring the entire header.
> 
> However, the preceding five spaces indentation for non-flat lists were
> _not_ included in the Perl version, but _were_ included in the built-in
> version. Let's adjust the former to align with the latter's behavior.

I had trouble understanding this. I think my confusion is that the 
padding was printed when the header was colored, but it was not inside 
the colored part whereas the subject lead be to think there was no 
indentation printed when the header was colored. I assume this change is 
so that we can use the same test for both versions?

Best Wishes

Phillip

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   git-add--interactive.perl | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index e713fe3d02..adbac2bc6d 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -483,10 +483,8 @@ sub list_and_choose {
>   		my $last_lf = 0;
>   
>   		if ($opts->{HEADER}) {
> -			if (!$opts->{LIST_FLAT}) {
> -				print "     ";
> -			}
> -			print colored $header_color, "$opts->{HEADER}\n";
> +			my $indent = $opts->{LIST_FLAT} ? "" : "     ";
> +			print colored $header_color, "$indent$opts->{HEADER}\n";
>   		}
>   		for ($i = 0; $i < @stuff; $i++) {
>   			my $chosen = $chosen[$i] ? '*' : ' ';
> 


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 04/11] add -i: use `reset_color` consistently
  2020-11-13 12:03     ` Phillip Wood
@ 2020-11-13 13:53       ` Johannes Schindelin
  2020-11-13 16:04         ` Phillip Wood
  0 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-13 13:53 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

> Hi Dscho
>
> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > We already maintain a list of colors in the `add_i_state`, therefore we
> > should use them.
>
> Playing devil's advocate for a moment - why do we have a reset entry in that
> list? The next patch makes sure it cannot be customized which is a good thing
> so why not drop the `reset` member from `add_i_state` entirely? The perl
> version needed to get the reset string from `git config` and store it
> somewhere but in the C version we have a perfectly good constant we can use
> instead.

Right.

On the other hand, does it hurt to keep that field for consistency with
the rest of the coloring stuff? It would probably cost more to make the
change you suggested than it would benefit us.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately
  2020-11-13 12:07     ` Phillip Wood
@ 2020-11-13 13:57       ` Johannes Schindelin
  2020-11-13 16:06         ` Phillip Wood
  0 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-13 13:57 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

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>
> >
> > The Perl version of this command colors the progress indicator and the
> > prompt message in one go, let's do the same in the built-in version.
>
> Why? the C version has access to an api that means we don't have to remember
> to print the reset string each time so why move away from that? I don't think
> it matters to the user that there are some extra escape codes in the prompt of
> the C version. The answer is probably "so we can use the same test as the perl
> version" which might be a good reason - if it is I think it would be helpful
> to say so in the commit message.

Honestly, the number one reason is so that the _same_ test passes using
the Perl version as well as with the built-in version, something that is
required by the `linux-clang` job in our CI build.

I am not really willing to change this, unless I hear a goooood reason to
complicate the test.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header
  2020-11-13 12:11     ` Phillip Wood
@ 2020-11-13 13:58       ` Johannes Schindelin
  2020-11-13 16:12         ` Phillip Wood
  0 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-13 13:58 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

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>
> >
> > The header is formatted by padding each column heading with spaces up to
> > the length of 12 characters. These padding spaces are naturally included
> > when coloring the entire header.
> >
> > However, the preceding five spaces indentation for non-flat lists were
> > _not_ included in the Perl version, but _were_ included in the built-in
> > version. Let's adjust the former to align with the latter's behavior.
>
> I had trouble understanding this. I think my confusion is that the padding was
> printed when the header was colored, but it was not inside the colored part
> whereas the subject lead be to think there was no indentation printed when the
> header was colored.

Right, this is ambiguous, but I thought it was clear from the first
paragraph that "included" means "when coloring".

I'm not a native speaker, though, so I welcome suggestions to improve
this.

> I assume this change is so that we can use the same test for both
> versions?

Correct.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain
  2020-11-11 12:28   ` [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
@ 2020-11-13 14:04     ` Philippe Blain
  2020-11-15 23:57       ` Johannes Schindelin
  2020-11-13 14:08     ` Phillip Wood
  1 sibling, 1 reply; 66+ messages in thread
From: Philippe Blain @ 2020-11-13 14:04 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: Git mailing list, Jeff King, Johannes Schindelin

Hi Dscho, 


> Le 11 nov. 2020 à 07:28, Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> a écrit :
> 
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain
  2020-11-11 12:28   ` [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
  2020-11-13 14:04     ` Philippe Blain
@ 2020-11-13 14:08     ` Phillip Wood
  2020-11-16  0:02       ` Johannes Schindelin
  1 sibling, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2020-11-13 14:08 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin

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


^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 04/11] add -i: use `reset_color` consistently
  2020-11-13 13:53       ` Johannes Schindelin
@ 2020-11-13 16:04         ` Phillip Wood
  2020-11-15 23:47           ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2020-11-13 16:04 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

Hi Dscho

On 13/11/2020 13:53, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Fri, 13 Nov 2020, Phillip Wood wrote:
> 
>> Hi Dscho
>>
>> On 11/11/2020 12:28, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> We already maintain a list of colors in the `add_i_state`, therefore we
>>> should use them.
>>
>> Playing devil's advocate for a moment - why do we have a reset entry in that
>> list? The next patch makes sure it cannot be customized which is a good thing
>> so why not drop the `reset` member from `add_i_state` entirely? The perl
>> version needed to get the reset string from `git config` and store it
>> somewhere but in the C version we have a perfectly good constant we can use
>> instead.
> 
> Right.
> 
> On the other hand, does it hurt to keep that field for consistency with
> the rest of the coloring stuff? 

It creates the illusion that `reset` is similar to the others but it 
isn't as we don't want it to be customizable.

> It would probably cost more to make the
> change you suggested than it would benefit us.

At the moment there is one use of `s->s.color_reset` and two of 
`GIT_COLOR_RESET` in add-patch.c

Best Wishes

Phillip

> Ciao,
> Dscho
> 

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately
  2020-11-13 13:57       ` Johannes Schindelin
@ 2020-11-13 16:06         ` Phillip Wood
  2020-11-15 23:55           ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2020-11-13 16:06 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

Hi Dscho

On 13/11/2020 13:57, Johannes Schindelin wrote:
> 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>
>>>
>>> The Perl version of this command colors the progress indicator and the
>>> prompt message in one go, let's do the same in the built-in version.
>>
>> Why? the C version has access to an api that means we don't have to remember
>> to print the reset string each time so why move away from that? I don't think
>> it matters to the user that there are some extra escape codes in the prompt of
>> the C version. The answer is probably "so we can use the same test as the perl
>> version" which might be a good reason - if it is I think it would be helpful
>> to say so in the commit message.
> 
> Honestly, the number one reason is so that the _same_ test passes using
> the Perl version as well as with the built-in version, something that is
> required by the `linux-clang` job in our CI build.

That's what I assumed - it would be good to document that in the commit 
message.

> I am not really willing to change this, unless I hear a goooood reason to
> complicate the test.

I guess we could change the perl version to match the C version as it is 
the perl version that will be retired but I'm not that fussed

Best Wishes

Phillip


> Ciao,
> Dscho
> 

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header
  2020-11-13 13:58       ` Johannes Schindelin
@ 2020-11-13 16:12         ` Phillip Wood
  2020-11-15 23:51           ` Johannes Schindelin
  0 siblings, 1 reply; 66+ messages in thread
From: Phillip Wood @ 2020-11-13 16:12 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

Hi Dscho

On 13/11/2020 13:58, Johannes Schindelin wrote:
> 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>
>>>
>>> The header is formatted by padding each column heading with spaces up to
>>> the length of 12 characters. These padding spaces are naturally included
>>> when coloring the entire header.
>>>
>>> However, the preceding five spaces indentation for non-flat lists were
>>> _not_ included in the Perl version, but _were_ included in the built-in
>>> version. Let's adjust the former to align with the latter's behavior.
>>
>> I had trouble understanding this. I think my confusion is that the padding was
>> printed when the header was colored, but it was not inside the colored part
>> whereas the subject lead be to think there was no indentation printed when the
>> header was colored.
> 
> Right, this is ambiguous, but I thought it was clear from the first
> paragraph that "included" means "when coloring".
> 
> I'm not a native speaker, though, so I welcome suggestions to improve
> this.

Maybe something along the lines of

add -i (Perl version): color header to match the C version

Both versions of `add -i` indent non-flat lists by five spaces. However 
when using color the C version prints these spaces after the ANSI color 
codes whereas the Perl version prints them before the color codes. 
Change the Perl version to match the C version to make testing easier.

Best Wishes

Phillip

>> I assume this change is so that we can use the same test for both
>> versions?
> 
> Correct.
> 
> Thanks,
> Dscho
> 

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
  2020-11-12 18:29           ` Jeff King
@ 2020-11-15 23:35             ` Johannes Schindelin
  2020-11-17  1:44               ` Jeff King
  0 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-15 23:35 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git

Hi Peff,

On Thu, 12 Nov 2020, Jeff King wrote:

> On Thu, Nov 12, 2020 at 03:04:01PM +0100, Johannes Schindelin wrote:
>
> > > Hmm. Right, I knew about that weirdness. But I assumed that the builtin
> > > add-interactive was doing the diffs in-core. Otherwise, why would we
> > > have seen the failure to load diff.color.frag in the first place?
> >
> > Oh, that's easy to explain: as you can verify reading
> > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898
> > the Perl version of `git add -p` insists on (re-)constructing the hunk
> > headers manually, and obviously it needs to color them manually, too. And
> > https://github.com/git/git/blob/e31aba42fb12/add-patch.c#L649-L672 shows
> > that the built-in version of `git add -p` slavishly follows that practice.
>
> But that is only when we split hunks (your link to the perl script is in
> split_hunks()). I agree we must color manually there when creating our
> own hunk header. But outside of that and patch-editing, the perl script
> does not otherwise recolor or rewrite (nor even really parse beyond
> line-splitting) what it gets from the colorized version.
>
> Whereas add-patch parses the colors off of the version and then
> re-colors every hunk header. Which seems doubly weird to me. Even if we
> were going to re-color every hunk (e.g., because we don't want to store
> the original hunk line, but instead a parsed version of it), why bother
> parsing the color version at all, and not just the machine-readable
> version?

Let's continue on this distraction for a bit before I go back to fixing
the patch series, which actually tries to fix a _different_ concern.

The reason why `add-patch.c` "parses the colors off" is that we want to
show the rest of the hunk header, in color, even after splitting hunks
(this will only be shown for the first hunk, of course).

But given that `git add -p` is somewhat of a fringe use, and using
`diffFilter` is _even_ more fringe, I do not really want to spend any
further time on this tangent.

> > > The answer seems to be that render_hunk() always _replaces_ the colors
> > > we got from running the external diff. Whereas the perl version only
> > > applied coloring when reading back in the results of an edit operation
> > > (and likewise used the frag color when generating a split hunk header).
> >
> > No, the Perl version also insists on applying `fraginfo_color`, see
> > https://github.com/git/git/blob/e31aba42fb12/git-add--interactive.perl#L885-L898
>
> Only when we split. Try this to give different colors between the
> interactive script and diff:
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index e713fe3d02..862a21ff1f 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -28,8 +28,9 @@
>  my $diff_use_color = $repo->get_colorbool('color.diff');
>  my ($fraginfo_color) =
>  	$diff_use_color ? (
> -		$repo->get_color('color.diff.frag', 'cyan'),
> +		$repo->get_color('color.diff.nonsense', 'yellow'),
>  	) : ();
> +# noop to create split hunk
>  my ($diff_plain_color) =
>  	$diff_use_color ? (
>  		$repo->get_color('color.diff.plain', ''),
>
> Running "git add -p" does not result in yellow hunk headers. But issuing
> a split command does.
>
> The distinction is mostly academic, because diff-tree and the
> interactive patch code should be using the same colors, so the result
> should look the same. It could matter if the diff-filter chooses
> different colors, though then the split headers will not match the
> originals in style. We _could_ run the newly created hunk header
> individually through the diff-filter, though I'm not sure how various
> filters would handle that.
>
> That's true of the perl version as well as the builtin one, but I think
> the builtin one's insistence on parsing the colored output is taking us
> in the wrong direction to eventually fix that.

My thinking back then was: what if _I_ want to use a diffFilter? For what
would I use it? Probably to emphasize certain hunk headers more, by adding
more color to the part after the line range.

Anyway. I stand by what I said above: I do not want to spend any further
time on this tangent, at least not right now. There are more pressing
challenges waiting for me, and I expect those other challenge to have a
much bigger "return on investment".

Ciao,
Dscho

> > > I'm not sure that what the builtin version is doing is wrong, but it
> > > seems like it's putting a lot of extra effort into parsing colors off of
> > > the colorized version. Whereas the perl version just assumes the lines
> > > match up. I do wonder if there are corner cases we might hit around
> > > filters here, though. The lines we get from a filter might bear no
> > > resemblance at all to diff lines. The only thing we require in the perl
> > > version is that they have correspond 1-to-1 with the unfiltered diff
> > > lines in meaning.
> >
> > They do have to correspond 1-to-1 because the assumption is that the
> > individual lines will then correspond one-to-one, too. This does not need
> > to be true, of course, but then the filter is probably less useful than
> > the user wants it to be.
>
> Right, I'm not disputing the 1-to-1 thing (I was after all the one who
> implemented interactive.diffilter, and added the "complain if the counts
> don't line up" check). But in the perl script they only need to
> correspond _semantically_, not syntactically.
>
> -Peff
>

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 00/11] Fix color handling in git add -i
  2020-11-12 18:40       ` Jeff King
@ 2020-11-15 23:40         ` Johannes Schindelin
  2020-11-17  1:49           ` Jeff King
  0 siblings, 1 reply; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-15 23:40 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain

[-- Attachment #1: Type: text/plain, Size: 4950 bytes --]

Hi Peff,

On Thu, 12 Nov 2020, Jeff King wrote:

> On Thu, Nov 12, 2020 at 03:20:38PM +0100, Johannes Schindelin wrote:
>
> > >   $ git -c interactive.difffilter='diff-so-fancy' add -p
> > >   ──────────────────────────────────────────────────────────────────────
> > >   modified: file
> > >   ──────────────────────────────────────────────────────────────────────
> > >   @ file:1 @
> > >   old
> > >   new
> > >   (1/1) Stage this hunk [y,n,q,a,d,e,?]?
> >
> > It might _seem_ that it works. But try to split a hunk. I did this with
> > the test case (interrupting it just before running `add -p`):
>
> Yes, there are definitely problems with how diff-so-fancy represents
> things (because they don't maintain a 1-to-1 correspondence). You should
> generally get a complaint like:
>
>   $ git -c interactive.difffilter='diff-so-fancy' add -p
>   fatal: mismatched output from interactive.diffFilter
>   hint: Your filter must maintain a one-to-one correspondence
>   hint: between its input and output lines.
>
> The diff-so-fancy folks have asked me about this, and I've made clear
> the problem to them, and that the solution is to have a mode which
> maintains the line correspondence. AFAIK there's no fix yet. I don't
> know how many people are actually using it in practice in its current
> buggy state.
>
> There's a big thread here:
>
>   https://github.com/so-fancy/diff-so-fancy/issues/35
>
> I don't really recommend reading the whole thing. Beyond what I just
> said, the interesting bits are:
>
>   - people recommend using the "delta" tool; it has a "--color-only"
>     option which does just diff-highlight-style coloring, but doesn't
>     break line correspondence
>
>   - somebody suggested instead of using interactive.difffilter, to add
>     an option to show each hunk in an individual pager command. So
>     add-interactive would never see the "fancy" version at all, but it
>     would be generated on the fly when the user sees it (and that filter
>     would have to be able to handle seeing individual hunks without the
>     diff header).
>
> All of which is to say that the current state is a bit of a mess, and I
> don't consider it completely necessary to fix it before the builtin
> version becomes the default. But:
>
>   - I think we should expect some possible complaints / bug reports
>     from fringe users of the already-somewhat-broken state
>
>   - IMHO the parsing of the filtered version done by the builtin is
>     going in the wrong direction (see my other mail for an elaboration)

It's a little bit of a surprise that this is the first time I hear about
this.

The way _I_ would go about fixing this is to look for a tell-tale that
we're looking at a `diff-so-fancy` style output, e.g. by looking for that
horizontal line, then adding special code to handle that.

This is not a minor undertaking, and it would currently require _two_
implementations: the Perl version and the built-in version. They would
look _very_ different from one another. Therefore, I would probably either
wait until the Perl version is retired, or selectively only adjust the
built-in version, if _I_ was to implement this.

But given that it does not work right now, and given that it has not been
working for a long time, I think it does not hurt so much if it is left in
the current state for a bit longer. I would love to focus on the patch
series that kicked off this discussion first, getting it done, before I
think about other `add -p` aspects.

Ciao,
Dscho

>
> > > While it may touch on some of the coloring code, it's otherwise
> > > orthogonal to the fixes here. And while the fix is likely to be to make
> > > render_hunk() stop re-coloring in the non-edit cases, your more-robust
> > > test here will still be checking what you expect (because it really is
> > > triggering the edit case, not relying on the extra coloring to see the
> > > effect of in-process color config).
> >
> > I don't actually think that we _can_ stop re-coloring the hunk header in
> > the general case because we need to keep this working even for split
> > hunks. It would be a very bad idea to make it work for non-split cases and
> > then something like `die()` only when the hunk is split. Better re-color
> > all of them, then, to not even risk that situation.
>
> Yeah, obviously calling die() in the split case is bad. And the fact
> that newly-created split hunk headers are not filtered the same way as
> the original hunk headers isn't ideal. But it's a pretty easy fix in the
> perl version, and not in the builtin version.
>
> -Peff
>

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 04/11] add -i: use `reset_color` consistently
  2020-11-13 16:04         ` Phillip Wood
@ 2020-11-15 23:47           ` Johannes Schindelin
  0 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-15 23:47 UTC (permalink / raw)
  To: phillip.wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

> On 13/11/2020 13:53, Johannes Schindelin wrote:
> >
> > 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>
> > > >
> > > > We already maintain a list of colors in the `add_i_state`, therefore we
> > > > should use them.
> > >
> > > Playing devil's advocate for a moment - why do we have a reset entry
> > > in that list? The next patch makes sure it cannot be customized
> > > which is a good thing so why not drop the `reset` member from
> > > `add_i_state` entirely? The perl version needed to get the reset
> > > string from `git config` and store it somewhere but in the C version
> > > we have a perfectly good constant we can use instead.
> >
> > Right.
> >
> > On the other hand, does it hurt to keep that field for consistency with
> > the rest of the coloring stuff?
>
> It creates the illusion that `reset` is similar to the others but it isn't as
> we don't want it to be customizable.

In a certain sense, however, it *is* customizable. Namely when the color
is turned _off_. Then `reset_color` is set to the empty string, and
`GIT_RESET_COLOR` still isn't.

> > It would probably cost more to make the change you suggested than it
> > would benefit us.
>
> At the moment there is one use of `s->s.color_reset` and two of
> `GIT_COLOR_RESET` in add-patch.c

I did spend the time to look at those call sites, and they seem to be
guarded by "if colored" conditions.

But as I said, I don't think that it will buy us a ton to treat resetting
color different from setting color.

I mean, who is to say that we won't add some sort of low-level mode of
`git add -p` at some stage that can be used by 3rd-party applications? We
would then have to switch back to the current code. And even if not, I
think that there is a benefit to the current code, where no caller has to
wonder whether the reset sequence has been overridden (e.g. by the empty
string) or not, it just says semantically that this is where we switch
back to regular color. And that's that.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header
  2020-11-13 16:12         ` Phillip Wood
@ 2020-11-15 23:51           ` Johannes Schindelin
  0 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-15 23:51 UTC (permalink / raw)
  To: phillip.wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

> On 13/11/2020 13:58, Johannes Schindelin wrote:
>
> > 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>
> > > >
> > > > The header is formatted by padding each column heading with spaces
> > > > up to the length of 12 characters. These padding spaces are
> > > > naturally included when coloring the entire header.
> > > >
> > > > However, the preceding five spaces indentation for non-flat lists
> > > > were _not_ included in the Perl version, but _were_ included in
> > > > the built-in version. Let's adjust the former to align with the
> > > > latter's behavior.
> > >
> > > I had trouble understanding this. I think my confusion is that the
> > > padding was printed when the header was colored, but it was not
> > > inside the colored part whereas the subject lead be to think there
> > > was no indentation printed when the header was colored.
> >
> > Right, this is ambiguous, but I thought it was clear from the first
> > paragraph that "included" means "when coloring".
> >
> > I'm not a native speaker, though, so I welcome suggestions to improve
> > this.
>
> Maybe something along the lines of
>
> add -i (Perl version): color header to match the C version
>
> Both versions of `add -i` indent non-flat lists by five spaces. However when
> using color the C version prints these spaces after the ANSI color codes
> whereas the Perl version prints them before the color codes. Change the Perl
> version to match the C version to make testing easier.

I wrapped it, and modified it slightly:

    add -i (Perl version): color header to match the C version

    Both versions of `add -i` indent non-flat lists by five spaces. However
    when using color the C version prints these spaces after the ANSI color
    codes whereas the Perl version prints them before the color codes.
    Change the Perl version to match the C version to allow for introducing
    a test that verifies that both versions produce the exact same output.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately
  2020-11-13 16:06         ` Phillip Wood
@ 2020-11-15 23:55           ` Johannes Schindelin
  0 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-15 23:55 UTC (permalink / raw)
  To: phillip.wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

Hi Phillip,

On Fri, 13 Nov 2020, Phillip Wood wrote:

> On 13/11/2020 13:57, Johannes Schindelin wrote:
> >
> > 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>
> > > >
> > > > The Perl version of this command colors the progress indicator and the
> > > > prompt message in one go, let's do the same in the built-in version.
> > >
> > > Why? the C version has access to an api that means we don't have to
> > > remember to print the reset string each time so why move away from
> > > that? I don't think it matters to the user that there are some extra
> > > escape codes in the prompt of the C version. The answer is probably
> > > "so we can use the same test as the perl version" which might be a
> > > good reason - if it is I think it would be helpful to say so in the
> > > commit message.
> >
> > Honestly, the number one reason is so that the _same_ test passes using
> > the Perl version as well as with the built-in version, something that is
> > required by the `linux-clang` job in our CI build.
>
> That's what I assumed - it would be good to document that in the commit
> message.

I did that:

    add -p (built-in): do not color the progress indicator separately

    The Perl version of this command colors the progress indicator and the
    prompt message in one go.

    Let's do the same in the built-in version so that the same upcoming test
    (which will compare the output of `git add -p` against a known-good
    version) will pass both for the Perl version as well as for the built-in
    version.

> > I am not really willing to change this, unless I hear a goooood reason to
> > complicate the test.
>
> I guess we could change the perl version to match the C version as it is the
> perl version that will be retired but I'm not that fussed

Well, since we want to supersede the Perl version by the built-in version,
the latter needs to imitate the former, no?

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain
  2020-11-13 14:04     ` Philippe Blain
@ 2020-11-15 23:57       ` Johannes Schindelin
  0 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-15 23:57 UTC (permalink / raw)
  To: Philippe Blain
  Cc: Johannes Schindelin via GitGitGadget, Git mailing list, Jeff King

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

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 <gitgitgadget@gmail.com> a écrit :
> >
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain
  2020-11-13 14:08     ` Phillip Wood
@ 2020-11-16  0:02       ` Johannes Schindelin
  0 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin @ 2020-11-16  0:02 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain, Jeff King

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

^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 00/11] Fix color handling in git add -i
  2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
                     ` (11 preceding siblings ...)
  2020-11-11 18:45   ` [PATCH v2 00/11] Fix color handling in git add -i Jeff King
@ 2020-11-16 16:08   ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
                       ` (11 more replies)
  12 siblings, 12 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin

This patch series started out as a tiny fix for a bug reported by Philippe
Blain in 
https://lore.kernel.org/git/313B8999-1E99-4695-A20D-E48840C30879@gmail.com/.
And then I only wanted to add a regression test to make sure that this does
not regress. And then I just wanted to ensure that it passes both with the
Perl version of git add -i as well as with the built-in version.

And in no time I was looking at a real patch series.

Changes since v2:

 * The commit messages of patches 7/11 and 9/11 now stress why we want to
   align the output of the Perl vs the built-in version so slavishly: to be
   able to validate both versions against prerecorded output.
 * A typo was fixed in the commit message of patch 10/11.

Changes since v1:

 * The regression test now actually exercises the re-coloring (that is the
   primary purpose of git add -p looking at the color.diff.* variables).
 * The way the built-in git add -p renders hunk headers of split hunks was
   aligned with how the Perl version does things.
 * We now consistently prefer color.diff.context over color.diff.plain, no
   matter whether using the Perl or the built-in version of git add -p.
 * The commit message for the regression test no longer confuses readers by
   mentioning dash.
 * The regression test was structured a bit better, first testing error
   message coloring, then the menu in git add -i and then the diff coloring
   in git add -p.

Johannes Schindelin (11):
  add -i (built-in): do show an error message for incorrect inputs
  add -i (built-in): send error messages to stderr
  add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk
    headers
  add -i: use `reset_color` consistently
  add -i (built-in): prevent the `reset` "color" from being configured
  add -i (built-in): use correct names to load color.diff.* config
  add -p (built-in): do not color the progress indicator separately
  add -i (built-in): use the same indentation as the Perl version
  add -i (Perl version): color header to match the C version
  add -p: prefer color.diff.context over color.diff.plain
  add -i: verify in the tests that colors can be overridden

 add-interactive.c          | 38 ++++++++++-------
 add-patch.c                | 25 +++++++-----
 git-add--interactive.perl  | 12 +++---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 127 insertions(+), 32 deletions(-)


base-commit: e4d83eee9239207622e2b1cc43967da5051c189c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-785%2Fdscho%2Ffix-add-i-colors-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-785/dscho/fix-add-i-colors-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/785

Range-diff vs v2:

  1:  6152122c04 =  1:  6152122c04 add -i (built-in): do show an error message for incorrect inputs
  2:  068813912b =  2:  068813912b add -i (built-in): send error messages to stderr
  3:  98deb538d9 =  3:  98deb538d9 add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers
  4:  c857c44932 =  4:  c857c44932 add -i: use `reset_color` consistently
  5:  337b45cad8 =  5:  337b45cad8 add -i (built-in): prevent the `reset` "color" from being configured
  6:  dcd2ffc458 =  6:  dcd2ffc458 add -i (built-in): use correct names to load color.diff.* config
  7:  73b6d60a80 !  7:  6a68bc5511 add -p (built-in): do not color the progress indicator separately
     @@ Commit message
          add -p (built-in): do not color the progress indicator separately
      
          The Perl version of this command colors the progress indicator and the
     -    prompt message in one go, let's do the same in the built-in version.
     +    prompt message in one go.
     +
     +    Let's do the same in the built-in version so that the same upcoming test
     +    (which will compare the output of `git add -p` against a known-good
     +    version) will pass both for the Perl version as well as for the built-in
     +    version.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
  8:  91ded2fbbe =  8:  168891f9f8 add -i (built-in): use the same indentation as the Perl version
  9:  304614751e !  9:  094a4ad90c add -i (Perl version): include indentation in the colored header
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    add -i (Perl version): include indentation in the colored header
     +    add -i (Perl version): color header to match the C version
      
     -    The header is formatted by padding each column heading with spaces up to
     -    the length of 12 characters. These padding spaces are naturally included
     -    when coloring the entire header.
     -
     -    However, the preceding five spaces indentation for non-flat lists were
     -    _not_ included in the Perl version, but _were_ included in the built-in
     -    version. Let's adjust the former to align with the latter's behavior.
     +    Both versions of `add -i` indent non-flat lists by five spaces. However
     +    when using color the C version prints these spaces after the ANSI color
     +    codes whereas the Perl version prints them before the color codes.
     +    Change the Perl version to match the C version to allow for introducing
     +    a test that verifies that both versions produce the exact same output.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
 10:  48d8e0badf ! 10:  9a4d2a33b5 add -p: prefer color.diff.context over color.diff.plain
     @@ Commit message
          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
     +    This still 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.
 11:  c94abff72f = 11:  492f46833a add -i: verify in the tests that colors can be overridden

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 66+ messages in thread

* [PATCH v3 01/11] add -i (built-in): do show an error message for incorrect inputs
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 02/11] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
                       ` (10 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

There is a neat feature in `git add -i` where it allows users to select
items via unique prefixes.

In the built-in version of `git add -i`, we specifically sort the items
(unless they are already sorted) and then perform a binary search to
figure out whether the input constitutes a unique prefix. Unfortunately,
by mistake this code misidentifies matches even if the input string is
not actually a prefix of any item.

For example, in the initial menu, where there is a `status` and an
`update` command, the input `tadaa` was mistaken as a prefix of
`update`.

Let's fix this by looking a bit closer whether the input is actually a
prefix of the item at the found insert index.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 555c4abf32..8ca503d803 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list)
 	else if (index + 1 < list->sorted.nr &&
 		 starts_with(list->sorted.items[index + 1].string, string))
 		return -1;
-	else if (index < list->sorted.nr)
+	else if (index < list->sorted.nr &&
+		 starts_with(list->sorted.items[index].string, string))
 		item = list->sorted.items[index].util;
 	else
 		return -1;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 02/11] add -i (built-in): send error messages to stderr
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers Johannes Schindelin via GitGitGadget
                       ` (9 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of that command already does that since a301973641f
(add -p: print errors in separate color, 2009-02-05). The built-in
version's development started by reimplementing the initial version from
5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though,
which still printed error messages to stdout.

Let's fix that by imitating the Perl version's behavior in the built-in
version of that command.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 8ca503d803..0f24992ca4 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s,
 
 			if (from < 0 || from >= items->items.nr ||
 			    (singleton && from + 1 != to)) {
-				color_fprintf_ln(stdout, s->error_color,
+				color_fprintf_ln(stderr, s->error_color,
 						 _("Huh (%s)?"), p);
 				break;
 			} else if (singleton) {
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 02/11] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
                       ` (8 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In libxdiff, imitating GNU diff, the hunk headers only show the line
count if it is different from 1. When splitting hunks, the Perl version
of `git add -p` already imitates this. Let's do the same in the built-in
version of said command.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index be4cf6e9e5..b6d53229bb 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -661,9 +661,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		else
 			new_offset += delta;
 
-		strbuf_addf(out, "@@ -%lu,%lu +%lu,%lu @@",
-			    old_offset, header->old_count,
-			    new_offset, header->new_count);
+		strbuf_addf(out, "@@ -%lu", old_offset);
+		if (header->old_count != 1)
+			strbuf_addf(out, ",%lu", header->old_count);
+		strbuf_addf(out, " +%lu", new_offset);
+		if (header->new_count != 1)
+			strbuf_addf(out, ",%lu", header->new_count);
+		strbuf_addstr(out, " @@");
+
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 04/11] add -i: use `reset_color` consistently
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (2 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 05/11] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
                       ` (7 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We already maintain a list of colors in the `add_i_state`, therefore we
should use them.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index b6d53229bb..bf89c43145 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -672,7 +672,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		if (len)
 			strbuf_add(out, p, len);
 		else if (colored)
-			strbuf_addf(out, "%s\n", GIT_COLOR_RESET);
+			strbuf_addf(out, "%s\n", s->s.reset_color);
 		else
 			strbuf_addch(out, '\n');
 	}
@@ -1065,7 +1065,7 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
 			      s->s.file_new_color :
 			      s->s.context_color);
 		strbuf_add(&s->colored, plain + current, eol - current);
-		strbuf_addstr(&s->colored, GIT_COLOR_RESET);
+		strbuf_addstr(&s->colored, s->s.reset_color);
 		if (next > eol)
 			strbuf_add(&s->colored, plain + eol, next - eol);
 		current = next;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 05/11] add -i (built-in): prevent the `reset` "color" from being configured
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (3 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 06/11] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
                       ` (6 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of that command sneakily uses `git config --get-color`
to figure out the ANSI sequence to reset the color, but passes the empty
string and therefore cannot actually match any config entry.

This was missed when re-implementing the command as a built-in command.
Let's fix this, preventing the `reset` sequence from being overridden
via the config.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 0f24992ca4..f3a1d7456e 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
 	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
 	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
 	init_color(r, s, "fraginfo", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
 	init_color(r, s, "context", s->context_color,
@@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 	init_color(r, s, "new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
+	strlcpy(s->reset_color,
+		s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN);
+
 	FREE_AND_NULL(s->interactive_diff_filter);
 	git_config_get_string("interactive.difffilter",
 			      &s->interactive_diff_filter);
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 06/11] add -i (built-in): use correct names to load color.diff.* config
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (4 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 05/11] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
                       ` (5 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The builtin version of add-interactive mistakenly loads diff colors from
color.interactive.* instead of color.diff.*. It also accidentally spells
`frag` as `fraginfo`.

Let's fix that.

Note also that we don't respect the historical `diff.color.*`. The perl
version never did, and those have been deprecated since 2007.

Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index f3a1d7456e..9126684348 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -12,10 +12,10 @@
 #include "prompt.h"
 
 static void init_color(struct repository *r, struct add_i_state *s,
-		       const char *slot_name, char *dst,
+		       const char *section_and_slot, char *dst,
 		       const char *default_color)
 {
-	char *key = xstrfmt("color.interactive.%s", slot_name);
+	char *key = xstrfmt("color.%s", section_and_slot);
 	const char *value;
 
 	if (!s->use_color)
@@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 			git_config_colorbool("color.interactive", value);
 	s->use_color = want_color(s->use_color);
 
-	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
-	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
-	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "fraginfo", s->fraginfo_color,
+	init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD);
+	init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED);
+	init_color(r, s, "interactive.prompt", s->prompt_color,
+		   GIT_COLOR_BOLD_BLUE);
+	init_color(r, s, "interactive.error", s->error_color,
+		   GIT_COLOR_BOLD_RED);
+
+	init_color(r, s, "diff.frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "context", s->context_color,
+	init_color(r, s, "diff.context", s->context_color,
 		diff_get_color(s->use_color, DIFF_CONTEXT));
-	init_color(r, s, "old", s->file_old_color,
+	init_color(r, s, "diff.old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
-	init_color(r, s, "new", s->file_new_color,
+	init_color(r, s, "diff.new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
 	strlcpy(s->reset_color,
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 07/11] add -p (built-in): do not color the progress indicator separately
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (5 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 06/11] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 08/11] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
                       ` (4 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The Perl version of this command colors the progress indicator and the
prompt message in one go.

Let's do the same in the built-in version so that the same upcoming test
(which will compare the output of `git add -p` against a known-good
version) will pass both for the Perl version as well as for the built-in
version.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-patch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index bf89c43145..2fad92ca37 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1461,15 +1461,15 @@ static int patch_update_file(struct add_p_state *s,
 		else
 			prompt_mode_type = PROMPT_HUNK;
 
-		color_fprintf(stdout, s->s.prompt_color,
-			      "(%"PRIuMAX"/%"PRIuMAX") ",
+		printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.prompt_color,
 			      (uintmax_t)hunk_index + 1,
 			      (uintmax_t)(file_diff->hunk_nr
 						? file_diff->hunk_nr
 						: 1));
-		color_fprintf(stdout, s->s.prompt_color,
-			      _(s->mode->prompt_mode[prompt_mode_type]),
-			      s->buf.buf);
+		printf(_(s->mode->prompt_mode[prompt_mode_type]),
+		       s->buf.buf);
+		if (*s->s.reset_color)
+			fputs(s->s.reset_color, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF)
 			break;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 08/11] add -i (built-in): use the same indentation as the Perl version
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (6 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 09/11] add -i (Perl version): color header to match the C version Johannes Schindelin via GitGitGadget
                       ` (3 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When copying the spaces used to indent non-flat lists in `git add -i`,
one space was appended by mistake. This makes the output of the built-in
version of `git add -i` inconsistent with the Perl version. Let's adjust
the built-in version to produce the same output as the Perl version.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 add-interactive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 9126684348..c298a8b80f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps)
 	print_file_item_data.color = data.color;
 	print_file_item_data.reset = data.reset;
 
-	strbuf_addstr(&header, "      ");
+	strbuf_addstr(&header, "     ");
 	strbuf_addf(&header, print_file_item_data.modified_fmt,
 		    _("staged"), _("unstaged"), _("path"));
 	opts.list_opts.header = header.buf;
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 09/11] add -i (Perl version): color header to match the C version
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (7 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 08/11] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
                       ` (2 subsequent siblings)
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Both versions of `add -i` indent non-flat lists by five spaces. However
when using color the C version prints these spaces after the ANSI color
codes whereas the Perl version prints them before the color codes.
Change the Perl version to match the C version to allow for introducing
a test that verifies that both versions produce the exact same output.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git-add--interactive.perl | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e713fe3d02..adbac2bc6d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -483,10 +483,8 @@ sub list_and_choose {
 		my $last_lf = 0;
 
 		if ($opts->{HEADER}) {
-			if (!$opts->{LIST_FLAT}) {
-				print "     ";
-			}
-			print colored $header_color, "$opts->{HEADER}\n";
+			my $indent = $opts->{LIST_FLAT} ? "" : "     ";
+			print colored $header_color, "$indent$opts->{HEADER}\n";
 		}
 		for ($i = 0; $i < @stuff; $i++) {
 			my $chosen = $chosen[$i] ? '*' : ' ';
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 10/11] add -p: prefer color.diff.context over color.diff.plain
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (8 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 09/11] add -i (Perl version): color header to match the C version Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-16 16:08     ` [PATCH v3 11/11] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
  2020-11-17  1:51     ` [PATCH v3 00/11] Fix color handling in git add -i Jeff King
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

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

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

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


^ permalink raw reply related	[flat|nested] 66+ messages in thread

* [PATCH v3 11/11] add -i: verify in the tests that colors can be overridden
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (9 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
@ 2020-11-16 16:08     ` Johannes Schindelin via GitGitGadget
  2020-11-17  1:51     ` [PATCH v3 00/11] Fix color handling in git add -i Jeff King
  11 siblings, 0 replies; 66+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-11-16 16:08 UTC (permalink / raw)
  To: git
  Cc: Philippe Blain, Jeff King, Johannes Schindelin, Phillip Wood,
	Johannes Schindelin, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Now that the Perl version produces the same output as the built-in
version (mostly fixing bugs in the latter), let's add a regression test
to verify that it stays this way.

Note that we only `grep` for the colored error message instead of
verifying that the entire `stderr` consists of just this one line: when
running the test script using the `-x` option to trace the
commands, the sub-shell in `force_color` causes those commands to be
traced into `err.raw` (unless running in Bash where we set the
`BASH_XTRACEFD` variable to avoid that).

Also note that the color reset in the `<BLUE>+<RESET><BLUE>new<RESET>`
line might look funny and unnecessary, as the corresponding `old` line
does not reset the color after the diff marker only to turn the color
back on right away.

However, this is a (necessary) side effect of the white-space check: in
`emit_line_ws_markup()`, we first emit the diff marker via
`emit_line_0()` and then the rest of the line via `ws_check_emit()`. To
leave them somewhat decoupled, the color has to be reset after the diff
marker to allow for the rest of the line to start with another color (or
inverted, in case of white-space issues).

Finally, we have to simulate hunk editing: the `git add -p` command
cannot rely on the internal diff machinery for coloring after letting
the user edit a hunk; It has to "re-color" the edited hunk. This is the
primary reason why that command is interested in the exact values of the
`color.diff.*` settings in the first place. To test this re-coloring, we
therefore have to pretend to edit a hunk and then show that hunk in the
regression test.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3701-add-interactive.sh | 84 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index ca04fac417..cc3f434a97 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -589,6 +589,90 @@ test_expect_success 'diffs can be colorized' '
 	grep "$(printf "\\033")" output
 '
 
+test_expect_success 'colors can be overridden' '
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context another-one >color-test &&
+
+	echo trigger an error message >input &&
+	force_color git \
+		-c color.interactive.error=blue \
+		add -i 2>err.raw <input &&
+	test_decode_color <err.raw >err &&
+	grep "<BLUE>Huh (trigger)?<RESET>" err &&
+
+	test_write_lines help quit >input &&
+	force_color git \
+		-c color.interactive.header=red \
+		-c color.interactive.help=green \
+		-c color.interactive.prompt=yellow \
+		add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual &&
+	cat >expect <<-\EOF &&
+	<RED>           staged     unstaged path<RESET>
+	  1:        +3/-0        +2/-1 color-test
+
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> <GREEN>status        - show paths with changes<RESET>
+	<GREEN>update        - add working tree state to the staged set of changes<RESET>
+	<GREEN>revert        - revert staged set of changes back to the HEAD version<RESET>
+	<GREEN>patch         - pick hunks and update selectively<RESET>
+	<GREEN>diff          - view diff between HEAD and index<RESET>
+	<GREEN>add untracked - add contents of untracked files to the staged set of changes<RESET>
+	<RED>*** Commands ***<RESET>
+	  1: <YELLOW>s<RESET>tatus	  2: <YELLOW>u<RESET>pdate	  3: <YELLOW>r<RESET>evert	  4: <YELLOW>a<RESET>dd untracked
+	  5: <YELLOW>p<RESET>atch	  6: <YELLOW>d<RESET>iff	  7: <YELLOW>q<RESET>uit	  8: <YELLOW>h<RESET>elp
+	<YELLOW>What now<RESET>> Bye.
+	EOF
+	test_cmp expect actual &&
+
+	: exercise recolor_hunk by editing and then look at the hunk again &&
+	test_write_lines s e K q >input &&
+	force_color git \
+		-c color.interactive.prompt=yellow \
+		-c color.diff.meta=italic \
+		-c color.diff.frag=magenta \
+		-c color.diff.context=cyan \
+		-c color.diff.old=bold \
+		-c color.diff.new=blue \
+		-c core.editor=touch \
+		add -p >actual.raw <input &&
+	test_decode_color <actual.raw >actual.decoded &&
+	sed "s/index [0-9a-f]*\\.\\.[0-9a-f]* 100644/<INDEX-LINE>/" <actual.decoded >actual &&
+	cat >expect <<-\EOF &&
+	<ITALIC>diff --git a/color-test b/color-test<RESET>
+	<ITALIC><INDEX-LINE><RESET>
+	<ITALIC>--- a/color-test<RESET>
+	<ITALIC>+++ b/color-test<RESET>
+	<MAGENTA>@@ -1,3 +1,4 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+<RESET><BLUE>new<RESET>
+	<CYAN> more-context<RESET>
+	<BLUE>+<RESET><BLUE>another-one<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
+	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+<RESET><BLUE>new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<CYAN> more-context<RESET>
+	<BLUE>+<RESET><BLUE>another-one<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<CYAN> context<RESET>
+	<BOLD>-old<RESET>
+	<BLUE>+new<RESET>
+	<CYAN> more-context<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? <RESET>
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 66+ messages in thread

* Re: [PATCH 9/9] add -i: verify in the tests that colors can be overridden
  2020-11-15 23:35             ` Johannes Schindelin
@ 2020-11-17  1:44               ` Jeff King
  0 siblings, 0 replies; 66+ messages in thread
From: Jeff King @ 2020-11-17  1:44 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git

On Mon, Nov 16, 2020 at 12:35:44AM +0100, Johannes Schindelin wrote:

> > Whereas add-patch parses the colors off of the version and then
> > re-colors every hunk header. Which seems doubly weird to me. Even if we
> > were going to re-color every hunk (e.g., because we don't want to store
> > the original hunk line, but instead a parsed version of it), why bother
> > parsing the color version at all, and not just the machine-readable
> > version?
> 
> Let's continue on this distraction for a bit before I go back to fixing
> the patch series, which actually tries to fix a _different_ concern.
> 
> The reason why `add-patch.c` "parses the colors off" is that we want to
> show the rest of the hunk header, in color, even after splitting hunks
> (this will only be shown for the first hunk, of course).

OK, this is the part I didn't quite understand. My contention was: if we
are regenerating a new hunk header after we split, why do we care what
was in the old one?

This "rest of the hunk header" is what I didn't get. You are talking
here about the funcname header. Which is not colored by default, but
people can set color.diff.func.

And here the builtin does differ from the perl script quite a bit. The
perl script generates the split hunk headers from scratch, and does not
bother to include the funcname in the split header:

  $ git show :foo.c
  void foo()
  {
  	call();
  	some();
  	functions();
  }
  
  $ cat foo.c
  void foo()
  {
  	call();
  	some();
  	more();
  	functions();
  	return;
  }

  $ git add -p
  diff --git a/foo.c b/foo.c
  index 270ccc7..0365419 100644
  --- a/foo.c
  +++ b/foo.c
  @@ -2,5 +2,7 @@ void foo()
   {
   	call();
   	some();
  +	more();
   	functions();
  +	return;
   }
  (1/1) Stage this hunk [y,n,q,a,d,s,e,?]? s
  Split into 2 hunks.
  @@ -2,4 +2,5 @@
   {
   	call();
   	some();
  +	more();
   	functions();
  (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? n
  @@ -5,2 +6,3 @@
   	functions();
  +	return;
   }
  (2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? n

So it does not appear at all after the split, colored or otherwise. I
think the behavior of the builtin version is better in this case.

It does place more restrictions on what the diffFilter can do, as we've
been discussing. The same thing could be accomplished by retaining the
uncolored func header, and then coloring it on the fly within
add-patch.c (exactly the same way we color the numeric part of the hunk
header).

It may also be worth covering this with a test (it was a surprise to me
that the builtin version was handling this, though as I said I do agree
it's an improvement).

> But given that `git add -p` is somewhat of a fringe use, and using
> `diffFilter` is _even_ more fringe, I do not really want to spend any
> further time on this tangent.

I agree that diffFilter is quite fringe. And as I said before, I'm fine
punting on it for now. Mostly this was just the first I had found out
about the difference, and because it is user-visible, it may be
something that comes up later. So I wanted to record a description of
the problem for later.

> My thinking back then was: what if _I_ want to use a diffFilter? For what
> would I use it? Probably to emphasize certain hunk headers more, by adding
> more color to the part after the line range.

Yes, that's what I use it for. I wrote it mostly to pipe through
diff-highlight, though also it was to help diff-so-fancy folks (which I
don't use myself, but they build on diff-highlight). However, it does
seem that they haven't really worked out the problems in the meantime.

-Peff

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v2 00/11] Fix color handling in git add -i
  2020-11-15 23:40         ` Johannes Schindelin
@ 2020-11-17  1:49           ` Jeff King
  0 siblings, 0 replies; 66+ messages in thread
From: Jeff King @ 2020-11-17  1:49 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain

On Mon, Nov 16, 2020 at 12:40:32AM +0100, Johannes Schindelin wrote:

> > All of which is to say that the current state is a bit of a mess, and I
> > don't consider it completely necessary to fix it before the builtin
> > version becomes the default. But:
> >
> >   - I think we should expect some possible complaints / bug reports
> >     from fringe users of the already-somewhat-broken state
> >
> >   - IMHO the parsing of the filtered version done by the builtin is
> >     going in the wrong direction (see my other mail for an elaboration)
> 
> It's a little bit of a surprise that this is the first time I hear about
> this.

Well, it is the first time I am finding out about it, too. ;)

> The way _I_ would go about fixing this is to look for a tell-tale that
> we're looking at a `diff-so-fancy` style output, e.g. by looking for that
> horizontal line, then adding special code to handle that.

IMHO that's the wrong direction. My intent with the feature was to give
filters as much flexibility as possible, and beyond having 1-to-1
correspondence of lines, we don't know or care what they produce. We
don't even know what other filters might exist out there (though I would
be surprised if there is anything more exotic than intra-line coloring;
other folks have pointed to "delta" for this use, but only in its
color-only mode).

If anybody wants to put work into it, I think a better direction is
having Git keep its own colored hunks and just present single-hunks to a
custom display command to show to the user. Then we can continue not to
care about the format of the individual filters, _and_ they can get the
opportunity to format our rewritten split-hunk headers, etc.

> But given that it does not work right now, and given that it has not been
> working for a long time, I think it does not hurt so much if it is left in
> the current state for a bit longer. I would love to focus on the patch
> series that kicked off this discussion first, getting it done, before I
> think about other `add -p` aspects.

Yeah, absolutely.

-Peff

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v3 00/11] Fix color handling in git add -i
  2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
                       ` (10 preceding siblings ...)
  2020-11-16 16:08     ` [PATCH v3 11/11] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
@ 2020-11-17  1:51     ` Jeff King
  2020-11-17  2:05       ` Jeff King
  2020-11-25 21:59       ` Junio C Hamano
  11 siblings, 2 replies; 66+ messages in thread
From: Jeff King @ 2020-11-17  1:51 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Johannes Schindelin, Phillip Wood

On Mon, Nov 16, 2020 at 04:08:21PM +0000, Johannes Schindelin via GitGitGadget wrote:

> Changes since v2:
> 
>  * The commit messages of patches 7/11 and 9/11 now stress why we want to
>    align the output of the Perl vs the built-in version so slavishly: to be
>    able to validate both versions against prerecorded output.
>  * A typo was fixed in the commit message of patch 10/11.

This version looks fine to me, and I agree is a good stopping point for
now (the only thing I'd consider adding is a test for the funcname in a
split-hunk header, but it would have to be expect_failure for now; it is
probably not worth the effort to rewrite the perl version to behave like
the builtin here if we think it's going away soon).

-Peff

^ permalink raw reply	[flat|nested] 66+ messages in thread

* Re: [PATCH v3 00/11] Fix color handling in git add -i
  2020-11-17  1:51     ` [PATCH v3 00/11] Fix color handling in git add -i Jeff King
@ 2020-11-17  2:05       ` Jeff King
  2020-11-25 21:59       ` Junio C Hamano
  1 sibling, 0 replies; 66+ messages in thread
From: Jeff King @ 2020-11-17  2:05 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Philippe Blain, Johannes Schindelin, Phillip Wood

On Mon, Nov 16, 2020 at 08:51:49PM -0500, Jeff King wrote:

> On Mon, Nov 16, 2020 at 04:08:21PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 
> > Changes since v2:
> > 
> >  * The commit messages of patches 7/11 and 9/11 now stress why we want to
> >    align the output of the Perl vs the built-in version so slavishly: to be
> >    able to validate both versions against prerecorded output.
> >  * A typo was fixed in the commit message of patch 10/11.
> 
> This version looks fine to me, and I agree is a good stopping point for
> now (the only thing I'd consider adding is a test for the funcname in a
> split-hunk header, but it would have to be expect_failure for now; it is
> probably not worth the effort to rewrite the perl version to behave like
> the builtin here if we think it's going away soon).

Actually, I couldn't resist (but again, I don't think this has to be
part of your series):

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e713fe3d02..90561ea0e2 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -30,6 +30,10 @@
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($funcname_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.func', ''),
+	) : ();
 my ($diff_plain_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.plain', ''),
@@ -780,11 +784,11 @@ sub hunk_splittable {
 
 sub parse_hunk_header {
 	my ($line) = @_;
-	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =
-	    $line =~ /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/;
+	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $funcname) =
+	    $line =~ /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@ ?(.*)/;
 	$o_cnt = 1 unless defined $o_cnt;
 	$n_cnt = 1 unless defined $n_cnt;
-	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt, $funcname);
 }
 
 sub format_hunk_header {
@@ -806,7 +810,8 @@ sub split_hunk {
 	# it can be split, but we would need to take care of
 	# overlaps later.
 
-	my ($o_ofs, undef, $n_ofs) = parse_hunk_header($text->[0]);
+	my ($o_ofs, undef, $n_ofs, undef, $funcname) =
+		parse_hunk_header($text->[0]);
 	my $hunk_start = 1;
 
       OUTER:
@@ -894,6 +899,11 @@ sub split_hunk {
 		if ($diff_use_color) {
 			$display_head = colored($fraginfo_color, $head);
 		}
+		if (length $funcname) {
+			my $f = colored($funcname_color, $funcname);
+			$display_head =~ s/$/ $f/;
+			$funcname = '';
+		}
 		unshift @{$hunk->{DISPLAY}}, $display_head;
 	}
 	return @split;

^ permalink raw reply related	[flat|nested] 66+ messages in thread

* Re: [PATCH v3 00/11] Fix color handling in git add -i
  2020-11-17  1:51     ` [PATCH v3 00/11] Fix color handling in git add -i Jeff King
  2020-11-17  2:05       ` Jeff King
@ 2020-11-25 21:59       ` Junio C Hamano
  1 sibling, 0 replies; 66+ messages in thread
From: Junio C Hamano @ 2020-11-25 21:59 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Schindelin via GitGitGadget, git, Philippe Blain,
	Johannes Schindelin, Phillip Wood

Jeff King <peff@peff.net> writes:

> On Mon, Nov 16, 2020 at 04:08:21PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> Changes since v2:
>> 
>>  * The commit messages of patches 7/11 and 9/11 now stress why we want to
>>    align the output of the Perl vs the built-in version so slavishly: to be
>>    able to validate both versions against prerecorded output.
>>  * A typo was fixed in the commit message of patch 10/11.
>
> This version looks fine to me, and I agree is a good stopping point for
> now (the only thing I'd consider adding is a test for the funcname in a
> split-hunk header, but it would have to be expect_failure for now; it is
> probably not worth the effort to rewrite the perl version to behave like
> the builtin here if we think it's going away soon).

Will mark the topic to be merged to 'next'.

It seems that these days I am too quick and eager to take more
things in 'seen' than the rate at I can convince myself that the
topics are ready and accumulate too many topics in the "undecided"
category too fast.  I probably should slow down the intake a bit to
match.

Thanks.

^ permalink raw reply	[flat|nested] 66+ messages in thread

end of thread, other threads:[~2020-11-25 22:00 UTC | newest]

Thread overview: 66+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-10 23:42 [PATCH 0/9] Fix color handling in git add -i Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 1/9] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 2/9] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 3/9] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 4/9] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 5/9] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 6/9] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 7/9] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 8/9] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
2020-11-10 23:42 ` [PATCH 9/9] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
2020-11-11  2:35   ` Jeff King
2020-11-11 15:53     ` Johannes Schindelin
2020-11-11 18:07       ` Jeff King
2020-11-12 14:04         ` Johannes Schindelin
2020-11-12 18:29           ` Jeff King
2020-11-15 23:35             ` Johannes Schindelin
2020-11-17  1:44               ` Jeff King
2020-11-11  2:36 ` [PATCH 0/9] Fix color handling in git add -i Jeff King
2020-11-11 12:28 ` [PATCH v2 00/11] " Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 02/11] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
2020-11-13 12:03     ` Phillip Wood
2020-11-13 13:53       ` Johannes Schindelin
2020-11-13 16:04         ` Phillip Wood
2020-11-15 23:47           ` Johannes Schindelin
2020-11-11 12:28   ` [PATCH v2 05/11] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 06/11] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
2020-11-13 12:07     ` Phillip Wood
2020-11-13 13:57       ` Johannes Schindelin
2020-11-13 16:06         ` Phillip Wood
2020-11-15 23:55           ` Johannes Schindelin
2020-11-11 12:28   ` [PATCH v2 08/11] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
2020-11-11 12:28   ` [PATCH v2 09/11] add -i (Perl version): include indentation in the colored header Johannes Schindelin via GitGitGadget
2020-11-13 12:11     ` Phillip Wood
2020-11-13 13:58       ` Johannes Schindelin
2020-11-13 16:12         ` Phillip Wood
2020-11-15 23:51           ` Johannes Schindelin
2020-11-11 12:28   ` [PATCH v2 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
2020-11-13 14:04     ` Philippe Blain
2020-11-15 23:57       ` Johannes Schindelin
2020-11-13 14:08     ` Phillip Wood
2020-11-16  0:02       ` Johannes Schindelin
2020-11-11 12:28   ` [PATCH v2 11/11] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
2020-11-11 18:45   ` [PATCH v2 00/11] Fix color handling in git add -i Jeff King
2020-11-12 14:20     ` Johannes Schindelin
2020-11-12 18:40       ` Jeff King
2020-11-15 23:40         ` Johannes Schindelin
2020-11-17  1:49           ` Jeff King
2020-11-16 16:08   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 01/11] add -i (built-in): do show an error message for incorrect inputs Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 02/11] add -i (built-in): send error messages to stderr Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 03/11] add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headers Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 04/11] add -i: use `reset_color` consistently Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 05/11] add -i (built-in): prevent the `reset` "color" from being configured Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 06/11] add -i (built-in): use correct names to load color.diff.* config Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 07/11] add -p (built-in): do not color the progress indicator separately Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 08/11] add -i (built-in): use the same indentation as the Perl version Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 09/11] add -i (Perl version): color header to match the C version Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 10/11] add -p: prefer color.diff.context over color.diff.plain Johannes Schindelin via GitGitGadget
2020-11-16 16:08     ` [PATCH v3 11/11] add -i: verify in the tests that colors can be overridden Johannes Schindelin via GitGitGadget
2020-11-17  1:51     ` [PATCH v3 00/11] Fix color handling in git add -i Jeff King
2020-11-17  2:05       ` Jeff King
2020-11-25 21:59       ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.