Patch text in git-add patch mode lacks whitespace highlighting

Patch text in git-add patch mode lacks whitespace highlighting

[Mike McGranahan]

Hello,

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.

-Mike

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

[Jeff King]

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

[PATCH] diff: move diff.wsErrorHighlight to "basic" config

[Jeff King]

On Fri, Jan 31, 2020 at 04:19:17AM -0500, Jeff King wrote:

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

The more I pondered this, the more it seems like the right thing. So
here's a patch to do it.

I also suspect that some of the color-moved config would benefit from
the same treatment, but I haven't yet convinced myself. Unlike this
option, which impacts a single line, the move detection covers multiple
hunks, which you'd be picking out independently. Would that be weird, or
would it make sense as it's just annotating the existing diff?

-- >8 --
Subject: [PATCH] diff: move diff.wsErrorHighlight to "basic" config

We parse diff.wsErrorHighlight in git_diff_ui_config(), meaning that it
doesn't take effect for plumbing commands, only for porcelains like
git-diff itself. This is mildly annoying as it means scripts like
add--interactive, which produce a user-visible diff with color, don't
respect the option.

We could teach that script to parse the config and pass it along as
--ws-error-highlight to the diff plumbing. But there's a simpler
solution.

It should be reasonably safe for plumbing to respect this option, as it
only kicks in when color is otherwise enabled. And anybody parsing
colorized output must already deal with the fact that color.diff.* may
change the exact output they see; those options have been part of
git_diff_basic_config() since its inception in 9a1805a872 (add a "basic"
diff config callback, 2008-01-04).

So we can just move it to the "basic" config, which fixes
add--interactive, along with any other script in the same boat, with a
very low risk of hurting any plumbing users.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c                     | 16 ++++++++--------
 t/t3701-add-interactive.sh | 13 +++++++++++++
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 8e2914c031..5e4471f15f 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;
 
@@ -450,6 +442,14 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return color_parse(value, diff_colors[slot]);
 	}
 
+	if (!strcmp(var, "diff.wserrorhighlight")) {
+		int val = parse_ws_error_highlight(value);
+		if (val < 0)
+			return -1;
+		ws_error_highlight_default = val;
+		return 0;
+	}
+
 	/* like GNU diff's --suppress-blank-empty option  */
 	if (!strcmp(var, "diff.suppressblankempty") ||
 			/* for backwards compatibility */
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 2182b1c030..a28182c526 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -553,6 +553,19 @@ test_expect_success 'diffs can be colorized' '
 	grep "$(printf "\\033")" output
 '
 
+test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
+	git reset --hard &&
+
+	echo "old " >test &&
+	git add test &&
+	echo "new " >test &&
+
+	printf y >y &&
+	force_color git -c diff.wsErrorHighlight=all add -p >output.raw 2>&1 <y &&
+	test_decode_color <output.raw >output &&
+	grep "old<" output
+'
+
 test_expect_success 'diffFilter filters diff' '
 	git reset --hard &&
 
-- 
2.25.0.538.g1d5d7023b0

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

[Mike McGranahan]

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

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

[Johannes Schindelin]

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

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

[Mike McGranahan]

On Fri, Jan 31, 2020 at 4:37 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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

Those commands do indeed reproduce the behavior I described.

-Mike

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

[Jeff King]

On Fri, Jan 31, 2020 at 01:37:44PM +0100, Johannes Schindelin wrote:

> If my reading is correct, and we want `git diff-files --color` to respect
> the `diff.wsErrorHighlight` setting, then this patch fixes that:
> [...]
> 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`)?

I think you can take my posting of an identical patch elsewhere in the
thread as a "yes". :)

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Sat, 1 Feb 2020, Jeff King wrote:

> On Fri, Jan 31, 2020 at 01:37:44PM +0100, Johannes Schindelin wrote:
>
> > If my reading is correct, and we want `git diff-files --color` to respect
> > the `diff.wsErrorHighlight` setting, then this patch fixes that:
> > [...]
> > 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`)?
>
> I think you can take my posting of an identical patch elsewhere in the
> thread as a "yes". :)

Thank you ;-)

That answer only applies to the first question, though. The second
question, whether other `diff.*` settings would like to enjoy the same
treatment, is still open for debate...

Thanks,
Dscho

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

[Jeff King]

On Sat, Feb 01, 2020 at 10:06:43PM +0100, Johannes Schindelin wrote:

