"git diff --no-pager --exit-code" errors out but returns zero exit code

"git diff --no-pager --exit-code" errors out but returns zero exit code

[Romain Chossart]

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.

Interestingly, running `git diff --no-pager --exit-code HEAD` shows a
usage instead and does return a non-zero exit code as expected.

Thanks,
-- 
Romain Chossart

[PATCH] diff: handle negative status in diff_result_code()

[Jeff King]

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@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
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

Re: [PATCH] diff: handle negative status in diff_result_code()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Thanks for your report. And I'm impressed you managed to find such an
> ancient bug. :)

Indeed.  Thanks, both.

> 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.

Inspecting some callers of diff_result_code() further finds
curiosities.  wt-status.c for example feeds results form
run_diff_index() and run_diff_files() to the function, neither of
which returns anything other than 0. They die or exit(128)
themselves, though, so they are OK without this fix.
builtin/describe.c is also in the same boat with its use of
run_diff_index().

But of course the presense of such codepaths does not invalidate the
fix in your patch.

> 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.

Good.  As you said, this is not an ungent regression fix, but I'd
prefer to see us look at all the current callers, as I wonder if
there are positive non-zero numbers coming from some callers, and
possibly design how this code should react to such "result" coming
in.

Again, thanks, both.

[PATCH] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()

[Junio C Hamano]

Many callers of run_diff_index() passed literal "1" for the option
flag word, which should better be spelled out as DIFF_INDEX_CACHED
for readablity.  Everybody else passes "0" that can stay as-is.

The other bit in the option flag word is DIFF_INDEX_MERGE_BASE, but
curiously there is only one caller that can pass it, which is "git
diff-index --merge-base" itself---no internal callers uses the
feature.

A bit tricky call to the function is in builtin/submodule--helper.c
where the .cached member in a private struct is set/reset as a plain
Boolean flag, which happens to be "1" and happens to match the value
of DIFF_INDEX_CACHED.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 add-interactive.c           | 2 +-
 builtin/am.c                | 4 ++--
 builtin/stash.c             | 2 +-
 builtin/submodule--helper.c | 2 +-
 diff-lib.c                  | 2 +-
 wt-status.c                 | 6 +++---
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git c/add-interactive.c w/add-interactive.c
index add9a1ad43..7fd00c5e25 100644
--- c/add-interactive.c
+++ w/add-interactive.c
@@ -569,7 +569,7 @@ static int get_modified_files(struct repository *r,
 			copy_pathspec(&rev.prune_data, ps);
 
 		if (s.mode == FROM_INDEX)
-			run_diff_index(&rev, 1);
+			run_diff_index(&rev, DIFF_INDEX_CACHED);
 		else {
 			rev.diffopt.flags.ignore_dirty_submodules = 1;
 			run_diff_files(&rev, 0);
diff --git c/builtin/am.c w/builtin/am.c
index 8bde034fae..202040b62e 100644
--- c/builtin/am.c
+++ w/builtin/am.c
@@ -1430,7 +1430,7 @@ static void write_index_patch(const struct am_state *state)
 	rev_info.diffopt.close_file = 1;
 	add_pending_object(&rev_info, &tree->object, "");
 	diff_setup_done(&rev_info.diffopt);
-	run_diff_index(&rev_info, 1);
+	run_diff_index(&rev_info, DIFF_INDEX_CACHED);
 	release_revisions(&rev_info);
 }
 
@@ -1593,7 +1593,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
 		rev_info.diffopt.filter |= diff_filter_bit('M');
 		add_pending_oid(&rev_info, "HEAD", &our_tree, 0);
 		diff_setup_done(&rev_info.diffopt);
-		run_diff_index(&rev_info, 1);
+		run_diff_index(&rev_info, DIFF_INDEX_CACHED);
 		release_revisions(&rev_info);
 	}
 
diff --git c/builtin/stash.c w/builtin/stash.c
index fe64cde9ce..fe5052f12f 100644
--- c/builtin/stash.c
+++ w/builtin/stash.c
@@ -1111,7 +1111,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)
 	add_head_to_pending(&rev);
 	diff_setup_done(&rev.diffopt);
 
-	result = run_diff_index(&rev, 1);
+	result = run_diff_index(&rev, DIFF_INDEX_CACHED);
 	if (diff_result_code(&rev.diffopt, result)) {
 		ret = 1;
 		goto done;
diff --git c/builtin/submodule--helper.c w/builtin/submodule--helper.c
index f6871efd95..125ea80d21 100644
--- c/builtin/submodule--helper.c
+++ w/builtin/submodule--helper.c
@@ -1141,7 +1141,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
 	}
 
 	if (diff_cmd == DIFF_INDEX)
-		run_diff_index(&rev, info->cached);
+		run_diff_index(&rev, info->cached ? DIFF_INDEX_CACHED : 0);
 	else
 		run_diff_files(&rev, 0);
 	prepare_submodule_summary(info, &list);
diff --git c/diff-lib.c w/diff-lib.c
index 6b0c6a7180..cfa3489111 100644
--- c/diff-lib.c
+++ w/diff-lib.c
@@ -682,7 +682,7 @@ int index_differs_from(struct repository *r,
 			rev.diffopt.flags.ignore_submodules = flags->ignore_submodules;
 	}
 	rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
-	run_diff_index(&rev, 1);
+	run_diff_index(&rev, DIFF_INDEX_CACHED);
 	has_changes = rev.diffopt.flags.has_changes;
 	release_revisions(&rev);
 	return (has_changes != 0);
diff --git c/wt-status.c w/wt-status.c
index 5b1378965c..bf8687b357 100644
--- c/wt-status.c
+++ w/wt-status.c
@@ -675,7 +675,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	rev.diffopt.flags.recursive = 1;
 
 	copy_pathspec(&rev.prune_data, &s->pathspec);
-	run_diff_index(&rev, 1);
+	run_diff_index(&rev, DIFF_INDEX_CACHED);
 	release_revisions(&rev);
 }
 
@@ -1156,7 +1156,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 		rev.diffopt.a_prefix = "c/";
 		rev.diffopt.b_prefix = "i/";
 	} /* else use prefix as per user config */
