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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 31, 2020, 12:37 UTC
Message-ID: <nycvar.QRO.7.76.6.2001311304140.46@tvgsbejvaqbjf.bet>
In-Reply-To: <CAK7jxYgJNvCp=m6rH31HNzN9Mqgaav7_YPvUMZmRb7mdYDZ_1g@mail.gmail.com>

Hi Mike,

On Thu, 30 Jan 2020, 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.

Well, let's first try to get a preciser report, to make sure that we're on the same page. These are the commands I ran:

	mkdir add-p-ws-error
	cd add-p-ws-error
	git init
	git config diff.wsErrorHighlight old
	echo "hello " >README
	git add README
	echo hello >README
	git -c add.interactive.usebuiltin=true add -p
	git -c add.interactive.usebuiltin=false add -p

In both `git add -p` invocations, I see the diff colored, and neither of them shows the red square after the removed "hello" that `git diff shows.

So this is not a change of behavior in the conversion from a Perl script to a built-in.

Investigating further, running with `GIT_TRACE=1` reveals that `add -p` internally calls `git diff-files --color -p --`. If you run that command manually, you will see that it indeed seems to ignore the `diff.wsErrorHighlight` setting.

The actual code running `git diff-files` is here: https://github.com/git-for-windows/git/blob/v2.25.0.windows.1/add-patch.c#L373-L436

I think it is correct to call the low-level `diff-files` here rather than the high-level `diff` that is intended for human consumption.

Looking at the code in https://github.com/git/git/blob/v2.25.0/builtin/diff-files.c#L28, I believe that we found the reason why `git diff-files --color` ignores the `wsErrorHighlight` setting.

Now, the documentation at https://git-scm.com/docs/git-config#Documentation/git-config.txt-diffwsErrorHighlight and even at https://git-scm.com/docs/git-diff-files#Documentation/git-diff-files.txt---ws-error-highlightltkindgt make it appear as if the code is not following the original intention.

If my reading is correct, and we want `git diff-files --color` to respect the `diff.wsErrorHighlight` setting, then this patch fixes that:

-- snip --

diff --git a/diff.c b/diff.c
index 8e2914c0312..63afb8638c8 100644
--- a/diff.c
+++ b/diff.c
@@ -414,14 +414,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 		return 0;
 	}

-	if (!strcmp(var, "diff.wserrorhighlight")) {
-		int val = parse_ws_error_highlight(value);
-		if (val < 0)
-			return -1;
-		ws_error_highlight_default = val;
-		return 0;
-	}
-
 	if (git_color_config(var, value, cb) < 0)
 		return -1;

@@ -469,6 +461,14 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return 0;
 	}

+	if (!strcmp(var, "diff.wserrorhighlight")) {
+		int val = parse_ws_error_highlight(value);
+		if (val < 0)
+			return -1;
+		ws_error_highlight_default = val;
+		return 0;
+	}
+
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;

-- snap --

The bigger question is whether other core developers agree with this? And
what other `diff.*` settings should be respected by `git diff-files` (and
of course, `git diff-index`)?

Ciao,
Johannes