commit-graph: detect read errors when verifying graph chain

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c88389df24ed5cb552ca22a4be0e29c625a9ac23..50c15d9bfe7dfbaad388463f7a28fd293607a7cc 100644 (file)
--- 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 a9b2428b56b5d87aa11608328b771d263014798d..a5ac0440f1a1ebb4fea97337cd536f51d6570287 100755 (executable)
--- 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
        )