-	run_diff_index(&rev, 1);
+	run_diff_index(&rev, DIFF_INDEX_CACHED);
 	if (s->verbose > 1 &&
 	    wt_status_check_worktree_changes(s, &dirty_submodules)) {
 		status_printf_ln(s, c,
@@ -2614,7 +2614,7 @@ int has_uncommitted_changes(struct repository *r,
 	}
 
 	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_index(&rev_info, 1);
+	result = run_diff_index(&rev_info, DIFF_INDEX_CACHED);
 	result = diff_result_code(&rev_info.diffopt, result);
 	release_revisions(&rev_info);
 	return result;

Re: [PATCH] diff: handle negative status in diff_result_code()

[Jeff King]

On Mon, Aug 21, 2023 at 08:56:26AM -0700, Junio C Hamano wrote:

> >   - 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.
> 
> Inspecting some callers of diff_result_code() further finds
> curiosities.  wt-status.c for example feeds results form
> run_diff_index() and run_diff_files() to the function, neither of
> which returns anything other than 0. They die or exit(128)
> themselves, though, so they are OK without this fix.
> builtin/describe.c is also in the same boat with its use of
> run_diff_index().
> 
> But of course the presense of such codepaths does not invalidate the
> fix in your patch.

Yeah, I admit I did not dig too much into the callers, as it was obvious
that any negative values were currently being mis-handled, and I knew at
least one caller passed them in (the one in builtin/diff.c).

And in fact I think that is the only problematic caller. Every other
caller either passes the result of run_diff_*(), which always return 0,
or just directly pass zero themselves!

It is only builtin/diff.c that calls its local builtin_diff_blobs(), and
so on, and those may return errors. So one direction is something like:

  - convert those calls to die() more directly; this has the potential
    side effect of producing a better message for the case that started
    this thread by calling usage(builtin_diff_usage) instead of a short
    error

  - drop the now-useless return codes from those functions, along with
    the already-useless ones from run_diff_files(), etc. At this point
    everybody will just be passing a blind "0" to diff_result_code()

  - drop the "status" parameter to diff_result_code()

That would make the code simpler. It does feel a bit like going in the
opposite direction of recent "pass errors up the stack rather than
dying" libification efforts. I think that's OK for the builtin_* helpers
in diff.c, which are just serving the diff porcelain. But things like
run_diff_files(), while pretty big operations, are something we might
call as small part of another operation (like git-describe).

We are not making things worse there, in the sense that their return
codes are currently meaningless. But removing the code entirely feels
like a step in the wrong direction. I dunno. Maybe we should retain
their return codes and have the callers die() if they fail (which would
currently be dead-code, but future-proof us for later cleanups).

-Peff

Re: [PATCH] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()

[Jeff King]

On Mon, Aug 21, 2023 at 09:21:50AM -0700, Junio C Hamano wrote:

> Many callers of run_diff_index() passed literal "1" for the option
> flag word, which should better be spelled out as DIFF_INDEX_CACHED
> for readablity.  Everybody else passes "0" that can stay as-is.
> 
> The other bit in the option flag word is DIFF_INDEX_MERGE_BASE, but
> curiously there is only one caller that can pass it, which is "git
> diff-index --merge-base" itself---no internal callers uses the
> feature.
> 
> A bit tricky call to the function is in builtin/submodule--helper.c
> where the .cached member in a private struct is set/reset as a plain
> Boolean flag, which happens to be "1" and happens to match the value
> of DIFF_INDEX_CACHED.

Good catch. I guess this is conversion that should have been done in
4c3fe82ef1 (diff-lib: accept option flags in run_diff_index(),
2020-09-20). That commit did fix up some callers, but missed these ones.

While this is conceptually orthogonal to what I'm looking at in the
other part of the thread, the textual conflicts are really annoying. I'm
going to float this to the front of my series and build on top.

-Peff

Re: [PATCH] diff: handle negative status in diff_result_code()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That would make the code simpler. It does feel a bit like going in the
> opposite direction of recent "pass errors up the stack rather than
> dying" libification efforts. I think that's OK for the builtin_* helpers
> in diff.c, which are just serving the diff porcelain. But things like
> run_diff_files(), while pretty big operations, are something we might
> call as small part of another operation (like git-describe).

True, for things in diff-lib.c we likely would want to go in the
opposite "return an error to be handled by the caller" route.

[PATCH v2 0/7] cleaning up diff_result_code()

[Jeff King]

On Mon, Aug 21, 2023 at 11:39:36AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > That would make the code simpler. It does feel a bit like going in the
> > opposite direction of recent "pass errors up the stack rather than
> > dying" libification efforts. I think that's OK for the builtin_* helpers
> > in diff.c, which are just serving the diff porcelain. But things like
> > run_diff_files(), while pretty big operations, are something we might
> > call as small part of another operation (like git-describe).
> 
> True, for things in diff-lib.c we likely would want to go in the
> opposite "return an error to be handled by the caller" route.

After poking at it a bit, I think it actually is OK to drop the return
codes here. There are some immediate benefits, and I'm not sure it
actually hampers libification that much; see patch 5 for my reasoning.

So here's what I came up with.

  [1/7]: diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()

    Your patch verbatim, since there's tons of textual conflicts
    otherwise.

  [2/7]: diff-files: avoid negative exit value

    A bonus fix that I noticed. It's the same problem as found
    elsewhere, but using a different code path, so it seemed easiest to
    fix on its own.

  [3/7]: diff: show usage for unknown builtin_diff_files() options

    This directly fixes the bug found by Romain (but in a different way
    than v1).

  [4/7]: diff: die when failing to read index in git-diff builtin

    Obvious cleanups.

  [5/7]: diff: drop useless return from run_diff_{files,index} functions

    Possibly controversial cleanups. ;)

  [6/7]: diff: drop useless return values in git-diff helpers
  [7/7]: diff: drop useless "status" parameter from diff_result_code()

    And then these are the payoff cleanups enabled by patch 5.

 add-interactive.c           |  2 +-
 builtin/add.c               |  3 +-
 builtin/am.c                |  4 +-
 builtin/describe.c          |  6 +--
 builtin/diff-files.c        | 12 ++----
 builtin/diff-index.c        |  4 +-
 builtin/diff-tree.c         |  2 +-
 builtin/diff.c              | 79 +++++++++++++++++--------------------
 builtin/log.c               |  2 +-
 builtin/stash.c             | 16 +++-----
 builtin/submodule--helper.c |  7 ++--
 diff-lib.c                  |  8 ++--
 diff-no-index.c             |  2 +-
 diff.c                      |  6 +--
 diff.h                      |  6 +--
 t/t4017-diff-retval.sh      |  5 +++
 wt-status.c                 | 12 +++---
 17 files changed, 81 insertions(+), 95 deletions(-)

No range-diff since it's effectively a brand-new series.

BTW, I know you were looking at --exit-code bugs recently in another
series. But I think this should all be orthogonal (both semantically
and textually). I'll try to give another review on that series, as well.

-Peff

[PATCH v2 1/7] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()

[Jeff King]

From: Junio C Hamano <gitster@pobox.com>

Many callers of run_diff_index() passed literal "1" for the option
flag word, which should better be spelled out as DIFF_INDEX_CACHED
for readablity.  Everybody else passes "0" that can stay as-is.

The other bit in the option flag word is DIFF_INDEX_MERGE_BASE, but
curiously there is only one caller that can pass it, which is "git
diff-index --merge-base" itself---no internal callers uses the
feature.

A bit tricky call to the function is in builtin/submodule--helper.c
where the .cached member in a private struct is set/reset as a plain
Boolean flag, which happens to be "1" and happens to match the value
of DIFF_INDEX_CACHED.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 add-interactive.c           | 2 +-
 builtin/am.c                | 4 ++--
 builtin/stash.c             | 2 +-
 builtin/submodule--helper.c | 2 +-
 diff-lib.c                  | 2 +-
 wt-status.c                 | 6 +++---
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index add9a1ad43..7fd00c5e25 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -569,7 +569,7 @@ static int get_modified_files(struct repository *r,
 			copy_pathspec(&rev.prune_data, ps);
 
 		if (s.mode == FROM_INDEX)
-			run_diff_index(&rev, 1);
+			run_diff_index(&rev, DIFF_INDEX_CACHED);
 		else {
 			rev.diffopt.flags.ignore_dirty_submodules = 1;
 			run_diff_files(&rev, 0);
diff --git a/builtin/am.c b/builtin/am.c
index 8bde034fae..202040b62e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1430,7 +1430,7 @@ static void write_index_patch(const struct am_state *state)
 	rev_info.diffopt.close_file = 1;
 	add_pending_object(&rev_info, &tree->object, "");
 	diff_setup_done(&rev_info.diffopt);
-	run_diff_index(&rev_info, 1);
+	run_diff_index(&rev_info, DIFF_INDEX_CACHED);
 	release_revisions(&rev_info);
 }
 
@@ -1593,7 +1593,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
 		rev_info.diffopt.filter |= diff_filter_bit('M');
 		add_pending_oid(&rev_info, "HEAD", &our_tree, 0);
 		diff_setup_done(&rev_info.diffopt);
-		run_diff_index(&rev_info, 1);
+		run_diff_index(&rev_info, DIFF_INDEX_CACHED);
 		release_revisions(&rev_info);
 	}
 
diff --git a/builtin/stash.c b/builtin/stash.c
index fe64cde9ce..fe5052f12f 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1111,7 +1111,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)
 	add_head_to_pending(&rev);
 	diff_setup_done(&rev.diffopt);
 
