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