Re: Patch text in git-add patch mode lacks whitespace highlighting

From: Jeff King <peff@peff.net>
Date: Jan 31, 2020, 09:19 UTC
Message-ID: <20200131091917.GC2857810@coredump.intra.peff.net>
In-Reply-To: <CAK7jxYgJNvCp=m6rH31HNzN9Mqgaav7_YPvUMZmRb7mdYDZ_1g@mail.gmail.com>

On Thu, Jan 30, 2020 at 01:53:52PM -0800, Mike McGranahan wrote:

Show 8 quoted lines

> I'm using version 2.25.0.windows.1. If I set config "wsErrorHighlight"
> to "old", and run `git diff -p`, the resulting patch text highlights
> whitespace differences in the old text.
> 
> If I then run git-add in interactive patch mode, I expect the diff to
> include the whitespace highlights. But actually, it does not.
> 
> Is this a bug? Thanks for your help.

Sort of. It's more of a feature that has not yet been implemented. ;)

Like the rest of the color config, diff.wsErrorHighlight is not enabled for scriptable plumbing commands like git-diff-index, etc; it is only used for the user-facing porcelain commands like git-diff.

So scripts like git-add--interactive, which use the plumbing under the hood, are responsible for deciding which config options should be respected and passing the correct command-line options on to the plumbing. We do that for color.diff, diff.algorithm, and a few others. But nobody has yet taught it that diff.wsErrorHighlight is safe to pass (for the user-facing "color" version of the diff).

The solution for the existing perl script would be something like this:

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index c4b75bcb7f..fac1d1e63e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -46,6 +46,7 @@
 my $normal_color = $repo->get_color("", "reset");
 
 my $diff_algorithm = $repo->config('diff.algorithm');
+my $diff_error_highlight = $repo->config('diff.wsErrorHighlight');
 my $diff_filter = $repo->config('interactive.difffilter');
 
 my $use_readkey = 0;
@@ -727,7 +728,11 @@ sub parse_diff {
 	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();
 	if ($diff_use_color) {
-		my @display_cmd = ("git", @diff_cmd, qw(--color --), $path);
+		my @display_cmd = ("git", @diff_cmd, qw(--color),
+			           $diff_error_highlight ?
+				     "--ws-error-highlight=$diff_error_highlight" :
+				     (),
+				   qw(--), $path);
 		if (defined $diff_filter) {
 			# quotemeta is overkill, but sufficient for shell-quoting
 			my $diff = join(' ', map { quotemeta } @display_cmd);

but this is all being rewritten in C. I'm not sure how the multiple
diffs are being handled there.

All that said, I kind of wonder if diff.wsErrorHighlight ought to be
respected by even plumbing commands. After all, it does nothing unless
color is enabled anyway, so I don't think it runs the risk of confusing
a user of the plumbing. It's no different than color.diff.*, which we
respect even in the plumbing commands (if the caller has manually asked
for color, of course).

-Peff