Message ID | 0fac668f8fc021af9f9c4df5134da59816307ccc.1708423309.git.ps@pks.im (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v2] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff` | expand |
On Tue, Feb 20, 2024 at 11:08:25AM +0100, Patrick Steinhardt wrote: > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index 6a36be1e63..96ae5d5880 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' ' > rm for-diff > ' > > -test_expect_success 'difftool ignores exit code' ' > - test_config difftool.error.cmd false && > - git difftool -y -t error branch > -' > +for opt in '' '--dir-diff' > +do > + test_expect_success "difftool ${opt} ignores exit code" " > + test_config difftool.error.cmd false && > + git difftool ${opt} -y -t error branch > + " > > -test_expect_success 'difftool forwards exit code with --trust-exit-code' ' > - test_config difftool.error.cmd false && > - test_must_fail git difftool -y --trust-exit-code -t error branch > -' > + test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" " > + test_config difftool.error.cmd false && > + test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch > + " > > -test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' ' > - test_config difftool.vimdiff.path false && > - test_must_fail git difftool -y --trust-exit-code -t vimdiff branch > -' > + test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" " > + test_config difftool.vimdiff.path false && > + test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch > + " > > -test_expect_success 'difftool honors difftool.trustExitCode = true' ' > - test_config difftool.error.cmd false && > - test_config difftool.trustExitCode true && > - test_must_fail git difftool -y -t error branch > -' > + test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" " > + test_config difftool.error.cmd false && > + test_config difftool.trustExitCode true && > + test_must_fail git difftool ${opt} -y -t error branch > + " > > -test_expect_success 'difftool honors difftool.trustExitCode = false' ' > - test_config difftool.error.cmd false && > - test_config difftool.trustExitCode false && > - git difftool -y -t error branch > -' > + test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" " > + test_config difftool.error.cmd false && > + test_config difftool.trustExitCode false && > + git difftool ${opt} -y -t error branch > + " > > -test_expect_success 'difftool ignores exit code with --no-trust-exit-code' ' > - test_config difftool.error.cmd false && > - test_config difftool.trustExitCode true && > - git difftool -y --no-trust-exit-code -t error branch > -' > + test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" " > + test_config difftool.error.cmd false && > + test_config difftool.trustExitCode true && > + git difftool ${opt} -y --no-trust-exit-code -t error branch > + " > > -test_expect_success 'difftool stops on error with --trust-exit-code' ' > - test_when_finished "rm -f for-diff .git/fail-right-file" && > - test_when_finished "git reset -- for-diff" && > - write_script .git/fail-right-file <<-\EOF && > - echo failed > - exit 1 > - EOF > - >for-diff && > - git add for-diff && > - test_must_fail git difftool -y --trust-exit-code \ > - --extcmd .git/fail-right-file branch >actual && > - test_line_count = 1 actual > -' > + test_expect_success "difftool ${opt} stops on error with --trust-exit-code" " > + test_when_finished 'rm -f for-diff .git/fail-right-file' && > + test_when_finished 'git reset -- for-diff' && > + write_script .git/fail-right-file <<-\EOF && > + echo failed > + exit 1 > + EOF > + >for-diff && > + git add for-diff && > + test_must_fail git difftool ${opt} -y --trust-exit-code \ > + --extcmd .git/fail-right-file branch >actual && > + test_line_count = 1 actual > + " > > -test_expect_success 'difftool honors exit status if command not found' ' > - test_config difftool.nonexistent.cmd i-dont-exist && > - test_config difftool.trustExitCode false && > - test_must_fail git difftool -y -t nonexistent branch > -' > + test_expect_success "difftool ${opt} honors exit status if command not found" " > + test_config difftool.nonexistent.cmd i-dont-exist && > + test_config difftool.trustExitCode false && > + if test "${opt}" = '--dir-diff' The quoting doesn't quite work here. When $opt is empty, this results in: expecting success of 7800.14 'difftool honors exit status if command not found': test_config difftool.nonexistent.cmd i-dont-exist && test_config difftool.trustExitCode false && if test = '--dir-diff' then expected_code=127 else expected_code=128 fi && test_expect_code ${expected_code} git difftool -y -t nonexistent branch + test_config difftool.nonexistent.cmd i-dont-exist + test_config difftool.trustExitCode false + test = --dir-diff ./t7800-difftool.sh: 14: test: =: unexpected operator > + then > + expected_code=127 > + else > + expected_code=128 > + fi && > + test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch > + " > +done > > test_expect_success 'difftool honors --gui' ' > difftool_test_setup && > -- > 2.44.0-rc1 >
SZEDER Gábor <szeder.dev@gmail.com> writes: >> -test_expect_success 'difftool honors exit status if command not found' ' >> - test_config difftool.nonexistent.cmd i-dont-exist && >> - test_config difftool.trustExitCode false && >> - test_must_fail git difftool -y -t nonexistent branch >> -' >> + test_expect_success "difftool ${opt} honors exit status if command not found" " >> + test_config difftool.nonexistent.cmd i-dont-exist && >> + test_config difftool.trustExitCode false && >> + if test "${opt}" = '--dir-diff' > > The quoting doesn't quite work here. Thanks for looking at them carefully. In general, when you want to interpolate an variable that exists outside test_expect_success, you should write it this way: for var in a "b c" do test_expect_success "message with $var interpolated" ' command and "$var" as its argument ' done The last parameter to test_expect_{success,failure} is eval'ed, so enclose it within a pair of single quotes, and let the eval to interpolate references to $variables at runtime (as opposed to when the parameters to test_expect_success are formulated) avoids a lot of surprises and headaches. Perhaps we should have something like the above as a hint in t/README?
On Fri, Mar 08, 2024 at 02:36:22PM -0800, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > >> -test_expect_success 'difftool honors exit status if command not found' ' > >> - test_config difftool.nonexistent.cmd i-dont-exist && > >> - test_config difftool.trustExitCode false && > >> - test_must_fail git difftool -y -t nonexistent branch > >> -' > >> + test_expect_success "difftool ${opt} honors exit status if command not found" " > >> + test_config difftool.nonexistent.cmd i-dont-exist && > >> + test_config difftool.trustExitCode false && > >> + if test "${opt}" = '--dir-diff' > > > > The quoting doesn't quite work here. > > Thanks for looking at them carefully. > > In general, when you want to interpolate an variable that exists > outside test_expect_success, you should write it this way: > > for var in a "b c" > do > test_expect_success "message with $var interpolated" ' > command and "$var" as its argument > ' > done > > The last parameter to test_expect_{success,failure} is eval'ed, so > enclose it within a pair of single quotes, and let the eval to > interpolate references to $variables at runtime (as opposed to when > the parameters to test_expect_success are formulated) avoids a lot > of surprises and headaches. > > Perhaps we should have something like the above as a hint in > t/README? Makes sense. I've sent a separate patch series addressing this issue in [1]. Thanks! Patrick [1]: https://lore.kernel.org/git/cover.1711028473.git.ps@pks.im/
diff tool unconditionally in that case. This was changed a month later via c41d3fedd8 (difftool--helper: add explicit exit statement, 2014-11-20), where an explicit `exit 0` was added to the end of git-difftool--helper.sh. While the stated intent of that commit was merely a cleanup, it had the consequence that we now to ignore the exit code of the diff tool when `--dir-diff` was set. This change in behaviour is thus very likely an unintended side effect of this patch. Now there are two ways to fix this: - We can either restore the original behaviour, which unconditionally returned the exit code of the diffing tool when `--dir-diff` is passed. - Or we can make the `--dir-diff` case respect the `--trust-exit-code` flag. The fact that we have been ignoring exit codes for 7 years by now makes me rather lean towards the latter option. Furthermore, respecting the flag in one case but not the other would needlessly make the user interface more complex. Fix the bug so that we also honor `--trust-exit-code` for dir diffs and adjust the documentation accordingly. Reported-by: Jean-Rémy Falleri <jr.falleri@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Range-diff against v1: 1: fd6cf7a85a ! 1: 0fac668f8f git-difftool--helper: honor `--trust-exit-code` with `--dir-diff` @@ git-difftool--helper.sh: then + exit $status + fi + -+ if test "$status" != 0 && -+ test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true ++ if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true + then + exit $status + fi Documentation/git-difftool.txt | 1 - git-difftool--helper.sh | 13 +++++ t/t7800-difftool.sh | 99 ++++++++++++++++++---------------- 3 files changed, 67 insertions(+), 46 deletions(-) diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt index c05f97aca9..a616f8b2e6 100644 --- a/Documentation/git-difftool.txt +++ b/Documentation/git-difftool.txt @@ -105,7 +105,6 @@ instead. `--no-symlinks` is the default on Windows. `merge.tool` until a tool is found. --[no-]trust-exit-code:: - 'git-difftool' invokes a diff tool individually on each file. Errors reported by the diff tool are ignored by default. Use `--trust-exit-code` to make 'git-difftool' exit when an invoked diff tool returns a non-zero exit code. diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh index e4e820e680..dd0c9a5b7f 100755 --- a/git-difftool--helper.sh +++ b/git-difftool--helper.sh @@ -91,6 +91,19 @@ then # ignore the error from the above --- run_merge_tool # will diagnose unusable tool by itself run_merge_tool "$merge_tool" false + + status=$? + if test $status -ge 126 + then + # Command not found (127), not executable (126) or + # exited via a signal (>= 128). + exit $status + fi + + if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true + then + exit $status + fi else # Launch the merge tool on each path provided by 'git diff' while test $# -gt 6 diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 6a36be1e63..96ae5d5880 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' ' rm for-diff ' -test_expect_success 'difftool ignores exit code' ' - test_config difftool.error.cmd false && - git difftool -y -t error branch -' +for opt in '' '--dir-diff' +do + test_expect_success "difftool ${opt} ignores exit code" " + test_config difftool.error.cmd false && + git difftool ${opt} -y -t error branch + " -test_expect_success 'difftool forwards exit code with --trust-exit-code' ' - test_config difftool.error.cmd false && - test_must_fail git difftool -y --trust-exit-code -t error branch -' + test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" " + test_config difftool.error.cmd false && + test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch + " -test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' ' - test_config difftool.vimdiff.path false && - test_must_fail git difftool -y --trust-exit-code -t vimdiff branch -' + test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" " + test_config difftool.vimdiff.path false && + test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch + " -test_expect_success 'difftool honors difftool.trustExitCode = true' ' - test_config difftool.error.cmd false && - test_config difftool.trustExitCode true && - test_must_fail git difftool -y -t error branch -' + test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" " + test_config difftool.error.cmd false && + test_config difftool.trustExitCode true && + test_must_fail git difftool ${opt} -y -t error branch + " -test_expect_success 'difftool honors difftool.trustExitCode = false' ' - test_config difftool.error.cmd false && - test_config difftool.trustExitCode false && - git difftool -y -t error branch -' + test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" " + test_config difftool.error.cmd false && + test_config difftool.trustExitCode false && + git difftool ${opt} -y -t error branch + " -test_expect_success 'difftool ignores exit code with --no-trust-exit-code' ' - test_config difftool.error.cmd false && - test_config difftool.trustExitCode true && - git difftool -y --no-trust-exit-code -t error branch -' + test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" " + test_config difftool.error.cmd false && + test_config difftool.trustExitCode true && + git difftool ${opt} -y --no-trust-exit-code -t error branch + " -test_expect_success 'difftool stops on error with --trust-exit-code' ' - test_when_finished "rm -f for-diff .git/fail-right-file" && - test_when_finished "git reset -- for-diff" && - write_script .git/fail-right-file <<-\EOF && - echo failed - exit 1 - EOF - >for-diff && - git add for-diff && - test_must_fail git difftool -y --trust-exit-code \ - --extcmd .git/fail-right-file branch >actual && - test_line_count = 1 actual -' + test_expect_success "difftool ${opt} stops on error with --trust-exit-code" " + test_when_finished 'rm -f for-diff .git/fail-right-file' && + test_when_finished 'git reset -- for-diff' && + write_script .git/fail-right-file <<-\EOF && + echo failed + exit 1 + EOF + >for-diff && + git add for-diff && + test_must_fail git difftool ${opt} -y --trust-exit-code \ + --extcmd .git/fail-right-file branch >actual && + test_line_count = 1 actual + " -test_expect_success 'difftool honors exit status if command not found' ' - test_config difftool.nonexistent.cmd i-dont-exist && - test_config difftool.trustExitCode false && - test_must_fail git difftool -y -t nonexistent branch -' + test_expect_success "difftool ${opt} honors exit status if command not found" " + test_config difftool.nonexistent.cmd i-dont-exist && + test_config difftool.trustExitCode false && + if test "${opt}" = '--dir-diff' + then + expected_code=127 + else + expected_code=128 + fi && + test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch + " +done test_expect_success 'difftool honors --gui' ' difftool_test_setup &&