From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Philippe Blain <levraiphilippeblain@gmail.com>,
Jeff King <peff@peff.net>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH v2 00/11] Fix color handling in git add -i
Date: Wed, 11 Nov 2020 12:28:13 +0000 [thread overview]
Message-ID: <pull.785.v2.git.1605097704.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.785.git.1605051739.gitgitgadget@gmail.com>
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
next prev parent reply other threads:[~2020-11-11 12:28 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Johannes Schindelin via GitGitGadget [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.785.v2.git.1605097704.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
--cc=levraiphilippeblain@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.