From patchwork Tue Sep 26 06:00:13 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13398701 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 11B96E8181D for ; Tue, 26 Sep 2023 06:00:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229722AbjIZGAX (ORCPT ); Tue, 26 Sep 2023 02:00:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41998 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229461AbjIZGAW (ORCPT ); Tue, 26 Sep 2023 02:00:22 -0400 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1A2E8FF for ; Mon, 25 Sep 2023 23:00:15 -0700 (PDT) Received: (qmail 14785 invoked by uid 109); 26 Sep 2023 06:00:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 26 Sep 2023 06:00:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 21475 invoked by uid 111); 26 Sep 2023 06:00:16 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Tue, 26 Sep 2023 02:00:16 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 26 Sep 2023 02:00:13 -0400 From: Jeff King To: git@vger.kernel.org Cc: Taylor Blau , Derrick Stolee Subject: [PATCH 4/6] commit-graph: detect read errors when verifying graph chain Message-ID: <20230926060013.GD1341418@coredump.intra.peff.net> References: <20230926055452.GA1341109@coredump.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230926055452.GA1341109@coredump.intra.peff.net> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Because it's OK to not have a graph file at all, the graph_verify() function needs to tell the difference between a missing file and a real error. So when loading a traditional graph file, we call open_commit_graph() separately from load_commit_graph_chain_fd_st(), and don't complain if the first one fails with ENOENT. When the function learned about chain files in 3da4b609bb (commit-graph: verify chains with --shallow mode, 2019-06-18), we couldn't be as careful, since the only way to load a chain was with read_commit_graph_one(), which did both the open/load as a single unit. So we'll miss errors in chain files we load, thinking instead that there was just no chain file at all. Note that we do still report some of these problems to stderr, as the loading function calls error() and warning(). But we'd exit with a successful exit code, which is wrong. We can fix that by using the recently split open/load functions for chains. That lets us treat the chain file just like a single file with respect to error handling here. An existing test (from 3da4b609bb) shows off the problem; we were expecting "commit-graph verify" to report success, but that makes no sense. We did not even verify the contents of the graph data, because we couldn't load it! I don't think this was an intentional exception, but rather just the test covering what happened to occur. Signed-off-by: Jeff King --- builtin/commit-graph.c | 23 ++++++++++++++++------- t/t5324-split-commit-graph.sh | 4 ++-- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c index c88389df24..50c15d9bfe 100644 --- a/builtin/commit-graph.c +++ b/builtin/commit-graph.c @@ -69,7 +69,8 @@ static int graph_verify(int argc, const char **argv, const char *prefix) struct commit_graph *graph = NULL; struct object_directory *odb = NULL; char *graph_name; - int open_ok; + char *chain_name; + enum { OPENED_NONE, OPENED_GRAPH, OPENED_CHAIN } opened = OPENED_NONE; int fd; struct stat st; int flags = 0; @@ -102,21 +103,29 @@ static int graph_verify(int argc, const char **argv, const char *prefix) odb = find_odb(the_repository, opts.obj_dir); graph_name = get_commit_graph_filename(odb); - open_ok = open_commit_graph(graph_name, &fd, &st); - if (!open_ok && errno != ENOENT) + chain_name = get_commit_graph_chain_filename(odb); + if (open_commit_graph(graph_name, &fd, &st)) + opened = OPENED_GRAPH; + else if (errno != ENOENT) die_errno(_("Could not open commit-graph '%s'"), graph_name); + else if (open_commit_graph_chain(chain_name, &fd, &st)) + opened = OPENED_CHAIN; + else if (errno != ENOENT) + die_errno(_("could not open commit-graph chain '%s'"), chain_name); FREE_AND_NULL(graph_name); + FREE_AND_NULL(chain_name); FREE_AND_NULL(options); - if (open_ok) + if (opened == OPENED_NONE) + return 0; + else if (opened == OPENED_GRAPH) graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb); else - graph = read_commit_graph_one(the_repository, odb); + graph = load_commit_graph_chain_fd_st(the_repository, fd, &st); - /* Return failure if open_ok predicted success */ if (!graph) - return !!open_ok; + return 1; ret = verify_commit_graph(the_repository, graph, flags); free_commit_graph(graph); diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index e335ef87a6..0ac7bbd1dc 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -317,11 +317,11 @@ test_expect_success 'verify after commit-graph-chain corruption' ' ( cd verify-chain && corrupt_file "$graphdir/commit-graph-chain" 30 "G" && - git commit-graph verify 2>test_err && + test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err && test_i18ngrep "invalid commit-graph chain" err && corrupt_file "$graphdir/commit-graph-chain" 30 "A" && - git commit-graph verify 2>test_err && + test_must_fail git commit-graph verify 2>test_err && grep -v "^+" test_err >err && test_i18ngrep "unable to find all commit-graph files" err )