Bug: diff.external --no-ext-diff suppresses --color-moved

Bug: diff.external --no-ext-diff suppresses --color-moved

[lolligerhans]

Hello,

I configured "diff.extern" but use aliases for "diff --no-ext-diff". This combination suppresses --color-moved (as well as the corresponding config "diff.colorMoved").

What did you do before the bug happened? (Steps to reproduce your issue)
  1. Prepare ~/.gitconfig:
            [diff]
               #external = echo
  2. In some repository, create a moved-lines diff between index and working
     directory.
     For example, commit this file (the next 9 lines verbatim):
            line 1 first one
            line 2 second two
            line 3 third three
            line 4 fourth four
            line 5 fifth five
            line 6 sixth six
            line 7 seventh seven
            line 8 eighth eight
            line 9 ninth nine
     Then, edit it (moving lines exactly) to:
            line 4 fourth four
            line 5 fifth five
            line 6 sixth six
            line 7 seventh seven
            line 8 eighth eight
            line 9 ninth nine
            line 1 first one
            line 2 second two
            line 3 third three
     In this state, the command 'git diff --color-moved' should highlight
     changes as line moves with default colors purple/cyan.
  3. In ~/.gitconfig, uncomment 'external'.
  4. In the same repository, trigger the bug by running:
            git diff --no-ext-diff --color-moved

What did you expect to happen? (Expected behavior)

  The diff should be recognized as moving lines and colorized accordingly. By
  default in purple/cyan.
  The diff should NOT be colorized red/green.

What happened instead? (Actual behavior)

  The diff is colorized in red/green.

What's different between what you expected and what actually happened?

  The colorization is expected to indicate moved lines.
  The actual colorization indicates deletion/insertion, as if '--color-moved' is
  ignored.

Anything else you want to add:

  - I assume this bug is up to date with the 'next' branch, because the command
            git log v2.45.2..origin/next | grep "no-ext"
    finds no match in the repository from github.
  - Works the same in the older v2.25.1
  - Works the same with 'diff.colorMoved' instead of --color-moved
  - Works the same for other values of 'diff.external'
  - Works the same when setting custom colors for 'color.diff.oldMoved' etc.
  - Works not the same when using --no-ext-diff alone. Only when using
    diff.external as well.

[System Info]
git version:
git version 2.45.2
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.15.0-107-generic #117~20.04.1-Ubuntu SMP Tue Apr 30 00:00:00 2024 x86_64
compiler info: gnuc: 9.4
libc info: glibc: 2.31
$SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks] (none)

[PATCH] diff: allow --color-moved with --no-ext-diff

[René Scharfe]

Since the option --color-moved was added by 2e2d5ac184 (diff.c: color
moved lines differently, 2017-06-30) we have been ignoring it if an
external diff program is configured, presumably because its overhead is
unnecessary in that case.

Do respect --color-moved if we don't actually use the configured
external diff, though.

Reported-by: lolligerhans@gmx.de
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff.c                     | 3 ++-
 t/t4015-diff-whitespace.sh | 9 +++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 6e432cb8fc..aa0fb77761 100644
--- a/diff.c
+++ b/diff.c
@@ -4965,7 +4965,8 @@ void diff_setup_done(struct diff_options *options)
 	if (options->flags.follow_renames)
 		diff_check_follow_pathspec(&options->pathspec, 1);

