Skip to content

Fix color handling in git add -i

Johannes Schindelin via GitGitGadget git 3 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v3 Latest11 patches 3 repliesIn review
  2. v211 patches 21 repliesSuperseded
  3. v19 patches 8 repliesSuperseded

Changes in v3 from the cover letter

* 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.

Patches in v3 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/11add -i (built-in): do show an error message for incorrect inputsadd-interactive.c+2 -10 replies
2/11add -i (built-in): send error messages to stderradd-interactive.c+1 -10 replies
3/11add -p (built-in): imitate `xdl_format_hunk_hdr()` generating hunk headersadd-patch.c+8 -30 replies
4/11add -i: use `reset_color` consistentlyadd-patch.c+2 -20 replies
5/11add -i (built-in): prevent the `reset` "color" from being configuredadd-interactive.c+3 -10 replies
6/11add -i (built-in): use correct names to load color.diff.* configadd-interactive.c+13 -100 replies
7/11add -p (built-in): do not color the progress indicator separatelyadd-patch.c+5 -50 replies
8/11add -i (built-in): use the same indentation as the Perl versionadd-interactive.c+1 -10 replies
9/11add -i (Perl version): color header to match the C versiongit-add--interactive.perl+2 -40 replies
10/11add -p: prefer color.diff.context over color.diff.plainadd-interactive.c git-add--interactive.perl+7 -50 replies
11/11add -i: verify in the tests that colors can be overriddent/t3701-add-interactive.sh+84 -00 replies

Reviews trailers per patch

None of the 11 patches has a Reviewed-by or Acked-by.

PatchJeff King
1/11
2/11
3/11
4/11
5/11
6/11Co-authored-by
7/11
8/11
9/11
10/11
11/11Co-authored-by

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes.

Read the v3 thread

Cover letter [PATCH v3 00/11] Fix color handling in git add -i

This patch series started out as a tiny fix for a bug reported by Philippe Blain in
https://lore.kernel.org/git/[email protected]/.
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.

  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 <[email protected]>

## 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.

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

Signature · 1 line
gitgitgadget