-	result = run_diff_index(&rev, 1);
+	result = run_diff_index(&rev, DIFF_INDEX_CACHED);
 	if (diff_result_code(&rev.diffopt, result)) {
 		ret = 1;
 		goto done;
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f6871efd95..125ea80d21 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1141,7 +1141,7 @@ static int compute_summary_module_list(struct object_id *head_oid,
 	}
 
 	if (diff_cmd == DIFF_INDEX)
-		run_diff_index(&rev, info->cached);
+		run_diff_index(&rev, info->cached ? DIFF_INDEX_CACHED : 0);
 	else
 		run_diff_files(&rev, 0);
 	prepare_submodule_summary(info, &list);
diff --git a/diff-lib.c b/diff-lib.c
index 6b0c6a7180..cfa3489111 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -682,7 +682,7 @@ int index_differs_from(struct repository *r,
 			rev.diffopt.flags.ignore_submodules = flags->ignore_submodules;
 	}
 	rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
-	run_diff_index(&rev, 1);
+	run_diff_index(&rev, DIFF_INDEX_CACHED);
 	has_changes = rev.diffopt.flags.has_changes;
 	release_revisions(&rev);
 	return (has_changes != 0);
diff --git a/wt-status.c b/wt-status.c
index 5b1378965c..bf8687b357 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -675,7 +675,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	rev.diffopt.flags.recursive = 1;
 
 	copy_pathspec(&rev.prune_data, &s->pathspec);
-	run_diff_index(&rev, 1);
+	run_diff_index(&rev, DIFF_INDEX_CACHED);
 	release_revisions(&rev);
 }
 
@@ -1156,7 +1156,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 		rev.diffopt.a_prefix = "c/";
 		rev.diffopt.b_prefix = "i/";
 	} /* else use prefix as per user config */
-	run_diff_index(&rev, 1);
+	run_diff_index(&rev, DIFF_INDEX_CACHED);
 	if (s->verbose > 1 &&
 	    wt_status_check_worktree_changes(s, &dirty_submodules)) {
 		status_printf_ln(s, c,
@@ -2614,7 +2614,7 @@ int has_uncommitted_changes(struct repository *r,
 	}
 
 	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_index(&rev_info, 1);
+	result = run_diff_index(&rev_info, DIFF_INDEX_CACHED);
 	result = diff_result_code(&rev_info.diffopt, result);
 	release_revisions(&rev_info);
 	return result;
-- 
2.42.0.rc2.423.g967ecb4f2b

[PATCH v2 2/7] diff-files: avoid negative exit value

[Jeff King]

If loading the index fails, we print an error and then return "-1" from
the function. But since this is a builtin, we end up with exit(-1),
which produces odd results since program exit codes are unsigned.
Because of integer conversion, it usually becomes 255, which is at least
still an error, but values above 128 are usually interpreted as signal
death.

Since we know the program is exiting immediately, we can just replace
the error return with a die().

Signed-off-by: Jeff King <peff@peff.net>
---
I'm actually not sure this can be triggered in practice; see patch 4 for
more discussion. Regardless, it seems like a strict improvement.

 builtin/diff-files.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 50330b8dd2..2e3e948583 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -80,14 +80,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	    (rev.diffopt.output_format & DIFF_FORMAT_PATCH))
 		diff_merges_set_dense_combined_if_unset(&rev);
 
-	if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0) {
-		perror("repo_read_index_preload");
-		result = -1;
-		goto cleanup;
-	}
+	if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)
+		die_errno("repo_read_index_preload");
 	result = run_diff_files(&rev, options);
 	result = diff_result_code(&rev.diffopt, result);