-	if (!options->use_color || external_diff())
+	if (!options->use_color ||
+	    (options->flags.allow_external && external_diff()))
 		options->color_moved = 0;

 	if (options->filter_not) {
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index b443626afd..a1478680b6 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '
 	test_cmp expected actual
 '

+test_expect_success '--color-moved with --no-ext-diff' '
+	test_config color.diff.oldMoved "normal red" &&
+	test_config color.diff.newMoved "normal green" &&
+	cp actual.raw expect &&
+	git -c diff.external=false diff HEAD --no-ext-diff \
+		--color-moved=zebra --color --no-renames >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'detect malicious moved code, inside file' '
 	test_config color.diff.oldMoved "normal red" &&
 	test_config color.diff.newMoved "normal green" &&
--
2.45.2

Aw: [PATCH] diff: allow --color-moved with --no-ext-diff

[lolligerhans]

Hello René,

using red/green coloration is the current buggy behavior. When setting oldMOved and newMoved I would expect the correct behaviour to the the same as the buggy one.

Could this test pass despite the bug?

Re: [PATCH] diff: allow --color-moved with --no-ext-diff

[René Scharfe]

Am 23.06.24 um 11:17 schrieb lolligerhans@gmx.de:
> using red/green coloration is the current buggy behavior. When
> setting oldMOved and newMoved I would expect the correct behaviour to
> the the same as the buggy one.
>
> Could this test pass despite the bug?
Good question, but it doesn't.  The test uses the colors "normal red"
and "normal green" for moved lines, i.e. unchanged text color on red and
green background, while regular changed lines use red and green text on
unchanged background.

René

Re: [PATCH] diff: allow --color-moved with --no-ext-diff

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index b443626afd..a1478680b6 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success '--color-moved with --no-ext-diff' '
> +	test_config color.diff.oldMoved "normal red" &&
> +	test_config color.diff.newMoved "normal green" &&

We are making sure we won't be affected by previous tests.  We
assume that we did not set color.diff.{old,new} to these two colors,
but that would be an OK assumption to make.

> +	cp actual.raw expect &&

But then this introduces a dependence to an earlier _specific_ test,
the one that created this version (among three) of actual.raw;

If we did this instead

	git diff --color --color-moved=zebra --no-renames HEAD >expect &&

it would make this a lot more self-contained.

> +	git -c diff.external=false diff HEAD --no-ext-diff \
> +		--color-moved=zebra --color --no-renames >actual &&

Also, please do stick to the normal CLI ocnvention, dashed options
come before the revs, i.e.

	git -c diff.external=false diff --no-ext-diff --color \
		--color-moved=zebra --no-renames HEAD >actual &&

Our tests shouldn't be setting a wrong example.

> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'detect malicious moved code, inside file' '
>  	test_config color.diff.oldMoved "normal red" &&
>  	test_config color.diff.newMoved "normal green" &&

Other than that, looking very good.

Thanks.

Re: [PATCH] diff: allow --color-moved with --no-ext-diff

[René Scharfe]

Am 24.06.24 um 18:21 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
>> index b443626afd..a1478680b6 100755
>> --- a/t/t4015-diff-whitespace.sh
>> +++ b/t/t4015-diff-whitespace.sh
>> @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '
>>  	test_cmp expected actual
>>  '
>>
>> +test_expect_success '--color-moved with --no-ext-diff' '
>> +	test_config color.diff.oldMoved "normal red" &&
>> +	test_config color.diff.newMoved "normal green" &&
>
> We are making sure we won't be affected by previous tests.  We
> assume that we did not set color.diff.{old,new} to these two colors,
> but that would be an OK assumption to make.

The previous test also uses test_config to set these values, which means
they get cleared at its end.  We need to set them again to reuse the
actual.raw file.

>> +	cp actual.raw expect &&
>
> But then this introduces a dependence to an earlier _specific_ test,
> the one that created this version (among three) of actual.raw;>
> If we did this instead
>
> 	git diff --color --color-moved=zebra --no-renames HEAD >expect &&
>
> it would make this a lot more self-contained.

Oh, yes, good idea!  We'd still rely on there being staged differences
that include moved lines,

>> +	git -c diff.external=false diff HEAD --no-ext-diff \
>> +		--color-moved=zebra --color --no-renames >actual &&
>
> Also, please do stick to the normal CLI ocnvention, dashed options
> come before the revs, i.e.
>
> 	git -c diff.external=false diff --no-ext-diff --color \
> 		--color-moved=zebra --no-renames HEAD >actual &&
>
> Our tests shouldn't be setting a wrong example.

I mimicked the style of the previous test to hint that we do the same
here, just with the diff.external/--no-ext-diff noop on top.  Not close
enough, perhaps, but also hard to spot with 40+ lines between them.

René

[PATCH v2] diff: allow --color-moved with --no-ext-diff

[René Scharfe]

We ignore the option --color-moved if an external diff program is
configured, presumably because its overhead is unnecessary in that case.
Respect the option if we don't actually use the external diff, though.

Reported-by: lolligerhans@gmx.de
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Changes since v1:
* use single word colors, leaving the background unchanged, as they are
  easier to read,
* run git diff to generate the expected content instead of reusing the
  output of the previous test ...
* ... and put the common arguments in a variable to make clear that we
  compare diff with and without a diff.external/--no-ext-diff dance,
* use echo instead of false as the (unused) external diff command to
  avoid giving the wrong impression that diff.external is a boolean
  option we turn off here.

  diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
  index a1478680b6..851cfe4f32 100755
  --- a/t/t4015-diff-whitespace.sh
  +++ b/t/t4015-diff-whitespace.sh
  @@ -1185,11 +1185,11 @@ test_expect_success 'detect moved code, complete file' '
   '

   test_expect_success '--color-moved with --no-ext-diff' '
  -	test_config color.diff.oldMoved "normal red" &&
  -	test_config color.diff.newMoved "normal green" &&
  -	cp actual.raw expect &&
  -	git -c diff.external=false diff HEAD --no-ext-diff \
  -		--color-moved=zebra --color --no-renames >actual &&
  +	test_config color.diff.oldMoved "yellow" &&
  +	test_config color.diff.newMoved "blue" &&
  +	args="--color --color-moved=zebra --no-renames HEAD" &&
  +	git diff $args >expect &&
  +	git -c diff.external=echo diff --no-ext-diff $args >actual &&
   	test_cmp expect actual
   '


 diff.c                     | 3 ++-
 t/t4015-diff-whitespace.sh | 9 +++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 6e432cb8fc..aa0fb77761 100644
--- a/diff.c
+++ b/diff.c
@@ -4965,7 +4965,8 @@ void diff_setup_done(struct diff_options *options)
 	if (options->flags.follow_renames)
 		diff_check_follow_pathspec(&options->pathspec, 1);

-	if (!options->use_color || external_diff())
+	if (!options->use_color ||
+	    (options->flags.allow_external && external_diff()))
 		options->color_moved = 0;

 	if (options->filter_not) {
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index b443626afd..851cfe4f32 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '
 	test_cmp expected actual
 '

+test_expect_success '--color-moved with --no-ext-diff' '
+	test_config color.diff.oldMoved "yellow" &&
+	test_config color.diff.newMoved "blue" &&
+	args="--color --color-moved=zebra --no-renames HEAD" &&
+	git diff $args >expect &&
+	git -c diff.external=echo diff --no-ext-diff $args >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'detect malicious moved code, inside file' '
 	test_config color.diff.oldMoved "normal red" &&
 	test_config color.diff.newMoved "normal green" &&
--
2.45.2

Re: [PATCH v2] diff: allow --color-moved with --no-ext-diff

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> * use echo instead of false as the (unused) external diff command to
>   avoid giving the wrong impression that diff.external is a boolean
>   option we turn off here.

I am fine with either "/bin/false" or "echo".  I kind of found it a
nice way to assert that --no-ext-diff is indeed in effect (if we did
not disable it, "false" would lead to "whoa, your external diff
driver returned a failure").

Thanks, will queue.

>  diff.c                     | 3 ++-
>  t/t4015-diff-whitespace.sh | 9 +++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/diff.c b/diff.c
> index 6e432cb8fc..aa0fb77761 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4965,7 +4965,8 @@ void diff_setup_done(struct diff_options *options)
>  	if (options->flags.follow_renames)
>  		diff_check_follow_pathspec(&options->pathspec, 1);
>
> -	if (!options->use_color || external_diff())
> +	if (!options->use_color ||
> +	    (options->flags.allow_external && external_diff()))
>  		options->color_moved = 0;
>
>  	if (options->filter_not) {
> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index b443626afd..851cfe4f32 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success '--color-moved with --no-ext-diff' '
> +	test_config color.diff.oldMoved "yellow" &&
> +	test_config color.diff.newMoved "blue" &&
> +	args="--color --color-moved=zebra --no-renames HEAD" &&
> +	git diff $args >expect &&
> +	git -c diff.external=echo diff --no-ext-diff $args >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'detect malicious moved code, inside file' '
>  	test_config color.diff.oldMoved "normal red" &&
>  	test_config color.diff.newMoved "normal green" &&
> --
> 2.45.2

Re: [PATCH] diff: allow --color-moved with --no-ext-diff

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>> If we did this instead
>>
>> 	git diff --color --color-moved=zebra --no-renames HEAD >expect &&
>>
>> it would make this a lot more self-contained.
>
> Oh, yes, good idea!  We'd still rely on there being staged differences
> that include moved lines,

Yeah, the primary point of this fix is that the output with
"--no-ext-diff" and configured external diff driver, and without
either, should be the same.  And running the same command twice,
once with and once without these two tweaks, and comparing their
results is exactly what we want to do.