On Sun, Aug 20, 2023 at 08:52:36PM +0100, Romain Chossart wrote:
> I recently found out (the hard way :-) ) that running the following
> command prints an appropriate error on stderr but returns a zero exit
> code:
>
> > $ git diff --no-pager --exit-code
> > error: invalid option: --no-pager
> > $ echo $?
> > 0
>
> I would expect a non-zero exit code to be returned.
Yikes, I would expect that, too.
> Interestingly, running `git diff --no-pager --exit-code HEAD` shows a
> usage instead and does return a non-zero exit code as expected.
The difference has to do with where the parsing occurs. Because "git
diff" has so many sub-modes, without the "HEAD" it passes options down
to a helper function, where we first notice the invalid option and
return an error. So the real bug here is how we handle errors with
exit-code, and extends beyond just invalid options.
Fix is below. AFAICT the bug has been around forever (I didn't check
earlier than v1.6.6, which shows it). So it is not new in the upcoming
v2.42, and we can probably apply it to the 'maint' track after the
release.
Thanks for your report. And I'm impressed you managed to find such an
ancient bug. :)
-- >8 --
Subject: [PATCH] diff: handle negative status in diff_result_code()
Most programs which run a diff (porcelain git-diff, diff-tree, etc) run
their result through diff_result_code() before presenting it to the user
as a program return code. That result generally comes from a library
function like builtin_diff_files(), etc, and may be "-1" if the function
bailed with an error.
There are two problems here:
- if --exit-code is not in use, then we pass the code along as-is.
Even though Unix exit codes are unsigned, this usually works OK
because the integer representation, and "-1" ends up as "255". But
it's not something we should rely on, and we've tried to avoid it
elsewhere. E.g. in 5391e94813 (branch: remove negative exit code,
2022-03-29) and 246f0edec0 (execv_dashed_external: stop exiting with
negative code, 2017-01-06) and probably others.
- when --exit-code is in use, we ignore the incoming "status" variable
entirely, and rely on the "has_changes" field. But if we saw an
error, this field is almost certainly invalid, and means we'd likely
just say "no changes", which is completely bogus. Likewise for
the "--check" format.
So let's intercept the negative value here and return an appropriate
code before even considering --exit-code, etc. And while doing so, we
can swap out the negative value for a more normal exit code.
The test here uses an invalid option to trigger the error condition,
since that's easy and reliable. But this could also happen, for example,
if we failed to load the index (in which case we'd just report "no
changes" with --exit-code, which is very wrong).
Reported-by: Romain Chossart <romainchossart@xxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
I know we talked recently about not matching "128" explicitly in tests,
but it seems like the least-bad thing here, as explained in the comment.
diff.c | 11 +++++++++++
t/t4017-diff-retval.sh | 10 ++++++++++
2 files changed, 21 insertions(+)
diff --git a/diff.c b/diff.c
index ee3eb629e3..b8cd829b1f 100644
--- a/diff.c
+++ b/diff.c
@@ -6980,6 +6980,17 @@ int diff_result_code(struct diff_options *opt, int status)
diff_warn_rename_limit("diff.renameLimit",
opt->needed_rename_limit,
opt->degraded_cc_to_c);
+
+ /*
+ * A negative status indicates an internal error in the diff routines.
+ * We want to avoid codes like "1" or "2" here that have a documented
+ * meaning (and obviously not "0" for success / no diff). But we also
+ * want to avoid negative values, since the result is passed out via
+ * exit(). Convert them all to 128, which matches die().
+ */
+ if (status < 0)
+ return 128;
+
if (!opt->flags.exit_with_status &&
!(opt->output_format & DIFF_FORMAT_CHECKDIFF))
return status;
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index 5bc28ad9f0..0278364fcd 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -138,4 +138,14 @@ test_expect_success 'check honors conflict marker length' '
git reset --hard
'
+test_expect_success 'errors are not confused by --exit-code' '
+ # The exact code here is not important and not documented. But
+ # we do want to make sure that it is not a meaningful documented
+ # value like "1", nor the result of whatever platform-specific cruft
+ # might result from a negative value. The easiest way to check
+ # that is just to match what we know the code does now. But it would be
+ # OK to change this to match if the code changes.
+ test_expect_code 128 git diff --nonsense --exit-code
+'
+
test_done
--
2.42.0.rc2.418.g602d5859fc
