mbox series

[v3,00/11] Fix color handling in git add -i

Message ID pull.785.v3.git.1605542912.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Fix color handling in git add -i | expand

Message

Han-Wen Nienhuys via GitGitGadget Nov. 16, 2020, 4:08 p.m. UTC
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

Comments

Jeff King Nov. 17, 2020, 1:51 a.m. UTC | #1
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
Jeff King Nov. 17, 2020, 2:05 a.m. UTC | #2
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;
Junio C Hamano Nov. 25, 2020, 9:59 p.m. UTC | #3
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.