@matheusbsbs / matheus-bagliano / commits / eb84c8b6ce

git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`

The `--trust-exit-code` option for git-diff-tool(1) was introduced via 2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26). When set, it makes us return the exit code of the invoked diff tool when diffing multiple files. This patch didn't change the code path where `--dir-diff` was passed because we already returned the exit code of the 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Feb 20, 2024 at 11:08 UTC eb84c8b6cef15fcd048711afce46ce40dc5c43f5
3 files changed +67 -46
Documentation/git-difftool.txt
-1
@@ -105,7 +105,6 @@ instead. `--no-symlinks` is the default on Windows.
105 `merge.tool` until a tool is found.
106
107 --[no-]trust-exit-code::
108 - 'git-difftool' invokes a diff tool individually on each file.
108 Errors reported by the diff tool are ignored by default.
109 Use `--trust-exit-code` to make 'git-difftool' exit when an
110 invoked diff tool returns a non-zero exit code.
git-difftool--helper.sh
+13
@@ -91,6 +91,19 @@ then
91 # ignore the error from the above --- run_merge_tool
92 # will diagnose unusable tool by itself
93 run_merge_tool "$merge_tool" false
94 +
95 + status=$?
96 + if test $status -ge 126
97 + then
98 + # Command not found (127), not executable (126) or
99 + # exited via a signal (>= 128).
100 + exit $status
101 + fi
102 +
103 + if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
104 + then
105 + exit $status
106 + fi
107 else
108 # Launch the merge tool on each path provided by 'git diff'
109 while test $# -gt 6
t/t7800-difftool.sh
+54 -45
@@ -91,58 +91,67 @@ test_expect_success 'difftool forwards arguments to diff' '
91 rm for-diff
92 '
93
94 -test_expect_success 'difftool ignores exit code' '
95 - test_config difftool.error.cmd false &&
96 - git difftool -y -t error branch
97 -'
94 +for opt in '' '--dir-diff'
95 +do
96 + test_expect_success "difftool ${opt} ignores exit code" "
97 + test_config difftool.error.cmd false &&
98 + git difftool ${opt} -y -t error branch
99 + "
100
99 -test_expect_success 'difftool forwards exit code with --trust-exit-code' '
100 - test_config difftool.error.cmd false &&
101 - test_must_fail git difftool -y --trust-exit-code -t error branch
102 -'
101 + test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" "
102 + test_config difftool.error.cmd false &&
103 + test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
104 + "
105
104 -test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '
105 - test_config difftool.vimdiff.path false &&
106 - test_must_fail git difftool -y --trust-exit-code -t vimdiff branch
107 -'
106 + test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" "
107 + test_config difftool.vimdiff.path false &&
108 + test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
109 + "
110
109 -test_expect_success 'difftool honors difftool.trustExitCode = true' '
110 - test_config difftool.error.cmd false &&
111 - test_config difftool.trustExitCode true &&
112 - test_must_fail git difftool -y -t error branch
113 -'
111 + test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" "
112 + test_config difftool.error.cmd false &&
113 + test_config difftool.trustExitCode true &&
114 + test_must_fail git difftool ${opt} -y -t error branch
115 + "
116
115 -test_expect_success 'difftool honors difftool.trustExitCode = false' '
116 - test_config difftool.error.cmd false &&
117 - test_config difftool.trustExitCode false &&
118 - git difftool -y -t error branch
119 -'
117 + test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" "
118 + test_config difftool.error.cmd false &&
119 + test_config difftool.trustExitCode false &&
120 + git difftool ${opt} -y -t error branch
121 + "
122
121 -test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '
122 - test_config difftool.error.cmd false &&
123 - test_config difftool.trustExitCode true &&
124 - git difftool -y --no-trust-exit-code -t error branch
125 -'
123 + test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" "
124 + test_config difftool.error.cmd false &&
125 + test_config difftool.trustExitCode true &&
126 + git difftool ${opt} -y --no-trust-exit-code -t error branch
127 + "
128
127 -test_expect_success 'difftool stops on error with --trust-exit-code' '
128 - test_when_finished "rm -f for-diff .git/fail-right-file" &&
129 - test_when_finished "git reset -- for-diff" &&
130 - write_script .git/fail-right-file <<-\EOF &&
131 - echo failed
132 - exit 1
133 - EOF
134 - >for-diff &&
135 - git add for-diff &&
136 - test_must_fail git difftool -y --trust-exit-code \
137 - --extcmd .git/fail-right-file branch >actual &&
138 - test_line_count = 1 actual
139 -'
129 + test_expect_success "difftool ${opt} stops on error with --trust-exit-code" "
130 + test_when_finished 'rm -f for-diff .git/fail-right-file' &&
131 + test_when_finished 'git reset -- for-diff' &&
132 + write_script .git/fail-right-file <<-\EOF &&
133 + echo failed
134 + exit 1
135 + EOF
136 + >for-diff &&
137 + git add for-diff &&
138 + test_must_fail git difftool ${opt} -y --trust-exit-code \
139 + --extcmd .git/fail-right-file branch >actual &&
140 + test_line_count = 1 actual
141 + "
142
141 -test_expect_success 'difftool honors exit status if command not found' '
142 - test_config difftool.nonexistent.cmd i-dont-exist &&
143 - test_config difftool.trustExitCode false &&
144 - test_must_fail git difftool -y -t nonexistent branch
145 -'
143 + test_expect_success "difftool ${opt} honors exit status if command not found" "
144 + test_config difftool.nonexistent.cmd i-dont-exist &&
145 + test_config difftool.trustExitCode false &&
146 + if test "${opt}" = '--dir-diff'
147 + then
148 + expected_code=127
149 + else
150 + expected_code=128
151 + fi &&
152 + test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch
153 + "
154 +done
155
156 test_expect_success 'difftool honors --gui' '
157 difftool_test_setup &&