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

[Mike McGranahan <mike@mcgwiz.com>]

I can confirm this fixes the unexpected behavior I reported. (Thanks!)
I can't speak directly to the plumbing as I don't regularly use thos
tools.

-Mike

On Fri, Jan 31, 2020 at 1:19 AM Jeff King <peff@peff.net> wrote:
>
> On Thu, Jan 30, 2020 at 01:53:52PM -0800, Mike McGranahan wrote:
>
> > 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