{"thread":{"id":"61672","subject":"Bug: diff.external --no-ext-diff suppresses --color-moved","startedAt":"2024-06-22T10:01:17Z","lastAt":"2024-06-25T00:39:39Z","messageCount":9,"participants":["lolligerhans@gmx.de","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"497539","messageId":"trinity-acbdb8fc-3dc3-4dca-890c-8bcb37405782-1719050465639@msvc-mesg-gmx004","threadId":"61672","inReplyTo":null,"subject":"Bug: diff.external --no-ext-diff suppresses --color-moved","fromName":"","fromEmail":"lolligerhans@gmx.de","sentAt":"2024-06-22T10:01:16Z","receivedAt":"2024-06-22T10:01:17Z","isPatch":false,"sender":{"key":"lolligerhans@gmx.de","avatar":null},"body":"Hello,\n\nI configured \"diff.extern\" but use aliases for \"diff --no-ext-diff\". This combination suppresses --color-moved (as well as the corresponding config \"diff.colorMoved\").\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n 1. Prepare ~/.gitconfig:\n [diff]\n #external = echo\n 2. In some repository, create a moved-lines diff between index and working\n directory.\n For example, commit this file (the next 9 lines verbatim):\n line 1 first one\n line 2 second two\n line 3 third three\n line 4 fourth four\n line 5 fifth five\n line 6 sixth six\n line 7 seventh seven\n line 8 eighth eight\n line 9 ninth nine\n Then, edit it (moving lines exactly) to:\n line 4 fourth four\n line 5 fifth five\n line 6 sixth six\n line 7 seventh seven\n line 8 eighth eight\n line 9 ninth nine\n line 1 first one\n line 2 second two\n line 3 third three\n In this state, the command 'git diff --color-moved' should highlight\n changes as line moves with default colors purple/cyan.\n 3. In ~/.gitconfig, uncomment 'external'.\n 4. In the same repository, trigger the bug by running:\n git diff --no-ext-diff --color-moved\n\nWhat did you expect to happen? (Expected behavior)\n\n The diff should be recognized as moving lines and colorized accordingly. By\n default in purple/cyan.\n The diff should NOT be colorized red/green.\n\nWhat happened instead? (Actual behavior)\n\n The diff is colorized in red/green.\n\nWhat's different between what you expected and what actually happened?\n\n The colorization is expected to indicate moved lines.\n The actual colorization indicates deletion/insertion, as if '--color-moved' is\n ignored.\n\nAnything else you want to add:\n\n - I assume this bug is up to date with the 'next' branch, because the command\n git log v2.45.2..origin/next | grep \"no-ext\"\n finds no match in the repository from github.\n - Works the same in the older v2.25.1\n - Works the same with 'diff.colorMoved' instead of --color-moved\n - Works the same for other values of 'diff.external'\n - Works the same when setting custom colors for 'color.diff.oldMoved' etc.\n - Works not the same when using --no-ext-diff alone. Only when using\n diff.external as well.\n\n[System Info]\ngit version:\ngit version 2.45.2\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.15.0-107-generic #117~20.04.1-Ubuntu SMP Tue Apr 30 00:00:00 2024 x86_64\ncompiler info: gnuc: 9.4\nlibc info: glibc: 2.31\n$SHELL (typically, interactive shell): /bin/bash\n\n[Enabled Hooks] (none)\n"},{"id":"497540","messageId":"8a8bd51e-9ce5-4a68-bfbe-f16dcbb7e89c@web.de","threadId":"61672","inReplyTo":"trinity-acbdb8fc-3dc3-4dca-890c-8bcb37405782-1719050465639@msvc-mesg-gmx004","subject":"[PATCH] diff: allow --color-moved with --no-ext-diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-22T19:41:30Z","receivedAt":"2024-06-22T19:41:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Since the option --color-moved was added by 2e2d5ac184 (diff.c: color\nmoved lines differently, 2017-06-30) we have been ignoring it if an\nexternal diff program is configured, presumably because its overhead is\nunnecessary in that case.\n\nDo respect --color-moved if we don't actually use the configured\nexternal diff, though.\n\nReported-by: lolligerhans@gmx.de\nSigned-off-by: René Scharfe \n---\n diff.c | 3 ++-\n t/t4015-diff-whitespace.sh | 9 +++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6e432cb8fc..aa0fb77761 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4965,7 +4965,8 @@ void diff_setup_done(struct diff_options *options)\n \tif (options->flags.follow_renames)\n \t\tdiff_check_follow_pathspec(&options->pathspec, 1);\n\n-\tif (!options->use_color || external_diff())\n+\tif (!options->use_color ||\n+\t (options->flags.allow_external && external_diff()))\n \t\toptions->color_moved = 0;\n\n \tif (options->filter_not) {\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex b443626afd..a1478680b6 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '\n \ttest_cmp expected actual\n '\n\n+test_expect_success '--color-moved with --no-ext-diff' '\n+\ttest_config color.diff.oldMoved \"normal red\" &&\n+\ttest_config color.diff.newMoved \"normal green\" &&\n+\tcp actual.raw expect &&\n+\tgit -c diff.external=false diff HEAD --no-ext-diff \\\n+\t\t--color-moved=zebra --color --no-renames >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'detect malicious moved code, inside file' '\n \ttest_config color.diff.oldMoved \"normal red\" &&\n \ttest_config color.diff.newMoved \"normal green\" &&\n--\n2.45.2\n"},{"id":"497544","messageId":"trinity-5166e20d-147e-4205-99b7-f13a007ed5f9-1719134235233@3c-app-gmx-bap30","threadId":"61672","inReplyTo":"8a8bd51e-9ce5-4a68-bfbe-f16dcbb7e89c@web.de","subject":"Aw: [PATCH] diff: allow --color-moved with --no-ext-diff","fromName":"","fromEmail":"lolligerhans@gmx.de","sentAt":"2024-06-23T09:17:15Z","receivedAt":"2024-06-23T09:17:16Z","isPatch":true,"sender":{"key":"lolligerhans@gmx.de","avatar":null},"body":"Hello René,\n\nusing 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.\n\nCould this test pass despite the bug?\n"},{"id":"497545","messageId":"e23dc86c-e32d-40f9-8305-86d4a25e79b6@web.de","threadId":"61672","inReplyTo":"trinity-5166e20d-147e-4205-99b7-f13a007ed5f9-1719134235233@3c-app-gmx-bap30","subject":"Re: [PATCH] diff: allow --color-moved with --no-ext-diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-23T09:46:43Z","receivedAt":"2024-06-23T09:46:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.06.24 um 11:17 schrieb lolligerhans@gmx.de:\n> using red/green coloration is the current buggy behavior. When\n> setting oldMOved and newMoved I would expect the correct behaviour to\n> the the same as the buggy one.\n>\n> Could this test pass despite the bug?\nGood question, but it doesn't. The test uses the colors \"normal red\"\nand \"normal green\" for moved lines, i.e. unchanged text color on red and\ngreen background, while regular changed lines use red and green text on\nunchanged background.\n\nRené\n"},{"id":"497576","messageId":"xmqqsex2cnwk.fsf@gitster.g","threadId":"61672","inReplyTo":"8a8bd51e-9ce5-4a68-bfbe-f16dcbb7e89c@web.de","subject":"Re: [PATCH] diff: allow --color-moved with --no-ext-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-24T16:21:15Z","receivedAt":"2024-06-24T16:21:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index b443626afd..a1478680b6 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '\n> \ttest_cmp expected actual\n> '\n>\n> +test_expect_success '--color-moved with --no-ext-diff' '\n> +\ttest_config color.diff.oldMoved \"normal red\" &&\n> +\ttest_config color.diff.newMoved \"normal green\" &&\n\nWe are making sure we won't be affected by previous tests. We\nassume that we did not set color.diff.{old,new} to these two colors,\nbut that would be an OK assumption to make.\n\n> +\tcp actual.raw expect &&\n\nBut then this introduces a dependence to an earlier _specific_ test,\nthe one that created this version (among three) of actual.raw;\n\nIf we did this instead\n\n\tgit diff --color --color-moved=zebra --no-renames HEAD >expect &&\n\nit would make this a lot more self-contained.\n\n> +\tgit -c diff.external=false diff HEAD --no-ext-diff \\\n> +\t\t--color-moved=zebra --color --no-renames >actual &&\n\nAlso, please do stick to the normal CLI ocnvention, dashed options\ncome before the revs, i.e.\n\n\tgit -c diff.external=false diff --no-ext-diff --color \\\n\t\t--color-moved=zebra --no-renames HEAD >actual &&\n\nOur tests shouldn't be setting a wrong example.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'detect malicious moved code, inside file' '\n> \ttest_config color.diff.oldMoved \"normal red\" &&\n> \ttest_config color.diff.newMoved \"normal green\" &&\n\nOther than that, looking very good.\n\nThanks.\n"},{"id":"497584","messageId":"48948980-ac98-452a-b2df-11cd81de56da@web.de","threadId":"61672","inReplyTo":"xmqqsex2cnwk.fsf@gitster.g","subject":"Re: [PATCH] diff: allow --color-moved with --no-ext-diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-24T19:15:43Z","receivedAt":"2024-06-24T19:15:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.06.24 um 18:21 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n>> index b443626afd..a1478680b6 100755\n>> --- a/t/t4015-diff-whitespace.sh\n>> +++ b/t/t4015-diff-whitespace.sh\n>> @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '\n>> \ttest_cmp expected actual\n>> '\n>>\n>> +test_expect_success '--color-moved with --no-ext-diff' '\n>> +\ttest_config color.diff.oldMoved \"normal red\" &&\n>> +\ttest_config color.diff.newMoved \"normal green\" &&\n>\n> We are making sure we won't be affected by previous tests. We\n> assume that we did not set color.diff.{old,new} to these two colors,\n> but that would be an OK assumption to make.\n\nThe previous test also uses test_config to set these values, which means\nthey get cleared at its end. We need to set them again to reuse the\nactual.raw file.\n\n>> +\tcp actual.raw expect &&\n>\n> But then this introduces a dependence to an earlier _specific_ test,\n> the one that created this version (among three) of actual.raw;>\n> If we did this instead\n>\n> \tgit diff --color --color-moved=zebra --no-renames HEAD >expect &&\n>\n> it would make this a lot more self-contained.\n\nOh, yes, good idea! We'd still rely on there being staged differences\nthat include moved lines,\n\n>> +\tgit -c diff.external=false diff HEAD --no-ext-diff \\\n>> +\t\t--color-moved=zebra --color --no-renames >actual &&\n>\n> Also, please do stick to the normal CLI ocnvention, dashed options\n> come before the revs, i.e.\n>\n> \tgit -c diff.external=false diff --no-ext-diff --color \\\n> \t\t--color-moved=zebra --no-renames HEAD >actual &&\n>\n> Our tests shouldn't be setting a wrong example.\n\nI mimicked the style of the previous test to hint that we do the same\nhere, just with the diff.external/--no-ext-diff noop on top. Not close\nenough, perhaps, but also hard to spot with 40+ lines between them.\n\nRené\n\n"},{"id":"497585","messageId":"fee1815c-80bb-42a4-97f3-d3f8e9b3a6ca@web.de","threadId":"61672","inReplyTo":"trinity-acbdb8fc-3dc3-4dca-890c-8bcb37405782-1719050465639@msvc-mesg-gmx004","subject":"[PATCH v2] diff: allow --color-moved with --no-ext-diff","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-06-24T19:15:45Z","receivedAt":"2024-06-24T19:15:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"We ignore the option --color-moved if an external diff program is\nconfigured, presumably because its overhead is unnecessary in that case.\nRespect the option if we don't actually use the external diff, though.\n\nReported-by: lolligerhans@gmx.de\nHelped-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\nChanges since v1:\n* use single word colors, leaving the background unchanged, as they are\n easier to read,\n* run git diff to generate the expected content instead of reusing the\n output of the previous test ...\n* ... and put the common arguments in a variable to make clear that we\n compare diff with and without a diff.external/--no-ext-diff dance,\n* use echo instead of false as the (unused) external diff command to\n avoid giving the wrong impression that diff.external is a boolean\n option we turn off here.\n\n diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n index a1478680b6..851cfe4f32 100755\n --- a/t/t4015-diff-whitespace.sh\n +++ b/t/t4015-diff-whitespace.sh\n @@ -1185,11 +1185,11 @@ test_expect_success 'detect moved code, complete file' '\n '\n\n test_expect_success '--color-moved with --no-ext-diff' '\n -\ttest_config color.diff.oldMoved \"normal red\" &&\n -\ttest_config color.diff.newMoved \"normal green\" &&\n -\tcp actual.raw expect &&\n -\tgit -c diff.external=false diff HEAD --no-ext-diff \\\n -\t\t--color-moved=zebra --color --no-renames >actual &&\n +\ttest_config color.diff.oldMoved \"yellow\" &&\n +\ttest_config color.diff.newMoved \"blue\" &&\n +\targs=\"--color --color-moved=zebra --no-renames HEAD\" &&\n +\tgit diff $args >expect &&\n +\tgit -c diff.external=echo diff --no-ext-diff $args >actual &&\n \ttest_cmp expect actual\n '\n\n\n diff.c | 3 ++-\n t/t4015-diff-whitespace.sh | 9 +++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6e432cb8fc..aa0fb77761 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4965,7 +4965,8 @@ void diff_setup_done(struct diff_options *options)\n \tif (options->flags.follow_renames)\n \t\tdiff_check_follow_pathspec(&options->pathspec, 1);\n\n-\tif (!options->use_color || external_diff())\n+\tif (!options->use_color ||\n+\t (options->flags.allow_external && external_diff()))\n \t\toptions->color_moved = 0;\n\n \tif (options->filter_not) {\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex b443626afd..851cfe4f32 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '\n \ttest_cmp expected actual\n '\n\n+test_expect_success '--color-moved with --no-ext-diff' '\n+\ttest_config color.diff.oldMoved \"yellow\" &&\n+\ttest_config color.diff.newMoved \"blue\" &&\n+\targs=\"--color --color-moved=zebra --no-renames HEAD\" &&\n+\tgit diff $args >expect &&\n+\tgit -c diff.external=echo diff --no-ext-diff $args >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'detect malicious moved code, inside file' '\n \ttest_config color.diff.oldMoved \"normal red\" &&\n \ttest_config color.diff.newMoved \"normal green\" &&\n--\n2.45.2\n"},{"id":"497586","messageId":"xmqqed8mawqp.fsf@gitster.g","threadId":"61672","inReplyTo":"fee1815c-80bb-42a4-97f3-d3f8e9b3a6ca@web.de","subject":"Re: [PATCH v2] diff: allow --color-moved with --no-ext-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-24T20:53:18Z","receivedAt":"2024-06-24T20:53:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> * use echo instead of false as the (unused) external diff command to\n> avoid giving the wrong impression that diff.external is a boolean\n> option we turn off here.\n\nI am fine with either \"/bin/false\" or \"echo\". I kind of found it a\nnice way to assert that --no-ext-diff is indeed in effect (if we did\nnot disable it, \"false\" would lead to \"whoa, your external diff\ndriver returned a failure\").\n\nThanks, will queue.\n\n> diff.c | 3 ++-\n> t/t4015-diff-whitespace.sh | 9 +++++++++\n> 2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 6e432cb8fc..aa0fb77761 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4965,7 +4965,8 @@ void diff_setup_done(struct diff_options *options)\n> \tif (options->flags.follow_renames)\n> \t\tdiff_check_follow_pathspec(&options->pathspec, 1);\n>\n> -\tif (!options->use_color || external_diff())\n> +\tif (!options->use_color ||\n> +\t (options->flags.allow_external && external_diff()))\n> \t\toptions->color_moved = 0;\n>\n> \tif (options->filter_not) {\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index b443626afd..851cfe4f32 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -1184,6 +1184,15 @@ test_expect_success 'detect moved code, complete file' '\n> \ttest_cmp expected actual\n> '\n>\n> +test_expect_success '--color-moved with --no-ext-diff' '\n> +\ttest_config color.diff.oldMoved \"yellow\" &&\n> +\ttest_config color.diff.newMoved \"blue\" &&\n> +\targs=\"--color --color-moved=zebra --no-renames HEAD\" &&\n> +\tgit diff $args >expect &&\n> +\tgit -c diff.external=echo diff --no-ext-diff $args >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'detect malicious moved code, inside file' '\n> \ttest_config color.diff.oldMoved \"normal red\" &&\n> \ttest_config color.diff.newMoved \"normal green\" &&\n> --\n> 2.45.2\n"},{"id":"497598","messageId":"xmqqmsn9am9p.fsf@gitster.g","threadId":"61672","inReplyTo":"48948980-ac98-452a-b2df-11cd81de56da@web.de","subject":"Re: [PATCH] diff: allow --color-moved with --no-ext-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-25T00:39:30Z","receivedAt":"2024-06-25T00:39:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> If we did this instead\n>>\n>> \tgit diff --color --color-moved=zebra --no-renames HEAD >expect &&\n>>\n>> it would make this a lot more self-contained.\n>\n> Oh, yes, good idea! We'd still rely on there being staged differences\n> that include moved lines,\n\nYeah, the primary point of this fix is that the output with\n\"--no-ext-diff\" and configured external diff driver, and without\neither, should be the same. And running the same command twice,\nonce with and once without these two tweaks, and comparing their\nresults is exactly what we want to do.\n"}]}