{"thread":{"id":"60927","subject":"Git difftool: interaction between --dir-diff and --trust-exit-code","startedAt":"2024-02-15T11:09:50Z","lastAt":"2024-03-21T13:50:18Z","messageCount":10,"participants":["Jean-Rémy Falleri","Patrick Steinhardt","Junio C Hamano","David Aguilar","SZEDER Gábor"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"488723","messageId":"976C9BF2-CB82-429A-B9FA-6A14BCFFCA3D@gmail.com","threadId":"60927","inReplyTo":null,"subject":"Git difftool: interaction between --dir-diff and --trust-exit-code","fromName":"Jean-Rémy Falleri","fromEmail":"jr.falleri@gmail.com","sentAt":"2024-02-15T11:09:35Z","receivedAt":"2024-02-15T11:09:50Z","isPatch":false,"sender":{"key":"jr.falleri@gmail.com","avatar":null},"body":"Hi all and thanks for the amazing work you do on Git.\n\nIt seems that the —trust-exit-code option from git-difftool is not working when one use —dir-diff.\n\nAs an example I have set up the following configuration :\n\n[difftool \"false\"]\ncmd = false\n\nAnd when I launch git-difftool in normal mode with —trust-exit-code, it works fine:\n$ git difftool -y -t false --trust-exit-code HEAD HEAD~1\n$ echo $?\n> 128\n\nHowever the same command in dir-diff mode is not working :\n$ git difftool -t false -d --trust-exit-code HEAD HEAD~1\n$ echo $?\n> 0\n\nFrom what I read in git/git-difftool—helper.sh it seems to not forward the exit status when $GIT_DIFFTOOL_DIRDIFF is on.\n\nI believe there is nothing in the documentation about this interaction. Maybe this is intended but I find that this could be useful to have this option working on dir-diff mode too. For instance in my use case I would want to signal an error when I detect breaking changes with the breaking change detector we are working on, that is hooked as a difftool.\n\nBest regards!\n---\nJean-Rémy Falleri (http://www.labri.fr/~falleri)\n\n"},{"id":"488786","messageId":"c071e44c52171b9b19a04d91666be48d762d19bf.1708072576.git.ps@pks.im","threadId":"60927","inReplyTo":"976C9BF2-CB82-429A-B9FA-6A14BCFFCA3D@gmail.com","subject":"[PATCH] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-16T08:39:18Z","receivedAt":"2024-02-16T08:39:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--trust-exit-code` option for git-diff-tool(1) was introduced via\n2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).\nWhen set, it makes us return the exit code of the invoked diff tool when\ndiffing multiple files. This patch didn't change the code path where\n`--dir-diff` was passed because we already returned the exit code of the\ndiff tool unconditionally in that case.\n\nThis was changed a month later via c41d3fedd8 (difftool--helper: add\nexplicit exit statement, 2014-11-20), where an explicit `exit 0` was\nadded to the end of git-difftool--helper.sh. While the stated intent of\nthat commit was merely a cleanup, it had the consequence that we now\nto ignore the exit code of the diff tool when `--dir-diff` was set. This\nchange in behaviour is thus very likely an unintended side effect of\nthis patch.\n\nNow there are two ways to fix this:\n\n  - We can either restore the original behaviour, which unconditionally\n    returned the exit code of the diffing tool when `--dir-diff` is\n    passed.\n\n  - Or we can make the `--dir-diff` case respect the `--trust-exit-code`\n    flag.\n\nThe fact that we have been ignoring exit codes for 7 years by now makes\nme rather lean towards the latter option. Furthermore, respecting the\nflag in one case but not the other would needlessly make the user\ninterface more complex.\n\nFix the bug so that we also honor `--trust-exit-code` for dir diffs and\nadjust the documentation accordingly.\n\nReported-by: Jean-Rémy Falleri <jr.falleri@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/git-difftool.txt |  1 -\n git-difftool--helper.sh        | 14 +++++\n t/t7800-difftool.sh            | 99 ++++++++++++++++++----------------\n 3 files changed, 68 insertions(+), 46 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex c05f97aca9..a616f8b2e6 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -105,7 +105,6 @@ instead.  `--no-symlinks` is the default on Windows.\n \t`merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n-\t'git-difftool' invokes a diff tool individually on each file.\n \tErrors reported by the diff tool are ignored by default.\n \tUse `--trust-exit-code` to make 'git-difftool' exit when an\n \tinvoked diff tool returns a non-zero exit code.\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex e4e820e680..09d8542917 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -91,6 +91,20 @@ then\n \t# ignore the error from the above --- run_merge_tool\n \t# will diagnose unusable tool by itself\n \trun_merge_tool \"$merge_tool\" false\n+\n+\tstatus=$?\n+\tif test $status -ge 126\n+\tthen\n+\t\t# Command not found (127), not executable (126) or\n+\t\t# exited via a signal (>= 128).\n+\t\texit $status\n+\tfi\n+\n+\tif test \"$status\" != 0 &&\n+\t\ttest \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n+\tthen\n+\t\texit $status\n+\tfi\n else\n \t# Launch the merge tool on each path provided by 'git diff'\n \twhile test $# -gt 6\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 6a36be1e63..96ae5d5880 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' '\n \trm for-diff\n '\n \n-test_expect_success 'difftool ignores exit code' '\n-\ttest_config difftool.error.cmd false &&\n-\tgit difftool -y -t error branch\n-'\n+for opt in '' '--dir-diff'\n+do\n+\ttest_expect_success \"difftool ${opt} ignores exit code\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\tgit difftool ${opt} -y -t error branch\n+\t\"\n \n-test_expect_success 'difftool forwards exit code with --trust-exit-code' '\n-\ttest_config difftool.error.cmd false &&\n-\ttest_must_fail git difftool -y --trust-exit-code -t error branch\n-'\n+\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t error branch\n+\t\"\n \n-test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '\n-\ttest_config difftool.vimdiff.path false &&\n-\ttest_must_fail git difftool -y --trust-exit-code -t vimdiff branch\n-'\n+\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code for built-ins\" \"\n+\t\ttest_config difftool.vimdiff.path false &&\n+\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch\n+\t\"\n \n-test_expect_success 'difftool honors difftool.trustExitCode = true' '\n-\ttest_config difftool.error.cmd false &&\n-\ttest_config difftool.trustExitCode true &&\n-\ttest_must_fail git difftool -y -t error branch\n-'\n+\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = true\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\ttest_config difftool.trustExitCode true &&\n+\t\ttest_must_fail git difftool ${opt} -y -t error branch\n+\t\"\n \n-test_expect_success 'difftool honors difftool.trustExitCode = false' '\n-\ttest_config difftool.error.cmd false &&\n-\ttest_config difftool.trustExitCode false &&\n-\tgit difftool -y -t error branch\n-'\n+\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = false\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\ttest_config difftool.trustExitCode false &&\n+\t\tgit difftool ${opt} -y -t error branch\n+\t\"\n \n-test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '\n-\ttest_config difftool.error.cmd false &&\n-\ttest_config difftool.trustExitCode true &&\n-\tgit difftool -y --no-trust-exit-code -t error branch\n-'\n+\ttest_expect_success \"difftool ${opt} ignores exit code with --no-trust-exit-code\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\ttest_config difftool.trustExitCode true &&\n+\t\tgit difftool ${opt} -y --no-trust-exit-code -t error branch\n+\t\"\n \n-test_expect_success 'difftool stops on error with --trust-exit-code' '\n-\ttest_when_finished \"rm -f for-diff .git/fail-right-file\" &&\n-\ttest_when_finished \"git reset -- for-diff\" &&\n-\twrite_script .git/fail-right-file <<-\\EOF &&\n-\techo failed\n-\texit 1\n-\tEOF\n-\t>for-diff &&\n-\tgit add for-diff &&\n-\ttest_must_fail git difftool -y --trust-exit-code \\\n-\t\t--extcmd .git/fail-right-file branch >actual &&\n-\ttest_line_count = 1 actual\n-'\n+\ttest_expect_success \"difftool ${opt} stops on error with --trust-exit-code\" \"\n+\t\ttest_when_finished 'rm -f for-diff .git/fail-right-file' &&\n+\t\ttest_when_finished 'git reset -- for-diff' &&\n+\t\twrite_script .git/fail-right-file <<-\\EOF &&\n+\t\techo failed\n+\t\texit 1\n+\t\tEOF\n+\t\t>for-diff &&\n+\t\tgit add for-diff &&\n+\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code \\\n+\t\t\t--extcmd .git/fail-right-file branch >actual &&\n+\t\ttest_line_count = 1 actual\n+\t\"\n \n-test_expect_success 'difftool honors exit status if command not found' '\n-\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n-\ttest_config difftool.trustExitCode false &&\n-\ttest_must_fail git difftool -y -t nonexistent branch\n-'\n+\ttest_expect_success \"difftool ${opt} honors exit status if command not found\" \"\n+\t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n+\t\ttest_config difftool.trustExitCode false &&\n+\t\tif test \"${opt}\" = '--dir-diff'\n+\t\tthen\n+\t\t\texpected_code=127\n+\t\telse\n+\t\t\texpected_code=128\n+\t\tfi &&\n+\t\ttest_expect_code \\${expected_code} git difftool ${opt} -y -t nonexistent branch\n+\t\"\n+done\n \n test_expect_success 'difftool honors --gui' '\n \tdifftool_test_setup &&\n-- \n2.44.0-rc1\n\n"},{"id":"488801","messageId":"xmqqttm8i8hb.fsf@gitster.g","threadId":"60927","inReplyTo":"c071e44c52171b9b19a04d91666be48d762d19bf.1708072576.git.ps@pks.im","subject":"Re: [PATCH] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-16T18:12:32Z","receivedAt":"2024-02-16T18:12:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The `--trust-exit-code` option for git-diff-tool(1) was introduced via\n> 2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).\n> When set, it makes us return the exit code of the invoked diff tool when\n> diffing multiple files. This patch didn't change the code path where\n> `--dir-diff` was passed because we already returned the exit code of the\n> diff tool unconditionally in that case.\n>\n> This was changed a month later via c41d3fedd8 (difftool--helper: add\n> explicit exit statement, 2014-11-20), where an explicit `exit 0` was\n> added to the end of git-difftool--helper.sh. While the stated intent of\n> that commit was merely a cleanup, it had the consequence that we now\n> to ignore the exit code of the diff tool when `--dir-diff` was set. This\n> change in behaviour is thus very likely an unintended side effect of\n> this patch.\n>\n> Now there are two ways to fix this:\n>\n>   - We can either restore the original behaviour, which unconditionally\n>     returned the exit code of the diffing tool when `--dir-diff` is\n>     passed.\n>\n>   - Or we can make the `--dir-diff` case respect the `--trust-exit-code`\n>     flag.\n>\n> The fact that we have been ignoring exit codes for 7 years by now makes\n> me rather lean towards the latter option. Furthermore, respecting the\n> flag in one case but not the other would needlessly make the user\n> interface more complex.\n>\n> Fix the bug so that we also honor `--trust-exit-code` for dir diffs and\n> adjust the documentation accordingly.\n>\n> Reported-by: Jean-Rémy Falleri <jr.falleri@gmail.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n\nSounds sensible.\n\nThe last time David was on list seems to be in April 2023; just in \ncase let's CC him for an Ack (or something else).\n\n>  Documentation/git-difftool.txt |  1 -\n>  git-difftool--helper.sh        | 14 +++++\n>  t/t7800-difftool.sh            | 99 ++++++++++++++++++----------------\n>  3 files changed, 68 insertions(+), 46 deletions(-)\n>\n> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\n> index c05f97aca9..a616f8b2e6 100644\n> --- a/Documentation/git-difftool.txt\n> +++ b/Documentation/git-difftool.txt\n> @@ -105,7 +105,6 @@ instead.  `--no-symlinks` is the default on Windows.\n>  \t`merge.tool` until a tool is found.\n>  \n>  --[no-]trust-exit-code::\n> -\t'git-difftool' invokes a diff tool individually on each file.\n>  \tErrors reported by the diff tool are ignored by default.\n>  \tUse `--trust-exit-code` to make 'git-difftool' exit when an\n>  \tinvoked diff tool returns a non-zero exit code.\n> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n> index e4e820e680..09d8542917 100755\n> --- a/git-difftool--helper.sh\n> +++ b/git-difftool--helper.sh\n> @@ -91,6 +91,20 @@ then\n>  \t# ignore the error from the above --- run_merge_tool\n>  \t# will diagnose unusable tool by itself\n>  \trun_merge_tool \"$merge_tool\" false\n> +\n> +\tstatus=$?\n> +\tif test $status -ge 126\n> +\tthen\n> +\t\t# Command not found (127), not executable (126) or\n> +\t\t# exited via a signal (>= 128).\n> +\t\texit $status\n> +\tfi\n\nSo these errors spawning the tool backend are always reported,\nregardless of the trust-exit-code settings.  OK.\n\n> +\tif test \"$status\" != 0 &&\n> +\t\ttest \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n> +\tthen\n> +\t\texit $status\n> +\tfi\n\nI found this somehow harder to reason about than necessary.  Just\n\n\tif test \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n\tthen\n\t\texit $status\n\tfi\n\nwould have been a more straight-forward expression of what we want\nto happen here, i.e. \"if we are told to report the tool's exit\nstatus, we do so, regardless of what the exit status is\".\n\nNot that the construct in your patch is wrong---we will exit with 0\nat the end even when \"trust-exit-code\" thing is true and the tool\nreturned success.\n"},{"id":"488980","messageId":"ZdR3UlDf-Y_pokd0@tanuki","threadId":"60927","inReplyTo":"xmqqttm8i8hb.fsf@gitster.g","subject":"Re: [PATCH] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:56:34Z","receivedAt":"2024-02-20T09:56:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Feb 16, 2024 at 10:12:32AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n> > index e4e820e680..09d8542917 100755\n> > --- a/git-difftool--helper.sh\n> > +++ b/git-difftool--helper.sh\n> > @@ -91,6 +91,20 @@ then\n> >  \t# ignore the error from the above --- run_merge_tool\n> >  \t# will diagnose unusable tool by itself\n> >  \trun_merge_tool \"$merge_tool\" false\n> > +\n> > +\tstatus=$?\n> > +\tif test $status -ge 126\n> > +\tthen\n> > +\t\t# Command not found (127), not executable (126) or\n> > +\t\t# exited via a signal (>= 128).\n> > +\t\texit $status\n> > +\tfi\n> \n> So these errors spawning the tool backend are always reported,\n> regardless of the trust-exit-code settings.  OK.\n> \n> > +\tif test \"$status\" != 0 &&\n> > +\t\ttest \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n> > +\tthen\n> > +\t\texit $status\n> > +\tfi\n> \n> I found this somehow harder to reason about than necessary.  Just\n> \n> \tif test \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n> \tthen\n> \t\texit $status\n> \tfi\n> \n> would have been a more straight-forward expression of what we want\n> to happen here, i.e. \"if we are told to report the tool's exit\n> status, we do so, regardless of what the exit status is\".\n> \n> Not that the construct in your patch is wrong---we will exit with 0\n> at the end even when \"trust-exit-code\" thing is true and the tool\n> returned success.\n\nFair point indeed. Looks like I was a bit too lazy here by simply\ncopying over the construct from the non-dir-diff case. Over there we\nneed to special case the 0 exit code because we don't want to exit the\nloop in that case. But here it's completely unnecessary.\n\nWill adapt, thanks!\n\nPatrick\n"},{"id":"488982","messageId":"0fac668f8fc021af9f9c4df5134da59816307ccc.1708423309.git.ps@pks.im","threadId":"60927","inReplyTo":"976C9BF2-CB82-429A-B9FA-6A14BCFFCA3D@gmail.com","subject":"[PATCH v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T10:08:25Z","receivedAt":"2024-02-20T10:08:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--trust-exit-code` option for git-diff-tool(1) was introduced via\n2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).\nWhen set, it makes us return the exit code of the invoked diff tool when\ndiffing multiple files. This patch didn't change the code path where\n`--dir-diff` was passed because we already returned the exit code of the\ndiff tool unconditionally in that case.\n\nThis was changed a month later via c41d3fedd8 (difftool--helper: add\nexplicit exit statement, 2014-11-20), where an explicit `exit 0` was\nadded to the end of git-difftool--helper.sh. While the stated intent of\nthat commit was merely a cleanup, it had the consequence that we now\nto ignore the exit code of the diff tool when `--dir-diff` was set. This\nchange in behaviour is thus very likely an unintended side effect of\nthis patch.\n\nNow there are two ways to fix this:\n\n  - We can either restore the original behaviour, which unconditionally\n    returned the exit code of the diffing tool when `--dir-diff` is\n    passed.\n\n  - Or we can make the `--dir-diff` case respect the `--trust-exit-code`\n    flag.\n\nThe fact that we have been ignoring exit codes for 7 years by now makes\nme rather lean towards the latter option. Furthermore, respecting the\nflag in one case but not the other would needlessly make the user\ninterface more complex.\n\nFix the bug so that we also honor `--trust-exit-code` for dir diffs and\nadjust the documentation accordingly.\n\nReported-by: Jean-Rémy Falleri <jr.falleri@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nRange-diff against v1:\n1:  fd6cf7a85a ! 1:  0fac668f8f git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`\n    @@ git-difftool--helper.sh: then\n     +\t\texit $status\n     +\tfi\n     +\n    -+\tif test \"$status\" != 0 &&\n    -+\t\ttest \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n    ++\tif test \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n     +\tthen\n     +\t\texit $status\n     +\tfi\n\n Documentation/git-difftool.txt |  1 -\n git-difftool--helper.sh        | 13 +++++\n t/t7800-difftool.sh            | 99 ++++++++++++++++++----------------\n 3 files changed, 67 insertions(+), 46 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex c05f97aca9..a616f8b2e6 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -105,7 +105,6 @@ instead.  `--no-symlinks` is the default on Windows.\n \t`merge.tool` until a tool is found.\n \n --[no-]trust-exit-code::\n-\t'git-difftool' invokes a diff tool individually on each file.\n \tErrors reported by the diff tool are ignored by default.\n \tUse `--trust-exit-code` to make 'git-difftool' exit when an\n \tinvoked diff tool returns a non-zero exit code.\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex e4e820e680..dd0c9a5b7f 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -91,6 +91,19 @@ then\n \t# ignore the error from the above --- run_merge_tool\n \t# will diagnose unusable tool by itself\n \trun_merge_tool \"$merge_tool\" false\n+\n+\tstatus=$?\n+\tif test $status -ge 126\n+\tthen\n+\t\t# Command not found (127), not executable (126) or\n+\t\t# exited via a signal (>= 128).\n+\t\texit $status\n+\tfi\n+\n+\tif test \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n+\tthen\n+\t\texit $status\n+\tfi\n else\n \t# Launch the merge tool on each path provided by 'git diff'\n \twhile test $# -gt 6\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 6a36be1e63..96ae5d5880 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' '\n \trm for-diff\n '\n \n-test_expect_success 'difftool ignores exit code' '\n-\ttest_config difftool.error.cmd false &&\n-\tgit difftool -y -t error branch\n-'\n+for opt in '' '--dir-diff'\n+do\n+\ttest_expect_success \"difftool ${opt} ignores exit code\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\tgit difftool ${opt} -y -t error branch\n+\t\"\n \n-test_expect_success 'difftool forwards exit code with --trust-exit-code' '\n-\ttest_config difftool.error.cmd false &&\n-\ttest_must_fail git difftool -y --trust-exit-code -t error branch\n-'\n+\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t error branch\n+\t\"\n \n-test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '\n-\ttest_config difftool.vimdiff.path false &&\n-\ttest_must_fail git difftool -y --trust-exit-code -t vimdiff branch\n-'\n+\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code for built-ins\" \"\n+\t\ttest_config difftool.vimdiff.path false &&\n+\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch\n+\t\"\n \n-test_expect_success 'difftool honors difftool.trustExitCode = true' '\n-\ttest_config difftool.error.cmd false &&\n-\ttest_config difftool.trustExitCode true &&\n-\ttest_must_fail git difftool -y -t error branch\n-'\n+\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = true\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\ttest_config difftool.trustExitCode true &&\n+\t\ttest_must_fail git difftool ${opt} -y -t error branch\n+\t\"\n \n-test_expect_success 'difftool honors difftool.trustExitCode = false' '\n-\ttest_config difftool.error.cmd false &&\n-\ttest_config difftool.trustExitCode false &&\n-\tgit difftool -y -t error branch\n-'\n+\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = false\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\ttest_config difftool.trustExitCode false &&\n+\t\tgit difftool ${opt} -y -t error branch\n+\t\"\n \n-test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '\n-\ttest_config difftool.error.cmd false &&\n-\ttest_config difftool.trustExitCode true &&\n-\tgit difftool -y --no-trust-exit-code -t error branch\n-'\n+\ttest_expect_success \"difftool ${opt} ignores exit code with --no-trust-exit-code\" \"\n+\t\ttest_config difftool.error.cmd false &&\n+\t\ttest_config difftool.trustExitCode true &&\n+\t\tgit difftool ${opt} -y --no-trust-exit-code -t error branch\n+\t\"\n \n-test_expect_success 'difftool stops on error with --trust-exit-code' '\n-\ttest_when_finished \"rm -f for-diff .git/fail-right-file\" &&\n-\ttest_when_finished \"git reset -- for-diff\" &&\n-\twrite_script .git/fail-right-file <<-\\EOF &&\n-\techo failed\n-\texit 1\n-\tEOF\n-\t>for-diff &&\n-\tgit add for-diff &&\n-\ttest_must_fail git difftool -y --trust-exit-code \\\n-\t\t--extcmd .git/fail-right-file branch >actual &&\n-\ttest_line_count = 1 actual\n-'\n+\ttest_expect_success \"difftool ${opt} stops on error with --trust-exit-code\" \"\n+\t\ttest_when_finished 'rm -f for-diff .git/fail-right-file' &&\n+\t\ttest_when_finished 'git reset -- for-diff' &&\n+\t\twrite_script .git/fail-right-file <<-\\EOF &&\n+\t\techo failed\n+\t\texit 1\n+\t\tEOF\n+\t\t>for-diff &&\n+\t\tgit add for-diff &&\n+\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code \\\n+\t\t\t--extcmd .git/fail-right-file branch >actual &&\n+\t\ttest_line_count = 1 actual\n+\t\"\n \n-test_expect_success 'difftool honors exit status if command not found' '\n-\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n-\ttest_config difftool.trustExitCode false &&\n-\ttest_must_fail git difftool -y -t nonexistent branch\n-'\n+\ttest_expect_success \"difftool ${opt} honors exit status if command not found\" \"\n+\t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n+\t\ttest_config difftool.trustExitCode false &&\n+\t\tif test \"${opt}\" = '--dir-diff'\n+\t\tthen\n+\t\t\texpected_code=127\n+\t\telse\n+\t\t\texpected_code=128\n+\t\tfi &&\n+\t\ttest_expect_code \\${expected_code} git difftool ${opt} -y -t nonexistent branch\n+\t\"\n+done\n \n test_expect_success 'difftool honors --gui' '\n \tdifftool_test_setup &&\n-- \n2.44.0-rc1\n\n"},{"id":"489590","messageId":"CAJDDKr5+3jszG=psh=kUGDjNCeTDGPSS-qDuN=JAq-3ua=bNDg@mail.gmail.com","threadId":"60927","inReplyTo":"xmqqttm8i8hb.fsf@gitster.g","subject":"Re: [PATCH] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2024-02-28T01:52:38Z","receivedAt":"2024-02-28T01:53:16Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Feb 16, 2024 at 10:12 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> > The `--trust-exit-code` option for git-diff-tool(1) was introduced via\n> > 2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).\n> > When set, it makes us return the exit code of the invoked diff tool when\n> > diffing multiple files. This patch didn't change the code path where\n> > `--dir-diff` was passed because we already returned the exit code of the\n> > diff tool unconditionally in that case.\n> >\n> > This was changed a month later via c41d3fedd8 (difftool--helper: add\n> > explicit exit statement, 2014-11-20), where an explicit `exit 0` was\n> > added to the end of git-difftool--helper.sh. While the stated intent of\n> > that commit was merely a cleanup, it had the consequence that we now\n> > to ignore the exit code of the diff tool when `--dir-diff` was set. This\n> > change in behaviour is thus very likely an unintended side effect of\n> > this patch.\n> >\n> > Now there are two ways to fix this:\n> >\n> >   - We can either restore the original behaviour, which unconditionally\n> >     returned the exit code of the diffing tool when `--dir-diff` is\n> >     passed.\n> >\n> >   - Or we can make the `--dir-diff` case respect the `--trust-exit-code`\n> >     flag.\n> >\n> > The fact that we have been ignoring exit codes for 7 years by now makes\n> > me rather lean towards the latter option. Furthermore, respecting the\n> > flag in one case but not the other would needlessly make the user\n> > interface more complex.\n> >\n> > Fix the bug so that we also honor `--trust-exit-code` for dir diffs and\n> > adjust the documentation accordingly.\n> >\n> > Reported-by: Jean-Rémy Falleri <jr.falleri@gmail.com>\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n>\n> Sounds sensible.\n>\n> The last time David was on list seems to be in April 2023; just in\n> case let's CC him for an Ack (or something else).\n\nThanks! I'm still lurking around.\nI've been meaning to make some Git-adjacent announcements and\ncontributions soon..\n\nUntil then:\n\nAcked-by: David Aguilar <davvid@gmail.com>\n-- \nDavid\n"},{"id":"489591","messageId":"xmqqplwhmj0n.fsf@gitster.g","threadId":"60927","inReplyTo":"CAJDDKr5+3jszG=psh=kUGDjNCeTDGPSS-qDuN=JAq-3ua=bNDg@mail.gmail.com","subject":"Re: [PATCH] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-28T02:15:20Z","receivedAt":"2024-02-28T02:15:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n>> The last time David was on list seems to be in April 2023; just in\n>> case let's CC him for an Ack (or something else).\n>\n> Thanks! I'm still lurking around.\n> I've been meaning to make some Git-adjacent announcements and\n> contributions soon..\n>\n> Until then:\n>\n> Acked-by: David Aguilar <davvid@gmail.com>\n\nThanks.  \n\nIt always is nice to hear from old-timer project participants.\n\n\n"},{"id":"490274","messageId":"20240308221229.GA1908@szeder.dev","threadId":"60927","inReplyTo":"0fac668f8fc021af9f9c4df5134da59816307ccc.1708423309.git.ps@pks.im","subject":"Re: [PATCH v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-03-08T22:12:29Z","receivedAt":"2024-03-08T22:12:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Feb 20, 2024 at 11:08:25AM +0100, Patrick Steinhardt wrote:\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 6a36be1e63..96ae5d5880 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' '\n>  \trm for-diff\n>  '\n>  \n> -test_expect_success 'difftool ignores exit code' '\n> -\ttest_config difftool.error.cmd false &&\n> -\tgit difftool -y -t error branch\n> -'\n> +for opt in '' '--dir-diff'\n> +do\n> +\ttest_expect_success \"difftool ${opt} ignores exit code\" \"\n> +\t\ttest_config difftool.error.cmd false &&\n> +\t\tgit difftool ${opt} -y -t error branch\n> +\t\"\n>  \n> -test_expect_success 'difftool forwards exit code with --trust-exit-code' '\n> -\ttest_config difftool.error.cmd false &&\n> -\ttest_must_fail git difftool -y --trust-exit-code -t error branch\n> -'\n> +\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code\" \"\n> +\t\ttest_config difftool.error.cmd false &&\n> +\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t error branch\n> +\t\"\n>  \n> -test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '\n> -\ttest_config difftool.vimdiff.path false &&\n> -\ttest_must_fail git difftool -y --trust-exit-code -t vimdiff branch\n> -'\n> +\ttest_expect_success \"difftool ${opt} forwards exit code with --trust-exit-code for built-ins\" \"\n> +\t\ttest_config difftool.vimdiff.path false &&\n> +\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch\n> +\t\"\n>  \n> -test_expect_success 'difftool honors difftool.trustExitCode = true' '\n> -\ttest_config difftool.error.cmd false &&\n> -\ttest_config difftool.trustExitCode true &&\n> -\ttest_must_fail git difftool -y -t error branch\n> -'\n> +\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = true\" \"\n> +\t\ttest_config difftool.error.cmd false &&\n> +\t\ttest_config difftool.trustExitCode true &&\n> +\t\ttest_must_fail git difftool ${opt} -y -t error branch\n> +\t\"\n>  \n> -test_expect_success 'difftool honors difftool.trustExitCode = false' '\n> -\ttest_config difftool.error.cmd false &&\n> -\ttest_config difftool.trustExitCode false &&\n> -\tgit difftool -y -t error branch\n> -'\n> +\ttest_expect_success \"difftool ${opt} honors difftool.trustExitCode = false\" \"\n> +\t\ttest_config difftool.error.cmd false &&\n> +\t\ttest_config difftool.trustExitCode false &&\n> +\t\tgit difftool ${opt} -y -t error branch\n> +\t\"\n>  \n> -test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '\n> -\ttest_config difftool.error.cmd false &&\n> -\ttest_config difftool.trustExitCode true &&\n> -\tgit difftool -y --no-trust-exit-code -t error branch\n> -'\n> +\ttest_expect_success \"difftool ${opt} ignores exit code with --no-trust-exit-code\" \"\n> +\t\ttest_config difftool.error.cmd false &&\n> +\t\ttest_config difftool.trustExitCode true &&\n> +\t\tgit difftool ${opt} -y --no-trust-exit-code -t error branch\n> +\t\"\n>  \n> -test_expect_success 'difftool stops on error with --trust-exit-code' '\n> -\ttest_when_finished \"rm -f for-diff .git/fail-right-file\" &&\n> -\ttest_when_finished \"git reset -- for-diff\" &&\n> -\twrite_script .git/fail-right-file <<-\\EOF &&\n> -\techo failed\n> -\texit 1\n> -\tEOF\n> -\t>for-diff &&\n> -\tgit add for-diff &&\n> -\ttest_must_fail git difftool -y --trust-exit-code \\\n> -\t\t--extcmd .git/fail-right-file branch >actual &&\n> -\ttest_line_count = 1 actual\n> -'\n> +\ttest_expect_success \"difftool ${opt} stops on error with --trust-exit-code\" \"\n> +\t\ttest_when_finished 'rm -f for-diff .git/fail-right-file' &&\n> +\t\ttest_when_finished 'git reset -- for-diff' &&\n> +\t\twrite_script .git/fail-right-file <<-\\EOF &&\n> +\t\techo failed\n> +\t\texit 1\n> +\t\tEOF\n> +\t\t>for-diff &&\n> +\t\tgit add for-diff &&\n> +\t\ttest_must_fail git difftool ${opt} -y --trust-exit-code \\\n> +\t\t\t--extcmd .git/fail-right-file branch >actual &&\n> +\t\ttest_line_count = 1 actual\n> +\t\"\n>  \n> -test_expect_success 'difftool honors exit status if command not found' '\n> -\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n> -\ttest_config difftool.trustExitCode false &&\n> -\ttest_must_fail git difftool -y -t nonexistent branch\n> -'\n> +\ttest_expect_success \"difftool ${opt} honors exit status if command not found\" \"\n> +\t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n> +\t\ttest_config difftool.trustExitCode false &&\n> +\t\tif test \"${opt}\" = '--dir-diff'\n\nThe quoting doesn't quite work here.  When $opt is empty, this results\nin:\n\n  expecting success of 7800.14 'difftool  honors exit status if command not found':\n                  test_config difftool.nonexistent.cmd i-dont-exist &&\n                  test_config difftool.trustExitCode false &&\n                  if test  = '--dir-diff'\n                  then\n                          expected_code=127\n                  else\n                          expected_code=128\n                  fi &&\n                  test_expect_code ${expected_code} git difftool  -y -t nonexistent branch\n  \n  + test_config difftool.nonexistent.cmd i-dont-exist\n  + test_config difftool.trustExitCode false\n  + test = --dir-diff\n  ./t7800-difftool.sh: 14: test: =: unexpected operator\n\n\n> +\t\tthen\n> +\t\t\texpected_code=127\n> +\t\telse\n> +\t\t\texpected_code=128\n> +\t\tfi &&\n> +\t\ttest_expect_code \\${expected_code} git difftool ${opt} -y -t nonexistent branch\n> +\t\"\n> +done\n>  \n>  test_expect_success 'difftool honors --gui' '\n>  \tdifftool_test_setup &&\n> -- \n> 2.44.0-rc1\n> \n\n\n"},{"id":"490277","messageId":"xmqqcys4fj15.fsf@gitster.g","threadId":"60927","inReplyTo":"20240308221229.GA1908@szeder.dev","subject":"Re: [PATCH v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-08T22:36:22Z","receivedAt":"2024-03-08T22:36:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>> -test_expect_success 'difftool honors exit status if command not found' '\n>> -\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n>> -\ttest_config difftool.trustExitCode false &&\n>> -\ttest_must_fail git difftool -y -t nonexistent branch\n>> -'\n>> +\ttest_expect_success \"difftool ${opt} honors exit status if command not found\" \"\n>> +\t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n>> +\t\ttest_config difftool.trustExitCode false &&\n>> +\t\tif test \"${opt}\" = '--dir-diff'\n>\n> The quoting doesn't quite work here.\n\nThanks for looking at them carefully.\n\nIn general, when you want to interpolate an variable that exists\noutside test_expect_success, you should write it this way:\n\n\tfor var in a \"b c\"\n\tdo\n\t\ttest_expect_success \"message with $var interpolated\" '\n\t\t\tcommand and \"$var\" as its argument\n\t\t'\n\tdone\n\nThe last parameter to test_expect_{success,failure} is eval'ed, so \nenclose it within a pair of single quotes, and let the eval to\ninterpolate references to $variables at runtime (as opposed to when\nthe parameters to test_expect_success are formulated) avoids a lot\nof surprises and headaches.\n\nPerhaps we should have something like the above as a hint in\nt/README?\n"},{"id":"491146","messageId":"Zfw7FUtDcKBaA0-6@tanuki","threadId":"60927","inReplyTo":"xmqqcys4fj15.fsf@gitster.g","subject":"Re: [PATCH v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T13:50:13Z","receivedAt":"2024-03-21T13:50:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 08, 2024 at 02:36:22PM -0800, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> >> -test_expect_success 'difftool honors exit status if command not found' '\n> >> -\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n> >> -\ttest_config difftool.trustExitCode false &&\n> >> -\ttest_must_fail git difftool -y -t nonexistent branch\n> >> -'\n> >> +\ttest_expect_success \"difftool ${opt} honors exit status if command not found\" \"\n> >> +\t\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n> >> +\t\ttest_config difftool.trustExitCode false &&\n> >> +\t\tif test \"${opt}\" = '--dir-diff'\n> >\n> > The quoting doesn't quite work here.\n> \n> Thanks for looking at them carefully.\n> \n> In general, when you want to interpolate an variable that exists\n> outside test_expect_success, you should write it this way:\n> \n> \tfor var in a \"b c\"\n> \tdo\n> \t\ttest_expect_success \"message with $var interpolated\" '\n> \t\t\tcommand and \"$var\" as its argument\n> \t\t'\n> \tdone\n> \n> The last parameter to test_expect_{success,failure} is eval'ed, so \n> enclose it within a pair of single quotes, and let the eval to\n> interpolate references to $variables at runtime (as opposed to when\n> the parameters to test_expect_success are formulated) avoids a lot\n> of surprises and headaches.\n> \n> Perhaps we should have something like the above as a hint in\n> t/README?\n\nMakes sense. I've sent a separate patch series addressing this issue in\n[1].\n\nThanks!\n\nPatrick\n\n[1]: https://lore.kernel.org/git/cover.1711028473.git.ps@pks.im/\n"}]}