-cleanup:
 	release_revisions(&rev);
 	return result;
 }
-- 
2.42.0.rc2.423.g967ecb4f2b

[PATCH v2 3/7] diff: show usage for unknown builtin_diff_files() options

[Jeff King]

The git-diff command has many modes (comparing worktree to index, index
to HEAD, individual blobs, etc). As a result, it dispatches to many
helper functions and cannot completely parse its options until we're in
those helper functions.

Most of them, when seeing an unknown option, exit immediately by calling
usage(). But builtin_diff_files(), which is the default if no revision
or blob arguments are given, instead prints an error() and returns -1.

One obvious shortcoming here is that the user doesn't get to see the
usual usage message. But there's a much more important bug: the -1
return is fed to diff_result_code(), which is not ready to handle it.
By default, it passes the code along as an exit code. We try to avoid
negative exit codes because they get converted to unsigned values, but
it should at least consistently show up as non-zero (i.e., a failure).

But much worse is that when --exit-code is in effect, diff_result_code()
will _ignore_ the status passed in by the caller, and instead only
report on whether the diff found changes. It didn't, of course, because
we never ran the diff, and the program unexpectedly exits with success!

We can fix this bug by just calling usage(), like the other helpers do.
Another option would of course be to teach diff_result_code() to handle
this value. But as we'll see in the next few patches, it can be cleaned
up even further. Let's just fix this bug directly to start with.

Reported-by: Romain Chossart <romainchossart@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/diff.c         | 6 ++++--
 t/t4017-diff-retval.sh | 5 +++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index b19530c996..d0e187ec18 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -269,8 +269,10 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
 			options |= DIFF_SILENT_ON_REMOVED;
 		else if (!strcmp(argv[1], "-h"))
 			usage(builtin_diff_usage);
-		else
-			return error(_("invalid option: %s"), argv[1]);
+		else {
+			error(_("invalid option: %s"), argv[1]);
+			usage(builtin_diff_usage);
+		}
 		argv++; argc--;
 	}
 
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index 5bc28ad9f0..f439f469bd 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -138,4 +138,9 @@ test_expect_success 'check honors conflict marker length' '
 	git reset --hard
 '
 
+test_expect_success 'option errors are not confused by --exit-code' '
+	test_must_fail git diff --exit-code --nonsense 2>err &&
+	grep '^usage:' err
+'
+
 test_done
-- 
2.42.0.rc2.423.g967ecb4f2b

[PATCH v2 4/7] diff: die when failing to read index in git-diff builtin

[Jeff King]

When the git-diff program fails to read the index in its diff-files or
diff-index helper functions, it propagates the error up the stack. This
eventually lands in diff_result_code(), which does not handle it well
(as discussed in the previous patch).

Since the only sensible thing here is to exit with an error code (and
what we were expecting the propagated error code to cause), let's just
do that directly.

There's no test here, as I'm not even sure this case can be triggered.
The index-reading functions tend to die() themselves when encountering
any errors, and the return value is just the number of entries in the
file (and so always 0 or positive). But let's err on the conservative
side and keep checking the return value. It may be worth digging into as
a separate topic (though index-reading is low-level enough that we
probably want to eventually teach it to propagate errors anyway for
lib-ification purposes, at which point this code would already be doing
the right thing).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/diff.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index d0e187ec18..005f415d36 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -163,12 +163,10 @@ static int builtin_diff_index(struct rev_info *revs,
 		setup_work_tree();
 		if (repo_read_index_preload(the_repository,
 					    &revs->diffopt.pathspec, 0) < 0) {
-			perror("repo_read_index_preload");
-			return -1;
+			die_errno("repo_read_index_preload");
 		}
 	} else if (repo_read_index(the_repository) < 0) {
-		perror("repo_read_cache");
-		return -1;
+		die_errno("repo_read_cache");
 	}
 	return run_diff_index(revs, option);
 }
@@ -289,8 +287,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
 	setup_work_tree();
 	if (repo_read_index_preload(the_repository, &revs->diffopt.pathspec,
 				    0) < 0) {
-		perror("repo_read_index_preload");
-		return -1;
+		die_errno("repo_read_index_preload");
 	}
 	return run_diff_files(revs, options);
 }
-- 
2.42.0.rc2.423.g967ecb4f2b

[PATCH v2 5/7] diff: drop useless return from run_diff_{files,index} functions

[Jeff King]

Neither of these functions ever returns a value other than zero.
Instead, they expect unrecoverable errors to exit immediately, and
things like "--exit-code" are stored inside the diff_options struct to
be handled later via diff_result_code().

Some callers do check the return values, but many don't bother. Let's
drop the useless return values, which are misleading callers about how
the functions work. This could be seen as a step in the wrong direction,
as we might want to eventually "lib-ify" these to more cleanly return
errors up the stack, in which case we'd have to add the return values
back in. But there are some benefits to doing this now:

  1. In the current code, somebody could accidentally add a "return -1"
     to one of the functions, which would be erroneously ignored by many
     callers. By removing the return code, the compiler can notice the
     mismatch and force the developer to decide what to do.

     Obviously the other option here is that we could start consistently
     checking the error code in every caller. But it would be dead code,
     and we wouldn't get any compile-time help in catching new cases.

  2. It communicates the situation to callers, who may want to choose a
     different function. These functions are really thin wrappers for
     doing git-diff-files and git-diff-index within the process. But
     callers who care about recovering from an error here are probably
     better off using the underlying library functions, many of
     which do return errors.

If somebody eventually wants to teach these functions to propagate
errors, they'll have to switch back to returning a value, effectively
reverting this patch. But at least then they will be starting with a
level playing field: they know that they will need to inspect each
caller to see how it should handle the error.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/add.c               |  3 +--
 builtin/describe.c          |  6 +++---
 builtin/diff-files.c        |  4 ++--
 builtin/diff-index.c        |  4 ++--
 builtin/diff.c              |  6 ++++--
 builtin/stash.c             | 14 +++++---------
 builtin/submodule--helper.c |  5 ++---
 diff-lib.c                  |  6 ++----
 diff.h                      |  4 ++--
 wt-status.c                 |  8 ++++----
 10 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 4b0dd798df..12c5aa6d1f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -194,8 +194,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	out = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
 	rev.diffopt.file = xfdopen(out, "w");
 	rev.diffopt.close_file = 1;
