{"thread":{"id":"60268","subject":"[PATCH 0/6] some \"commit-graph verify\" fixes for chains","startedAt":"2023-09-26T05:55:01Z","lastAt":"2023-10-05T17:38:45Z","messageCount":16,"participants":["Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"482320","messageId":"20230926055452.GA1341109@coredump.intra.peff.net","threadId":"60268","inReplyTo":null,"subject":"[PATCH 0/6] some \"commit-graph verify\" fixes for chains","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-26T05:54:52Z","receivedAt":"2023-09-26T05:55:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While yak-shaving another topic, I noticed that there are a bunch of\nerror cases that \"git commit-graph verify\" does not notice when checking\nchained graph files. This series should make it more robust.\n\nSome of these cases are actually covered in t5324 already, but the tests\nexpect the verify command not to fail, which just seems wrong. I don't\nsee anything in the commit message or mailing list explaining it, so I\nthink it was just \"well, this is how it behaves now\", and not some\nclever scheme.\n\n  [1/6]: commit-graph: factor out chain opening function\n  [2/6]: commit-graph: check mixed generation validation when loading chain file\n  [3/6]: t5324: harmonize sha1/sha256 graph chain corruption\n  [4/6]: commit-graph: detect read errors when verifying graph chain\n  [5/6]: commit-graph: tighten chain size check\n  [6/6]: commit-graph: report incomplete chains during verification\n\n builtin/commit-graph.c        |  31 +++++++---\n commit-graph.c                | 109 ++++++++++++++++++++++------------\n commit-graph.h                |   4 ++\n t/t5324-split-commit-graph.sh |  48 +++++++++++++--\n 4 files changed, 141 insertions(+), 51 deletions(-)\n\n-Peff\n\nPS If you are curious about the yak-shave, it is:\n\n     3. \"commit-graph verify\" fails to propagate some errors (this\n\tseries)\n     2. we are missing a bunch of bounds checks on commit-graph files;\n\tfixing that makes the tests inconsistent because of (3)\n     1. there are some unused parameters that should be used for bounds\n\tchecks, hence (2)\n\n  Just in case anybody thought there was a part of my life that was not\n  motivated by removing -Wunused-parameter warnings.\n"},{"id":"482321","messageId":"20230926055657.GA1341418@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230926055452.GA1341109@coredump.intra.peff.net","subject":"[PATCH 1/6] commit-graph: factor out chain opening function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-26T05:56:57Z","receivedAt":"2023-09-26T05:57:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The load_commit_graph_chain() function opens the chain file and all of\nthe slices of graph that it points to. If there is no chain file (which\nis a totally normal condition), we return NULL. But if we run into\nerrors with the chain file or loading the actual graph data, we also\nreturn NULL, and the caller cannot tell the difference.\n\nThe caller can check for ENOENT for the unremarkable \"no such file\"\ncase. But I'm hesitant to assume that the rest of the function would\nnever accidentally set errno to ENOENT itself, since it is opening the\nslice files (and that would mean the caller fails to notice a real\nerror).\n\nSo let's break this into two functions: one to open the file, and one to\nactually load it. This matches the interface we provide for the\nnon-chain graph file, which will also come in handy in a moment when we\nfix some bugs in the \"git commit-graph verify\" code.\n\nSome notes:\n\n  - I've kept the \"1 is good, 0 is bad\" return convention (and the weird\n    \"fd\" out-parameter) used by the matching open_commit_graph()\n    function and other parts of the commit-graph code. This is unlike\n    most of the rest of Git (which would just return the fd, with -1 for\n    error), but it makes sense to stay consistent with the adjacent bits\n    of the API here.\n\n  - The existing chain loading function will quietly return if the file\n    is too small to hold a single entry. I've retained that behavior\n    (and explicitly set ENOENT in the opener function) for now, under\n    the notion that it's probably valid (though I'd imagine unusual) to\n    have an empty chain file.\n\nThere are two small behavior changes here, but I think both are strictly\npositive:\n\n  1. The original blindly did a stat() before checking if fopen()\n     succeeded, meaning we were making a pointless extra stat call.\n\n  2. We now use fstat() to check the file size. The previous code using\n     a regular stat() on the pathname meant we could technically race\n     with somebody updating the chain file, and end up with a size that\n     does not match what we just opened with fopen(). I doubt anybody\n     ever hit this in practice, but it may have caused an out-of-bounds\n     read.\n\nWe'll retain the load_commit_graph_chain() function which does both the\nopen and reading steps (most existing callers do not care about seeing\nerrors anyway, since loading commit-graphs is optimistic).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c | 58 +++++++++++++++++++++++++++++++++-----------------\n commit-graph.h |  3 +++\n 2 files changed, 42 insertions(+), 19 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5e8a3a5085..12cdd9af8e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -513,31 +513,34 @@ static int add_graph_to_chain(struct commit_graph *g,\n \treturn 1;\n }\n \n-static struct commit_graph *load_commit_graph_chain(struct repository *r,\n-\t\t\t\t\t\t    struct object_directory *odb)\n+int open_commit_graph_chain(const char *chain_file,\n+\t\t\t    int *fd, struct stat *st)\n+{\n+\t*fd = git_open(chain_file);\n+\tif (*fd < 0)\n+\t\treturn 0;\n+\tif (fstat(*fd, st)) {\n+\t\tclose(*fd);\n+\t\treturn 0;\n+\t}\n+\tif (st->st_size <= the_hash_algo->hexsz) {\n+\t\tclose(*fd);\n+\t\terrno = ENOENT;\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n+struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n+\t\t\t\t\t\t   int fd, struct stat *st)\n {\n \tstruct commit_graph *graph_chain = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n-\tstruct stat st;\n \tstruct object_id *oids;\n \tint i = 0, valid = 1, count;\n-\tchar *chain_name = get_commit_graph_chain_filename(odb);\n-\tFILE *fp;\n-\tint stat_res;\n+\tFILE *fp = xfdopen(fd, \"r\");\n \n-\tfp = fopen(chain_name, \"r\");\n-\tstat_res = stat(chain_name, &st);\n-\tfree(chain_name);\n-\n-\tif (!fp)\n-\t\treturn NULL;\n-\tif (stat_res ||\n-\t    st.st_size <= the_hash_algo->hexsz) {\n-\t\tfclose(fp);\n-\t\treturn NULL;\n-\t}\n-\n-\tcount = st.st_size / (the_hash_algo->hexsz + 1);\n+\tcount = st->st_size / (the_hash_algo->hexsz + 1);\n \tCALLOC_ARRAY(oids, count);\n \n \tprepare_alt_odb(r);\n@@ -585,6 +588,23 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \treturn graph_chain;\n }\n \n+static struct commit_graph *load_commit_graph_chain(struct repository *r,\n+\t\t\t\t\t\t    struct object_directory *odb)\n+{\n+\tchar *chain_file = get_commit_graph_chain_filename(odb);\n+\tstruct stat st;\n+\tint fd;\n+\tstruct commit_graph *g = NULL;\n+\n+\tif (open_commit_graph_chain(chain_file, &fd, &st)) {\n+\t\t/* ownership of fd is taken over by load function */\n+\t\tg = load_commit_graph_chain_fd_st(r, fd, &st);\n+\t}\n+\n+\tfree(chain_file);\n+\treturn g;\n+}\n+\n /*\n  * returns 1 if and only if all graphs in the chain have\n  * corrected commit dates stored in the generation_data chunk.\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 5e534f0fcc..3b662fd2b5 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -26,6 +26,7 @@ struct string_list;\n char *get_commit_graph_filename(struct object_directory *odb);\n char *get_commit_graph_chain_filename(struct object_directory *odb);\n int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n+int open_commit_graph_chain(const char *chain_file, int *fd, struct stat *st);\n \n /*\n  * Given a commit struct, try to fill the commit struct info, including:\n@@ -105,6 +106,8 @@ struct commit_graph {\n struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n \t\t\t\t\t\t int fd, struct stat *st,\n \t\t\t\t\t\t struct object_directory *odb);\n+struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n+\t\t\t\t\t\t   int fd, struct stat *st);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n \n-- \n2.42.0.758.gd56856b565\n\n"},{"id":"482322","messageId":"20230926055751.GB1341418@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230926055452.GA1341109@coredump.intra.peff.net","subject":"[PATCH 2/6] commit-graph: check mixed generation validation when loading chain file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-26T05:57:51Z","receivedAt":"2023-09-26T05:57:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In read_commit_graph_one(), we call validate_mixed_generation_chain()\nafter loading the graph. Even though we don't check the return value,\nthis has the side effect of clearing the read_generation_data flag,\nwhich is important when working with mixed generation numbers.\n\nBut doing this in load_commit_graph_chain_fd_st() makes more sense:\n\n  1. We are calling it even when we did not load a chain at all, which\n     is pointless (you cannot have mixed generations in a single file).\n\n  2. For now, all callers load the graph via read_commit_graph_one().\n     But the point of factoring out the open/load in the previous commit\n     was to let \"commit-graph verify\" call them separately. So it needs\n     to trigger this function as part of the load.\n\n     Without this patch, the mixed-generation tests in t5324 would start\n     failing on \"git commit-graph verify\" calls, once we switch to using\n     a separate open/load call there.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c | 54 +++++++++++++++++++++++++-------------------------\n 1 file changed, 27 insertions(+), 27 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 12cdd9af8e..8b29c6de24 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -473,6 +473,31 @@ static struct commit_graph *load_commit_graph_v1(struct repository *r,\n \treturn g;\n }\n \n+/*\n+ * returns 1 if and only if all graphs in the chain have\n+ * corrected commit dates stored in the generation_data chunk.\n+ */\n+static int validate_mixed_generation_chain(struct commit_graph *g)\n+{\n+\tint read_generation_data = 1;\n+\tstruct commit_graph *p = g;\n+\n+\twhile (read_generation_data && p) {\n+\t\tread_generation_data = p->read_generation_data;\n+\t\tp = p->base_graph;\n+\t}\n+\n+\tif (read_generation_data)\n+\t\treturn 1;\n+\n+\twhile (g) {\n+\t\tg->read_generation_data = 0;\n+\t\tg = g->base_graph;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int add_graph_to_chain(struct commit_graph *g,\n \t\t\t      struct commit_graph *chain,\n \t\t\t      struct object_id *oids,\n@@ -581,6 +606,8 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n \t\t}\n \t}\n \n+\tvalidate_mixed_generation_chain(graph_chain);\n+\n \tfree(oids);\n \tfclose(fp);\n \tstrbuf_release(&line);\n@@ -605,31 +632,6 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \treturn g;\n }\n \n-/*\n- * returns 1 if and only if all graphs in the chain have\n- * corrected commit dates stored in the generation_data chunk.\n- */\n-static int validate_mixed_generation_chain(struct commit_graph *g)\n-{\n-\tint read_generation_data = 1;\n-\tstruct commit_graph *p = g;\n-\n-\twhile (read_generation_data && p) {\n-\t\tread_generation_data = p->read_generation_data;\n-\t\tp = p->base_graph;\n-\t}\n-\n-\tif (read_generation_data)\n-\t\treturn 1;\n-\n-\twhile (g) {\n-\t\tg->read_generation_data = 0;\n-\t\tg = g->base_graph;\n-\t}\n-\n-\treturn 0;\n-}\n-\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb)\n {\n@@ -638,8 +640,6 @@ struct commit_graph *read_commit_graph_one(struct repository *r,\n \tif (!g)\n \t\tg = load_commit_graph_chain(r, odb);\n \n-\tvalidate_mixed_generation_chain(g);\n-\n \treturn g;\n }\n \n-- \n2.42.0.758.gd56856b565\n\n"},{"id":"482323","messageId":"20230926055836.GC1341418@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230926055452.GA1341109@coredump.intra.peff.net","subject":"[PATCH 3/6] t5324: harmonize sha1/sha256 graph chain corruption","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-26T05:58:36Z","receivedAt":"2023-09-26T05:58:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In t5324.20, we corrupt a hex character 60 bytes into the graph chain\nfile. Since the file consists of two hash identifiers, one per line, the\ncorruption differs between sha1 and sha256. In a sha1 repository, the\ncorruption is on the second line, and in a sha256 repository, it is on\nthe first.\n\nWe should of course detect the problem with either line. But as the next\nfew patches will show (and fix), that is not the case (in fact, we\ncurrently do not exit non-zero for either line!). And while at the end\nof our series we'll catch all errors, our intermediate states will have\ndiffering behavior between the two hashes.\n\nLet's make this test behave consistently with either hash by always\ncorrupting the first line. We'll add additional tests that explicitly\ncover the second line as we fix those bugs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5324-split-commit-graph.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 36c4141e67..e335ef87a6 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -316,11 +316,11 @@ test_expect_success 'verify after commit-graph-chain corruption' '\n \tgit clone --no-hardlinks . verify-chain &&\n \t(\n \t\tcd verify-chain &&\n-\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 60 \"G\" &&\n+\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"G\" &&\n \t\tgit commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"invalid commit-graph chain\" err &&\n-\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 60 \"A\" &&\n+\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"A\" &&\n \t\tgit commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"unable to find all commit-graph files\" err\n-- \n2.42.0.758.gd56856b565\n\n"},{"id":"482324","messageId":"20230926060013.GD1341418@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230926055452.GA1341109@coredump.intra.peff.net","subject":"[PATCH 4/6] commit-graph: detect read errors when verifying graph chain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-26T06:00:13Z","receivedAt":"2023-09-26T06:00:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because it's OK to not have a graph file at all, the graph_verify()\nfunction needs to tell the difference between a missing file and a real\nerror.  So when loading a traditional graph file, we call\nopen_commit_graph() separately from load_commit_graph_chain_fd_st(), and\ndon't complain if the first one fails with ENOENT.\n\nWhen the function learned about chain files in 3da4b609bb (commit-graph:\nverify chains with --shallow mode, 2019-06-18), we couldn't be as\ncareful, since the only way to load a chain was with\nread_commit_graph_one(), which did both the open/load as a single unit.\nSo we'll miss errors in chain files we load, thinking instead that there\nwas just no chain file at all.\n\nNote that we do still report some of these problems to stderr, as the\nloading function calls error() and warning(). But we'd exit with a\nsuccessful exit code, which is wrong.\n\nWe can fix that by using the recently split open/load functions for\nchains. That lets us treat the chain file just like a single file with\nrespect to error handling here.\n\nAn existing test (from 3da4b609bb) shows off the problem; we were\nexpecting \"commit-graph verify\" to report success, but that makes no\nsense. We did not even verify the contents of the graph data, because we\ncouldn't load it! I don't think this was an intentional exception, but\nrather just the test covering what happened to occur.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit-graph.c        | 23 ++++++++++++++++-------\n t/t5324-split-commit-graph.sh |  4 ++--\n 2 files changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c88389df24..50c15d9bfe 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -69,7 +69,8 @@ static int graph_verify(int argc, const char **argv, const char *prefix)\n \tstruct commit_graph *graph = NULL;\n \tstruct object_directory *odb = NULL;\n \tchar *graph_name;\n-\tint open_ok;\n+\tchar *chain_name;\n+\tenum { OPENED_NONE, OPENED_GRAPH, OPENED_CHAIN } opened = OPENED_NONE;\n \tint fd;\n \tstruct stat st;\n \tint flags = 0;\n@@ -102,21 +103,29 @@ static int graph_verify(int argc, const char **argv, const char *prefix)\n \n \todb = find_odb(the_repository, opts.obj_dir);\n \tgraph_name = get_commit_graph_filename(odb);\n-\topen_ok = open_commit_graph(graph_name, &fd, &st);\n-\tif (!open_ok && errno != ENOENT)\n+\tchain_name = get_commit_graph_chain_filename(odb);\n+\tif (open_commit_graph(graph_name, &fd, &st))\n+\t\topened = OPENED_GRAPH;\n+\telse if (errno != ENOENT)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n+\telse if (open_commit_graph_chain(chain_name, &fd, &st))\n+\t\topened = OPENED_CHAIN;\n+\telse if (errno != ENOENT)\n+\t\tdie_errno(_(\"could not open commit-graph chain '%s'\"), chain_name);\n \n \tFREE_AND_NULL(graph_name);\n+\tFREE_AND_NULL(chain_name);\n \tFREE_AND_NULL(options);\n \n-\tif (open_ok)\n+\tif (opened == OPENED_NONE)\n+\t\treturn 0;\n+\telse if (opened == OPENED_GRAPH)\n \t\tgraph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);\n \telse\n-\t\tgraph = read_commit_graph_one(the_repository, odb);\n+\t\tgraph = load_commit_graph_chain_fd_st(the_repository, fd, &st);\n \n-\t/* Return failure if open_ok predicted success */\n \tif (!graph)\n-\t\treturn !!open_ok;\n+\t\treturn 1;\n \n \tret = verify_commit_graph(the_repository, graph, flags);\n \tfree_commit_graph(graph);\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex e335ef87a6..0ac7bbd1dc 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -317,11 +317,11 @@ test_expect_success 'verify after commit-graph-chain corruption' '\n \t(\n \t\tcd verify-chain &&\n \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"G\" &&\n-\t\tgit commit-graph verify 2>test_err &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"invalid commit-graph chain\" err &&\n \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"A\" &&\n-\t\tgit commit-graph verify 2>test_err &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"unable to find all commit-graph files\" err\n \t)\n-- \n2.42.0.758.gd56856b565\n\n"},{"id":"482325","messageId":"20230926060300.GE1341418@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230926055452.GA1341109@coredump.intra.peff.net","subject":"[PATCH 5/6] commit-graph: tighten chain size check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-26T06:03:00Z","receivedAt":"2023-09-26T06:03:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we open a commit-graph-chain file, if it's smaller than a single\nentry, we just quietly treat that as ENOENT. That make some sense if the\nfile is truly zero bytes, but it means that \"commit-graph verify\" will\nquietly ignore a file that contains garbage if that garbage happens to\nbe short.\n\nInstead, let's only simulate ENOENT when the file is truly empty, and\notherwise return EINVAL. The normal graph-loading routines don't care,\nbut \"commit-graph verify\" will notice and complain about the difference.\n\nIt's not entirely clear to me that the 0-is-ENOENT case actually happens\nin real life, so we could perhaps just eliminate this special-case\naltogether. But this is how we've always behaved, so I'm preserving it\nin the name of backwards compatibility (though again, it really only\nmatters for \"verify\", as the regular routines are happy to load what\nthey can).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThere's a related issue which I didn't fix in this series, which is that\nthe \"load\" function does a truncating division of (size / hash_size + 1)\nto find the number of entries (the +1 is for the newline). So a\nless-than-hash-sized chunk of garbage at the end of the file will be\nignored.\n\nThe simplest way to fix that is that we can insist that the size mod\nhash_size+1 is 0. But I was wary of being too picky here (e.g., what\nabout a file that doesn't end with newline) especially in a way that\naffected more than just \"verify\".\n\n commit-graph.c                | 10 ++++++++--\n t/t5324-split-commit-graph.sh | 12 ++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 8b29c6de24..b1d3e5bf94 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -548,9 +548,15 @@ int open_commit_graph_chain(const char *chain_file,\n \t\tclose(*fd);\n \t\treturn 0;\n \t}\n-\tif (st->st_size <= the_hash_algo->hexsz) {\n+\tif (st->st_size < the_hash_algo->hexsz) {\n \t\tclose(*fd);\n-\t\terrno = ENOENT;\n+\t\tif (!st->st_size) {\n+\t\t\t/* treat empty files the same as missing */\n+\t\t\terrno = ENOENT;\n+\t\t} else {\n+\t\t\twarning(\"commit-graph chain file too small\");\n+\t\t\terrno = EINVAL;\n+\t\t}\n \t\treturn 0;\n \t}\n \treturn 1;\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 0ac7bbd1dc..86d56debf6 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -327,6 +327,18 @@ test_expect_success 'verify after commit-graph-chain corruption' '\n \t)\n '\n \n+test_expect_success 'verify notices too-short chain file' '\n+\tgit clone --no-hardlinks . verify-chain-garbage &&\n+\t(\n+\t\tcd verify-chain-garbage &&\n+\t\tgit commit-graph verify &&\n+\t\techo \"garbage\" >$graphdir/commit-graph-chain &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n+\t\tgrep -v \"^+\" test_err >err &&\n+\t\tgrep \"commit-graph chain file too small\" err\n+\t)\n+'\n+\n test_expect_success 'verify across alternates' '\n \tgit clone --no-hardlinks . verify-alt &&\n \t(\n-- \n2.42.0.758.gd56856b565\n\n"},{"id":"482326","messageId":"20230926060430.GF1341418@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230926055452.GA1341109@coredump.intra.peff.net","subject":"[PATCH 6/6] commit-graph: report incomplete chains during verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-26T06:04:30Z","receivedAt":"2023-09-26T06:04:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The load_commit_graph_chain_fd_st() function will stop loading chains\nwhen it sees an error. But if it has loaded any graph slice at all, it\nwill return it. This is a good thing for normal use (we use what data we\ncan, and this is just an optimization). But it's a bad thing for\n\"commit-graph verify\", which should be careful about finding any\nirregularities. We do complain to stderr with a warning(), but the\nverify command still exits with a successful return code.\n\nThe new tests here cover corruption of both the base and tip slices of\nthe chain. The corruption of the base file already works (it is the\nfirst file we look at, so when we see the error we return NULL). The\n\"tip\" case is what is fixed by this patch (it complains to stderr but\nstill returns the base slice).\n\nNote that this also causes us to adjust a test later in the file that\nsimilarly corrupts a tip (though confusingly the test script calls this\n\"base\"). It checks stderr but erroneously expects the whole \"verify\"\ncommand to exit with a successful code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit-graph.c        | 10 +++++++++-\n commit-graph.c                |  9 +++++++--\n commit-graph.h                |  3 ++-\n t/t5324-split-commit-graph.sh | 28 +++++++++++++++++++++++++++-\n 4 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 50c15d9bfe..f5e66e9969 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -74,6 +74,7 @@ static int graph_verify(int argc, const char **argv, const char *prefix)\n \tint fd;\n \tstruct stat st;\n \tint flags = 0;\n+\tint incomplete_chain = 0;\n \tint ret;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n@@ -122,13 +123,20 @@ static int graph_verify(int argc, const char **argv, const char *prefix)\n \telse if (opened == OPENED_GRAPH)\n \t\tgraph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);\n \telse\n-\t\tgraph = load_commit_graph_chain_fd_st(the_repository, fd, &st);\n+\t\tgraph = load_commit_graph_chain_fd_st(the_repository, fd, &st,\n+\t\t\t\t\t\t      &incomplete_chain);\n \n \tif (!graph)\n \t\treturn 1;\n \n \tret = verify_commit_graph(the_repository, graph, flags);\n \tfree_commit_graph(graph);\n+\n+\tif (incomplete_chain) {\n+\t\terror(\"one or more commit-graph chain files could not be loaded\");\n+\t\tret |= 1;\n+\t}\n+\n \treturn ret;\n }\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex b1d3e5bf94..148af50058 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -563,14 +563,17 @@ int open_commit_graph_chain(const char *chain_file,\n }\n \n struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n-\t\t\t\t\t\t   int fd, struct stat *st)\n+\t\t\t\t\t\t   int fd, struct stat *st,\n+\t\t\t\t\t\t   int *incomplete_chain)\n {\n \tstruct commit_graph *graph_chain = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct object_id *oids;\n \tint i = 0, valid = 1, count;\n \tFILE *fp = xfdopen(fd, \"r\");\n \n+\t*incomplete_chain = 0;\n+\n \tcount = st->st_size / (the_hash_algo->hexsz + 1);\n \tCALLOC_ARRAY(oids, count);\n \n@@ -608,6 +611,7 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n \n \t\tif (!valid) {\n \t\t\twarning(_(\"unable to find all commit-graph files\"));\n+\t\t\t*incomplete_chain = 1;\n \t\t\tbreak;\n \t\t}\n \t}\n@@ -630,8 +634,9 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \tstruct commit_graph *g = NULL;\n \n \tif (open_commit_graph_chain(chain_file, &fd, &st)) {\n+\t\tint incomplete;\n \t\t/* ownership of fd is taken over by load function */\n-\t\tg = load_commit_graph_chain_fd_st(r, fd, &st);\n+\t\tg = load_commit_graph_chain_fd_st(r, fd, &st, &incomplete);\n \t}\n \n \tfree(chain_file);\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 3b662fd2b5..20ada7e891 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -107,7 +107,8 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n \t\t\t\t\t\t int fd, struct stat *st,\n \t\t\t\t\t\t struct object_directory *odb);\n struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n-\t\t\t\t\t\t   int fd, struct stat *st);\n+\t\t\t\t\t\t   int fd, struct stat *st,\n+\t\t\t\t\t\t   int *incomplete_chain);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n \ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 86d56debf6..598660557a 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -285,6 +285,32 @@ test_expect_success 'verify hashes along chain, even in shallow' '\n \t)\n '\n \n+test_expect_success 'verify notices chain slice which is bogus (base)' '\n+\tgit clone --no-hardlinks . verify-chain-bogus-base &&\n+\t(\n+\t\tcd verify-chain-bogus-base &&\n+\t\tgit commit-graph verify &&\n+\t\tbase_file=$graphdir/graph-$(sed -n 1p $graphdir/commit-graph-chain).graph &&\n+\t\techo \"garbage\" >$base_file &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n+\t\tgrep -v \"^+\" test_err >err &&\n+\t\tgrep \"commit-graph file is too small\" err\n+\t)\n+'\n+\n+test_expect_success 'verify notices chain slice which is bogus (tip)' '\n+\tgit clone --no-hardlinks . verify-chain-bogus-tip &&\n+\t(\n+\t\tcd verify-chain-bogus-tip &&\n+\t\tgit commit-graph verify &&\n+\t\ttip_file=$graphdir/graph-$(sed -n 2p $graphdir/commit-graph-chain).graph &&\n+\t\techo \"garbage\" >$tip_file &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n+\t\tgrep -v \"^+\" test_err >err &&\n+\t\tgrep \"commit-graph file is too small\" err\n+\t)\n+'\n+\n test_expect_success 'verify --shallow does not check base contents' '\n \tgit clone --no-hardlinks . verify-shallow &&\n \t(\n@@ -306,7 +332,7 @@ test_expect_success 'warn on base graph chunk incorrect' '\n \t\tgit commit-graph verify &&\n \t\tbase_file=$graphdir/graph-$(tail -n 1 $graphdir/commit-graph-chain).graph &&\n \t\tcorrupt_file \"$base_file\" $(test_oid base) \"\\01\" &&\n-\t\tgit commit-graph verify --shallow 2>test_err &&\n+\t\ttest_must_fail git commit-graph verify --shallow 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"commit-graph chain does not match\" err\n \t)\n-- \n2.42.0.758.gd56856b565\n"},{"id":"482419","messageId":"20230928043058.GA57926@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230926060430.GF1341418@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] commit-graph: report incomplete chains during verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-28T04:30:58Z","receivedAt":"2023-09-28T04:31:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 26, 2023 at 02:04:30AM -0400, Jeff King wrote:\n\n> @@ -608,6 +611,7 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n>  \n>  \t\tif (!valid) {\n>  \t\t\twarning(_(\"unable to find all commit-graph files\"));\n> +\t\t\t*incomplete_chain = 1;\n>  \t\t\tbreak;\n>  \t\t}\n>  \t}\n\nReviewing my own patch since I noticed it misses a case. ;)\n\nThe whole function here looks like (abbreviated):\n\n  int valid = 1;\n\n  for (i = 0; i < count; i++) {\n          if (get_oid_hex(line.buf, &oids[i])) {\n                  warning(_(\"invalid commit-graph chain: line '%s' not a hash\"),\n                          line.buf);\n                  valid = 0;\n                  break;\n          }\n  \n          valid = 0;\n          for (odb = r->objects->odb; odb; odb = odb->next) {\n                  struct commit_graph *g = load_commit_graph_one(r, graph_name, odb);\n                  if (g) {\n                          if (add_graph_to_chain(g, graph_chain, oids, i)) {\n                                  graph_chain = g;\n                                  valid = 1;\n                          }\n  \n                          break;\n                  }\n          }\n  \n          if (!valid) {\n                  warning(_(\"unable to find all commit-graph files\"));\n                  break;\n          }\n  }\n\nMy patch updates the final \"if (!valid)\" check, which covers anything\nthat happened in the loop over the odb (i.e., finding and opening the\nfile itself). But if we see a line which does not parse as an oid, we\nbreak out of the outer loop earlier. We set \"valid\", but nobody looks at\nit! So the caller would not be correctly notified that we had an error\nin that case.\n\nThe fix is simple: we can just check \"valid\" after leaving the outer\nloop, which covers both cases. And we'll want an extra test to check it.\nThis is actually covered by the test modified earlier in patch 3, where\nsha1 and sha256 produced different results. I fixed it there by\nconsistently corrupting the first line of the file, but we really want\nto check both cases.\n\nI'll send a re-roll in a moment.\n\n-Peff\n"},{"id":"482420","messageId":"20230928043746.GB57926@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230926055452.GA1341109@coredump.intra.peff.net","subject":"[PATCH v2 0/6] some \"commit-graph verify\" fixes for chains","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-28T04:37:46Z","receivedAt":"2023-09-28T04:37:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 26, 2023 at 01:54:52AM -0400, Jeff King wrote:\n\n>   [1/6]: commit-graph: factor out chain opening function\n>   [2/6]: commit-graph: check mixed generation validation when loading chain file\n>   [3/6]: t5324: harmonize sha1/sha256 graph chain corruption\n>   [4/6]: commit-graph: detect read errors when verifying graph chain\n>   [5/6]: commit-graph: tighten chain size check\n>   [6/6]: commit-graph: report incomplete chains during verification\n\nHere's a re-roll that fixes a missed case in patch 6 (I sent more\ndetails elsewhere in the thread).\n\nRange-diff is below, but the changes are:\n\n  - patch 3 extends its tests to cover corrupting both the first and\n    second lines of the file. The test and directory renaming spills\n    over into the range-diff context for the other patches (and I\n    renamed the directory used in the \"too-short chain file\" test in\n    patch 5 to align better with the others).\n\n  - patch 6 covers the case that get_oid_hex() fails (previously it only\n    detected that a parsed hex failed to result in us loading a valid\n    graph file). The range-diff is IMHO quite hard to read here, but the\n    patch itself is quite simple.\n\n1:  baa20e6391 = 1:  a4893f325f commit-graph: factor out chain opening function\n2:  b77cc15584 = 2:  722181d8ed commit-graph: check mixed generation validation when loading chain file\n3:  af350d1d41 < -:  ---------- t5324: harmonize sha1/sha256 graph chain corruption\n-:  ---------- > 3:  7de47054c7 t5324: harmonize sha1/sha256 graph chain corruption\n4:  54ce3863f8 ! 4:  313c051a38 commit-graph: detect read errors when verifying graph chain\n    @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv, con\n      \tfree_commit_graph(graph);\n     \n      ## t/t5324-split-commit-graph.sh ##\n    -@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption' '\n    +@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (base)' '\n      \t(\n    - \t\tcd verify-chain &&\n    + \t\tcd verify-chain-base &&\n      \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"G\" &&\n     -\t\tgit commit-graph verify 2>test_err &&\n     +\t\ttest_must_fail git commit-graph verify 2>test_err &&\n5:  273f5e8b87 ! 5:  6d9efd761a commit-graph: tighten chain size check\n    @@ commit-graph.c: int open_commit_graph_chain(const char *chain_file,\n      \treturn 1;\n     \n      ## t/t5324-split-commit-graph.sh ##\n    -@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption' '\n    +@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (tip)' '\n      \t)\n      '\n      \n     +test_expect_success 'verify notices too-short chain file' '\n    -+\tgit clone --no-hardlinks . verify-chain-garbage &&\n    ++\tgit clone --no-hardlinks . verify-chain-short &&\n     +\t(\n    -+\t\tcd verify-chain-garbage &&\n    ++\t\tcd verify-chain-short &&\n     +\t\tgit commit-graph verify &&\n     +\t\techo \"garbage\" >$graphdir/commit-graph-chain &&\n     +\t\ttest_must_fail git commit-graph verify 2>test_err &&\n6:  2661efed03 ! 6:  8fc82e2254 commit-graph: report incomplete chains during verification\n    @@ Commit message\n         \"tip\" case is what is fixed by this patch (it complains to stderr but\n         still returns the base slice).\n     \n    +    Likewise the existing tests for corruption of the commit-graph-chain\n    +    file itself need to be updated. We already exited non-zero correctly for\n    +    the \"base\" case, but the \"tip\" case can now do so, too.\n    +\n         Note that this also causes us to adjust a test later in the file that\n         similarly corrupts a tip (though confusingly the test script calls this\n         \"base\"). It checks stderr but erroneously expects the whole \"verify\"\n    @@ commit-graph.c: int open_commit_graph_chain(const char *chain_file,\n      {\n      \tstruct commit_graph *graph_chain = NULL;\n      \tstruct strbuf line = STRBUF_INIT;\n    - \tstruct object_id *oids;\n    - \tint i = 0, valid = 1, count;\n    - \tFILE *fp = xfdopen(fd, \"r\");\n    - \n    -+\t*incomplete_chain = 0;\n    -+\n    - \tcount = st->st_size / (the_hash_algo->hexsz + 1);\n    - \tCALLOC_ARRAY(oids, count);\n    - \n     @@ commit-graph.c: struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n    + \tfclose(fp);\n    + \tstrbuf_release(&line);\n    + \n    ++\t*incomplete_chain = !valid;\n    + \treturn graph_chain;\n    + }\n      \n    - \t\tif (!valid) {\n    - \t\t\twarning(_(\"unable to find all commit-graph files\"));\n    -+\t\t\t*incomplete_chain = 1;\n    - \t\t\tbreak;\n    - \t\t}\n    - \t}\n     @@ commit-graph.c: static struct commit_graph *load_commit_graph_chain(struct repository *r,\n      \tstruct commit_graph *g = NULL;\n      \n    @@ t/t5324-split-commit-graph.sh: test_expect_success 'warn on base graph chunk inc\n      \t\tgrep -v \"^+\" test_err >err &&\n      \t\ttest_i18ngrep \"commit-graph chain does not match\" err\n      \t)\n    +@@ t/t5324-split-commit-graph.sh: test_expect_success 'verify after commit-graph-chain corruption (tip)' '\n    + \t(\n    + \t\tcd verify-chain-tip &&\n    + \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 70 \"G\" &&\n    +-\t\tgit commit-graph verify 2>test_err &&\n    ++\t\ttest_must_fail git commit-graph verify 2>test_err &&\n    + \t\tgrep -v \"^+\" test_err >err &&\n    + \t\ttest_i18ngrep \"invalid commit-graph chain\" err &&\n    + \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 70 \"A\" &&\n    +-\t\tgit commit-graph verify 2>test_err &&\n    ++\t\ttest_must_fail git commit-graph verify 2>test_err &&\n    + \t\tgrep -v \"^+\" test_err >err &&\n    + \t\ttest_i18ngrep \"unable to find all commit-graph files\" err\n    + \t)\n\n  [1/6]: commit-graph: factor out chain opening function\n  [2/6]: commit-graph: check mixed generation validation when loading chain file\n  [3/6]: t5324: harmonize sha1/sha256 graph chain corruption\n  [4/6]: commit-graph: detect read errors when verifying graph chain\n  [5/6]: commit-graph: tighten chain size check\n  [6/6]: commit-graph: report incomplete chains during verification\n\n builtin/commit-graph.c        |  31 +++++++---\n commit-graph.c                | 109 +++++++++++++++++++++-------------\n commit-graph.h                |   4 ++\n t/t5324-split-commit-graph.sh |  69 ++++++++++++++++++---\n 4 files changed, 158 insertions(+), 55 deletions(-)\n\n-Peff\n"},{"id":"482421","messageId":"20230928043807.GA58367@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230928043746.GB57926@coredump.intra.peff.net","subject":"[PATCH v2 1/6] commit-graph: factor out chain opening function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-28T04:38:07Z","receivedAt":"2023-09-28T04:38:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The load_commit_graph_chain() function opens the chain file and all of\nthe slices of graph that it points to. If there is no chain file (which\nis a totally normal condition), we return NULL. But if we run into\nerrors with the chain file or loading the actual graph data, we also\nreturn NULL, and the caller cannot tell the difference.\n\nThe caller can check for ENOENT for the unremarkable \"no such file\"\ncase. But I'm hesitant to assume that the rest of the function would\nnever accidentally set errno to ENOENT itself, since it is opening the\nslice files (and that would mean the caller fails to notice a real\nerror).\n\nSo let's break this into two functions: one to open the file, and one to\nactually load it. This matches the interface we provide for the\nnon-chain graph file, which will also come in handy in a moment when we\nfix some bugs in the \"git commit-graph verify\" code.\n\nSome notes:\n\n  - I've kept the \"1 is good, 0 is bad\" return convention (and the weird\n    \"fd\" out-parameter) used by the matching open_commit_graph()\n    function and other parts of the commit-graph code. This is unlike\n    most of the rest of Git (which would just return the fd, with -1 for\n    error), but it makes sense to stay consistent with the adjacent bits\n    of the API here.\n\n  - The existing chain loading function will quietly return if the file\n    is too small to hold a single entry. I've retained that behavior\n    (and explicitly set ENOENT in the opener function) for now, under\n    the notion that it's probably valid (though I'd imagine unusual) to\n    have an empty chain file.\n\nThere are two small behavior changes here, but I think both are strictly\npositive:\n\n  1. The original blindly did a stat() before checking if fopen()\n     succeeded, meaning we were making a pointless extra stat call.\n\n  2. We now use fstat() to check the file size. The previous code using\n     a regular stat() on the pathname meant we could technically race\n     with somebody updating the chain file, and end up with a size that\n     does not match what we just opened with fopen(). I doubt anybody\n     ever hit this in practice, but it may have caused an out-of-bounds\n     read.\n\nWe'll retain the load_commit_graph_chain() function which does both the\nopen and reading steps (most existing callers do not care about seeing\nerrors anyway, since loading commit-graphs is optimistic).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSame as v1.\n\n commit-graph.c | 58 +++++++++++++++++++++++++++++++++-----------------\n commit-graph.h |  3 +++\n 2 files changed, 42 insertions(+), 19 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5e8a3a5085..12cdd9af8e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -513,31 +513,34 @@ static int add_graph_to_chain(struct commit_graph *g,\n \treturn 1;\n }\n \n-static struct commit_graph *load_commit_graph_chain(struct repository *r,\n-\t\t\t\t\t\t    struct object_directory *odb)\n+int open_commit_graph_chain(const char *chain_file,\n+\t\t\t    int *fd, struct stat *st)\n+{\n+\t*fd = git_open(chain_file);\n+\tif (*fd < 0)\n+\t\treturn 0;\n+\tif (fstat(*fd, st)) {\n+\t\tclose(*fd);\n+\t\treturn 0;\n+\t}\n+\tif (st->st_size <= the_hash_algo->hexsz) {\n+\t\tclose(*fd);\n+\t\terrno = ENOENT;\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n+struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n+\t\t\t\t\t\t   int fd, struct stat *st)\n {\n \tstruct commit_graph *graph_chain = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n-\tstruct stat st;\n \tstruct object_id *oids;\n \tint i = 0, valid = 1, count;\n-\tchar *chain_name = get_commit_graph_chain_filename(odb);\n-\tFILE *fp;\n-\tint stat_res;\n+\tFILE *fp = xfdopen(fd, \"r\");\n \n-\tfp = fopen(chain_name, \"r\");\n-\tstat_res = stat(chain_name, &st);\n-\tfree(chain_name);\n-\n-\tif (!fp)\n-\t\treturn NULL;\n-\tif (stat_res ||\n-\t    st.st_size <= the_hash_algo->hexsz) {\n-\t\tfclose(fp);\n-\t\treturn NULL;\n-\t}\n-\n-\tcount = st.st_size / (the_hash_algo->hexsz + 1);\n+\tcount = st->st_size / (the_hash_algo->hexsz + 1);\n \tCALLOC_ARRAY(oids, count);\n \n \tprepare_alt_odb(r);\n@@ -585,6 +588,23 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \treturn graph_chain;\n }\n \n+static struct commit_graph *load_commit_graph_chain(struct repository *r,\n+\t\t\t\t\t\t    struct object_directory *odb)\n+{\n+\tchar *chain_file = get_commit_graph_chain_filename(odb);\n+\tstruct stat st;\n+\tint fd;\n+\tstruct commit_graph *g = NULL;\n+\n+\tif (open_commit_graph_chain(chain_file, &fd, &st)) {\n+\t\t/* ownership of fd is taken over by load function */\n+\t\tg = load_commit_graph_chain_fd_st(r, fd, &st);\n+\t}\n+\n+\tfree(chain_file);\n+\treturn g;\n+}\n+\n /*\n  * returns 1 if and only if all graphs in the chain have\n  * corrected commit dates stored in the generation_data chunk.\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 5e534f0fcc..3b662fd2b5 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -26,6 +26,7 @@ struct string_list;\n char *get_commit_graph_filename(struct object_directory *odb);\n char *get_commit_graph_chain_filename(struct object_directory *odb);\n int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n+int open_commit_graph_chain(const char *chain_file, int *fd, struct stat *st);\n \n /*\n  * Given a commit struct, try to fill the commit struct info, including:\n@@ -105,6 +106,8 @@ struct commit_graph {\n struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n \t\t\t\t\t\t int fd, struct stat *st,\n \t\t\t\t\t\t struct object_directory *odb);\n+struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n+\t\t\t\t\t\t   int fd, struct stat *st);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n \n-- \n2.42.0.773.ga6e30199be\n\n"},{"id":"482422","messageId":"20230928043817.GB58367@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230928043746.GB57926@coredump.intra.peff.net","subject":"[PATCH v2 2/6] commit-graph: check mixed generation validation when loading chain file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-28T04:38:17Z","receivedAt":"2023-09-28T04:38:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In read_commit_graph_one(), we call validate_mixed_generation_chain()\nafter loading the graph. Even though we don't check the return value,\nthis has the side effect of clearing the read_generation_data flag,\nwhich is important when working with mixed generation numbers.\n\nBut doing this in load_commit_graph_chain_fd_st() makes more sense:\n\n  1. We are calling it even when we did not load a chain at all, which\n     is pointless (you cannot have mixed generations in a single file).\n\n  2. For now, all callers load the graph via read_commit_graph_one().\n     But the point of factoring out the open/load in the previous commit\n     was to let \"commit-graph verify\" call them separately. So it needs\n     to trigger this function as part of the load.\n\n     Without this patch, the mixed-generation tests in t5324 would start\n     failing on \"git commit-graph verify\" calls, once we switch to using\n     a separate open/load call there.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSame as v1.\n\n commit-graph.c | 54 +++++++++++++++++++++++++-------------------------\n 1 file changed, 27 insertions(+), 27 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 12cdd9af8e..8b29c6de24 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -473,6 +473,31 @@ static struct commit_graph *load_commit_graph_v1(struct repository *r,\n \treturn g;\n }\n \n+/*\n+ * returns 1 if and only if all graphs in the chain have\n+ * corrected commit dates stored in the generation_data chunk.\n+ */\n+static int validate_mixed_generation_chain(struct commit_graph *g)\n+{\n+\tint read_generation_data = 1;\n+\tstruct commit_graph *p = g;\n+\n+\twhile (read_generation_data && p) {\n+\t\tread_generation_data = p->read_generation_data;\n+\t\tp = p->base_graph;\n+\t}\n+\n+\tif (read_generation_data)\n+\t\treturn 1;\n+\n+\twhile (g) {\n+\t\tg->read_generation_data = 0;\n+\t\tg = g->base_graph;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int add_graph_to_chain(struct commit_graph *g,\n \t\t\t      struct commit_graph *chain,\n \t\t\t      struct object_id *oids,\n@@ -581,6 +606,8 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n \t\t}\n \t}\n \n+\tvalidate_mixed_generation_chain(graph_chain);\n+\n \tfree(oids);\n \tfclose(fp);\n \tstrbuf_release(&line);\n@@ -605,31 +632,6 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \treturn g;\n }\n \n-/*\n- * returns 1 if and only if all graphs in the chain have\n- * corrected commit dates stored in the generation_data chunk.\n- */\n-static int validate_mixed_generation_chain(struct commit_graph *g)\n-{\n-\tint read_generation_data = 1;\n-\tstruct commit_graph *p = g;\n-\n-\twhile (read_generation_data && p) {\n-\t\tread_generation_data = p->read_generation_data;\n-\t\tp = p->base_graph;\n-\t}\n-\n-\tif (read_generation_data)\n-\t\treturn 1;\n-\n-\twhile (g) {\n-\t\tg->read_generation_data = 0;\n-\t\tg = g->base_graph;\n-\t}\n-\n-\treturn 0;\n-}\n-\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb)\n {\n@@ -638,8 +640,6 @@ struct commit_graph *read_commit_graph_one(struct repository *r,\n \tif (!g)\n \t\tg = load_commit_graph_chain(r, odb);\n \n-\tvalidate_mixed_generation_chain(g);\n-\n \treturn g;\n }\n \n-- \n2.42.0.773.ga6e30199be\n\n"},{"id":"482423","messageId":"20230928043847.GC58367@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230928043746.GB57926@coredump.intra.peff.net","subject":"[PATCH v2 3/6] t5324: harmonize sha1/sha256 graph chain corruption","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-28T04:38:47Z","receivedAt":"2023-09-28T04:38:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In t5324.20, we corrupt a hex character 60 bytes into the graph chain\nfile. Since the file consists of two hash identifiers, one per line, the\ncorruption differs between sha1 and sha256. In a sha1 repository, the\ncorruption is on the second line, and in a sha256 repository, it is on\nthe first.\n\nWe should of course detect the problem with either line. But as the next\nfew patches will show (and fix), that is not the case (in fact, we\ncurrently do not exit non-zero for either line!). And while at the end\nof our series we'll catch all errors, our intermediate states will have\ndiffering behavior between the two hashes.\n\nLet's make sure we test corruption of both the first and second lines,\nand do so consistently with either hash by choosing offsets which are\nalways in the first hash (30 bytes) or in the second (70).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIn v2 we are now testing both first and second lines, instead of just\nthe first.\n\n t/t5324-split-commit-graph.sh | 25 ++++++++++++++++++++-----\n 1 file changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 36c4141e67..a9b2428b56 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -312,15 +312,30 @@ test_expect_success 'warn on base graph chunk incorrect' '\n \t)\n '\n \n-test_expect_success 'verify after commit-graph-chain corruption' '\n-\tgit clone --no-hardlinks . verify-chain &&\n+test_expect_success 'verify after commit-graph-chain corruption (base)' '\n+\tgit clone --no-hardlinks . verify-chain-base &&\n \t(\n-\t\tcd verify-chain &&\n-\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 60 \"G\" &&\n+\t\tcd verify-chain-base &&\n+\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"G\" &&\n \t\tgit commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"invalid commit-graph chain\" err &&\n-\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 60 \"A\" &&\n+\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"A\" &&\n+\t\tgit commit-graph verify 2>test_err &&\n+\t\tgrep -v \"^+\" test_err >err &&\n+\t\ttest_i18ngrep \"unable to find all commit-graph files\" err\n+\t)\n+'\n+\n+test_expect_success 'verify after commit-graph-chain corruption (tip)' '\n+\tgit clone --no-hardlinks . verify-chain-tip &&\n+\t(\n+\t\tcd verify-chain-tip &&\n+\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 70 \"G\" &&\n+\t\tgit commit-graph verify 2>test_err &&\n+\t\tgrep -v \"^+\" test_err >err &&\n+\t\ttest_i18ngrep \"invalid commit-graph chain\" err &&\n+\t\tcorrupt_file \"$graphdir/commit-graph-chain\" 70 \"A\" &&\n \t\tgit commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"unable to find all commit-graph files\" err\n-- \n2.42.0.773.ga6e30199be\n\n"},{"id":"482424","messageId":"20230928043859.GD58367@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230928043746.GB57926@coredump.intra.peff.net","subject":"[PATCH v2 4/6] commit-graph: detect read errors when verifying graph chain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-28T04:38:59Z","receivedAt":"2023-09-28T04:39:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because it's OK to not have a graph file at all, the graph_verify()\nfunction needs to tell the difference between a missing file and a real\nerror.  So when loading a traditional graph file, we call\nopen_commit_graph() separately from load_commit_graph_chain_fd_st(), and\ndon't complain if the first one fails with ENOENT.\n\nWhen the function learned about chain files in 3da4b609bb (commit-graph:\nverify chains with --shallow mode, 2019-06-18), we couldn't be as\ncareful, since the only way to load a chain was with\nread_commit_graph_one(), which did both the open/load as a single unit.\nSo we'll miss errors in chain files we load, thinking instead that there\nwas just no chain file at all.\n\nNote that we do still report some of these problems to stderr, as the\nloading function calls error() and warning(). But we'd exit with a\nsuccessful exit code, which is wrong.\n\nWe can fix that by using the recently split open/load functions for\nchains. That lets us treat the chain file just like a single file with\nrespect to error handling here.\n\nAn existing test (from 3da4b609bb) shows off the problem; we were\nexpecting \"commit-graph verify\" to report success, but that makes no\nsense. We did not even verify the contents of the graph data, because we\ncouldn't load it! I don't think this was an intentional exception, but\nrather just the test covering what happened to occur.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit-graph.c        | 23 ++++++++++++++++-------\n t/t5324-split-commit-graph.sh |  4 ++--\n 2 files changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c88389df24..50c15d9bfe 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -69,7 +69,8 @@ static int graph_verify(int argc, const char **argv, const char *prefix)\n \tstruct commit_graph *graph = NULL;\n \tstruct object_directory *odb = NULL;\n \tchar *graph_name;\n-\tint open_ok;\n+\tchar *chain_name;\n+\tenum { OPENED_NONE, OPENED_GRAPH, OPENED_CHAIN } opened = OPENED_NONE;\n \tint fd;\n \tstruct stat st;\n \tint flags = 0;\n@@ -102,21 +103,29 @@ static int graph_verify(int argc, const char **argv, const char *prefix)\n \n \todb = find_odb(the_repository, opts.obj_dir);\n \tgraph_name = get_commit_graph_filename(odb);\n-\topen_ok = open_commit_graph(graph_name, &fd, &st);\n-\tif (!open_ok && errno != ENOENT)\n+\tchain_name = get_commit_graph_chain_filename(odb);\n+\tif (open_commit_graph(graph_name, &fd, &st))\n+\t\topened = OPENED_GRAPH;\n+\telse if (errno != ENOENT)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n+\telse if (open_commit_graph_chain(chain_name, &fd, &st))\n+\t\topened = OPENED_CHAIN;\n+\telse if (errno != ENOENT)\n+\t\tdie_errno(_(\"could not open commit-graph chain '%s'\"), chain_name);\n \n \tFREE_AND_NULL(graph_name);\n+\tFREE_AND_NULL(chain_name);\n \tFREE_AND_NULL(options);\n \n-\tif (open_ok)\n+\tif (opened == OPENED_NONE)\n+\t\treturn 0;\n+\telse if (opened == OPENED_GRAPH)\n \t\tgraph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);\n \telse\n-\t\tgraph = read_commit_graph_one(the_repository, odb);\n+\t\tgraph = load_commit_graph_chain_fd_st(the_repository, fd, &st);\n \n-\t/* Return failure if open_ok predicted success */\n \tif (!graph)\n-\t\treturn !!open_ok;\n+\t\treturn 1;\n \n \tret = verify_commit_graph(the_repository, graph, flags);\n \tfree_commit_graph(graph);\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex a9b2428b56..a5ac0440f1 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -317,11 +317,11 @@ test_expect_success 'verify after commit-graph-chain corruption (base)' '\n \t(\n \t\tcd verify-chain-base &&\n \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"G\" &&\n-\t\tgit commit-graph verify 2>test_err &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"invalid commit-graph chain\" err &&\n \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 30 \"A\" &&\n-\t\tgit commit-graph verify 2>test_err &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"unable to find all commit-graph files\" err\n \t)\n-- \n2.42.0.773.ga6e30199be\n\n"},{"id":"482425","messageId":"20230928043910.GE58367@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230928043746.GB57926@coredump.intra.peff.net","subject":"[PATCH v2 5/6] commit-graph: tighten chain size check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-28T04:39:10Z","receivedAt":"2023-09-28T04:39:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we open a commit-graph-chain file, if it's smaller than a single\nentry, we just quietly treat that as ENOENT. That make some sense if the\nfile is truly zero bytes, but it means that \"commit-graph verify\" will\nquietly ignore a file that contains garbage if that garbage happens to\nbe short.\n\nInstead, let's only simulate ENOENT when the file is truly empty, and\notherwise return EINVAL. The normal graph-loading routines don't care,\nbut \"commit-graph verify\" will notice and complain about the difference.\n\nIt's not entirely clear to me that the 0-is-ENOENT case actually happens\nin real life, so we could perhaps just eliminate this special-case\naltogether. But this is how we've always behaved, so I'm preserving it\nin the name of backwards compatibility (though again, it really only\nmatters for \"verify\", as the regular routines are happy to load what\nthey can).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit-graph.c                | 10 ++++++++--\n t/t5324-split-commit-graph.sh | 12 ++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 8b29c6de24..b1d3e5bf94 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -548,9 +548,15 @@ int open_commit_graph_chain(const char *chain_file,\n \t\tclose(*fd);\n \t\treturn 0;\n \t}\n-\tif (st->st_size <= the_hash_algo->hexsz) {\n+\tif (st->st_size < the_hash_algo->hexsz) {\n \t\tclose(*fd);\n-\t\terrno = ENOENT;\n+\t\tif (!st->st_size) {\n+\t\t\t/* treat empty files the same as missing */\n+\t\t\terrno = ENOENT;\n+\t\t} else {\n+\t\t\twarning(\"commit-graph chain file too small\");\n+\t\t\terrno = EINVAL;\n+\t\t}\n \t\treturn 0;\n \t}\n \treturn 1;\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex a5ac0440f1..be58545810 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -342,6 +342,18 @@ test_expect_success 'verify after commit-graph-chain corruption (tip)' '\n \t)\n '\n \n+test_expect_success 'verify notices too-short chain file' '\n+\tgit clone --no-hardlinks . verify-chain-short &&\n+\t(\n+\t\tcd verify-chain-short &&\n+\t\tgit commit-graph verify &&\n+\t\techo \"garbage\" >$graphdir/commit-graph-chain &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n+\t\tgrep -v \"^+\" test_err >err &&\n+\t\tgrep \"commit-graph chain file too small\" err\n+\t)\n+'\n+\n test_expect_success 'verify across alternates' '\n \tgit clone --no-hardlinks . verify-alt &&\n \t(\n-- \n2.42.0.773.ga6e30199be\n\n"},{"id":"482426","messageId":"20230928043951.GF58367@coredump.intra.peff.net","threadId":"60268","inReplyTo":"20230928043746.GB57926@coredump.intra.peff.net","subject":"[PATCH v2 6/6] commit-graph: report incomplete chains during verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-28T04:39:51Z","receivedAt":"2023-09-28T04:39:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The load_commit_graph_chain_fd_st() function will stop loading chains\nwhen it sees an error. But if it has loaded any graph slice at all, it\nwill return it. This is a good thing for normal use (we use what data we\ncan, and this is just an optimization). But it's a bad thing for\n\"commit-graph verify\", which should be careful about finding any\nirregularities. We do complain to stderr with a warning(), but the\nverify command still exits with a successful return code.\n\nThe new tests here cover corruption of both the base and tip slices of\nthe chain. The corruption of the base file already works (it is the\nfirst file we look at, so when we see the error we return NULL). The\n\"tip\" case is what is fixed by this patch (it complains to stderr but\nstill returns the base slice).\n\nLikewise the existing tests for corruption of the commit-graph-chain\nfile itself need to be updated. We already exited non-zero correctly for\nthe \"base\" case, but the \"tip\" case can now do so, too.\n\nNote that this also causes us to adjust a test later in the file that\nsimilarly corrupts a tip (though confusingly the test script calls this\n\"base\"). It checks stderr but erroneously expects the whole \"verify\"\ncommand to exit with a successful code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nv2 now covers failed get_oid_hex() call on non-base lines.\n\n builtin/commit-graph.c        | 10 +++++++++-\n commit-graph.c                |  7 +++++--\n commit-graph.h                |  3 ++-\n t/t5324-split-commit-graph.sh | 32 +++++++++++++++++++++++++++++---\n 4 files changed, 45 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 50c15d9bfe..f5e66e9969 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -74,6 +74,7 @@ static int graph_verify(int argc, const char **argv, const char *prefix)\n \tint fd;\n \tstruct stat st;\n \tint flags = 0;\n+\tint incomplete_chain = 0;\n \tint ret;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n@@ -122,13 +123,20 @@ static int graph_verify(int argc, const char **argv, const char *prefix)\n \telse if (opened == OPENED_GRAPH)\n \t\tgraph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);\n \telse\n-\t\tgraph = load_commit_graph_chain_fd_st(the_repository, fd, &st);\n+\t\tgraph = load_commit_graph_chain_fd_st(the_repository, fd, &st,\n+\t\t\t\t\t\t      &incomplete_chain);\n \n \tif (!graph)\n \t\treturn 1;\n \n \tret = verify_commit_graph(the_repository, graph, flags);\n \tfree_commit_graph(graph);\n+\n+\tif (incomplete_chain) {\n+\t\terror(\"one or more commit-graph chain files could not be loaded\");\n+\t\tret |= 1;\n+\t}\n+\n \treturn ret;\n }\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex b1d3e5bf94..1a56efcf69 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -563,7 +563,8 @@ int open_commit_graph_chain(const char *chain_file,\n }\n \n struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n-\t\t\t\t\t\t   int fd, struct stat *st)\n+\t\t\t\t\t\t   int fd, struct stat *st,\n+\t\t\t\t\t\t   int *incomplete_chain)\n {\n \tstruct commit_graph *graph_chain = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n@@ -618,6 +619,7 @@ struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n \tfclose(fp);\n \tstrbuf_release(&line);\n \n+\t*incomplete_chain = !valid;\n \treturn graph_chain;\n }\n \n@@ -630,8 +632,9 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \tstruct commit_graph *g = NULL;\n \n \tif (open_commit_graph_chain(chain_file, &fd, &st)) {\n+\t\tint incomplete;\n \t\t/* ownership of fd is taken over by load function */\n-\t\tg = load_commit_graph_chain_fd_st(r, fd, &st);\n+\t\tg = load_commit_graph_chain_fd_st(r, fd, &st, &incomplete);\n \t}\n \n \tfree(chain_file);\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 3b662fd2b5..20ada7e891 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -107,7 +107,8 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n \t\t\t\t\t\t int fd, struct stat *st,\n \t\t\t\t\t\t struct object_directory *odb);\n struct commit_graph *load_commit_graph_chain_fd_st(struct repository *r,\n-\t\t\t\t\t\t   int fd, struct stat *st);\n+\t\t\t\t\t\t   int fd, struct stat *st,\n+\t\t\t\t\t\t   int *incomplete_chain);\n struct commit_graph *read_commit_graph_one(struct repository *r,\n \t\t\t\t\t   struct object_directory *odb);\n \ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex be58545810..06bb897f02 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -285,6 +285,32 @@ test_expect_success 'verify hashes along chain, even in shallow' '\n \t)\n '\n \n+test_expect_success 'verify notices chain slice which is bogus (base)' '\n+\tgit clone --no-hardlinks . verify-chain-bogus-base &&\n+\t(\n+\t\tcd verify-chain-bogus-base &&\n+\t\tgit commit-graph verify &&\n+\t\tbase_file=$graphdir/graph-$(sed -n 1p $graphdir/commit-graph-chain).graph &&\n+\t\techo \"garbage\" >$base_file &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n+\t\tgrep -v \"^+\" test_err >err &&\n+\t\tgrep \"commit-graph file is too small\" err\n+\t)\n+'\n+\n+test_expect_success 'verify notices chain slice which is bogus (tip)' '\n+\tgit clone --no-hardlinks . verify-chain-bogus-tip &&\n+\t(\n+\t\tcd verify-chain-bogus-tip &&\n+\t\tgit commit-graph verify &&\n+\t\ttip_file=$graphdir/graph-$(sed -n 2p $graphdir/commit-graph-chain).graph &&\n+\t\techo \"garbage\" >$tip_file &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n+\t\tgrep -v \"^+\" test_err >err &&\n+\t\tgrep \"commit-graph file is too small\" err\n+\t)\n+'\n+\n test_expect_success 'verify --shallow does not check base contents' '\n \tgit clone --no-hardlinks . verify-shallow &&\n \t(\n@@ -306,7 +332,7 @@ test_expect_success 'warn on base graph chunk incorrect' '\n \t\tgit commit-graph verify &&\n \t\tbase_file=$graphdir/graph-$(tail -n 1 $graphdir/commit-graph-chain).graph &&\n \t\tcorrupt_file \"$base_file\" $(test_oid base) \"\\01\" &&\n-\t\tgit commit-graph verify --shallow 2>test_err &&\n+\t\ttest_must_fail git commit-graph verify --shallow 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"commit-graph chain does not match\" err\n \t)\n@@ -332,11 +358,11 @@ test_expect_success 'verify after commit-graph-chain corruption (tip)' '\n \t(\n \t\tcd verify-chain-tip &&\n \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 70 \"G\" &&\n-\t\tgit commit-graph verify 2>test_err &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"invalid commit-graph chain\" err &&\n \t\tcorrupt_file \"$graphdir/commit-graph-chain\" 70 \"A\" &&\n-\t\tgit commit-graph verify 2>test_err &&\n+\t\ttest_must_fail git commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n \t\ttest_i18ngrep \"unable to find all commit-graph files\" err\n \t)\n-- \n2.42.0.773.ga6e30199be\n"},{"id":"482696","messageId":"ZR70jsa4TTl3k940@nand.local","threadId":"60268","inReplyTo":"20230928043746.GB57926@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/6] some \"commit-graph verify\" fixes for chains","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-05T17:38:22Z","receivedAt":"2023-10-05T17:38:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 28, 2023 at 12:37:46AM -0400, Jeff King wrote:\n> On Tue, Sep 26, 2023 at 01:54:52AM -0400, Jeff King wrote:\n>\n> >   [1/6]: commit-graph: factor out chain opening function\n> >   [2/6]: commit-graph: check mixed generation validation when loading chain file\n> >   [3/6]: t5324: harmonize sha1/sha256 graph chain corruption\n> >   [4/6]: commit-graph: detect read errors when verifying graph chain\n> >   [5/6]: commit-graph: tighten chain size check\n> >   [6/6]: commit-graph: report incomplete chains during verification\n>\n> Here's a re-roll that fixes a missed case in patch 6 (I sent more\n> details elsewhere in the thread).\n\nThanks, and apologies for taking a little longer than I would have liked\nto review both of these rounds. This round looks great to me (and it's\nalready in 'next').\n\nThanks,\nTaylor\n"}]}