{"thread":{"id":"60132","subject":"\"git diff --no-pager --exit-code\" errors out but returns zero exit code","startedAt":"2023-08-20T19:57:51Z","lastAt":"2023-08-23T19:01:21Z","messageCount":19,"participants":["Romain Chossart","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"480832","messageId":"CAHxTZ4AUxow1p7mNG0fuRtfhkaOu=Wpc7cYvM8wRaB80hPNf9Q@mail.gmail.com","threadId":"60132","inReplyTo":null,"subject":"\"git diff --no-pager --exit-code\" errors out but returns zero exit code","fromName":"Romain Chossart","fromEmail":"romainchossart@gmail.com","sentAt":"2023-08-20T19:52:36Z","receivedAt":"2023-08-20T19:57:51Z","isPatch":false,"sender":{"key":"romainchossart@gmail.com","avatar":null},"body":"I recently found out (the hard way :-) ) that running the following\ncommand prints an appropriate error on stderr but returns a zero exit\ncode:\n\n> $ git diff --no-pager --exit-code\n> error: invalid option: --no-pager\n> $ echo $?\n> 0\n\nI would expect a non-zero exit code to be returned.\n\nInterestingly, running `git diff --no-pager --exit-code HEAD` shows a\nusage instead and does return a non-zero exit code as expected.\n\nThanks,\n-- \nRomain Chossart\n"},{"id":"480834","messageId":"20230821003532.GA1113755@coredump.intra.peff.net","threadId":"60132","inReplyTo":"CAHxTZ4AUxow1p7mNG0fuRtfhkaOu=Wpc7cYvM8wRaB80hPNf9Q@mail.gmail.com","subject":"[PATCH] diff: handle negative status in diff_result_code()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T00:35:32Z","receivedAt":"2023-08-21T00:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 20, 2023 at 08:52:36PM +0100, Romain Chossart wrote:\n\n> I recently found out (the hard way :-) ) that running the following\n> command prints an appropriate error on stderr but returns a zero exit\n> code:\n> \n> > $ git diff --no-pager --exit-code\n> > error: invalid option: --no-pager\n> > $ echo $?\n> > 0\n> \n> I would expect a non-zero exit code to be returned.\n\nYikes, I would expect that, too.\n\n> Interestingly, running `git diff --no-pager --exit-code HEAD` shows a\n> usage instead and does return a non-zero exit code as expected.\n\nThe difference has to do with where the parsing occurs. Because \"git\ndiff\" has so many sub-modes, without the \"HEAD\" it passes options down\nto a helper function, where we first notice the invalid option and\nreturn an error. So the real bug here is how we handle errors with\nexit-code, and extends beyond just invalid options.\n\nFix is below. AFAICT the bug has been around forever (I didn't check\nearlier than v1.6.6, which shows it). So it is not new in the upcoming\nv2.42, and we can probably apply it to the 'maint' track after the\nrelease.\n\nThanks for your report. And I'm impressed you managed to find such an\nancient bug. :)\n\n-- >8 --\nSubject: [PATCH] diff: handle negative status in diff_result_code()\n\nMost programs which run a diff (porcelain git-diff, diff-tree, etc) run\ntheir result through diff_result_code() before presenting it to the user\nas a program return code. That result generally comes from a library\nfunction like builtin_diff_files(), etc, and may be \"-1\" if the function\nbailed with an error.\n\nThere are two problems here:\n\n  - if --exit-code is not in use, then we pass the code along as-is.\n    Even though Unix exit codes are unsigned, this usually works OK\n    because the integer representation, and \"-1\" ends up as \"255\". But\n    it's not something we should rely on, and we've tried to avoid it\n    elsewhere. E.g. in 5391e94813 (branch: remove negative exit code,\n    2022-03-29) and 246f0edec0 (execv_dashed_external: stop exiting with\n    negative code, 2017-01-06) and probably others.\n\n  - when --exit-code is in use, we ignore the incoming \"status\" variable\n    entirely, and rely on the \"has_changes\" field. But if we saw an\n    error, this field is almost certainly invalid, and means we'd likely\n    just say \"no changes\", which is completely bogus. Likewise for\n    the \"--check\" format.\n\nSo let's intercept the negative value here and return an appropriate\ncode before even considering --exit-code, etc. And while doing so, we\ncan swap out the negative value for a more normal exit code.\n\nThe test here uses an invalid option to trigger the error condition,\nsince that's easy and reliable. But this could also happen, for example,\nif we failed to load the index (in which case we'd just report \"no\nchanges\" with --exit-code, which is very wrong).\n\nReported-by: Romain Chossart <romainchossart@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI know we talked recently about not matching \"128\" explicitly in tests,\nbut it seems like the least-bad thing here, as explained in the comment.\n\n diff.c                 | 11 +++++++++++\n t/t4017-diff-retval.sh | 10 ++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex ee3eb629e3..b8cd829b1f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6980,6 +6980,17 @@ int diff_result_code(struct diff_options *opt, int status)\n \tdiff_warn_rename_limit(\"diff.renameLimit\",\n \t\t\t       opt->needed_rename_limit,\n \t\t\t       opt->degraded_cc_to_c);\n+\n+\t/*\n+\t * A negative status indicates an internal error in the diff routines.\n+\t * We want to avoid codes like \"1\" or \"2\" here that have a documented\n+\t * meaning (and obviously not \"0\" for success / no diff). But we also\n+\t * want to avoid negative values, since the result is passed out via\n+\t * exit(). Convert them all to 128, which matches die().\n+\t */\n+\tif (status < 0)\n+\t\treturn 128;\n+\n \tif (!opt->flags.exit_with_status &&\n \t    !(opt->output_format & DIFF_FORMAT_CHECKDIFF))\n \t\treturn status;\ndiff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh\nindex 5bc28ad9f0..0278364fcd 100755\n--- a/t/t4017-diff-retval.sh\n+++ b/t/t4017-diff-retval.sh\n@@ -138,4 +138,14 @@ test_expect_success 'check honors conflict marker length' '\n \tgit reset --hard\n '\n \n+test_expect_success 'errors are not confused by --exit-code' '\n+\t# The exact code here is not important and not documented. But\n+\t# we do want to make sure that it is not a meaningful documented\n+\t# value like \"1\", nor the result of whatever platform-specific cruft\n+\t# might result from a negative value. The easiest way to check\n+\t# that is just to match what we know the code does now. But it would be\n+\t# OK to change this to match if the code changes.\n+\ttest_expect_code 128 git diff --nonsense --exit-code\n+'\n+\n test_done\n-- \n2.42.0.rc2.418.g602d5859fc\n\n"},{"id":"480841","messageId":"xmqqlee4s82d.fsf@gitster.g","threadId":"60132","inReplyTo":"20230821003532.GA1113755@coredump.intra.peff.net","subject":"Re: [PATCH] diff: handle negative status in diff_result_code()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-21T15:56:26Z","receivedAt":"2023-08-21T15:56:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Thanks for your report. And I'm impressed you managed to find such an\n> ancient bug. :)\n\nIndeed.  Thanks, both.\n\n> Most programs which run a diff (porcelain git-diff, diff-tree, etc) run\n> their result through diff_result_code() before presenting it to the user\n> as a program return code. That result generally comes from a library\n> function like builtin_diff_files(), etc, and may be \"-1\" if the function\n> bailed with an error.\n>\n>\n> There are two problems here:\n>\n>   - if --exit-code is not in use, then we pass the code along as-is.\n>     Even though Unix exit codes are unsigned, this usually works OK\n>     because the integer representation, and \"-1\" ends up as \"255\". But\n>     it's not something we should rely on, and we've tried to avoid it\n>     elsewhere. E.g. in 5391e94813 (branch: remove negative exit code,\n>     2022-03-29) and 246f0edec0 (execv_dashed_external: stop exiting with\n>     negative code, 2017-01-06) and probably others.\n>\n>   - when --exit-code is in use, we ignore the incoming \"status\" variable\n>     entirely, and rely on the \"has_changes\" field. But if we saw an\n>     error, this field is almost certainly invalid, and means we'd likely\n>     just say \"no changes\", which is completely bogus. Likewise for\n>     the \"--check\" format.\n\nInspecting some callers of diff_result_code() further finds\ncuriosities.  wt-status.c for example feeds results form\nrun_diff_index() and run_diff_files() to the function, neither of\nwhich returns anything other than 0. They die or exit(128)\nthemselves, though, so they are OK without this fix.\nbuiltin/describe.c is also in the same boat with its use of\nrun_diff_index().\n\nBut of course the presense of such codepaths does not invalidate the\nfix in your patch.\n\n> So let's intercept the negative value here and return an appropriate\n> code before even considering --exit-code, etc. And while doing so, we\n> can swap out the negative value for a more normal exit code.\n\nGood.  As you said, this is not an ungent regression fix, but I'd\nprefer to see us look at all the current callers, as I wonder if\nthere are positive non-zero numbers coming from some callers, and\npossibly design how this code should react to such \"result\" coming\nin.\n\nAgain, thanks, both.\n"},{"id":"480844","messageId":"xmqqfs4cqsbl.fsf@gitster.g","threadId":"60132","inReplyTo":"xmqqlee4s82d.fsf@gitster.g","subject":"[PATCH] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-21T16:21:50Z","receivedAt":"2023-08-21T16:21:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Many callers of run_diff_index() passed literal \"1\" for the option\nflag word, which should better be spelled out as DIFF_INDEX_CACHED\nfor readablity.  Everybody else passes \"0\" that can stay as-is.\n\nThe other bit in the option flag word is DIFF_INDEX_MERGE_BASE, but\ncuriously there is only one caller that can pass it, which is \"git\ndiff-index --merge-base\" itself---no internal callers uses the\nfeature.\n\nA bit tricky call to the function is in builtin/submodule--helper.c\nwhere the .cached member in a private struct is set/reset as a plain\nBoolean flag, which happens to be \"1\" and happens to match the value\nof DIFF_INDEX_CACHED.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n add-interactive.c           | 2 +-\n builtin/am.c                | 4 ++--\n builtin/stash.c             | 2 +-\n builtin/submodule--helper.c | 2 +-\n diff-lib.c                  | 2 +-\n wt-status.c                 | 6 +++---\n 6 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git c/add-interactive.c w/add-interactive.c\nindex add9a1ad43..7fd00c5e25 100644\n--- c/add-interactive.c\n+++ w/add-interactive.c\n@@ -569,7 +569,7 @@ static int get_modified_files(struct repository *r,\n \t\t\tcopy_pathspec(&rev.prune_data, ps);\n \n \t\tif (s.mode == FROM_INDEX)\n-\t\t\trun_diff_index(&rev, 1);\n+\t\t\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \t\telse {\n \t\t\trev.diffopt.flags.ignore_dirty_submodules = 1;\n \t\t\trun_diff_files(&rev, 0);\ndiff --git c/builtin/am.c w/builtin/am.c\nindex 8bde034fae..202040b62e 100644\n--- c/builtin/am.c\n+++ w/builtin/am.c\n@@ -1430,7 +1430,7 @@ static void write_index_patch(const struct am_state *state)\n \trev_info.diffopt.close_file = 1;\n \tadd_pending_object(&rev_info, &tree->object, \"\");\n \tdiff_setup_done(&rev_info.diffopt);\n-\trun_diff_index(&rev_info, 1);\n+\trun_diff_index(&rev_info, DIFF_INDEX_CACHED);\n \trelease_revisions(&rev_info);\n }\n \n@@ -1593,7 +1593,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \t\trev_info.diffopt.filter |= diff_filter_bit('M');\n \t\tadd_pending_oid(&rev_info, \"HEAD\", &our_tree, 0);\n \t\tdiff_setup_done(&rev_info.diffopt);\n-\t\trun_diff_index(&rev_info, 1);\n+\t\trun_diff_index(&rev_info, DIFF_INDEX_CACHED);\n \t\trelease_revisions(&rev_info);\n \t}\n \ndiff --git c/builtin/stash.c w/builtin/stash.c\nindex fe64cde9ce..fe5052f12f 100644\n--- c/builtin/stash.c\n+++ w/builtin/stash.c\n@@ -1111,7 +1111,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)\n \tadd_head_to_pending(&rev);\n \tdiff_setup_done(&rev.diffopt);\n \n-\tresult = run_diff_index(&rev, 1);\n+\tresult = run_diff_index(&rev, DIFF_INDEX_CACHED);\n \tif (diff_result_code(&rev.diffopt, result)) {\n \t\tret = 1;\n \t\tgoto done;\ndiff --git c/builtin/submodule--helper.c w/builtin/submodule--helper.c\nindex f6871efd95..125ea80d21 100644\n--- c/builtin/submodule--helper.c\n+++ w/builtin/submodule--helper.c\n@@ -1141,7 +1141,7 @@ static int compute_summary_module_list(struct object_id *head_oid,\n \t}\n \n \tif (diff_cmd == DIFF_INDEX)\n-\t\trun_diff_index(&rev, info->cached);\n+\t\trun_diff_index(&rev, info->cached ? DIFF_INDEX_CACHED : 0);\n \telse\n \t\trun_diff_files(&rev, 0);\n \tprepare_submodule_summary(info, &list);\ndiff --git c/diff-lib.c w/diff-lib.c\nindex 6b0c6a7180..cfa3489111 100644\n--- c/diff-lib.c\n+++ w/diff-lib.c\n@@ -682,7 +682,7 @@ int index_differs_from(struct repository *r,\n \t\t\trev.diffopt.flags.ignore_submodules = flags->ignore_submodules;\n \t}\n \trev.diffopt.ita_invisible_in_index = ita_invisible_in_index;\n-\trun_diff_index(&rev, 1);\n+\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \thas_changes = rev.diffopt.flags.has_changes;\n \trelease_revisions(&rev);\n \treturn (has_changes != 0);\ndiff --git c/wt-status.c w/wt-status.c\nindex 5b1378965c..bf8687b357 100644\n--- c/wt-status.c\n+++ w/wt-status.c\n@@ -675,7 +675,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \trev.diffopt.flags.recursive = 1;\n \n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n-\trun_diff_index(&rev, 1);\n+\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \trelease_revisions(&rev);\n }\n \n@@ -1156,7 +1156,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n \t\trev.diffopt.a_prefix = \"c/\";\n \t\trev.diffopt.b_prefix = \"i/\";\n \t} /* else use prefix as per user config */\n-\trun_diff_index(&rev, 1);\n+\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \tif (s->verbose > 1 &&\n \t    wt_status_check_worktree_changes(s, &dirty_submodules)) {\n \t\tstatus_printf_ln(s, c,\n@@ -2614,7 +2614,7 @@ int has_uncommitted_changes(struct repository *r,\n \t}\n \n \tdiff_setup_done(&rev_info.diffopt);\n-\tresult = run_diff_index(&rev_info, 1);\n+\tresult = run_diff_index(&rev_info, DIFF_INDEX_CACHED);\n \tresult = diff_result_code(&rev_info.diffopt, result);\n \trelease_revisions(&rev_info);\n \treturn result;\n"},{"id":"480851","messageId":"20230821180943.GA2617193@coredump.intra.peff.net","threadId":"60132","inReplyTo":"xmqqlee4s82d.fsf@gitster.g","subject":"Re: [PATCH] diff: handle negative status in diff_result_code()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T18:09:43Z","receivedAt":"2023-08-21T18:09:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2023 at 08:56:26AM -0700, Junio C Hamano wrote:\n\n> >   - when --exit-code is in use, we ignore the incoming \"status\" variable\n> >     entirely, and rely on the \"has_changes\" field. But if we saw an\n> >     error, this field is almost certainly invalid, and means we'd likely\n> >     just say \"no changes\", which is completely bogus. Likewise for\n> >     the \"--check\" format.\n> \n> Inspecting some callers of diff_result_code() further finds\n> curiosities.  wt-status.c for example feeds results form\n> run_diff_index() and run_diff_files() to the function, neither of\n> which returns anything other than 0. They die or exit(128)\n> themselves, though, so they are OK without this fix.\n> builtin/describe.c is also in the same boat with its use of\n> run_diff_index().\n> \n> But of course the presense of such codepaths does not invalidate the\n> fix in your patch.\n\nYeah, I admit I did not dig too much into the callers, as it was obvious\nthat any negative values were currently being mis-handled, and I knew at\nleast one caller passed them in (the one in builtin/diff.c).\n\nAnd in fact I think that is the only problematic caller. Every other\ncaller either passes the result of run_diff_*(), which always return 0,\nor just directly pass zero themselves!\n\nIt is only builtin/diff.c that calls its local builtin_diff_blobs(), and\nso on, and those may return errors. So one direction is something like:\n\n  - convert those calls to die() more directly; this has the potential\n    side effect of producing a better message for the case that started\n    this thread by calling usage(builtin_diff_usage) instead of a short\n    error\n\n  - drop the now-useless return codes from those functions, along with\n    the already-useless ones from run_diff_files(), etc. At this point\n    everybody will just be passing a blind \"0\" to diff_result_code()\n\n  - drop the \"status\" parameter to diff_result_code()\n\nThat would make the code simpler. It does feel a bit like going in the\nopposite direction of recent \"pass errors up the stack rather than\ndying\" libification efforts. I think that's OK for the builtin_* helpers\nin diff.c, which are just serving the diff porcelain. But things like\nrun_diff_files(), while pretty big operations, are something we might\ncall as small part of another operation (like git-describe).\n\nWe are not making things worse there, in the sense that their return\ncodes are currently meaningless. But removing the code entirely feels\nlike a step in the wrong direction. I dunno. Maybe we should retain\ntheir return codes and have the callers die() if they fail (which would\ncurrently be dead-code, but future-proof us for later cleanups).\n\n-Peff\n"},{"id":"480854","messageId":"20230821183611.GB2617193@coredump.intra.peff.net","threadId":"60132","inReplyTo":"xmqqfs4cqsbl.fsf@gitster.g","subject":"Re: [PATCH] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T18:36:11Z","receivedAt":"2023-08-21T18:36:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2023 at 09:21:50AM -0700, Junio C Hamano wrote:\n\n> Many callers of run_diff_index() passed literal \"1\" for the option\n> flag word, which should better be spelled out as DIFF_INDEX_CACHED\n> for readablity.  Everybody else passes \"0\" that can stay as-is.\n> \n> The other bit in the option flag word is DIFF_INDEX_MERGE_BASE, but\n> curiously there is only one caller that can pass it, which is \"git\n> diff-index --merge-base\" itself---no internal callers uses the\n> feature.\n> \n> A bit tricky call to the function is in builtin/submodule--helper.c\n> where the .cached member in a private struct is set/reset as a plain\n> Boolean flag, which happens to be \"1\" and happens to match the value\n> of DIFF_INDEX_CACHED.\n\nGood catch. I guess this is conversion that should have been done in\n4c3fe82ef1 (diff-lib: accept option flags in run_diff_index(),\n2020-09-20). That commit did fix up some callers, but missed these ones.\n\nWhile this is conceptually orthogonal to what I'm looking at in the\nother part of the thread, the textual conflicts are really annoying. I'm\ngoing to float this to the front of my series and build on top.\n\n-Peff\n"},{"id":"480856","messageId":"xmqqfs4cp7dj.fsf@gitster.g","threadId":"60132","inReplyTo":"20230821180943.GA2617193@coredump.intra.peff.net","subject":"Re: [PATCH] diff: handle negative status in diff_result_code()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-21T18:39:36Z","receivedAt":"2023-08-21T19:00:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> That would make the code simpler. It does feel a bit like going in the\n> opposite direction of recent \"pass errors up the stack rather than\n> dying\" libification efforts. I think that's OK for the builtin_* helpers\n> in diff.c, which are just serving the diff porcelain. But things like\n> run_diff_files(), while pretty big operations, are something we might\n> call as small part of another operation (like git-describe).\n\nTrue, for things in diff-lib.c we likely would want to go in the\nopposite \"return an error to be handled by the caller\" route.\n"},{"id":"480861","messageId":"20230821201358.GA2663749@coredump.intra.peff.net","threadId":"60132","inReplyTo":"xmqqfs4cp7dj.fsf@gitster.g","subject":"[PATCH v2 0/7] cleaning up diff_result_code()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:13:58Z","receivedAt":"2023-08-21T20:14:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 21, 2023 at 11:39:36AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > That would make the code simpler. It does feel a bit like going in the\n> > opposite direction of recent \"pass errors up the stack rather than\n> > dying\" libification efforts. I think that's OK for the builtin_* helpers\n> > in diff.c, which are just serving the diff porcelain. But things like\n> > run_diff_files(), while pretty big operations, are something we might\n> > call as small part of another operation (like git-describe).\n> \n> True, for things in diff-lib.c we likely would want to go in the\n> opposite \"return an error to be handled by the caller\" route.\n\nAfter poking at it a bit, I think it actually is OK to drop the return\ncodes here. There are some immediate benefits, and I'm not sure it\nactually hampers libification that much; see patch 5 for my reasoning.\n\nSo here's what I came up with.\n\n  [1/7]: diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()\n\n    Your patch verbatim, since there's tons of textual conflicts\n    otherwise.\n\n  [2/7]: diff-files: avoid negative exit value\n\n    A bonus fix that I noticed. It's the same problem as found\n    elsewhere, but using a different code path, so it seemed easiest to\n    fix on its own.\n\n  [3/7]: diff: show usage for unknown builtin_diff_files() options\n\n    This directly fixes the bug found by Romain (but in a different way\n    than v1).\n\n  [4/7]: diff: die when failing to read index in git-diff builtin\n\n    Obvious cleanups.\n\n  [5/7]: diff: drop useless return from run_diff_{files,index} functions\n\n    Possibly controversial cleanups. ;)\n\n  [6/7]: diff: drop useless return values in git-diff helpers\n  [7/7]: diff: drop useless \"status\" parameter from diff_result_code()\n\n    And then these are the payoff cleanups enabled by patch 5.\n\n add-interactive.c           |  2 +-\n builtin/add.c               |  3 +-\n builtin/am.c                |  4 +-\n builtin/describe.c          |  6 +--\n builtin/diff-files.c        | 12 ++----\n builtin/diff-index.c        |  4 +-\n builtin/diff-tree.c         |  2 +-\n builtin/diff.c              | 79 +++++++++++++++++--------------------\n builtin/log.c               |  2 +-\n builtin/stash.c             | 16 +++-----\n builtin/submodule--helper.c |  7 ++--\n diff-lib.c                  |  8 ++--\n diff-no-index.c             |  2 +-\n diff.c                      |  6 +--\n diff.h                      |  6 +--\n t/t4017-diff-retval.sh      |  5 +++\n wt-status.c                 | 12 +++---\n 17 files changed, 81 insertions(+), 95 deletions(-)\n\nNo range-diff since it's effectively a brand-new series.\n\nBTW, I know you were looking at --exit-code bugs recently in another\nseries. But I think this should all be orthogonal (both semantically\nand textually). I'll try to give another review on that series, as well.\n\n-Peff\n"},{"id":"480862","messageId":"20230821201414.GA1798590@coredump.intra.peff.net","threadId":"60132","inReplyTo":"20230821201358.GA2663749@coredump.intra.peff.net","subject":"[PATCH v2 1/7] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:14:14Z","receivedAt":"2023-08-21T20:14:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\nMany callers of run_diff_index() passed literal \"1\" for the option\nflag word, which should better be spelled out as DIFF_INDEX_CACHED\nfor readablity.  Everybody else passes \"0\" that can stay as-is.\n\nThe other bit in the option flag word is DIFF_INDEX_MERGE_BASE, but\ncuriously there is only one caller that can pass it, which is \"git\ndiff-index --merge-base\" itself---no internal callers uses the\nfeature.\n\nA bit tricky call to the function is in builtin/submodule--helper.c\nwhere the .cached member in a private struct is set/reset as a plain\nBoolean flag, which happens to be \"1\" and happens to match the value\nof DIFF_INDEX_CACHED.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n add-interactive.c           | 2 +-\n builtin/am.c                | 4 ++--\n builtin/stash.c             | 2 +-\n builtin/submodule--helper.c | 2 +-\n diff-lib.c                  | 2 +-\n wt-status.c                 | 6 +++---\n 6 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex add9a1ad43..7fd00c5e25 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -569,7 +569,7 @@ static int get_modified_files(struct repository *r,\n \t\t\tcopy_pathspec(&rev.prune_data, ps);\n \n \t\tif (s.mode == FROM_INDEX)\n-\t\t\trun_diff_index(&rev, 1);\n+\t\t\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \t\telse {\n \t\t\trev.diffopt.flags.ignore_dirty_submodules = 1;\n \t\t\trun_diff_files(&rev, 0);\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8bde034fae..202040b62e 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1430,7 +1430,7 @@ static void write_index_patch(const struct am_state *state)\n \trev_info.diffopt.close_file = 1;\n \tadd_pending_object(&rev_info, &tree->object, \"\");\n \tdiff_setup_done(&rev_info.diffopt);\n-\trun_diff_index(&rev_info, 1);\n+\trun_diff_index(&rev_info, DIFF_INDEX_CACHED);\n \trelease_revisions(&rev_info);\n }\n \n@@ -1593,7 +1593,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \t\trev_info.diffopt.filter |= diff_filter_bit('M');\n \t\tadd_pending_oid(&rev_info, \"HEAD\", &our_tree, 0);\n \t\tdiff_setup_done(&rev_info.diffopt);\n-\t\trun_diff_index(&rev_info, 1);\n+\t\trun_diff_index(&rev_info, DIFF_INDEX_CACHED);\n \t\trelease_revisions(&rev_info);\n \t}\n \ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex fe64cde9ce..fe5052f12f 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1111,7 +1111,7 @@ static int check_changes_tracked_files(const struct pathspec *ps)\n \tadd_head_to_pending(&rev);\n \tdiff_setup_done(&rev.diffopt);\n \n-\tresult = run_diff_index(&rev, 1);\n+\tresult = run_diff_index(&rev, DIFF_INDEX_CACHED);\n \tif (diff_result_code(&rev.diffopt, result)) {\n \t\tret = 1;\n \t\tgoto done;\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f6871efd95..125ea80d21 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1141,7 +1141,7 @@ static int compute_summary_module_list(struct object_id *head_oid,\n \t}\n \n \tif (diff_cmd == DIFF_INDEX)\n-\t\trun_diff_index(&rev, info->cached);\n+\t\trun_diff_index(&rev, info->cached ? DIFF_INDEX_CACHED : 0);\n \telse\n \t\trun_diff_files(&rev, 0);\n \tprepare_submodule_summary(info, &list);\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 6b0c6a7180..cfa3489111 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -682,7 +682,7 @@ int index_differs_from(struct repository *r,\n \t\t\trev.diffopt.flags.ignore_submodules = flags->ignore_submodules;\n \t}\n \trev.diffopt.ita_invisible_in_index = ita_invisible_in_index;\n-\trun_diff_index(&rev, 1);\n+\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \thas_changes = rev.diffopt.flags.has_changes;\n \trelease_revisions(&rev);\n \treturn (has_changes != 0);\ndiff --git a/wt-status.c b/wt-status.c\nindex 5b1378965c..bf8687b357 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -675,7 +675,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \trev.diffopt.flags.recursive = 1;\n \n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n-\trun_diff_index(&rev, 1);\n+\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \trelease_revisions(&rev);\n }\n \n@@ -1156,7 +1156,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n \t\trev.diffopt.a_prefix = \"c/\";\n \t\trev.diffopt.b_prefix = \"i/\";\n \t} /* else use prefix as per user config */\n-\trun_diff_index(&rev, 1);\n+\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n \tif (s->verbose > 1 &&\n \t    wt_status_check_worktree_changes(s, &dirty_submodules)) {\n \t\tstatus_printf_ln(s, c,\n@@ -2614,7 +2614,7 @@ int has_uncommitted_changes(struct repository *r,\n \t}\n \n \tdiff_setup_done(&rev_info.diffopt);\n-\tresult = run_diff_index(&rev_info, 1);\n+\tresult = run_diff_index(&rev_info, DIFF_INDEX_CACHED);\n \tresult = diff_result_code(&rev_info.diffopt, result);\n \trelease_revisions(&rev_info);\n \treturn result;\n-- \n2.42.0.rc2.423.g967ecb4f2b\n\n"},{"id":"480863","messageId":"20230821201510.GB1798590@coredump.intra.peff.net","threadId":"60132","inReplyTo":"20230821201358.GA2663749@coredump.intra.peff.net","subject":"[PATCH v2 2/7] diff-files: avoid negative exit value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:15:10Z","receivedAt":"2023-08-21T20:15:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If loading the index fails, we print an error and then return \"-1\" from\nthe function. But since this is a builtin, we end up with exit(-1),\nwhich produces odd results since program exit codes are unsigned.\nBecause of integer conversion, it usually becomes 255, which is at least\nstill an error, but values above 128 are usually interpreted as signal\ndeath.\n\nSince we know the program is exiting immediately, we can just replace\nthe error return with a die().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm actually not sure this can be triggered in practice; see patch 4 for\nmore discussion. Regardless, it seems like a strict improvement.\n\n builtin/diff-files.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 50330b8dd2..2e3e948583 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -80,14 +80,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t    (rev.diffopt.output_format & DIFF_FORMAT_PATCH))\n \t\tdiff_merges_set_dense_combined_if_unset(&rev);\n \n-\tif (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0) {\n-\t\tperror(\"repo_read_index_preload\");\n-\t\tresult = -1;\n-\t\tgoto cleanup;\n-\t}\n+\tif (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)\n+\t\tdie_errno(\"repo_read_index_preload\");\n \tresult = run_diff_files(&rev, options);\n \tresult = diff_result_code(&rev.diffopt, result);\n-cleanup:\n \trelease_revisions(&rev);\n \treturn result;\n }\n-- \n2.42.0.rc2.423.g967ecb4f2b\n\n"},{"id":"480864","messageId":"20230821201626.GC1798590@coredump.intra.peff.net","threadId":"60132","inReplyTo":"20230821201358.GA2663749@coredump.intra.peff.net","subject":"[PATCH v2 3/7] diff: show usage for unknown builtin_diff_files() options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:16:26Z","receivedAt":"2023-08-21T20:16:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git-diff command has many modes (comparing worktree to index, index\nto HEAD, individual blobs, etc). As a result, it dispatches to many\nhelper functions and cannot completely parse its options until we're in\nthose helper functions.\n\nMost of them, when seeing an unknown option, exit immediately by calling\nusage(). But builtin_diff_files(), which is the default if no revision\nor blob arguments are given, instead prints an error() and returns -1.\n\nOne obvious shortcoming here is that the user doesn't get to see the\nusual usage message. But there's a much more important bug: the -1\nreturn is fed to diff_result_code(), which is not ready to handle it.\nBy default, it passes the code along as an exit code. We try to avoid\nnegative exit codes because they get converted to unsigned values, but\nit should at least consistently show up as non-zero (i.e., a failure).\n\nBut much worse is that when --exit-code is in effect, diff_result_code()\nwill _ignore_ the status passed in by the caller, and instead only\nreport on whether the diff found changes. It didn't, of course, because\nwe never ran the diff, and the program unexpectedly exits with success!\n\nWe can fix this bug by just calling usage(), like the other helpers do.\nAnother option would of course be to teach diff_result_code() to handle\nthis value. But as we'll see in the next few patches, it can be cleaned\nup even further. Let's just fix this bug directly to start with.\n\nReported-by: Romain Chossart <romainchossart@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/diff.c         | 6 ++++--\n t/t4017-diff-retval.sh | 5 +++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex b19530c996..d0e187ec18 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -269,8 +269,10 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \t\t\toptions |= DIFF_SILENT_ON_REMOVED;\n \t\telse if (!strcmp(argv[1], \"-h\"))\n \t\t\tusage(builtin_diff_usage);\n-\t\telse\n-\t\t\treturn error(_(\"invalid option: %s\"), argv[1]);\n+\t\telse {\n+\t\t\terror(_(\"invalid option: %s\"), argv[1]);\n+\t\t\tusage(builtin_diff_usage);\n+\t\t}\n \t\targv++; argc--;\n \t}\n \ndiff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh\nindex 5bc28ad9f0..f439f469bd 100755\n--- a/t/t4017-diff-retval.sh\n+++ b/t/t4017-diff-retval.sh\n@@ -138,4 +138,9 @@ test_expect_success 'check honors conflict marker length' '\n \tgit reset --hard\n '\n \n+test_expect_success 'option errors are not confused by --exit-code' '\n+\ttest_must_fail git diff --exit-code --nonsense 2>err &&\n+\tgrep '^usage:' err\n+'\n+\n test_done\n-- \n2.42.0.rc2.423.g967ecb4f2b\n\n"},{"id":"480865","messageId":"20230821201727.GD1798590@coredump.intra.peff.net","threadId":"60132","inReplyTo":"20230821201358.GA2663749@coredump.intra.peff.net","subject":"[PATCH v2 4/7] diff: die when failing to read index in git-diff builtin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:17:27Z","receivedAt":"2023-08-21T20:17:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the git-diff program fails to read the index in its diff-files or\ndiff-index helper functions, it propagates the error up the stack. This\neventually lands in diff_result_code(), which does not handle it well\n(as discussed in the previous patch).\n\nSince the only sensible thing here is to exit with an error code (and\nwhat we were expecting the propagated error code to cause), let's just\ndo that directly.\n\nThere's no test here, as I'm not even sure this case can be triggered.\nThe index-reading functions tend to die() themselves when encountering\nany errors, and the return value is just the number of entries in the\nfile (and so always 0 or positive). But let's err on the conservative\nside and keep checking the return value. It may be worth digging into as\na separate topic (though index-reading is low-level enough that we\nprobably want to eventually teach it to propagate errors anyway for\nlib-ification purposes, at which point this code would already be doing\nthe right thing).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/diff.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex d0e187ec18..005f415d36 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -163,12 +163,10 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\tsetup_work_tree();\n \t\tif (repo_read_index_preload(the_repository,\n \t\t\t\t\t    &revs->diffopt.pathspec, 0) < 0) {\n-\t\t\tperror(\"repo_read_index_preload\");\n-\t\t\treturn -1;\n+\t\t\tdie_errno(\"repo_read_index_preload\");\n \t\t}\n \t} else if (repo_read_index(the_repository) < 0) {\n-\t\tperror(\"repo_read_cache\");\n-\t\treturn -1;\n+\t\tdie_errno(\"repo_read_cache\");\n \t}\n \treturn run_diff_index(revs, option);\n }\n@@ -289,8 +287,7 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \tsetup_work_tree();\n \tif (repo_read_index_preload(the_repository, &revs->diffopt.pathspec,\n \t\t\t\t    0) < 0) {\n-\t\tperror(\"repo_read_index_preload\");\n-\t\treturn -1;\n+\t\tdie_errno(\"repo_read_index_preload\");\n \t}\n \treturn run_diff_files(revs, options);\n }\n-- \n2.42.0.rc2.423.g967ecb4f2b\n\n"},{"id":"480866","messageId":"20230821201855.GE1798590@coredump.intra.peff.net","threadId":"60132","inReplyTo":"20230821201358.GA2663749@coredump.intra.peff.net","subject":"[PATCH v2 5/7] diff: drop useless return from run_diff_{files,index} functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:18:55Z","receivedAt":"2023-08-21T20:18:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Neither of these functions ever returns a value other than zero.\nInstead, they expect unrecoverable errors to exit immediately, and\nthings like \"--exit-code\" are stored inside the diff_options struct to\nbe handled later via diff_result_code().\n\nSome callers do check the return values, but many don't bother. Let's\ndrop the useless return values, which are misleading callers about how\nthe functions work. This could be seen as a step in the wrong direction,\nas we might want to eventually \"lib-ify\" these to more cleanly return\nerrors up the stack, in which case we'd have to add the return values\nback in. But there are some benefits to doing this now:\n\n  1. In the current code, somebody could accidentally add a \"return -1\"\n     to one of the functions, which would be erroneously ignored by many\n     callers. By removing the return code, the compiler can notice the\n     mismatch and force the developer to decide what to do.\n\n     Obviously the other option here is that we could start consistently\n     checking the error code in every caller. But it would be dead code,\n     and we wouldn't get any compile-time help in catching new cases.\n\n  2. It communicates the situation to callers, who may want to choose a\n     different function. These functions are really thin wrappers for\n     doing git-diff-files and git-diff-index within the process. But\n     callers who care about recovering from an error here are probably\n     better off using the underlying library functions, many of\n     which do return errors.\n\nIf somebody eventually wants to teach these functions to propagate\nerrors, they'll have to switch back to returning a value, effectively\nreverting this patch. But at least then they will be starting with a\nlevel playing field: they know that they will need to inspect each\ncaller to see how it should handle the error.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/add.c               |  3 +--\n builtin/describe.c          |  6 +++---\n builtin/diff-files.c        |  4 ++--\n builtin/diff-index.c        |  4 ++--\n builtin/diff.c              |  6 ++++--\n builtin/stash.c             | 14 +++++---------\n builtin/submodule--helper.c |  5 ++---\n diff-lib.c                  |  6 ++----\n diff.h                      |  4 ++--\n wt-status.c                 |  8 ++++----\n 10 files changed, 27 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 4b0dd798df..12c5aa6d1f 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -194,8 +194,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tout = xopen(file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n \trev.diffopt.file = xfdopen(out, \"w\");\n \trev.diffopt.close_file = 1;\n-\tif (run_diff_files(&rev, 0))\n-\t\tdie(_(\"Could not write patch\"));\n+\trun_diff_files(&rev, 0);\n \n \tif (launch_editor(file, NULL, NULL))\n \t\tdie(_(\"editing patch failed\"));\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex b28a4a1f82..8cdc25b748 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -668,7 +668,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tstruct lock_file index_lock = LOCK_INIT;\n \t\t\tstruct rev_info revs;\n \t\t\tstruct strvec args = STRVEC_INIT;\n-\t\t\tint fd, result;\n+\t\t\tint fd;\n \n \t\t\tsetup_work_tree();\n \t\t\tprepare_repo_settings(the_repository);\n@@ -685,9 +685,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tstrvec_pushv(&args, diff_index_args);\n \t\t\tif (setup_revisions(args.nr, args.v, &revs, NULL) != 1)\n \t\t\t\tBUG(\"malformed internal diff-index command line\");\n-\t\t\tresult = run_diff_index(&revs, 0);\n+\t\t\trun_diff_index(&revs, 0);\n \n-\t\t\tif (!diff_result_code(&revs.diffopt, result))\n+\t\t\tif (!diff_result_code(&revs.diffopt, 0))\n \t\t\t\tsuffix = NULL;\n \t\t\telse\n \t\t\t\tsuffix = dirty;\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 2e3e948583..04070607b1 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -82,8 +82,8 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \n \tif (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)\n \t\tdie_errno(\"repo_read_index_preload\");\n-\tresult = run_diff_files(&rev, options);\n-\tresult = diff_result_code(&rev.diffopt, result);\n+\trun_diff_files(&rev, options);\n+\tresult = diff_result_code(&rev.diffopt, 0);\n \trelease_revisions(&rev);\n \treturn result;\n }\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 9db7139b83..2c6a179832 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -72,8 +72,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\tperror(\"repo_read_index\");\n \t\treturn -1;\n \t}\n-\tresult = run_diff_index(&rev, option);\n-\tresult = diff_result_code(&rev.diffopt, result);\n+\trun_diff_index(&rev, option);\n+\tresult = diff_result_code(&rev.diffopt, 0);\n \trelease_revisions(&rev);\n \treturn result;\n }\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 005f415d36..e1f7647c84 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -168,7 +168,8 @@ static int builtin_diff_index(struct rev_info *revs,\n \t} else if (repo_read_index(the_repository) < 0) {\n \t\tdie_errno(\"repo_read_cache\");\n \t}\n-\treturn run_diff_index(revs, option);\n+\trun_diff_index(revs, option);\n+\treturn 0;\n }\n \n static int builtin_diff_tree(struct rev_info *revs,\n@@ -289,7 +290,8 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \t\t\t\t    0) < 0) {\n \t\tdie_errno(\"repo_read_index_preload\");\n \t}\n-\treturn run_diff_files(revs, options);\n+\trun_diff_files(revs, options);\n+\treturn 0;\n }\n \n struct symdiff {\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex fe5052f12f..e799b660f0 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1089,7 +1089,6 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,\n  */\n static int check_changes_tracked_files(const struct pathspec *ps)\n {\n-\tint result;\n \tstruct rev_info rev;\n \tstruct object_id dummy;\n \tint ret = 0;\n@@ -1111,14 +1110,14 @@ static int check_changes_tracked_files(const struct pathspec *ps)\n \tadd_head_to_pending(&rev);\n \tdiff_setup_done(&rev.diffopt);\n \n-\tresult = run_diff_index(&rev, DIFF_INDEX_CACHED);\n-\tif (diff_result_code(&rev.diffopt, result)) {\n+\trun_diff_index(&rev, DIFF_INDEX_CACHED);\n+\tif (diff_result_code(&rev.diffopt, 0)) {\n \t\tret = 1;\n \t\tgoto done;\n \t}\n \n-\tresult = run_diff_files(&rev, 0);\n-\tif (diff_result_code(&rev.diffopt, result)) {\n+\trun_diff_files(&rev, 0);\n+\tif (diff_result_code(&rev.diffopt, 0)) {\n \t\tret = 1;\n \t\tgoto done;\n \t}\n@@ -1309,10 +1308,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps\n \n \tadd_pending_object(&rev, parse_object(the_repository, &info->b_commit),\n \t\t\t   \"\");\n-\tif (run_diff_index(&rev, 0)) {\n-\t\tret = -1;\n-\t\tgoto done;\n-\t}\n+\trun_diff_index(&rev, 0);\n \n \tcp_upd_index.git_cmd = 1;\n \tstrvec_pushl(&cp_upd_index.args, \"update-index\",\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 125ea80d21..3764ed1f9c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -629,7 +629,6 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \tchar *displaypath;\n \tstruct strvec diff_files_args = STRVEC_INIT;\n \tstruct rev_info rev = REV_INFO_INIT;\n-\tint diff_files_result;\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *git_dir;\n \tstruct setup_revision_opt opt = {\n@@ -669,9 +668,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \trepo_init_revisions(the_repository, &rev, NULL);\n \trev.abbrev = 0;\n \tsetup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);\n-\tdiff_files_result = run_diff_files(&rev, 0);\n+\trun_diff_files(&rev, 0);\n \n-\tif (!diff_result_code(&rev.diffopt, diff_files_result)) {\n+\tif (!diff_result_code(&rev.diffopt, 0)) {\n \t\tprint_status(flags, ' ', path, ce_oid,\n \t\t\t     displaypath);\n \t} else if (!(flags & OPT_CACHED)) {\ndiff --git a/diff-lib.c b/diff-lib.c\nindex cfa3489111..d8aa777a73 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -96,7 +96,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \treturn changed;\n }\n \n-int run_diff_files(struct rev_info *revs, unsigned int option)\n+void run_diff_files(struct rev_info *revs, unsigned int option)\n {\n \tint entries, i;\n \tint diff_unmerged_stage = revs->max_count;\n@@ -272,7 +272,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \ttrace_performance_since(start, \"diff-files\");\n-\treturn 0;\n }\n \n /*\n@@ -606,7 +605,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n \tfree_commit_list(merge_bases);\n }\n \n-int run_diff_index(struct rev_info *revs, unsigned int option)\n+void run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\n \tint cached = !!(option & DIFF_INDEX_CACHED);\n@@ -640,7 +639,6 @@ int run_diff_index(struct rev_info *revs, unsigned int option)\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \ttrace_performance_leave(\"diff-index\");\n-\treturn 0;\n }\n \n int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)\ndiff --git a/diff.h b/diff.h\nindex 260c454155..528f00d5e1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -637,11 +637,11 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n #define DIFF_SILENT_ON_REMOVED 01\n /* report racily-clean paths as modified */\n #define DIFF_RACY_IS_MODIFIED 02\n-int run_diff_files(struct rev_info *revs, unsigned int option);\n+void run_diff_files(struct rev_info *revs, unsigned int option);\n \n #define DIFF_INDEX_CACHED 01\n #define DIFF_INDEX_MERGE_BASE 02\n-int run_diff_index(struct rev_info *revs, unsigned int option);\n+void run_diff_index(struct rev_info *revs, unsigned int option);\n \n int do_diff_cache(const struct object_id *, struct diff_options *);\n int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\ndiff --git a/wt-status.c b/wt-status.c\nindex bf8687b357..545cea948f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2580,8 +2580,8 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)\n \t}\n \trev_info.diffopt.flags.quick = 1;\n \tdiff_setup_done(&rev_info.diffopt);\n-\tresult = run_diff_files(&rev_info, 0);\n-\tresult = diff_result_code(&rev_info.diffopt, result);\n+\trun_diff_files(&rev_info, 0);\n+\tresult = diff_result_code(&rev_info.diffopt, 0);\n \trelease_revisions(&rev_info);\n \treturn result;\n }\n@@ -2614,8 +2614,8 @@ int has_uncommitted_changes(struct repository *r,\n \t}\n \n \tdiff_setup_done(&rev_info.diffopt);\n-\tresult = run_diff_index(&rev_info, DIFF_INDEX_CACHED);\n-\tresult = diff_result_code(&rev_info.diffopt, result);\n+\trun_diff_index(&rev_info, DIFF_INDEX_CACHED);\n+\tresult = diff_result_code(&rev_info.diffopt, 0);\n \trelease_revisions(&rev_info);\n \treturn result;\n }\n-- \n2.42.0.rc2.423.g967ecb4f2b\n\n"},{"id":"480867","messageId":"20230821201944.GF1798590@coredump.intra.peff.net","threadId":"60132","inReplyTo":"20230821201358.GA2663749@coredump.intra.peff.net","subject":"[PATCH v2 6/7] diff: drop useless return values in git-diff helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:19:44Z","receivedAt":"2023-08-21T20:19:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since git-diff has many diff modes, it dispatches to many helpers to\nperform each one. But every helper simply returns \"0\", as it exits\ndirectly if there are serious errors (and options like --exit-code are\nhandled afterwards). So let's get rid of these useless return values,\nwhich makes the code flow more clear.\n\nThere's very little chance that we'd later want to propagate errors\ninstead of dying immediately. These are all static-local helpers for the\ngit-diff program implementing its various modes. More \"lib-ified\" code\nwould directly call the underlying functions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/diff.c | 62 +++++++++++++++++++++++---------------------------\n 1 file changed, 28 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex e1f7647c84..3eba691b82 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -77,9 +77,9 @@ static void stuff_change(struct diff_options *opt,\n \tdiff_queue(&diff_queued_diff, one, two);\n }\n \n-static int builtin_diff_b_f(struct rev_info *revs,\n-\t\t\t    int argc, const char **argv UNUSED,\n-\t\t\t    struct object_array_entry **blob)\n+static void builtin_diff_b_f(struct rev_info *revs,\n+\t\t\t     int argc, const char **argv UNUSED,\n+\t\t\t     struct object_array_entry **blob)\n {\n \t/* Blob vs file in the working tree*/\n \tstruct stat st;\n@@ -109,12 +109,11 @@ static int builtin_diff_b_f(struct rev_info *revs,\n \t\t     path);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n-\treturn 0;\n }\n \n-static int builtin_diff_blobs(struct rev_info *revs,\n-\t\t\t      int argc, const char **argv UNUSED,\n-\t\t\t      struct object_array_entry **blob)\n+static void builtin_diff_blobs(struct rev_info *revs,\n+\t\t\t       int argc, const char **argv UNUSED,\n+\t\t\t       struct object_array_entry **blob)\n {\n \tconst unsigned mode = canon_mode(S_IFREG | 0644);\n \n@@ -134,11 +133,10 @@ static int builtin_diff_blobs(struct rev_info *revs,\n \t\t     blob_path(blob[0]), blob_path(blob[1]));\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n-\treturn 0;\n }\n \n-static int builtin_diff_index(struct rev_info *revs,\n-\t\t\t      int argc, const char **argv)\n+static void builtin_diff_index(struct rev_info *revs,\n+\t\t\t       int argc, const char **argv)\n {\n \tunsigned int option = 0;\n \twhile (1 < argc) {\n@@ -169,13 +167,12 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\tdie_errno(\"repo_read_cache\");\n \t}\n \trun_diff_index(revs, option);\n-\treturn 0;\n }\n \n-static int builtin_diff_tree(struct rev_info *revs,\n-\t\t\t     int argc, const char **argv,\n-\t\t\t     struct object_array_entry *ent0,\n-\t\t\t     struct object_array_entry *ent1)\n+static void builtin_diff_tree(struct rev_info *revs,\n+\t\t\t      int argc, const char **argv,\n+\t\t\t      struct object_array_entry *ent0,\n+\t\t\t      struct object_array_entry *ent1)\n {\n \tconst struct object_id *(oid[2]);\n \tstruct object_id mb_oid;\n@@ -208,13 +205,12 @@ static int builtin_diff_tree(struct rev_info *revs,\n \t}\n \tdiff_tree_oid(oid[0], oid[1], \"\", &revs->diffopt);\n \tlog_tree_diff_flush(revs);\n-\treturn 0;\n }\n \n-static int builtin_diff_combined(struct rev_info *revs,\n-\t\t\t\t int argc, const char **argv UNUSED,\n-\t\t\t\t struct object_array_entry *ent,\n-\t\t\t\t int ents, int first_non_parent)\n+static void builtin_diff_combined(struct rev_info *revs,\n+\t\t\t\t  int argc, const char **argv UNUSED,\n+\t\t\t\t  struct object_array_entry *ent,\n+\t\t\t\t  int ents, int first_non_parent)\n {\n \tstruct oid_array parents = OID_ARRAY_INIT;\n \tint i;\n@@ -235,7 +231,6 @@ static int builtin_diff_combined(struct rev_info *revs,\n \t}\n \tdiff_tree_combined(&ent[first_non_parent].item->oid, &parents, revs);\n \toid_array_clear(&parents);\n-\treturn 0;\n }\n \n static void refresh_index_quietly(void)\n@@ -253,7 +248,7 @@ static void refresh_index_quietly(void)\n \trepo_update_index_if_able(the_repository, &lock_file);\n }\n \n-static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)\n+static void builtin_diff_files(struct rev_info *revs, int argc, const char **argv)\n {\n \tunsigned int options = 0;\n \n@@ -291,7 +286,6 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \t\tdie_errno(\"repo_read_index_preload\");\n \t}\n \trun_diff_files(revs, options);\n-\treturn 0;\n }\n \n struct symdiff {\n@@ -405,7 +399,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n-\tint result = 0;\n+\tint result;\n \tstruct symdiff sdiff;\n \n \t/*\n@@ -584,17 +578,17 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tif (!ent.nr) {\n \t\tswitch (blobs) {\n \t\tcase 0:\n-\t\t\tresult = builtin_diff_files(&rev, argc, argv);\n+\t\t\tbuiltin_diff_files(&rev, argc, argv);\n \t\t\tbreak;\n \t\tcase 1:\n \t\t\tif (paths != 1)\n \t\t\t\tusage(builtin_diff_usage);\n-\t\t\tresult = builtin_diff_b_f(&rev, argc, argv, blob);\n+\t\t\tbuiltin_diff_b_f(&rev, argc, argv, blob);\n \t\t\tbreak;\n \t\tcase 2:\n \t\t\tif (paths)\n \t\t\t\tusage(builtin_diff_usage);\n-\t\t\tresult = builtin_diff_blobs(&rev, argc, argv, blob);\n+\t\t\tbuiltin_diff_blobs(&rev, argc, argv, blob);\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tusage(builtin_diff_usage);\n@@ -603,18 +597,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \telse if (blobs)\n \t\tusage(builtin_diff_usage);\n \telse if (ent.nr == 1)\n-\t\tresult = builtin_diff_index(&rev, argc, argv);\n+\t\tbuiltin_diff_index(&rev, argc, argv);\n \telse if (ent.nr == 2) {\n \t\tif (sdiff.warn)\n \t\t\twarning(_(\"%s...%s: multiple merge bases, using %s\"),\n \t\t\t\tsdiff.left, sdiff.right, sdiff.base);\n-\t\tresult = builtin_diff_tree(&rev, argc, argv,\n-\t\t\t\t\t   &ent.objects[0], &ent.objects[1]);\n+\t\tbuiltin_diff_tree(&rev, argc, argv,\n+\t\t\t\t  &ent.objects[0], &ent.objects[1]);\n \t} else\n-\t\tresult = builtin_diff_combined(&rev, argc, argv,\n-\t\t\t\t\t       ent.objects, ent.nr,\n-\t\t\t\t\t       first_non_parent);\n-\tresult = diff_result_code(&rev.diffopt, result);\n+\t\tbuiltin_diff_combined(&rev, argc, argv,\n+\t\t\t\t      ent.objects, ent.nr,\n+\t\t\t\t      first_non_parent);\n+\tresult = diff_result_code(&rev.diffopt, 0);\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\n \trelease_revisions(&rev);\n-- \n2.42.0.rc2.423.g967ecb4f2b\n\n"},{"id":"480868","messageId":"20230821202046.GG1798590@coredump.intra.peff.net","threadId":"60132","inReplyTo":"20230821201358.GA2663749@coredump.intra.peff.net","subject":"[PATCH v2 7/7] diff: drop useless \"status\" parameter from diff_result_code()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-21T20:20:46Z","receivedAt":"2023-08-21T20:20:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many programs use diff_result_code() to get a user-visible program exit\ncode from a diff result (e.g., checking opts.found_changes if\n--exit-code was requested).\n\nThis function also takes a \"status\" parameter, which seems at first\nglance that it could be used to propagate an error encountered when\ncomputing the diff. But it doesn't work that way:\n\n  - negative values are passed through as-is, but are not appropriate as\n    program exit codes\n\n  - when --exit-code or --check is in effect, we _ignore_ the passed-in\n    status completely. So a failed diff which did not have a chance to\n    set opts.found_changes would erroneously report \"success, no\n    changes\" instead of propagating the error.\n\nAfter recent cleanups, neither of these bugs is possible to trigger, as\nevery caller just passes in \"0\". So rather than fixing them, we can\nsimply drop the useless parameter instead.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/describe.c          | 2 +-\n builtin/diff-files.c        | 2 +-\n builtin/diff-index.c        | 2 +-\n builtin/diff-tree.c         | 2 +-\n builtin/diff.c              | 2 +-\n builtin/log.c               | 2 +-\n builtin/stash.c             | 6 +++---\n builtin/submodule--helper.c | 2 +-\n diff-no-index.c             | 2 +-\n diff.c                      | 6 ++----\n diff.h                      | 2 +-\n wt-status.c                 | 4 ++--\n 12 files changed, 16 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 8cdc25b748..a9e375882b 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -687,7 +687,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\t\tBUG(\"malformed internal diff-index command line\");\n \t\t\trun_diff_index(&revs, 0);\n \n-\t\t\tif (!diff_result_code(&revs.diffopt, 0))\n+\t\t\tif (!diff_result_code(&revs.diffopt))\n \t\t\t\tsuffix = NULL;\n \t\t\telse\n \t\t\t\tsuffix = dirty;\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 04070607b1..f38912cd40 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -83,7 +83,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tif (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0)\n \t\tdie_errno(\"repo_read_index_preload\");\n \trun_diff_files(&rev, options);\n-\tresult = diff_result_code(&rev.diffopt, 0);\n+\tresult = diff_result_code(&rev.diffopt);\n \trelease_revisions(&rev);\n \treturn result;\n }\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 2c6a179832..220f341ffa 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -73,7 +73,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\treturn -1;\n \t}\n \trun_diff_index(&rev, option);\n-\tresult = diff_result_code(&rev.diffopt, 0);\n+\tresult = diff_result_code(&rev.diffopt);\n \trelease_revisions(&rev);\n \treturn result;\n }\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex c9ba35f143..86be634286 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -232,5 +232,5 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tdiff_free(&opt->diffopt);\n \t}\n \n-\treturn diff_result_code(&opt->diffopt, 0);\n+\treturn diff_result_code(&opt->diffopt);\n }\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 3eba691b82..0b313549c7 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -608,7 +608,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tbuiltin_diff_combined(&rev, argc, argv,\n \t\t\t\t      ent.objects, ent.nr,\n \t\t\t\t      first_non_parent);\n-\tresult = diff_result_code(&rev.diffopt, 0);\n+\tresult = diff_result_code(&rev.diffopt);\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\n \trelease_revisions(&rev);\ndiff --git a/builtin/log.c b/builtin/log.c\nindex db3a88bfe9..5d808c92f4 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -549,7 +549,7 @@ static int cmd_log_walk_no_free(struct rev_info *rev)\n \t    rev->diffopt.flags.check_failed) {\n \t\treturn 02;\n \t}\n-\treturn diff_result_code(&rev->diffopt, 0);\n+\treturn diff_result_code(&rev->diffopt);\n }\n \n static int cmd_log_walk(struct rev_info *rev)\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex e799b660f0..53e8868ba1 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -973,7 +973,7 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n \t}\n \tlog_tree_diff_flush(&rev);\n \n-\tret = diff_result_code(&rev.diffopt, 0);\n+\tret = diff_result_code(&rev.diffopt);\n cleanup:\n \tstrvec_clear(&stash_args);\n \tfree_stash_info(&info);\n@@ -1111,13 +1111,13 @@ static int check_changes_tracked_files(const struct pathspec *ps)\n \tdiff_setup_done(&rev.diffopt);\n \n \trun_diff_index(&rev, DIFF_INDEX_CACHED);\n-\tif (diff_result_code(&rev.diffopt, 0)) {\n+\tif (diff_result_code(&rev.diffopt)) {\n \t\tret = 1;\n \t\tgoto done;\n \t}\n \n \trun_diff_files(&rev, 0);\n-\tif (diff_result_code(&rev.diffopt, 0)) {\n+\tif (diff_result_code(&rev.diffopt)) {\n \t\tret = 1;\n \t\tgoto done;\n \t}\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 3764ed1f9c..6f3bf33e61 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -670,7 +670,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \tsetup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);\n \trun_diff_files(&rev, 0);\n \n-\tif (!diff_result_code(&rev.diffopt, 0)) {\n+\tif (!diff_result_code(&rev.diffopt)) {\n \t\tprint_status(flags, ' ', path, ce_oid,\n \t\t\t     displaypath);\n \t} else if (!(flags & OPT_CACHED)) {\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 4771cf02aa..8aead3e332 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -364,7 +364,7 @@ int diff_no_index(struct rev_info *revs,\n \t * The return code for --no-index imitates diff(1):\n \t * 0 = no changes, 1 = changes, else error\n \t */\n-\tret = diff_result_code(&revs->diffopt, 0);\n+\tret = diff_result_code(&revs->diffopt);\n \n out:\n \tfor (i = 0; i < ARRAY_SIZE(to_free); i++)\ndiff --git a/diff.c b/diff.c\nindex ee3eb629e3..2de5d3d098 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6973,16 +6973,14 @@ void diffcore_std(struct diff_options *options)\n \toptions->found_follow = 0;\n }\n \n-int diff_result_code(struct diff_options *opt, int status)\n+int diff_result_code(struct diff_options *opt)\n {\n \tint result = 0;\n \n \tdiff_warn_rename_limit(\"diff.renameLimit\",\n \t\t\t       opt->needed_rename_limit,\n \t\t\t       opt->degraded_cc_to_c);\n-\tif (!opt->flags.exit_with_status &&\n-\t    !(opt->output_format & DIFF_FORMAT_CHECKDIFF))\n-\t\treturn status;\n+\n \tif (opt->flags.exit_with_status &&\n \t    opt->flags.has_changes)\n \t\tresult |= 01;\ndiff --git a/diff.h b/diff.h\nindex 528f00d5e1..caf1528bf0 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -647,7 +647,7 @@ int do_diff_cache(const struct object_id *, struct diff_options *);\n int diff_flush_patch_id(struct diff_options *, struct object_id *, int);\n void flush_one_hunk(struct object_id *result, git_hash_ctx *ctx);\n \n-int diff_result_code(struct diff_options *, int);\n+int diff_result_code(struct diff_options *);\n \n int diff_no_index(struct rev_info *,\n \t\t  int implicit_no_index, int, const char **);\ndiff --git a/wt-status.c b/wt-status.c\nindex 545cea948f..981adb09f3 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2581,7 +2581,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)\n \trev_info.diffopt.flags.quick = 1;\n \tdiff_setup_done(&rev_info.diffopt);\n \trun_diff_files(&rev_info, 0);\n-\tresult = diff_result_code(&rev_info.diffopt, 0);\n+\tresult = diff_result_code(&rev_info.diffopt);\n \trelease_revisions(&rev_info);\n \treturn result;\n }\n@@ -2615,7 +2615,7 @@ int has_uncommitted_changes(struct repository *r,\n \n \tdiff_setup_done(&rev_info.diffopt);\n \trun_diff_index(&rev_info, DIFF_INDEX_CACHED);\n-\tresult = diff_result_code(&rev_info.diffopt, 0);\n+\tresult = diff_result_code(&rev_info.diffopt);\n \trelease_revisions(&rev_info);\n \treturn result;\n }\n-- \n2.42.0.rc2.423.g967ecb4f2b\n"},{"id":"480905","messageId":"xmqqzg2knj5q.fsf@gitster.g","threadId":"60132","inReplyTo":"20230821183611.GB2617193@coredump.intra.peff.net","subject":"Re: [PATCH] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-21T22:08:01Z","receivedAt":"2023-08-21T22:08:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> While this is conceptually orthogonal to what I'm looking at in the\n> other part of the thread, the textual conflicts are really annoying. I'm\n> going to float this to the front of my series and build on top.\n\nOh, sorry, this is not in any hurry so please feel free to ignore or\nswallow or deal with in any way ;-)  I just wasn't aware that you'd\nbe digging further around the area.\n\nTHanks.\n"},{"id":"480939","messageId":"xmqq7cpmirnx.fsf@gitster.g","threadId":"60132","inReplyTo":"20230821201727.GD1798590@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/7] diff: die when failing to read index in git-diff builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-22T23:27:46Z","receivedAt":"2023-08-22T23:27:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> lib-ification purposes, at which point this code would already be doing\n> the right thing).\n\nYup, I very much like that approach and attitude.  Proactively doing\nthe right thing even if the other parts of the code is not yet\nready.  Good.\n\nThanks.\n"},{"id":"480940","messageId":"xmqq350air6e.fsf@gitster.g","threadId":"60132","inReplyTo":"20230821202046.GG1798590@coredump.intra.peff.net","subject":"Re: [PATCH v2 7/7] diff: drop useless \"status\" parameter from diff_result_code()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-22T23:38:17Z","receivedAt":"2023-08-22T23:38:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Many programs use diff_result_code() to get a user-visible program exit\n> code from a diff result (e.g., checking opts.found_changes if\n> --exit-code was requested).\n>\n> This function also takes a \"status\" parameter, which seems at first\n> glance that it could be used to propagate an error encountered when\n> computing the diff. But it doesn't work that way:\n>\n>   - negative values are passed through as-is, but are not appropriate as\n>     program exit codes\n>\n>   - when --exit-code or --check is in effect, we _ignore_ the passed-in\n>     status completely. So a failed diff which did not have a chance to\n>     set opts.found_changes would erroneously report \"success, no\n>     changes\" instead of propagating the error.\n>\n> After recent cleanups, neither of these bugs is possible to trigger, as\n\nHere \"after recent cleanups\" refers to the changes to make them\ndie() upon seeing an error, instead of using it to call this\nfunction with non-zero in the status parameter?  At least, they were\nsignaling errors correctly when --exit-code is not in use, but now\nall the callers are responsible for exiting with non-zero status to\nsignal an error even when --exit-code is *not* used.\n\n> every caller just passes in \"0\". So rather than fixing them, we can\n> simply drop the useless parameter instead.\n\nOK.\n\n"},{"id":"480963","messageId":"20230823190022.GA2355007@coredump.intra.peff.net","threadId":"60132","inReplyTo":"xmqq350air6e.fsf@gitster.g","subject":"Re: [PATCH v2 7/7] diff: drop useless \"status\" parameter from diff_result_code()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-23T19:00:22Z","receivedAt":"2023-08-23T19:01:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 22, 2023 at 04:38:17PM -0700, Junio C Hamano wrote:\n\n> > This function also takes a \"status\" parameter, which seems at first\n> > glance that it could be used to propagate an error encountered when\n> > computing the diff. But it doesn't work that way:\n> >\n> >   - negative values are passed through as-is, but are not appropriate as\n> >     program exit codes\n> >\n> >   - when --exit-code or --check is in effect, we _ignore_ the passed-in\n> >     status completely. So a failed diff which did not have a chance to\n> >     set opts.found_changes would erroneously report \"success, no\n> >     changes\" instead of propagating the error.\n> >\n> > After recent cleanups, neither of these bugs is possible to trigger, as\n> \n> Here \"after recent cleanups\" refers to the changes to make them\n> die() upon seeing an error, instead of using it to call this\n> function with non-zero in the status parameter?  At least, they were\n> signaling errors correctly when --exit-code is not in use, but now\n> all the callers are responsible for exiting with non-zero status to\n> signal an error even when --exit-code is *not* used.\n\nSort of. Prior to this series, callers were responsible for ferrying\nexit codes to diff_result_code(), which mis-handled them (a little for\nthe path without --exit-code, and badly with it). Now they are\nresponsible for handling the errors themselves. I'd say that is not much\nmore work than passing them along, and provides better outcomes (they\ncan produce more useful error messages, or die() as appropriate).\n\nBut what I really meant with \"after recent cleanups\" was just that\nnobody is even bothering to pass anything but 0 to diff_result_code().\nWhich was already true before the series for every code path except the\nfew index/argument parsing paths in builtin/diff.c, and after the\ncleanups is entirely true.\n\n-Peff\n"}]}