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

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)

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

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?

Am 23.06.24 um 11:17 schrieb lolligerhans@gmx.de:

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é

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

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.

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.

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.

Other than that, looking very good.

Thanks.

Am 24.06.24 um 18:21 schrieb Junio C Hamano:

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.

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

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é

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

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.

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

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

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.