-	if (run_diff_files(&rev, 0))
-		die(_("Could not write patch"));
+	run_diff_files(&rev, 0);
 
 	if (launch_editor(file, NULL, NULL))
 		die(_("editing patch failed"));
diff --git a/builtin/describe.c b/builtin/describe.c
index b28a4a1f82..8cdc25b748 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -668,7 +668,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			struct lock_file index_lock = LOCK_INIT;
 			struct rev_info revs;
 			struct strvec args = STRVEC_INIT;
-			int fd, result;
+			int fd;
 
 			setup_work_tree();
 			prepare_repo_settings(the_repository);
@@ -685,9 +685,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			strvec_pushv(&args, diff_index_args);
 			if (setup_revisions(args.nr, args.v, &revs, NULL) != 1)
 				BUG("malformed internal diff-index command line");
-			result = run_diff_index(&revs, 0);
+			run_diff_index(&revs, 0);
 
-			if (!diff_result_code(&revs.diffopt, result))
+			if (!diff_result_code(&revs.diffopt, 0))
 				suffix = NULL;
 			else
 				suffix = dirty;
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 2e3e948583..04070607b1 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -82,8 +82,8 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 
 	if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)
 		die_errno("repo_read_index_preload");
-	result = run_diff_files(&rev, options);
-	result = diff_result_code(&rev.diffopt, result);
+	run_diff_files(&rev, options);
+	result = diff_result_code(&rev.diffopt, 0);
 	release_revisions(&rev);
 	return result;
 }
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 9db7139b83..2c6a179832 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -72,8 +72,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		perror("repo_read_index");
 		return -1;
 	}
-	result = run_diff_index(&rev, option);
-	result = diff_result_code(&rev.diffopt, result);
+	run_diff_index(&rev, option);
+	result = diff_result_code(&rev.diffopt, 0);
 	release_revisions(&rev);
 	return result;
 }
diff --git a/builtin/diff.c b/builtin/diff.c
index 005f415d36..e1f7647c84 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -168,7 +168,8 @@ static int builtin_diff_index(struct rev_info *revs,
 	} else if (repo_read_index(the_repository) < 0) {
 		die_errno("repo_read_cache");
 	}
-	return run_diff_index(revs, option);
+	run_diff_index(revs, option);
+	return 0;
 }
 
 static int builtin_diff_tree(struct rev_info *revs,
@@ -289,7 +290,8 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
 				    0) < 0) {
 		die_errno("repo_read_index_preload");
 	}
