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