> > > If my reading is correct, and we want `git diff-files --color` to respect
> > > the `diff.wsErrorHighlight` setting, then this patch fixes that:
> > > [...]
> > > 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`)?
> >
> > I think you can take my posting of an identical patch elsewhere in the
> > thread as a "yes". :)
> 
> Thank you ;-)
> 
> That answer only applies to the first question, though. The second
> question, whether other `diff.*` settings would like to enjoy the same
> treatment, is still open for debate...

I hoped you would read the other part of the thread where I mused on
that same question. :)

The short of my answer is that I think the color-moved stuff might be a
candidate, but it's sufficiently different that I think it should be
decided on as a separate patch.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Mon, 3 Feb 2020, Jeff King wrote:

> On Sat, Feb 01, 2020 at 10:06:43PM +0100, Johannes Schindelin wrote:
>
> > > > If my reading is correct, and we want `git diff-files --color` to respect
> > > > the `diff.wsErrorHighlight` setting, then this patch fixes that:
> > > > [...]
> > > > 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`)?
> > >
> > > I think you can take my posting of an identical patch elsewhere in the
> > > thread as a "yes". :)
> >
> > Thank you ;-)
> >
> > That answer only applies to the first question, though. The second
> > question, whether other `diff.*` settings would like to enjoy the same
> > treatment, is still open for debate...
>
> I hoped you would read the other part of the thread where I mused on
> that same question. :)

Sorry, I totally missed that. Will re-read.

> The short of my answer is that I think the color-moved stuff might be a
> candidate, but it's sufficiently different that I think it should be
> decided on as a separate patch.