-	return run_diff_files(revs, options);
+	run_diff_files(revs, options);
+	return 0;
 }
 
 struct symdiff {
diff --git a/builtin/stash.c b/builtin/stash.c
index fe5052f12f..e799b660f0 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1089,7 +1089,6 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,
  */
 static int check_changes_tracked_files(const struct pathspec *ps)
 {
-	int result;
 	struct rev_info rev;
 	struct object_id dummy;
 	int ret = 0;
@@ -1111,14 +1110,14 @@ static int check_changes_tracked_files(const struct pathspec *ps)
 	add_head_to_pending(&rev);
 	diff_setup_done(&rev.diffopt);
 
-	result = run_diff_index(&rev, DIFF_INDEX_CACHED);
-	if (diff_result_code(&rev.diffopt, result)) {
+	run_diff_index(&rev, DIFF_INDEX_CACHED);
+	if (diff_result_code(&rev.diffopt, 0)) {
 		ret = 1;
 		goto done;
 	}
 
-	result = run_diff_files(&rev, 0);
-	if (diff_result_code(&rev.diffopt, result)) {
+	run_diff_files(&rev, 0);
+	if (diff_result_code(&rev.diffopt, 0)) {
 		ret = 1;
 		goto done;
 	}
@@ -1309,10 +1308,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 
 	add_pending_object(&rev, parse_object(the_repository, &info->b_commit),
 			   "");
-	if (run_diff_index(&rev, 0)) {
-		ret = -1;
-		goto done;
-	}
+	run_diff_index(&rev, 0);
 
 	cp_upd_index.git_cmd = 1;
 	strvec_pushl(&cp_upd_index.args, "update-index",
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 125ea80d21..3764ed1f9c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -629,7 +629,6 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	char *displaypath;
 	struct strvec diff_files_args = STRVEC_INIT;
 	struct rev_info rev = REV_INFO_INIT;
-	int diff_files_result;
 	struct strbuf buf = STRBUF_INIT;
 	const char *git_dir;
 	struct setup_revision_opt opt = {
@@ -669,9 +668,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	repo_init_revisions(the_repository, &rev, NULL);
 	rev.abbrev = 0;
 	setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);
-	diff_files_result = run_diff_files(&rev, 0);
+	run_diff_files(&rev, 0);
 
-	if (!diff_result_code(&rev.diffopt, diff_files_result)) {
+	if (!diff_result_code(&rev.diffopt, 0)) {
 		print_status(flags, ' ', path, ce_oid,
 			     displaypath);
 	} else if (!(flags & OPT_CACHED)) {
diff --git a/diff-lib.c b/diff-lib.c
index cfa3489111..d8aa777a73 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -96,7 +96,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 	return changed;
 }
 
-int run_diff_files(struct rev_info *revs, unsigned int option)
+void run_diff_files(struct rev_info *revs, unsigned int option)
 {
 	int entries, i;
 	int diff_unmerged_stage = revs->max_count;
@@ -272,7 +272,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 	trace_performance_since(start, "diff-files");
-	return 0;
 }
 
 /*
@@ -606,7 +605,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
 	free_commit_list(merge_bases);
 }
 
-int run_diff_index(struct rev_info *revs, unsigned int option)
+void run_diff_index(struct rev_info *revs, unsigned int option)
 {
 	struct object_array_entry *ent;
 	int cached = !!(option & DIFF_INDEX_CACHED);
@@ -640,7 +639,6 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 	trace_performance_leave("diff-index");
-	return 0;
 }
 
 int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)
diff --git a/diff.h b/diff.h
index 260c454155..528f00d5e1 100644
--- a/diff.h
+++ b/diff.h
@@ -637,11 +637,11 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
 #define DIFF_SILENT_ON_REMOVED 01
 /* report racily-clean paths as modified */
 #define DIFF_RACY_IS_MODIFIED 02
-int run_diff_files(struct rev_info *revs, unsigned int option);
+void run_diff_files(struct rev_info *revs, unsigned int option);
 
 #define DIFF_INDEX_CACHED 01
 #define DIFF_INDEX_MERGE_BASE 02
-int run_diff_index(struct rev_info *revs, unsigned int option);
+void run_diff_index(struct rev_info *revs, unsigned int option);
 
 int do_diff_cache(const struct object_id *, struct diff_options *);
 int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
diff --git a/wt-status.c b/wt-status.c
index bf8687b357..545cea948f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2580,8 +2580,8 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)
 	}
 	rev_info.diffopt.flags.quick = 1;
 	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_files(&rev_info, 0);
-	result = diff_result_code(&rev_info.diffopt, result);
+	run_diff_files(&rev_info, 0);
+	result = diff_result_code(&rev_info.diffopt, 0);
 	release_revisions(&rev_info);
 	return result;
 }
@@ -2614,8 +2614,8 @@ int has_uncommitted_changes(struct repository *r,
 	}
 
 	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_index(&rev_info, DIFF_INDEX_CACHED);
-	result = diff_result_code(&rev_info.diffopt, result);
+	run_diff_index(&rev_info, DIFF_INDEX_CACHED);
+	result = diff_result_code(&rev_info.diffopt, 0);
 	release_revisions(&rev_info);
 	return result;
 }
-- 
2.42.0.rc2.423.g967ecb4f2b

[PATCH v2 6/7] diff: drop useless return values in git-diff helpers

[Jeff King]

Since git-diff has many diff modes, it dispatches to many helpers to
perform each one. But every helper simply returns "0", as it exits
directly if there are serious errors (and options like --exit-code are
handled afterwards). So let's get rid of these useless return values,
which makes the code flow more clear.

There's very little chance that we'd later want to propagate errors
instead of dying immediately. These are all static-local helpers for the
git-diff program implementing its various modes. More "lib-ified" code
would directly call the underlying functions.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/diff.c | 62 +++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index e1f7647c84..3eba691b82 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -77,9 +77,9 @@ static void stuff_change(struct diff_options *opt,
 	diff_queue(&diff_queued_diff, one, two);
 }
 
-static int builtin_diff_b_f(struct rev_info *revs,
-			    int argc, const char **argv UNUSED,
-			    struct object_array_entry **blob)
+static void builtin_diff_b_f(struct rev_info *revs,
+			     int argc, const char **argv UNUSED,
+			     struct object_array_entry **blob)
 {
 	/* Blob vs file in the working tree*/
 	struct stat st;
@@ -109,12 +109,11 @@ static int builtin_diff_b_f(struct rev_info *revs,
 		     path);
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
-	return 0;
 }
 
-static int builtin_diff_blobs(struct rev_info *revs,
-			      int argc, const char **argv UNUSED,
-			      struct object_array_entry **blob)
+static void builtin_diff_blobs(struct rev_info *revs,
+			       int argc, const char **argv UNUSED,
+			       struct object_array_entry **blob)
 {
 	const unsigned mode = canon_mode(S_IFREG | 0644);
 
@@ -134,11 +133,10 @@ static int builtin_diff_blobs(struct rev_info *revs,
 		     blob_path(blob[0]), blob_path(blob[1]));
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
-	return 0;
 }
 
-static int builtin_diff_index(struct rev_info *revs,
-			      int argc, const char **argv)
+static void builtin_diff_index(struct rev_info *revs,
+			       int argc, const char **argv)
 {
 	unsigned int option = 0;
 	while (1 < argc) {
@@ -169,13 +167,12 @@ static int builtin_diff_index(struct rev_info *revs,
 		die_errno("repo_read_cache");
 	}
 	run_diff_index(revs, option);
-	return 0;
 }
 
-static int builtin_diff_tree(struct rev_info *revs,
-			     int argc, const char **argv,
-			     struct object_array_entry *ent0,
-			     struct object_array_entry *ent1)
+static void builtin_diff_tree(struct rev_info *revs,
+			      int argc, const char **argv,
+			      struct object_array_entry *ent0,
+			      struct object_array_entry *ent1)
 {
 	const struct object_id *(oid[2]);
 	struct object_id mb_oid;
@@ -208,13 +205,12 @@ static int builtin_diff_tree(struct rev_info *revs,
 	}
 	diff_tree_oid(oid[0], oid[1], "", &revs->diffopt);
 	log_tree_diff_flush(revs);
-	return 0;
 }
 
-static int builtin_diff_combined(struct rev_info *revs,
-				 int argc, const char **argv UNUSED,
-				 struct object_array_entry *ent,
-				 int ents, int first_non_parent)
+static void builtin_diff_combined(struct rev_info *revs,
+				  int argc, const char **argv UNUSED,
+				  struct object_array_entry *ent,
+				  int ents, int first_non_parent)
 {
 	struct oid_array parents = OID_ARRAY_INIT;
 	int i;
@@ -235,7 +231,6 @@ static int builtin_diff_combined(struct rev_info *revs,
 	}
 	diff_tree_combined(&ent[first_non_parent].item->oid, &parents, revs);
 	oid_array_clear(&parents);
-	return 0;
 }
 
 static void refresh_index_quietly(void)
@@ -253,7 +248,7 @@ static void refresh_index_quietly(void)
 	repo_update_index_if_able(the_repository, &lock_file);
 }
 
-static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)
+static void builtin_diff_files(struct rev_info *revs, int argc, const char **argv)
 {
 	unsigned int options = 0;
 
@@ -291,7 +286,6 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv
 		die_errno("repo_read_index_preload");
 	}
 	run_diff_files(revs, options);
-	return 0;
 }
 
 struct symdiff {
@@ -405,7 +399,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	int blobs = 0, paths = 0;
 	struct object_array_entry *blob[2];
 	int nongit = 0, no_index = 0;
-	int result = 0;
+	int result;
 	struct symdiff sdiff;
 
 	/*
@@ -584,17 +578,17 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	if (!ent.nr) {
 		switch (blobs) {
 		case 0:
-			result = builtin_diff_files(&rev, argc, argv);
+			builtin_diff_files(&rev, argc, argv);
 			break;
 		case 1:
 			if (paths != 1)
 				usage(builtin_diff_usage);
-			result = builtin_diff_b_f(&rev, argc, argv, blob);
+			builtin_diff_b_f(&rev, argc, argv, blob);
 			break;
 		case 2:
 			if (paths)
 				usage(builtin_diff_usage);
-			result = builtin_diff_blobs(&rev, argc, argv, blob);
+			builtin_diff_blobs(&rev, argc, argv, blob);
 			break;
 		default:
 			usage(builtin_diff_usage);
@@ -603,18 +597,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	else if (blobs)
 		usage(builtin_diff_usage);
 	else if (ent.nr == 1)
-		result = builtin_diff_index(&rev, argc, argv);
+		builtin_diff_index(&rev, argc, argv);
 	else if (ent.nr == 2) {
 		if (sdiff.warn)
 			warning(_("%s...%s: multiple merge bases, using %s"),
 				sdiff.left, sdiff.right, sdiff.base);
-		result = builtin_diff_tree(&rev, argc, argv,
-					   &ent.objects[0], &ent.objects[1]);
+		builtin_diff_tree(&rev, argc, argv,
+				  &ent.objects[0], &ent.objects[1]);
 	} else
-		result = builtin_diff_combined(&rev, argc, argv,
-					       ent.objects, ent.nr,
-					       first_non_parent);
-	result = diff_result_code(&rev.diffopt, result);
+		builtin_diff_combined(&rev, argc, argv,
+				      ent.objects, ent.nr,
+				      first_non_parent);
+	result = diff_result_code(&rev.diffopt, 0);
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
 	release_revisions(&rev);
-- 
2.42.0.rc2.423.g967ecb4f2b

[PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code()

[Jeff King]

Many programs use diff_result_code() to get a user-visible program exit
code from a diff result (e.g., checking opts.found_changes if
--exit-code was requested).

This function also takes a "status" parameter, which seems at first
glance that it could be used to propagate an error encountered when
computing the diff. But it doesn't work that way:

  - negative values are passed through as-is, but are not appropriate as
    program exit codes

  - when --exit-code or --check is in effect, we _ignore_ the passed-in
    status completely. So a failed diff which did not have a chance to
    set opts.found_changes would erroneously report "success, no
    changes" instead of propagating the error.

After recent cleanups, neither of these bugs is possible to trigger, as
every caller just passes in "0". So rather than fixing them, we can
simply drop the useless parameter instead.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/describe.c          | 2 +-
 builtin/diff-files.c        | 2 +-
 builtin/diff-index.c        | 2 +-
 builtin/diff-tree.c         | 2 +-
 builtin/diff.c              | 2 +-
 builtin/log.c               | 2 +-
 builtin/stash.c             | 6 +++---
 builtin/submodule--helper.c | 2 +-
 diff-no-index.c             | 2 +-
 diff.c                      | 6 ++----
 diff.h                      | 2 +-
 wt-status.c                 | 4 ++--
 12 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 8cdc25b748..a9e375882b 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -687,7 +687,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 				BUG("malformed internal diff-index command line");
 			run_diff_index(&revs, 0);
 
-			if (!diff_result_code(&revs.diffopt, 0))
+			if (!diff_result_code(&revs.diffopt))
 				suffix = NULL;
 			else
 				suffix = dirty;
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 04070607b1..f38912cd40 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -83,7 +83,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)
 		die_errno("repo_read_index_preload");
 	run_diff_files(&rev, options);
-	result = diff_result_code(&rev.diffopt, 0);
+	result = diff_result_code(&rev.diffopt);
 	release_revisions(&rev);
 	return result;
 }
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 2c6a179832..220f341ffa 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -73,7 +73,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		return -1;
 	}
 	run_diff_index(&rev, option);
-	result = diff_result_code(&rev.diffopt, 0);
+	result = diff_result_code(&rev.diffopt);
 	release_revisions(&rev);
 	return result;
 }
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index c9ba35f143..86be634286 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -232,5 +232,5 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		diff_free(&opt->diffopt);
 	}
 
-	return diff_result_code(&opt->diffopt, 0);
+	return diff_result_code(&opt->diffopt);
 }
diff --git a/builtin/diff.c b/builtin/diff.c
index 3eba691b82..0b313549c7 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -608,7 +608,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		builtin_diff_combined(&rev, argc, argv,
 				      ent.objects, ent.nr,
 				      first_non_parent);
-	result = diff_result_code(&rev.diffopt, 0);
+	result = diff_result_code(&rev.diffopt);
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
 	release_revisions(&rev);
diff --git a/builtin/log.c b/builtin/log.c
index db3a88bfe9..5d808c92f4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -549,7 +549,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)
 	    rev->diffopt.flags.check_failed) {
 		return 02;
 	}
-	return diff_result_code(&rev->diffopt, 0);
+	return diff_result_code(&rev->diffopt);
 }
 
 static int cmd_log_walk(struct rev_info *rev)
diff --git a/builtin/stash.c b/builtin/stash.c
index e799b660f0..53e8868ba1 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -973,7 +973,7 @@ static int show_stash(int argc, const char **argv, const char *prefix)
 	}
 	log_tree_diff_flush(&rev);
 
-	ret = diff_result_code(&rev.diffopt, 0);
+	ret = diff_result_code(&rev.diffopt);
 cleanup:
 	strvec_clear(&stash_args);
 	free_stash_info(&info);
@@ -1111,13 +1111,13 @@ static int check_changes_tracked_files(const struct pathspec *ps)
 	diff_setup_done(&rev.diffopt);
 
 	run_diff_index(&rev, DIFF_INDEX_CACHED);
-	if (diff_result_code(&rev.diffopt, 0)) {
+	if (diff_result_code(&rev.diffopt)) {
 		ret = 1;
 		goto done;
 	}
 
 	run_diff_files(&rev, 0);
-	if (diff_result_code(&rev.diffopt, 0)) {
+	if (diff_result_code(&rev.diffopt)) {
 		ret = 1;
 		goto done;
 	}
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3764ed1f9c..6f3bf33e61 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -670,7 +670,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	setup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);
 	run_diff_files(&rev, 0);
 
-	if (!diff_result_code(&rev.diffopt, 0)) {
+	if (!diff_result_code(&rev.diffopt)) {
 		print_status(flags, ' ', path, ce_oid,
 			     displaypath);
 	} else if (!(flags & OPT_CACHED)) {
diff --git a/diff-no-index.c b/diff-no-index.c
index 4771cf02aa..8aead3e332 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -364,7 +364,7 @@ int diff_no_index(struct rev_info *revs,
 	 * The return code for --no-index imitates diff(1):
 	 * 0 = no changes, 1 = changes, else error
 	 */
-	ret = diff_result_code(&revs->diffopt, 0);
+	ret = diff_result_code(&revs->diffopt);
 
 out:
 	for (i = 0; i < ARRAY_SIZE(to_free); i++)
diff --git a/diff.c b/diff.c
index ee3eb629e3..2de5d3d098 100644
--- a/diff.c
+++ b/diff.c
@@ -6973,16 +6973,14 @@ void diffcore_std(struct diff_options *options)
 	options->found_follow = 0;
 }
 
