[PATCH v2 4/6] commit-graph: detect read errors when verifying graph chain
- From
Jeff King <peff@peff.net>
- Date
- Sep 28, 2023, 04:38 UTC
- Message-ID
- <20230928043859.GD58367@coredump.intra.peff.net>
- In-Reply-To
- <20230928043746.GB57926@coredump.intra.peff.net>
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 <peff@peff.net> --- 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 a9b2428b56..a5ac0440f1 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 (base)' ' ( cd verify-chain-base && 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 )
-- 2.42.0.773.ga6e30199be