From patchwork Mon Aug 21 20:14:14 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13359780 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 85D3EEE49A6 for ; Mon, 21 Aug 2023 20:14:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230418AbjHUUOW (ORCPT ); Mon, 21 Aug 2023 16:14:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47274 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230414AbjHUUOV (ORCPT ); Mon, 21 Aug 2023 16:14:21 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1219C13E for ; Mon, 21 Aug 2023 13:14:15 -0700 (PDT) Received: (qmail 17333 invoked by uid 109); 21 Aug 2023 20:14:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 21 Aug 2023 20:14:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18539 invoked by uid 111); 21 Aug 2023 20:14:16 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 21 Aug 2023 16:14:16 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 21 Aug 2023 16:14:14 -0400 From: Jeff King To: Junio C Hamano Cc: Romain Chossart , git@vger.kernel.org Subject: [PATCH v2 1/7] diff: spell DIFF_INDEX_CACHED out when calling run_diff_index() Message-ID: <20230821201414.GA1798590@coredump.intra.peff.net> References: <20230821201358.GA2663749@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230821201358.GA2663749@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org From: 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 Signed-off-by: Jeff King --- 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; From patchwork Mon Aug 21 20:15:10 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13359781 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 58883EE49A5 for ; Mon, 21 Aug 2023 20:15:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230425AbjHUUPN (ORCPT ); Mon, 21 Aug 2023 16:15:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54038 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229564AbjHUUPM (ORCPT ); Mon, 21 Aug 2023 16:15:12 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 88A79BE for ; Mon, 21 Aug 2023 13:15:11 -0700 (PDT) Received: (qmail 17348 invoked by uid 109); 21 Aug 2023 20:15:11 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 21 Aug 2023 20:15:11 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18625 invoked by uid 111); 21 Aug 2023 20:15:11 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 21 Aug 2023 16:15:11 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 21 Aug 2023 16:15:10 -0400 From: Jeff King To: Junio C Hamano Cc: Romain Chossart , git@vger.kernel.org Subject: [PATCH v2 2/7] diff-files: avoid negative exit value Message-ID: <20230821201510.GB1798590@coredump.intra.peff.net> References: <20230821201358.GA2663749@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230821201358.GA2663749@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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; } From patchwork Mon Aug 21 20:16:26 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13359782 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B0ACBEE49A0 for ; Mon, 21 Aug 2023 20:16:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230028AbjHUUQa (ORCPT ); Mon, 21 Aug 2023 16:16:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37558 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229436AbjHUUQ3 (ORCPT ); Mon, 21 Aug 2023 16:16:29 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3574ABE for ; Mon, 21 Aug 2023 13:16:28 -0700 (PDT) Received: (qmail 17355 invoked by uid 109); 21 Aug 2023 20:16:27 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 21 Aug 2023 20:16:27 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18646 invoked by uid 111); 21 Aug 2023 20:16:28 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 21 Aug 2023 16:16:28 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 21 Aug 2023 16:16:26 -0400 From: Jeff King To: Junio C Hamano Cc: Romain Chossart , git@vger.kernel.org Subject: [PATCH v2 3/7] diff: show usage for unknown builtin_diff_files() options Message-ID: <20230821201626.GC1798590@coredump.intra.peff.net> References: <20230821201358.GA2663749@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230821201358.GA2663749@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 Signed-off-by: Jeff King --- 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 From patchwork Mon Aug 21 20:17:27 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13359788 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 38B51EE49A0 for ; Mon, 21 Aug 2023 20:17:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230054AbjHUURa (ORCPT ); Mon, 21 Aug 2023 16:17:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34184 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229436AbjHUUR3 (ORCPT ); Mon, 21 Aug 2023 16:17:29 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3948CBE for ; Mon, 21 Aug 2023 13:17:28 -0700 (PDT) Received: (qmail 17368 invoked by uid 109); 21 Aug 2023 20:17:27 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 21 Aug 2023 20:17:27 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18662 invoked by uid 111); 21 Aug 2023 20:17:28 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 21 Aug 2023 16:17:28 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 21 Aug 2023 16:17:27 -0400 From: Jeff King To: Junio C Hamano Cc: Romain Chossart , git@vger.kernel.org Subject: [PATCH v2 4/7] diff: die when failing to read index in git-diff builtin Message-ID: <20230821201727.GD1798590@coredump.intra.peff.net> References: <20230821201358.GA2663749@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230821201358.GA2663749@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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); } From patchwork Mon Aug 21 20:18:55 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13359789 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 21B5BEE49A5 for ; Mon, 21 Aug 2023 20:18:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230388AbjHUUS7 (ORCPT ); Mon, 21 Aug 2023 16:18:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50376 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229436AbjHUUS6 (ORCPT ); Mon, 21 Aug 2023 16:18:58 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9DF70E4 for ; Mon, 21 Aug 2023 13:18:56 -0700 (PDT) Received: (qmail 17375 invoked by uid 109); 21 Aug 2023 20:18:56 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 21 Aug 2023 20:18:56 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18681 invoked by uid 111); 21 Aug 2023 20:18:56 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 21 Aug 2023 16:18:56 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 21 Aug 2023 16:18:55 -0400 From: Jeff King To: Junio C Hamano Cc: Romain Chossart , git@vger.kernel.org Subject: [PATCH v2 5/7] diff: drop useless return from run_diff_{files,index} functions Message-ID: <20230821201855.GE1798590@coredump.intra.peff.net> References: <20230821201358.GA2663749@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230821201358.GA2663749@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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; } From patchwork Mon Aug 21 20:19:44 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13359790 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 570A4EE49A0 for ; Mon, 21 Aug 2023 20:19:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230418AbjHUUTs (ORCPT ); Mon, 21 Aug 2023 16:19:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46700 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229436AbjHUUTr (ORCPT ); Mon, 21 Aug 2023 16:19:47 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 25759E4 for ; Mon, 21 Aug 2023 13:19:46 -0700 (PDT) Received: (qmail 17386 invoked by uid 109); 21 Aug 2023 20:19:45 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 21 Aug 2023 20:19:45 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18692 invoked by uid 111); 21 Aug 2023 20:19:46 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 21 Aug 2023 16:19:46 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 21 Aug 2023 16:19:44 -0400 From: Jeff King To: Junio C Hamano Cc: Romain Chossart , git@vger.kernel.org Subject: [PATCH v2 6/7] diff: drop useless return values in git-diff helpers Message-ID: <20230821201944.GF1798590@coredump.intra.peff.net> References: <20230821201358.GA2663749@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230821201358.GA2663749@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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); From patchwork Mon Aug 21 20:20:46 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13359791 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A525EEE49A5 for ; Mon, 21 Aug 2023 20:20:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230444AbjHUUUu (ORCPT ); Mon, 21 Aug 2023 16:20:50 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51772 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230441AbjHUUUt (ORCPT ); Mon, 21 Aug 2023 16:20:49 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 74D48E3 for ; Mon, 21 Aug 2023 13:20:47 -0700 (PDT) Received: (qmail 17397 invoked by uid 109); 21 Aug 2023 20:20:47 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 21 Aug 2023 20:20:47 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 18735 invoked by uid 111); 21 Aug 2023 20:20:47 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 21 Aug 2023 16:20:47 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 21 Aug 2023 16:20:46 -0400 From: Jeff King To: Junio C Hamano Cc: Romain Chossart , git@vger.kernel.org Subject: [PATCH v2 7/7] diff: drop useless "status" parameter from diff_result_code() Message-ID: <20230821202046.GG1798590@coredump.intra.peff.net> References: <20230821201358.GA2663749@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230821201358.GA2663749@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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 --- 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; }