I actually wonder whether we should do something completely different. The
problem with `git diff-files --color`, after all, is that `diff-files` was
never intended to produce user-facing output, so the `--color` is somewhat
of a contradiction here. The fact that `diff-files` is a low-level (or
"plumbing") command means that by nature, it wants to control a lot more
what the user-provided config can change (typically, scripts calling
`diff-files` expect a certain format, and it would not do at all to heed
`diff.noPrefix`, for example.

In that respect, `git add -p` using `diff-files` is kind of wrong: we
_want_ to show the result to the user, with no processing at all (execpt
the user-provided `diffFilter`).

So why not introduce a new option to `diff-files` and `diff-index` to ask
it _specifically_ to heed diff UI config settings? I.e. a command-line
option that makes it call

        git_config(git_diff_ui_config, NULL);

instead of

        git_config(git_diff_basic_config, NULL); /* no "diff" UI options */

?

Ciao,
Dscho

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

[Jeff King]

On Mon, Feb 03, 2020 at 01:37:48PM +0100, Johannes Schindelin wrote:

> > The short of my answer is that I think the color-moved stuff might be a
> > candidate, but it's sufficiently different that I think it should be
> > decided on as a separate patch.
> 
> I actually wonder whether we should do something completely different. The
> problem with `git diff-files --color`, after all, is that `diff-files` was
> never intended to produce user-facing output, so the `--color` is somewhat
> of a contradiction here. The fact that `diff-files` is a low-level (or
> "plumbing") command means that by nature, it wants to control a lot more
> what the user-provided config can change (typically, scripts calling
> `diff-files` expect a certain format, and it would not do at all to heed
> `diff.noPrefix`, for example.
> 
> In that respect, `git add -p` using `diff-files` is kind of wrong: we
> _want_ to show the result to the user, with no processing at all (execpt
> the user-provided `diffFilter`).

Sort of. The problem is that we need two matching copies of the diff:
one to apply, and one to show the user. They don't need to be
byte-for-byte identical, but they should correlate at the level of
individual lines. And the "one to apply" can take on some user-selected
options, as long as the result can still be applied.

> So why not introduce a new option to `diff-files` and `diff-index` to ask
> it _specifically_ to heed diff UI config settings? I.e. a command-line
> option that makes it call
> 
>         git_config(git_diff_ui_config, NULL);
> 
> instead of
> 
>         git_config(git_diff_basic_config, NULL); /* no "diff" UI options */

Would you pass that option to both of the diff calls, or just the one
generating the human-readable input?

If just the human-readable one, then many options that change the line
count would be problems: diff.context, diff.interhunkcontext,
diff.orderfile, etc.

If both, then some options would be problematic for applying. Just
looking over the list, these jump out at me:

  - color.diff=always would obviously be an issue (though TBH I think
    anybody doing that is inviting a lot of breakages anyway)

  - diff.external would be a problem if it kicked in, though I think it
    would require --ext-diff to actually do anything

  - diff.submodule would generate diffs that can't be applied

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If just the human-readable one, then many options that change the line
> count would be problems: diff.context, diff.interhunkcontext,
> diff.orderfile, etc.
>
> If both, then some options would be problematic for applying. Just
> looking over the list, these jump out at me:
>
>   - color.diff=always would obviously be an issue (though TBH I think
>     anybody doing that is inviting a lot of breakages anyway)
>
>   - diff.external would be a problem if it kicked in, though I think it
>     would require --ext-diff to actually do anything
>
>   - diff.submodule would generate diffs that can't be applied

What can truly help "git add -p" might be a new output mode from
"git diff", perhaps, in which instead of writing a stream of text
(with color codes intermixed), the output machinery makes a call to
API-user supplied callbacks to report ("type of output line", "the
payload", "list of coloring information") tuples for each line, so
that the API-user can synthesize _both_ versions it currently uses
with a single invocation of "diff"?  There may be tons of "output
line" types, like the diff header, hunk header, context, removed,
added, etc., and depending on the type, the form the payload takes
may be different (e.g. a context line may be just a plain text, a
hunk header line may be a five-tuple (pre- and post-context line
numbers and line countsm plus the function-header line text).

As long as the call the API-user makes to the diff machinery grabs
everything that is needed for both human and machine consumption in
a single call, there is no risk of getting confused by two sets of
inconsistent patch data.

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

[Jeff King]

On Tue, Feb 04, 2020 at 10:29:53AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If just the human-readable one, then many options that change the line
> > count would be problems: diff.context, diff.interhunkcontext,
> > diff.orderfile, etc.
> >
> > If both, then some options would be problematic for applying. Just
> > looking over the list, these jump out at me:
> >
> >   - color.diff=always would obviously be an issue (though TBH I think
> >     anybody doing that is inviting a lot of breakages anyway)
> >
> >   - diff.external would be a problem if it kicked in, though I think it
> >     would require --ext-diff to actually do anything
> >
> >   - diff.submodule would generate diffs that can't be applied
> 
> What can truly help "git add -p" might be a new output mode from
> "git diff", perhaps, in which instead of writing a stream of text
> (with color codes intermixed), the output machinery makes a call to
> API-user supplied callbacks to report ("type of output line", "the
> payload", "list of coloring information") tuples for each line, so
> that the API-user can synthesize _both_ versions it currently uses
> with a single invocation of "diff"?  There may be tons of "output
> line" types, like the diff header, hunk header, context, removed,
> added, etc., and depending on the type, the form the payload takes
> may be different (e.g. a context line may be just a plain text, a
> hunk header line may be a five-tuple (pre- and post-context line
> numbers and line countsm plus the function-header line text).

I agree that would be a nice thing to have, and would let us avoid
running the same diff twice. But I don't think it solves the problem you
quoted above, unless all of the features are represented in some
machine-readable form. E.g., what would diff.submodule do in such an
output, and how would "add -p" decide to show it?

> As long as the call the API-user makes to the diff machinery grabs
> everything that is needed for both human and machine consumption in
> a single call, there is no risk of getting confused by two sets of
> inconsistent patch data.

Yes, but we still can't say "use machine-readable output, and turn on
all the config bells and whistles" because we'd have to know how to
handle every bell and whistle.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Mon, 3 Feb 2020, Jeff King wrote:

> On Mon, Feb 03, 2020 at 01:37:48PM +0100, Johannes Schindelin wrote:
>
> > > The short of my answer is that I think the color-moved stuff might be a
> > > candidate, but it's sufficiently different that I think it should be
> > > decided on as a separate patch.
> >
> > I actually wonder whether we should do something completely different. The
> > problem with `git diff-files --color`, after all, is that `diff-files` was
> > never intended to produce user-facing output, so the `--color` is somewhat
> > of a contradiction here. The fact that `diff-files` is a low-level (or
> > "plumbing") command means that by nature, it wants to control a lot more
> > what the user-provided config can change (typically, scripts calling
> > `diff-files` expect a certain format, and it would not do at all to heed
> > `diff.noPrefix`, for example.
> >
> > In that respect, `git add -p` using `diff-files` is kind of wrong: we
> > _want_ to show the result to the user, with no processing at all (execpt
> > the user-provided `diffFilter`).
>
> Sort of. The problem is that we need two matching copies of the diff:
> one to apply, and one to show the user. They don't need to be
> byte-for-byte identical, but they should correlate at the level of
> individual lines. And the "one to apply" can take on some user-selected
> options, as long as the result can still be applied.
>
> > So why not introduce a new option to `diff-files` and `diff-index` to ask
> > it _specifically_ to heed diff UI config settings? I.e. a command-line
> > option that makes it call
> >
> >         git_config(git_diff_ui_config, NULL);
> >
> > instead of
> >
> >         git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>
> Would you pass that option to both of the diff calls, or just the one
> generating the human-readable input?
>
> If just the human-readable one, then many options that change the line
> count would be problems: diff.context, diff.interhunkcontext,
> diff.orderfile, etc.

Darn, you have a very valid point :-(

> If both, then some options would be problematic for applying. Just
> looking over the list, these jump out at me:
>
>   - color.diff=always would obviously be an issue (though TBH I think
>     anybody doing that is inviting a lot of breakages anyway)
>
>   - diff.external would be a problem if it kicked in, though I think it
>     would require --ext-diff to actually do anything
>
>   - diff.submodule would generate diffs that can't be applied

So I guess it is back to your patch, maybe amended by a move of the
color-moved setting.

Ciao,
Dscho