-int diff_result_code(struct diff_options *opt, int status)
+int diff_result_code(struct diff_options *opt)
 {
 	int result = 0;
 
 	diff_warn_rename_limit("diff.renameLimit",
 			       opt->needed_rename_limit,
 			       opt->degraded_cc_to_c);
-	if (!opt->flags.exit_with_status &&
-	    !(opt->output_format & DIFF_FORMAT_CHECKDIFF))
-		return status;
+
 	if (opt->flags.exit_with_status &&
 	    opt->flags.has_changes)
 		result |= 01;
diff --git a/diff.h b/diff.h
index 528f00d5e1..caf1528bf0 100644
--- a/diff.h
+++ b/diff.h
@@ -647,7 +647,7 @@ int do_diff_cache(const struct object_id *, struct diff_options *);
 int diff_flush_patch_id(struct diff_options *, struct object_id *, int);
 void flush_one_hunk(struct object_id *result, git_hash_ctx *ctx);
 
-int diff_result_code(struct diff_options *, int);
+int diff_result_code(struct diff_options *);
 
 int diff_no_index(struct rev_info *,
 		  int implicit_no_index, int, const char **);
diff --git a/wt-status.c b/wt-status.c
index 545cea948f..981adb09f3 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2581,7 +2581,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)
 	rev_info.diffopt.flags.quick = 1;
 	diff_setup_done(&rev_info.diffopt);
 	run_diff_files(&rev_info, 0);
