git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
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

  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] " 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

  List information: http://vger.kernel.org/majordomo-info.html

* 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 \
    --subject='Re: [PATCH v2 00/11] Fix color handling in git add -i' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).