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@xxxxxxxxx/. 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@xxxxxxxx> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> ## 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