-	result = diff_result_code(&rev_info.diffopt, 0);
+	result = diff_result_code(&rev_info.diffopt);
 	release_revisions(&rev_info);
 	return result;
 }
@@ -2615,7 +2615,7 @@ int has_uncommitted_changes(struct repository *r,
 
 	diff_setup_done(&rev_info.diffopt);
 	run_diff_index(&rev_info, DIFF_INDEX_CACHED);
-	result = diff_result_code(&rev_info.diffopt, 0);
+	result = diff_result_code(&rev_info.diffopt);
 	release_revisions(&rev_info);
 	return result;
 }
-- 
2.42.0.rc2.423.g967ecb4f2b

Re: [PATCH] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> While this is conceptually orthogonal to what I'm looking at in the
> other part of the thread, the textual conflicts are really annoying. I'm
> going to float this to the front of my series and build on top.

Oh, sorry, this is not in any hurry so please feel free to ignore or
swallow or deal with in any way ;-)  I just wasn't aware that you'd
be digging further around the area.

THanks.

Re: [PATCH v2 4/7] diff: die when failing to read index in git-diff builtin

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> lib-ification purposes, at which point this code would already be doing
> the right thing).

Yup, I very much like that approach and attitude.  Proactively doing
the right thing even if the other parts of the code is not yet
ready.  Good.

Thanks.

Re: [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Many programs use diff_result_code() to get a user-visible program exit
> code from a diff result (e.g., checking opts.found_changes if
> --exit-code was requested).
>
> This function also takes a "status" parameter, which seems at first
> glance that it could be used to propagate an error encountered when
> computing the diff. But it doesn't work that way:
>
>   - negative values are passed through as-is, but are not appropriate as
>     program exit codes
>
>   - when --exit-code or --check is in effect, we _ignore_ the passed-in
>     status completely. So a failed diff which did not have a chance to
>     set opts.found_changes would erroneously report "success, no
>     changes" instead of propagating the error.
>
> After recent cleanups, neither of these bugs is possible to trigger, as

Here "after recent cleanups" refers to the changes to make them
die() upon seeing an error, instead of using it to call this
function with non-zero in the status parameter?  At least, they were
signaling errors correctly when --exit-code is not in use, but now
all the callers are responsible for exiting with non-zero status to
signal an error even when --exit-code is *not* used.

> every caller just passes in "0". So rather than fixing them, we can
> simply drop the useless parameter instead.

OK.

Re: [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code()

[Jeff King]

On Tue, Aug 22, 2023 at 04:38:17PM -0700, Junio C Hamano wrote:

> > This function also takes a "status" parameter, which seems at first
> > glance that it could be used to propagate an error encountered when
> > computing the diff. But it doesn't work that way:
> >
> >   - negative values are passed through as-is, but are not appropriate as
> >     program exit codes
> >
> >   - when --exit-code or --check is in effect, we _ignore_ the passed-in
> >     status completely. So a failed diff which did not have a chance to
> >     set opts.found_changes would erroneously report "success, no
> >     changes" instead of propagating the error.
> >
> > After recent cleanups, neither of these bugs is possible to trigger, as
> 
> Here "after recent cleanups" refers to the changes to make them
> die() upon seeing an error, instead of using it to call this
> function with non-zero in the status parameter?  At least, they were
> signaling errors correctly when --exit-code is not in use, but now
> all the callers are responsible for exiting with non-zero status to
> signal an error even when --exit-code is *not* used.

Sort of. Prior to this series, callers were responsible for ferrying
exit codes to diff_result_code(), which mis-handled them (a little for
the path without --exit-code, and badly with it). Now they are
responsible for handling the errors themselves. I'd say that is not much
more work than passing them along, and provides better outcomes (they
can produce more useful error messages, or die() as appropriate).

But what I really meant with "after recent cleanups" was just that
nobody is even bothering to pass anything but 0 to diff_result_code().
Which was already true before the series for every code path except the
few index/argument parsing paths in builtin/diff.c, and after the
cleanups is entirely true.

-Peff