{"thread":{"id":"57972","subject":"commit-graph overflow generation chicken and egg","startedAt":"2022-06-08T19:33:14Z","lastAt":"2022-07-06T09:13:30Z","messageCount":13,"participants":["Jeff King","Derrick Stolee","Ævar Arnfjörð Bjarmason","Patrick Steinhardt","Will Chandler","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"456884","messageId":"YqD5dgalb9EPnz85@coredump.intra.peff.net","threadId":"57972","inReplyTo":null,"subject":"commit-graph overflow generation chicken and egg","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-08T19:33:10Z","receivedAt":"2022-06-08T19:33:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I hadn't touched my git.git repository for a while, so I upgraded to the\nmost recent version of Git (v2.36.1) and was met with this:\n\n  $ git rev-list --all\n  fatal: commit-graph requires overflow generation data but has none\n\nNot very friendly, but OK, maybe my commit graph is out of date. Let's\nregenerate it:\n\n  $ git gc\n  fatal: commit-graph requires overflow generation data but has none\n  fatal: failed to run repack\n\nOK, we can't get far enough in the gc to rebuild the commit graph. Let's\ntry doing it manually:\n\n  $ git commit-graph write --reachable\n  fatal: commit-graph requires overflow generation data but has none\n  $ git commit-graph write\n  fatal: commit-graph requires overflow generation data but has none\n\nYikes. Here's where it happens within the write process:\n\n  $ GIT_PROGRESS_DELAY=0 git commit-graph write\n  Finding commits for commit graph among packed objects: 100% (360229/360229), done.\n  Loading known commits in commit graph: 100% (78366/78366), done.\n  Expanding reachable commits in commit graph: 78366, done.\n  Clearing commit marks in commit graph: 100% (78366/78366), done.\n  Finding extra edges in commit graph: 100% (78366/78366), done.\n  Computing commit graph topological levels: 100% (78366/78366), done.\n  Computing commit graph generation numbers: 100% (78366/78366), done.\n  fatal: commit-graph requires overflow generation data but has none\n\nNow being the enterprising fellow that I am, I was able to get out of it\nlike this:\n\n  $ rm -f objects/info/commit-graph\n  $ git gc\n\nBut I wonder if this is a foot-gun waiting for some other user. I'm not\nsure how I got into the broken state exactly. The repo was last touched\nin December using a version of Git running 'next'. It worked fine with\nversions of Git prior to 6dbf4b8172 (commit-graph: declare bankruptcy on\nGDAT chunks, 2022-03-02). It's entirely possible that the bad state was\ngenerated by a version of Git that wasn't ever released, and this isn't\na problem that normal humans would ever run into. It does feel a bit\nunfriendly that neither gc nor commit-graph could unstick things,\nthough. Especially because 6dbf4b8172 says:\n\n  [...]a previous version of Git wrote possibly erroneous data in these\n  chunks with the IDs \"GDAT\" and \"GDOV\". By changing the IDs, newer\n  versions of Git will silently ignore those older chunks[...]\n\nPresumably we _are_ ignoring those chunks, but some other part of the\ncommit-graph file has a dependency on them (and of course we don't have\nthe new GDA2/GDO2 chunks to read in their place). If that's true, then\nthe solution may be a more graceful \"we can't use this commit graph\"\nerror return rather than the \"fatal:\" message seen above.\n\nI have a copy of the broken repo state if anybody would care to look at\nit.\n\n-Peff\n"},{"id":"456886","messageId":"581c7ef2-3de4-eb8a-bfbb-d4bca3522a2d@github.com","threadId":"57972","inReplyTo":"YqD5dgalb9EPnz85@coredump.intra.peff.net","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-08T20:08:03Z","receivedAt":"2022-06-08T20:08:12Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/8/2022 3:33 PM, Jeff King wrote:\n> I hadn't touched my git.git repository for a while, so I upgraded to the\n> most recent version of Git (v2.36.1) and was met with this:\n> \n>   $ git rev-list --all\n>   fatal: commit-graph requires overflow generation data but has none\n> \n> Not very friendly, but OK, maybe my commit graph is out of date. Let's\n> regenerate it:\n> \n>   $ git gc\n>   fatal: commit-graph requires overflow generation data but has none\n>   fatal: failed to run repack\n> \n> OK, we can't get far enough in the gc to rebuild the commit graph. Let's\n> try doing it manually:\n> \n>   $ git commit-graph write --reachable\n>   fatal: commit-graph requires overflow generation data but has none\n>   $ git commit-graph write\n>   fatal: commit-graph requires overflow generation data but has none\n> \n> Yikes. Here's where it happens within the write process:\n> \n>   $ GIT_PROGRESS_DELAY=0 git commit-graph write\n>   Finding commits for commit graph among packed objects: 100% (360229/360229), done.\n>   Loading known commits in commit graph: 100% (78366/78366), done.\n>   Expanding reachable commits in commit graph: 78366, done.\n>   Clearing commit marks in commit graph: 100% (78366/78366), done.\n>   Finding extra edges in commit graph: 100% (78366/78366), done.\n>   Computing commit graph topological levels: 100% (78366/78366), done.\n>   Computing commit graph generation numbers: 100% (78366/78366), done.\n>   fatal: commit-graph requires overflow generation data but has none\n> \n> Now being the enterprising fellow that I am, I was able to get out of it\n> like this:\n> \n>   $ rm -f objects/info/commit-graph\n>   $ git gc\n> \n> But I wonder if this is a foot-gun waiting for some other user. I'm not\n> sure how I got into the broken state exactly. The repo was last touched\n> in December using a version of Git running 'next'. It worked fine with\n> versions of Git prior to 6dbf4b8172 (commit-graph: declare bankruptcy on\n> GDAT chunks, 2022-03-02). It's entirely possible that the bad state was\n> generated by a version of Git that wasn't ever released, and this isn't\n> a problem that normal humans would ever run into. It does feel a bit\n> unfriendly that neither gc nor commit-graph could unstick things,\n> though. Especially because 6dbf4b8172 says:\n> \n>   [...]a previous version of Git wrote possibly erroneous data in these\n>   chunks with the IDs \"GDAT\" and \"GDOV\". By changing the IDs, newer\n>   versions of Git will silently ignore those older chunks[...]\n\nYou've done enough homework to discover exactly what's going on here,\nincluding talking about the commit that I was going to point out.\n\n> Presumably we _are_ ignoring those chunks, but some other part of the\n> commit-graph file has a dependency on them (and of course we don't have\n> the new GDA2/GDO2 chunks to read in their place). If that's true, then\n> the solution may be a more graceful \"we can't use this commit graph\"\n> error return rather than the \"fatal:\" message seen above.\n> \n> I have a copy of the broken repo state if anybody would care to look at\n> it.\n\nI'd love to see the full binary, but for the sake of sharing on the\nlist, could you give the following output?\n\n\txxd .git/objects/info/commit-graph | head\n\nor any other command that shows the first few hex bytes along with\ntheir ASCII equivalents. Here is one that used Git 2.34.0:\n\n00000000: 4347 5048 0101 0500 4f49 4446 0000 0000  CGPH....OIDF....\n00000010: 0000 0050 4f49 444c 0000 0000 0000 0450  ...POIDL.......P\n00000020: 4344 4154 0000 0000 002d cf0c 4744 4154  CDAT.....-..GDAT\n00000030: 0000 0000 0080 3bf8 4544 4745 0000 0000  ......;.EDGE....\n00000040: 0089 6484 0000 0000 0000 0000 0089 6660  ..d...........f`\n00000050: 0000 0246 0000 0498 0000 06c8 0000 0908  ...F............\n00000060: 0000 0b4b 0000 0d58 0000 0f9e 0000 1207  ...K...X........\n00000070: 0000 1423 0000 168e 0000 18d3 0000 1b36  ...#...........6\n00000080: 0000 1d67 0000 1f9b 0000 2219 0000 2456  ...g......\"...$V\n00000090: 0000 26a2 0000 2908 0000 2b5c 0000 2d96  ..&...)...+\\..-.\n\nand here is one with latest master:\n\n00000000: 4347 5048 0101 0500 4f49 4446 0000 0000  CGPH....OIDF....\n00000010: 0000 0050 4f49 444c 0000 0000 0000 0450  ...POIDL.......P\n00000020: 4344 4154 0000 0000 002e 20b0 4744 4132  CDAT...... .GDA2\n00000030: 0000 0000 0081 2090 4544 4745 0000 0000  ...... .EDGE....\n00000040: 008a 5970 0000 0000 0000 0000 008a 5b4c  ..Yp..........[L\n00000050: 0000 024c 0000 04a1 0000 06d6 0000 091a  ...L............\n00000060: 0000 0b60 0000 0d70 0000 0fbb 0000 1229  ...`...p.......)\n00000070: 0000 1448 0000 16b6 0000 1905 0000 1b6c  ...H...........l\n00000080: 0000 1da3 0000 1fd9 0000 225b 0000 249d  ..........\"[..$.\n00000090: 0000 26f0 0000 2956 0000 2bac 0000 2dea  ..&...)V..+...-.\n\nIf you have a GDA2 chunk, then we are somehow incorrectly writing the\noffset there. However, I don't see this happening in the latest code.\n\nWe have this macro in commit.h:\n\n#define GENERATION_NUMBER_V2_OFFSET_MAX ((1ULL << 31) - 1)\n\nand this in commit-graph.c:\n\n#define CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW (1ULL << 31)\n\nbeing used here:\n\nstatic int write_graph_chunk_generation_data(struct hashfile *f,\n\t\t\t\t\t     void *data)\n{\n\tstruct write_commit_graph_context *ctx = data;\n\tint i, num_generation_data_overflows = 0;\n\n\tfor (i = 0; i < ctx->commits.nr; i++) {\n\t\tstruct commit *c = ctx->commits.list[i];\n\t\ttimestamp_t offset;\n\t\trepo_parse_commit(ctx->r, c);\n\t\toffset = commit_graph_data_at(c)->generation - c->date;\n\t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n\n\t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n\t\t\toffset = CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW | num_generation_data_overflows;\n\t\t\tnum_generation_data_overflows++;\n\t\t}\n\n\t\thashwrite_be32(f, offset);\n\t}\n\n\treturn 0;\n}\n\nBut this hasn't been changed since eb9071912f (commit-graph: anonymize\ndata in chunk_write_fn, 2021-02-05) and 90cb1c47c7 (commit-graph: always\nparse before commit_graph_data_at(), 2021-02-01) which were not meaningfully\ndifferent from the first implementation in e8b63005c4 (commit-graph:\nimplement generation data chunk, 2021-01-16).\n\nHowever, the lack of the large offset chunk could be due to the bug fixed by\n75979d9460 (commit-graph: fix ordering bug in generation numbers,\n2022-03-01). Perhaps that was the thing that was missing from your version?\n\nBut otherwise, I'm stumped. I'd be very interested to see a repro from a\nfresh repository. That is: what situation do we need to be in to write such\nan offset without including the large offset chunk?\n\nThanks,\n-Stolee\n"},{"id":"456906","messageId":"YqEuE2iAmMVLlUSu@coredump.intra.peff.net","threadId":"57972","inReplyTo":"581c7ef2-3de4-eb8a-bfbb-d4bca3522a2d@github.com","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-08T23:17:39Z","receivedAt":"2022-06-08T23:17:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2022 at 04:08:03PM -0400, Derrick Stolee wrote:\n\n> I'd love to see the full binary, but for the sake of sharing on the\n> list, could you give the following output?\n> \n> \txxd .git/objects/info/commit-graph | head\n> \n> or any other command that shows the first few hex bytes along with\n> their ASCII equivalents. Here is one that used Git 2.34.0:\n> [...]\n\nInteresting. My earlier email was a bit misleading. I do in fact have a\nGDA2 chunk. And looking at the timestamp on the commit-graph file, it's\nfrom May 24th. I hadn't been keeping the repo up to date regularly, but\nI did occasionally pull and rebuild. So I think it was a much more\nrecent version of Git that built the problematic file, though it's\npossible it was carrying forward bad data.\n\nSo 6dbf4b8172ef may be a bit of a red herring, if the file has a GDA2\nsection that was simply ignored before that commit.\n\nLooking at my reflog, my best guess for the version of Git that produced\nthe file is e46751e96fa.\n\n> However, the lack of the large offset chunk could be due to the bug fixed by\n> 75979d9460 (commit-graph: fix ordering bug in generation numbers,\n> 2022-03-01). Perhaps that was the thing that was missing from your version?\n\nSo I _think_ I would have had that, though there's a good chance that an\nolder version of the commit-graph file was written using a version of\nGit without it.\n\n> But otherwise, I'm stumped. I'd be very interested to see a repro from a\n> fresh repository. That is: what situation do we need to be in to write such\n> an offset without including the large offset chunk?\n\nNot exactly a fresh reproduction, but you can grab my broken file from:\n\n  https://peff.net/tmp/broken-commit-graph\n\nDropping it into a fresh clone of git.git shows the problem.\n\nI tried a few obvious from-scratch reproductions like building a file\nwith 75979d9460^ (so with the generation number bug), and then jumping\nforward to e46751e96fa (so bug fixed, but now we write GDA2), but\ncouldn't get it to trigger.\n\nIt may not be worth spending too much time on, if this is a weird\none-off caused by a mix of buggy unreleased versions of Git. If real\nusers aren't seeing it, and we know the nuclear option is \"rm\ncommit-graph\", then that may be enough.\n\n-Peff\n"},{"id":"456925","messageId":"220609.86h74utg6j.gmgdl@evledraar.gmail.com","threadId":"57972","inReplyTo":"581c7ef2-3de4-eb8a-bfbb-d4bca3522a2d@github.com","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-09T07:49:15Z","receivedAt":"2022-06-09T08:05:45Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 08 2022, Derrick Stolee wrote:\n\n> On 6/8/2022 3:33 PM, Jeff King wrote:\n>> [...]\n> You've done enough homework to discover exactly what's going on here,\n> including talking about the commit that I was going to point out.\n>\n>> Presumably we _are_ ignoring those chunks, but some other part of the\n>> commit-graph file has a dependency on them (and of course we don't have\n>> the new GDA2/GDO2 chunks to read in their place). If that's true, then\n>> the solution may be a more graceful \"we can't use this commit graph\"\n>> error return rather than the \"fatal:\" message seen above.\n>> \n>> I have a copy of the broken repo state if anybody would care to look at\n>> it.\n>\n> I'd love to see the full binary, but for the sake of sharing on the\n> list, could you give the following output?\n>\n> [...]\n>\n> But otherwise, I'm stumped. I'd be very interested to see a repro from a\n> fresh repository. That is: what situation do we need to be in to write such\n> an offset without including the large offset chunk?\n\nIt's certainly interesting to see *how* we got to this state, but just\nso we're on the same page: I fundimentally don't think it matters to the\n*real* bug here.\n\nWhich is that at the very least f90fca638e9 (commit-graph: consolidate\nfill_commit_graph_info, 2021-01-16) and e8b63005c48 (commit-graph:\nimplement generation data chunk, 2021-01-16) (CC'd author) have a bad\nregression on earlier fixes that read-only operations of the\ncommit-graph *must not die*. I.e. the \"parse\" and \"verify\" paths of the\ncommit-graph.c code shouldn't call exit(), die() etc.\n\nI.e. the changes I made in 2ac138d568a (commit-graph: fix segfault on\ne.g. \"git status\", 2019-03-25), 61df89c8e55 (commit-graph: don't early\nexit(1) on e.g. \"git status\", 2019-03-25) and 43d35618055 (commit-graph\nwrite: don't die if the existing graph is corrupt, 2019-03-25).\n\nThe below patch is a start at fixing some of that, but as noted in the\nTODO comments I really wouldn't trust it as-is (and haven't looked\ndeeply into this).\n\nIf you replace your graph with Jeff's corrupt one and run \"git status\",\n\"git log\" etc. it's still emitting one verbose complaint, but it no\nlonger does so in loops (at least for these paths, but e.g. \"git gc\" is\nstill doing that).\n\nBut it does get us to where we can run \"git gc\", and while complaining\ntoo much along the way will write out a new & valid commit graph at the\nend (\"[... comments are mine\"):\n\t\n\t$ git gc\n\t[...]\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n\t[... a lot of the above lines snipped out ...]\n\tEnumerating objects: 1306881, done.\n\tCounting objects: 100% (1306881/1306881), done.\n\tDelta compression using up to 8 threads\n\tCompressing objects: 100% (262837/262837), done.\n\tWriting objects: 100% (1306881/1306881), done.\n\tSelecting bitmap commits: 378069, done.\n\tBuilding bitmaps: 100% (380/380), done.\n\tTotal 1306881 (delta 1047925), reused 1293996 (delta 1036311), pack-reused 0\n\tRemoving duplicate objects: 100% (256/256), done.\n\tcommit-graph requires overflow generation data but has none\n\tcommit-graph requires overflow generation data but has none\n        [... these 2x warnings came from \"git prune\" ...]\n\tChecking connectivity: 10359, done.\n\tExpanding reachable commits in commit graph: 344133, done.\n\tcommit-graph requires overflow generation data but has none\n\tComputing commit changed paths Bloom filters: 100% (344133/344133), done.\n\tWriting out commit graph in 7 passes: 100% (2408931/2408931), done.\n\nIf anyone wants to run with it the below hack has my SOB.\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 92d45033366..58e238aaa57 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -202,6 +202,20 @@ static struct commit_graph *alloc_commit_graph(void)\n \n extern int read_replace_refs;\n \n+static int verify_commit_graph_error;\n+\n+__attribute__((format (printf, 1, 2)))\n+static void graph_report(const char *fmt, ...)\n+{\n+\tva_list ap;\n+\n+\tverify_commit_graph_error = 1;\n+\tva_start(ap, fmt);\n+\tvfprintf(stderr, fmt, ap);\n+\tfprintf(stderr, \"\\n\");\n+\tva_end(ap);\n+}\n+\n static int commit_graph_compatible(struct repository *r)\n {\n \tif (!r->gitdir)\n@@ -220,6 +234,9 @@ static int commit_graph_compatible(struct repository *r)\n \tif (is_repository_shallow(r))\n \t\treturn 0;\n \n+\tif (verify_commit_graph_error) /* TODO: will be stale? */\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \n@@ -625,6 +642,10 @@ static int prepare_commit_graph(struct repository *r)\n {\n \tstruct object_directory *odb;\n \n+\tif (verify_commit_graph_error) /* TODO: will be stale? */\n+\t\treturn 0;\n+\n+\n \t/*\n \t * Early return if there is no git dir or if the commit graph is\n \t * disabled.\n@@ -766,7 +787,7 @@ static struct commit_list **insert_parent_or_die(struct repository *r,\n \treturn &commit_list_insert(c, pptr)->next;\n }\n \n-static void fill_commit_graph_info(struct commit *item, struct commit_graph *g, uint32_t pos)\n+static int fill_commit_graph_info(struct commit *item, struct commit_graph *g, uint32_t pos)\n {\n \tconst unsigned char *commit_data;\n \tstruct commit_graph_data *graph_data;\n@@ -776,8 +797,10 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,\n \twhile (pos < g->num_commits_in_base)\n \t\tg = g->base_graph;\n \n-\tif (pos >= g->num_commits + g->num_commits_in_base)\n-\t\tdie(_(\"invalid commit position. commit-graph is likely corrupt\"));\n+\tif (pos >= g->num_commits + g->num_commits_in_base) {\n+\t\tgraph_report(_(\"invalid commit position. commit-graph is likely corrupt\"));\n+\t\treturn 0;\n+\t}\n \n \tlex_index = pos - g->num_commits_in_base;\n \tcommit_data = g->chunk_commit_data + GRAPH_DATA_WIDTH * lex_index;\n@@ -793,8 +816,10 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,\n \t\toffset = (timestamp_t)get_be32(g->chunk_generation_data + sizeof(uint32_t) * lex_index);\n \n \t\tif (offset & CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW) {\n-\t\t\tif (!g->chunk_generation_data_overflow)\n-\t\t\t\tdie(_(\"commit-graph requires overflow generation data but has none\"));\n+\t\t\tif (!g->chunk_generation_data_overflow) {\n+\t\t\t\tgraph_report(_(\"commit-graph requires overflow generation data but has none\"));\n+\t\t\t\treturn 0;\n+\t\t\t}\n \n \t\t\toffset_pos = offset ^ CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW;\n \t\t\tgraph_data->generation = item->date + get_be64(g->chunk_generation_data_overflow + 8 * offset_pos);\n@@ -805,6 +830,8 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,\n \n \tif (g->topo_levels)\n \t\t*topo_level_slab_at(g->topo_levels, item) = get_be32(commit_data + g->hash_len + 8) >> 2;\n+\n+\treturn 1;\n }\n \n static inline void set_commit_tree(struct commit *c, struct tree *t)\n@@ -825,7 +852,8 @@ static int fill_commit_in_graph(struct repository *r,\n \twhile (pos < g->num_commits_in_base)\n \t\tg = g->base_graph;\n \n-\tfill_commit_graph_info(item, g, pos);\n+\tif (!fill_commit_graph_info(item, g, pos))\n+\t\treturn 0;\n \n \tlex_index = pos - g->num_commits_in_base;\n \tcommit_data = g->chunk_commit_data + (g->hash_len + 16) * lex_index;\n@@ -946,6 +974,8 @@ int parse_commit_in_graph(struct repository *r, struct commit *item)\n void load_commit_graph_info(struct repository *r, struct commit *item)\n {\n \tuint32_t pos;\n+\tif (verify_commit_graph_error) /* TODO: will be stale? */\n+\t\treturn;\n \tif (!prepare_commit_graph(r))\n \t\treturn;\n \tif (find_commit_pos_in_graph(item, r->objects->commit_graph, &pos))\n@@ -2444,19 +2474,6 @@ int write_commit_graph(struct object_directory *odb,\n }\n \n #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2\n-static int verify_commit_graph_error;\n-\n-__attribute__((format (printf, 1, 2)))\n-static void graph_report(const char *fmt, ...)\n-{\n-\tva_list ap;\n-\n-\tverify_commit_graph_error = 1;\n-\tva_start(ap, fmt);\n-\tvfprintf(stderr, fmt, ap);\n-\tfprintf(stderr, \"\\n\");\n-\tva_end(ap);\n-}\n \n #define GENERATION_ZERO_EXISTS 1\n #define GENERATION_NUMBER_EXISTS 2\n@@ -2510,9 +2527,11 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \t\t}\n \n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n-\t\tif (!parse_commit_in_graph_one(r, g, graph_commit))\n+\t\tif (!parse_commit_in_graph_one(r, g, graph_commit)) {\n \t\t\tgraph_report(_(\"failed to parse commit %s from commit-graph\"),\n \t\t\t\t     oid_to_hex(&cur_oid));\n+\t\t\treturn verify_commit_graph_error;\n+\t\t}\n \t}\n \n \twhile (cur_fanout_pos < 256) {\n\n\n"},{"id":"456947","messageId":"YqIRD8IwUFt6T8p+@coredump.intra.peff.net","threadId":"57972","inReplyTo":"220609.86h74utg6j.gmgdl@evledraar.gmail.com","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-09T15:26:07Z","receivedAt":"2022-06-09T15:26:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2022 at 09:49:15AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> It's certainly interesting to see *how* we got to this state, but just\n> so we're on the same page: I fundimentally don't think it matters to the\n> *real* bug here.\n> \n> Which is that at the very least f90fca638e9 (commit-graph: consolidate\n> fill_commit_graph_info, 2021-01-16) and e8b63005c48 (commit-graph:\n> implement generation data chunk, 2021-01-16) (CC'd author) have a bad\n> regression on earlier fixes that read-only operations of the\n> commit-graph *must not die*. I.e. the \"parse\" and \"verify\" paths of the\n> commit-graph.c code shouldn't call exit(), die() etc.\n\nYeah, I'd agree that this is a good philosophy to follow. The\ncommit-graph data is meant to be an optimization, and we can always\ncontinue without it.\n\n> If you replace your graph with Jeff's corrupt one and run \"git status\",\n> \"git log\" etc. it's still emitting one verbose complaint, but it no\n> longer does so in loops (at least for these paths, but e.g. \"git gc\" is\n> still doing that).\n> \n> But it does get us to where we can run \"git gc\", and while complaining\n> too much along the way will write out a new & valid commit graph at the\n> end (\"[... comments are mine\"):\n\nYeah, getting through \"git gc\" is the key thing here. Then the problem\nsolves itself, sometimes even automatically (via auto-gc).\n\n-Peff\n"},{"id":"456948","messageId":"a6ff4bcf-b00b-d19e-d7a8-4a2833dd14b8@github.com","threadId":"57972","inReplyTo":"YqIRD8IwUFt6T8p+@coredump.intra.peff.net","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-09T15:39:49Z","receivedAt":"2022-06-09T15:39:56Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/9/2022 11:26 AM, Jeff King wrote:\n> On Thu, Jun 09, 2022 at 09:49:15AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n>> It's certainly interesting to see *how* we got to this state, but just\n>> so we're on the same page: I fundimentally don't think it matters to the\n>> *real* bug here.\n>>\n>> Which is that at the very least f90fca638e9 (commit-graph: consolidate\n>> fill_commit_graph_info, 2021-01-16) and e8b63005c48 (commit-graph:\n>> implement generation data chunk, 2021-01-16) (CC'd author) have a bad\n>> regression on earlier fixes that read-only operations of the\n>> commit-graph *must not die*. I.e. the \"parse\" and \"verify\" paths of the\n>> commit-graph.c code shouldn't call exit(), die() etc.\n> \n> Yeah, I'd agree that this is a good philosophy to follow. The\n> commit-graph data is meant to be an optimization, and we can always\n> continue without it.\n\nI agree that the die() is part of what is frustrating here, but we need\nto be careful: when we recognize that the commit-graph data is erroneous\n_at this stage_ we may have already made decisions based on the existence\nof a commit-graph (such as \"we should trust generation numbers\" or \"we\nhave parsed some of the commits using the commit-graph\") and so we cannot\nguarantee that the process will complete with correct results from this\npoint.\n \n>> If you replace your graph with Jeff's corrupt one and run \"git status\",\n>> \"git log\" etc. it's still emitting one verbose complaint, but it no\n>> longer does so in loops (at least for these paths, but e.g. \"git gc\" is\n>> still doing that).\n>>\n>> But it does get us to where we can run \"git gc\", and while complaining\n>> too much along the way will write out a new & valid commit graph at the\n>> end (\"[... comments are mine\"):\n> \n> Yeah, getting through \"git gc\" is the key thing here. Then the problem\n> solves itself, sometimes even automatically (via auto-gc).\n\nPerhaps we could change the die() behavior to be a warning() plus a\nreset of all commit data if we are in a mode that can handle that error.\nSpecifically, during a commit-graph write.\n\nI think the current die() behavior is the safest thing to do right now\nuntil someone has time to think through these scenarios carefully.\n\nThanks,\n-Stolee\n"},{"id":"458380","messageId":"Yr7jY6GjUkOzHNh6@ncase","threadId":"57972","inReplyTo":"YqEuE2iAmMVLlUSu@coredump.intra.peff.net","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-07-01T12:06:59Z","receivedAt":"2022-07-01T12:07:18Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jun 08, 2022 at 07:17:39PM -0400, Jeff King wrote:\n> On Wed, Jun 08, 2022 at 04:08:03PM -0400, Derrick Stolee wrote:\n> \n> > I'd love to see the full binary, but for the sake of sharing on the\n> > list, could you give the following output?\n> > \n> > \txxd .git/objects/info/commit-graph | head\n> > \n> > or any other command that shows the first few hex bytes along with\n> > their ASCII equivalents. Here is one that used Git 2.34.0:\n> > [...]\n> \n> Interesting. My earlier email was a bit misleading. I do in fact have a\n> GDA2 chunk. And looking at the timestamp on the commit-graph file, it's\n> from May 24th. I hadn't been keeping the repo up to date regularly, but\n> I did occasionally pull and rebuild. So I think it was a much more\n> recent version of Git that built the problematic file, though it's\n> possible it was carrying forward bad data.\n> \n> So 6dbf4b8172ef may be a bit of a red herring, if the file has a GDA2\n> section that was simply ignored before that commit.\n> \n> Looking at my reflog, my best guess for the version of Git that produced\n> the file is e46751e96fa.\n> \n> > However, the lack of the large offset chunk could be due to the bug fixed by\n> > 75979d9460 (commit-graph: fix ordering bug in generation numbers,\n> > 2022-03-01). Perhaps that was the thing that was missing from your version?\n> \n> So I _think_ I would have had that, though there's a good chance that an\n> older version of the commit-graph file was written using a version of\n> Git without it.\n> \n> > But otherwise, I'm stumped. I'd be very interested to see a repro from a\n> > fresh repository. That is: what situation do we need to be in to write such\n> > an offset without including the large offset chunk?\n> \n> Not exactly a fresh reproduction, but you can grab my broken file from:\n> \n>   https://peff.net/tmp/broken-commit-graph\n> \n> Dropping it into a fresh clone of git.git shows the problem.\n> \n> I tried a few obvious from-scratch reproductions like building a file\n> with 75979d9460^ (so with the generation number bug), and then jumping\n> forward to e46751e96fa (so bug fixed, but now we write GDA2), but\n> couldn't get it to trigger.\n> \n> It may not be worth spending too much time on, if this is a weird\n> one-off caused by a mix of buggy unreleased versions of Git. If real\n> users aren't seeing it, and we know the nuclear option is \"rm\n> commit-graph\", then that may be enough.\n> \n> -Peff\n\nI have also repeatedly run into the same problem. I had already\ndiscussed this with Derrick in the past in [1], but back then we also\ndeclared bancruptcy and said that this seems to only be caused by some\nweird in-between states of Git versions.\n\nI have experienced the issue again in git.git now, again without having\na clue how I arrived at that state. The funny thing is that I explicitly\ntried to reproduce the error in that repo a few days ago, without any\nsuccess at all, by writing commit-graphs with different Git versions.\nOnly today when I got back to it completely unsuspecting did Git start\nto complain.\n\nBut more imporantly, we started to see the issue in one of our repos in\nour staging systems as well [2], where we're currently running with a\nmixture of Git v2.35.1 and v2.36.1 with a small set of patches on top of\nthem. None of the patches are related to commit-graphs though. The repo\nin question is a pooled repository (like last time I reported the bug),\nwhere the pool itself has a single commit-graph with GDAT chunks and the\npool member has a single commit-graph with GDA2 chunks.\n\nI spent a lot of time today to try and come up with a reproducer to get\nto this state from a clean repo, but again with no success so far. Also,\nstaring at the code for extended periods of time didn't result in any\ninsights.\n\nThis issue continues to puzzle me.\n\nPatrick\n\n[1]: http://public-inbox.org/git/Yh3rZX6cJpkHmRZc@ncase/\n[2]: https://gitlab.com/gitlab-org/gitlab/-/issues/365903\n"},{"id":"458444","messageId":"YsLE+DVa5Hd/NqdD@ncase","threadId":"57972","inReplyTo":"Yr7jY6GjUkOzHNh6@ncase","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-07-04T10:46:16Z","receivedAt":"2022-07-04T10:46:32Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jul 01, 2022 at 02:07:03PM +0200, Patrick Steinhardt wrote:\n> On Wed, Jun 08, 2022 at 07:17:39PM -0400, Jeff King wrote:\n> > On Wed, Jun 08, 2022 at 04:08:03PM -0400, Derrick Stolee wrote:\n> > \n> > > I'd love to see the full binary, but for the sake of sharing on the\n> > > list, could you give the following output?\n> > > \n> > > \txxd .git/objects/info/commit-graph | head\n> > > \n> > > or any other command that shows the first few hex bytes along with\n> > > their ASCII equivalents. Here is one that used Git 2.34.0:\n> > > [...]\n> > \n> > Interesting. My earlier email was a bit misleading. I do in fact have a\n> > GDA2 chunk. And looking at the timestamp on the commit-graph file, it's\n> > from May 24th. I hadn't been keeping the repo up to date regularly, but\n> > I did occasionally pull and rebuild. So I think it was a much more\n> > recent version of Git that built the problematic file, though it's\n> > possible it was carrying forward bad data.\n> > \n> > So 6dbf4b8172ef may be a bit of a red herring, if the file has a GDA2\n> > section that was simply ignored before that commit.\n> > \n> > Looking at my reflog, my best guess for the version of Git that produced\n> > the file is e46751e96fa.\n> > \n> > > However, the lack of the large offset chunk could be due to the bug fixed by\n> > > 75979d9460 (commit-graph: fix ordering bug in generation numbers,\n> > > 2022-03-01). Perhaps that was the thing that was missing from your version?\n> > \n> > So I _think_ I would have had that, though there's a good chance that an\n> > older version of the commit-graph file was written using a version of\n> > Git without it.\n> > \n> > > But otherwise, I'm stumped. I'd be very interested to see a repro from a\n> > > fresh repository. That is: what situation do we need to be in to write such\n> > > an offset without including the large offset chunk?\n> > \n> > Not exactly a fresh reproduction, but you can grab my broken file from:\n> > \n> >   https://peff.net/tmp/broken-commit-graph\n> > \n> > Dropping it into a fresh clone of git.git shows the problem.\n> > \n> > I tried a few obvious from-scratch reproductions like building a file\n> > with 75979d9460^ (so with the generation number bug), and then jumping\n> > forward to e46751e96fa (so bug fixed, but now we write GDA2), but\n> > couldn't get it to trigger.\n> > \n> > It may not be worth spending too much time on, if this is a weird\n> > one-off caused by a mix of buggy unreleased versions of Git. If real\n> > users aren't seeing it, and we know the nuclear option is \"rm\n> > commit-graph\", then that may be enough.\n> > \n> > -Peff\n> \n> I have also repeatedly run into the same problem. I had already\n> discussed this with Derrick in the past in [1], but back then we also\n> declared bancruptcy and said that this seems to only be caused by some\n> weird in-between states of Git versions.\n> \n> I have experienced the issue again in git.git now, again without having\n> a clue how I arrived at that state. The funny thing is that I explicitly\n> tried to reproduce the error in that repo a few days ago, without any\n> success at all, by writing commit-graphs with different Git versions.\n> Only today when I got back to it completely unsuspecting did Git start\n> to complain.\n> \n> But more imporantly, we started to see the issue in one of our repos in\n> our staging systems as well [2], where we're currently running with a\n> mixture of Git v2.35.1 and v2.36.1 with a small set of patches on top of\n> them. None of the patches are related to commit-graphs though. The repo\n> in question is a pooled repository (like last time I reported the bug),\n> where the pool itself has a single commit-graph with GDAT chunks and the\n> pool member has a single commit-graph with GDA2 chunks.\n> \n> I spent a lot of time today to try and come up with a reproducer to get\n> to this state from a clean repo, but again with no success so far. Also,\n> staring at the code for extended periods of time didn't result in any\n> insights.\n> \n> This issue continues to puzzle me.\n> \n> Patrick\n> \n> [1]: http://public-inbox.org/git/Yh3rZX6cJpkHmRZc@ncase/\n> [2]: https://gitlab.com/gitlab-org/gitlab/-/issues/365903\n\nWhile I still haven't been able to reproduce the error, I did find a\ndifferent error. Here's the reproducer, which works with Git v2.37.0 and\nolder:\n\n```\n+ rm -rf /tmp/repo\n+ git init /tmp/repo\nInitialized empty Git repository in /tmp/repo/.git/\n+ cd /tmp/repo/\n+ GIT_COMMITTER_DATE='2000-01-01T00:00:00 +0100'\n+ git commit --allow-empty -mx\n[main (root-commit) 62ebc8d] x\n+ git branch other\n+ GIT_COMMITTER_DATE='1970-01-01T00:00:00 +0100'\n+ git commit --allow-empty -mx\n[main c628d6d] x\n+ GIT_COMMITTER_DATE='2040-01-01T00:00:00 +0100'\n+ git commit --allow-empty -mx\n[main 0d73218] x\n+ git commit-graph write --reachable --split=replace\n+ git switch other\nSwitched to branch 'other'\n+ GIT_COMMITTER_DATE='2000-01-01T00:00:00 +0100'\n+ git commit --allow-empty -mx\n[other 7d03e12] x\n+ git commit-graph write --reachable --split=replace\n+ git commit-graph verify\ncommit date for commit c628d6dc7292b6d481f0ec4ed39ed2bb4a8cff49 in commit-graph is 17179865584 != 18446744073709548016\nVerifying commits in commit graph: 100% (4/4), done.\n```\n\nI may have a look at a later point at what's happening, but for the time\nbeing I'll continue to hunt down the other bug. Still wanted to document\nmy finding out here.\n\nPatrick\n"},{"id":"458458","messageId":"a154e109-3f4c-c500-3365-d47879abf30d@github.com","threadId":"57972","inReplyTo":"YsLE+DVa5Hd/NqdD@ncase","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-04T20:50:57Z","receivedAt":"2022-07-04T20:51:21Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/4/22 4:46 AM, Patrick Steinhardt wrote:\n> On Fri, Jul 01, 2022 at 02:07:03PM +0200, Patrick Steinhardt wrote:\n\n> While I still haven't been able to reproduce the error, I did find a\n> different error. Here's the reproducer, which works with Git v2.37.0 and\n> older:\n\n\nHi Patrick. Thanks for taking a close look. There is\nan issue here, and it's due to using a negative\ntimestamp:\n\n> + GIT_COMMITTER_DATE='1970-01-01T00:00:00 +0100'\n\nBecause of the \"+0100\" in the time zone, this date\nbecomes a negative value. The commit-graph does not\nstore dates with more than 34 bits (and Git does\nnot handle negative timestamps very well? Peff can\nclarify here).\n\nThe commit-graph could certainly warn better here to\nsay we do not have enough date bits to store this\ntimestamp (the same would happen with a date beyond\n2138 or something like that).\n\nHowever, this is a failure since the commit-graph first\nstarted parsing dates in 177722b3442 (commit: integrate\ncommit graph with commit parsing, 2018-04-10).\n\nThanks,\n-Stolee\n"},{"id":"458495","messageId":"DD88D523-0ECA-4474-9AA5-1D4A431E532A@wfchandler.org","threadId":"57972","inReplyTo":"a154e109-3f4c-c500-3365-d47879abf30d@github.com","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Will Chandler","fromEmail":"wfc@wfchandler.org","sentAt":"2022-07-05T21:03:44Z","receivedAt":"2022-07-05T21:03:51Z","isPatch":false,"sender":{"key":"wfc@wfchandler.org","avatar":null},"body":"Hi all,\n\nI think I've reproduced this problem on v2.36.1 and main.\n\nFirst, remove any existing commit graphs:\n\n  $ rm -rf .git/objects/info/commit-graph*\n\nGenerate a commit graph without GDA2:\n\n  $ git -c commitGraph.generationVersion=1 commit-graph write\n\nPeff's bad commit graph included bloom filters, so let's update the\ncommit graph to include them and add GDA2:\n\n  $ git -c commitGraph.generationVersion=2 commit-graph write --changed-paths\n\nThe commit graph is now broken:\n\n  $ git rev-list --all\n  fatal: commit-graph requires overflow generation data but has none\n\nThe header has a GDA2 section, but no GDO2:\n\n  $ xxd .git/objects/info/commit-graphs/*graph | head\n  00000000: 4347 5048 0101 0700 4f49 4446 0000 0000  CGPH....OIDF....\n  00000010: 0000 0068 4f49 444c 0000 0000 0000 0468  ...hOIDL.......h\n  00000020: 4344 4154 0000 0000 0015 43b4 4744 4132  CDAT......C.GDA2\n  00000030: 0000 0000 003b 8270 4544 4745 0000 0000  .....;.pEDGE....\n  00000040: 003f c24c 4249 4458 0000 0000 003f c3cc  .?.LBIDX.....?..\n  00000050: 4244 4154 0000 0000 0044 03a8 0000 0000  BDAT.....D......\n  00000060: 0000 0000 0049 79df 0000 010e 0000 0216  .....Iy.........\n  00000070: 0000 031f 0000 042f 0000 0535 0000 0623  ......./...5...#\n  00000080: 0000 0730 0000 086d 0000 096d 0000 0a8c  ...0...m...m....\n  00000090: 0000 0b8e 0000 0c96 0000 0d9c 0000 0ea2  ................\n\nViewing the GDA2 block, the overflow bit is set for virtually all of the commits:\n\n  $ xxd -s 0x003b8270 .git/objects/info/commit-graphs/*.graph | head\n  003b8270: 8000 0000 8000 0001 8000 0002 8000 0003  ................\n  003b8280: 8000 0004 8000 0005 8000 0006 8000 0007  ................\n  003b8290: 8000 0008 8000 0009 8000 000a 8000 000b  ................\n  003b82a0: 8000 000c 8000 000d 8000 000e 8000 000f  ................\n  003b82b0: 8000 0010 8000 0011 8000 0012 8000 0013  ................\n  003b82c0: 8000 0014 8000 0015 8000 0016 8000 0017  ................\n  003b82d0: 8000 0018 8000 0019 8000 001a 8000 001b  ................\n  003b82e0: 8000 001c 8000 001d 8000 001e 8000 001f  ................\n  003b82f0: 8000 0020 8000 0021 8000 0022 8000 0023  ... ...!...\"...#\n  003b8300: 8000 0024 8000 0025 8000 0026 8000 0027  ...$...%...&...'\n\nStepping though, it looks like this is the scenario:\n\ncompute_generation_numbers correctly calculates the generation, e.g.:\n\n  {\n    graph_pos = 0,\n    generation = 1502820840\n  }\n\nBecause the existing commit graph did not include generation data,\ngraph.read_generation_data is 0. When compute_bloom_filters executes, it\nwill call fill_commit_graph_info which checks that value and falls back\nto the older generation calculation if false:\n\n  if (g->read_generation_data) {\n  \toffset = (timestamp_t)get_be32(g->chunk_generation_data + sizeof(uint32_t) * lex_index);\n\n  \tif (offset & CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW) {\n  \t\tif (!g->chunk_generation_data_overflow)\n  \t\t\tdie(_(\"commit-graph requires overflow generation data but has none\"));\n\n  \t\toffset_pos = offset ^ CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW;\n  \t\tgraph_data->generation = item->date + get_be64(g->chunk_generation_data_overflow + 8 * offset_pos);\n  \t} else\n  \t\tgraph_data->generation = item->date + offset;\n  } else\n  \tgraph_data->generation = get_be32(commit_data + g->hash_len + 8) >> 2;\n\nThis re-writes the commit data to:\n\n  {\n    graph_pos = 0,\n    generation = 17631\n  }\n\nThe smaller generation value underflows when the commit date is\nsubtracted from it by write_graph_chunk_generation_data, causing the\noverflow flag to be set:\n\n  static int write_graph_chunk_generation_data(struct hashfile *f,\n\t\t\t\t\t     void *data)\n  {\n  \tstruct write_commit_graph_context *ctx = data;\n  \tint i, num_generation_data_overflows = 0;\n\n  \tfor (i = 0; i < ctx->commits.nr; i++) {\n  \t\tstruct commit *c = ctx->commits.list[i];\n  \t\ttimestamp_t offset;\n  \t\trepo_parse_commit(ctx->r, c);\n  \t\toffset = commit_graph_data_at(c)->generation - c->date;\n  \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n\n  \t\tif (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n  \t\t\toffset = CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW | num_generation_data_overflows;\n  \t\t\tnum_generation_data_overflows++;\n  \t\t}\n\n  \t\thashwrite_be32(f, offset);\n  \t}\n\n  \treturn 0;\n  }\n\nWe've already decided by this point to skip writing the overflow chunk\nbased on compute_generation_numbers, so the commit graph is now\ninvalid.\n"},{"id":"458497","messageId":"YsS7H4i5DqUZVQ5i@nand.local","threadId":"57972","inReplyTo":"DD88D523-0ECA-4474-9AA5-1D4A431E532A@wfchandler.org","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-05T22:28:47Z","receivedAt":"2022-07-05T22:28:51Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 05, 2022 at 05:03:44PM -0400, Will Chandler wrote:\n> Hi all,\n>\n> I think I've reproduced this problem on v2.36.1 and main.\n\nGreat find. I played around with this a little bit locally and your\nreproduction works for me on any sufficiently-large repository. Looking\naround in the code, I'm pretty confident that this is the issue.\n\n>   $ git -c commitGraph.generationVersion=2 commit-graph write --changed-paths\n\nThis step is critical: without it we never end up overwriting the\n`->generation` data, and the conversion works fine.\n\n> Because the existing commit graph did not include generation data,\n> graph.read_generation_data is 0. When compute_bloom_filters executes, it\n> will call fill_commit_graph_info which checks that value and falls back\n> to the older generation calculation if false:\n>\n>   if (g->read_generation_data) {\n>   \toffset = (timestamp_t)get_be32(g->chunk_generation_data + sizeof(uint32_t) * lex_index);\n>\n>   \tif (offset & CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW) {\n>   \t\tif (!g->chunk_generation_data_overflow)\n>   \t\t\tdie(_(\"commit-graph requires overflow generation data but has none\"));\n>\n>   \t\toffset_pos = offset ^ CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW;\n>   \t\tgraph_data->generation = item->date + get_be64(g->chunk_generation_data_overflow + 8 * offset_pos);\n>   \t} else\n>   \t\tgraph_data->generation = item->date + offset;\n>   } else\n>   \tgraph_data->generation = get_be32(commit_data + g->hash_len + 8) >> 2;\n>\n> This re-writes the commit data to:\n>\n>   {\n>     graph_pos = 0,\n>     generation = 17631\n>   }\n\nNicely explained. To me, it seems like the problem is that we're reusing\nthe same slab to:\n\n  - store data that we're going to write as a part of commit-graph\n    generation, and\n  - store auxiliary data about commits that we have *read* from a\n    commit-graph\n\nA complete fix might be to use a separate slab to store data that we\nread from data that we are about to write, to avoid polluting the latter\nwith the former.\n\nIn the meantime, a more minimal fix would be to avoid reading and\noverwriting the generation data where we can avoid it. AFAICT, this is\nthe only spot that would need to be changed. The following patch does\nthe trick for me:\n\n--- >8 ---\n\ndiff --git a/bloom.c b/bloom.c\nindex 5e297038bb..863052fa68 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -30,10 +30,9 @@ static inline unsigned char get_bitmask(uint32_t pos)\n\n static int load_bloom_filter_from_graph(struct commit_graph *g,\n \t\t\t\t\tstruct bloom_filter *filter,\n-\t\t\t\t\tstruct commit *c)\n+\t\t\t\t\tuint32_t graph_pos)\n {\n \tuint32_t lex_pos, start_index, end_index;\n-\tuint32_t graph_pos = commit_graph_position(c);\n\n \twhile (graph_pos < g->num_commits_in_base)\n \t\tg = g->base_graph;\n@@ -203,9 +202,11 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,\n \tfilter = bloom_filter_slab_at(&bloom_filters, c);\n\n \tif (!filter->data) {\n-\t\tload_commit_graph_info(r, c);\n-\t\tif (commit_graph_position(c) != COMMIT_NOT_FROM_GRAPH)\n-\t\t\tload_bloom_filter_from_graph(r->objects->commit_graph, filter, c);\n+\t\tuint32_t pos;\n+\t\tif ((repo_find_commit_pos_in_graph(r, c, &pos)) &&\n+\t\t    (pos != COMMIT_NOT_FROM_GRAPH))\n+\t\t\tload_bloom_filter_from_graph(r->objects->commit_graph,\n+\t\t\t\t\t\t     filter, pos);\n \t}\n\n \tif (filter->data && filter->len)\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 92d4503336..2d9fad5910 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -889,6 +889,14 @@ static int find_commit_pos_in_graph(struct commit *item, struct commit_graph *g,\n \t}\n }\n\n+int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,\n+\t\t\t\t  uint32_t *pos)\n+{\n+\tif (!prepare_commit_graph(r))\n+\t\treturn 0;\n+\treturn find_commit_pos_in_graph(c, r->objects->commit_graph, pos);\n+}\n+\n struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)\n {\n \tstruct commit *commit;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 2e3ac35237..52ddfbe349 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -40,6 +40,9 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n  */\n int parse_commit_in_graph(struct repository *r, struct commit *item);\n\n+int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,\n+\t\t\t\t  uint32_t *pos);\n+\n /*\n  * Look up the given commit ID in the commit-graph. This will only return a\n  * commit if the ID exists both in the graph and in the object database such\n\n--- 8< ---\n\nThanks,\nTaylor\n"},{"id":"458511","messageId":"YsVNRXPFFdjlvMy9@coredump.intra.peff.net","threadId":"57972","inReplyTo":"YsS7H4i5DqUZVQ5i@nand.local","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-06T08:52:21Z","receivedAt":"2022-07-06T08:52:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 05, 2022 at 06:28:47PM -0400, Taylor Blau wrote:\n\n> > This re-writes the commit data to:\n> >\n> >   {\n> >     graph_pos = 0,\n> >     generation = 17631\n> >   }\n> \n> Nicely explained. To me, it seems like the problem is that we're reusing\n> the same slab to:\n> \n>   - store data that we're going to write as a part of commit-graph\n>     generation, and\n>   - store auxiliary data about commits that we have *read* from a\n>     commit-graph\n> \n> A complete fix might be to use a separate slab to store data that we\n> read from data that we are about to write, to avoid polluting the latter\n> with the former.\n\nYeah, that was my first instinct, too. But from a skim of the code, I\nthink that may be complicated, as the two uses don't seem to be\ndifferentiated well. Or at least it resisted some basic hacky efforts I\nmade (e.g., splitting commit_graph_data_at() from the more limited\nreaders). I'm not that familiar with the commit-graph writing code; I\nsuspect Stolee might have something intelligent to say.\n\n> In the meantime, a more minimal fix would be to avoid reading and\n> overwriting the generation data where we can avoid it. AFAICT, this is\n> the only spot that would need to be changed. The following patch does\n> the trick for me:\n\nYeah, it fixes it for me, too, but it leaves me with many questions. ;)\nSpecifically:\n\n  - if it's so easy to get the position, why do we have the position in\n    the slab in the first place? Or is it for the cases where we're\n    writing (though then the question is: why do we fill it for the read\n    case, then)?\n\n  - if I understand correctly, the real sin here was calling\n    commit_graph_position() during the write. How can we find\n    other potential problem spots? From my (again, admittedly not well\n    developed) view of the code, it seems like an intentional choice\n    that you could intermingle computed and from-disk results.\n\nAt any rate, thanks to both you and Will for moving this forward.\n\n-Peff\n"},{"id":"458512","messageId":"YsVRq82ewvvIyxDT@coredump.intra.peff.net","threadId":"57972","inReplyTo":"a154e109-3f4c-c500-3365-d47879abf30d@github.com","subject":"Re: commit-graph overflow generation chicken and egg","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-07-06T09:11:07Z","receivedAt":"2022-07-06T09:13:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 04, 2022 at 02:50:57PM -0600, Derrick Stolee wrote:\n\n> > + GIT_COMMITTER_DATE='1970-01-01T00:00:00 +0100'\n> \n> Because of the \"+0100\" in the time zone, this date\n> becomes a negative value. The commit-graph does not\n> store dates with more than 34 bits (and Git does\n> not handle negative timestamps very well? Peff can\n> clarify here).\n\nI don't think we handle them at all. Certainly we'll refuse to parse\nany, but here we get tricked by the timezone, and end up with the\nunderflow. Git writes 2^64 - 3600 into the commit object with this one.\n\nThe code in gm_time_t() tries to catch this (as well as overflow), but\nthis one gets handled by parse_date_basic(), which doesn't. Adding this\ncauses it barf:\n\ndiff --git a/date.c b/date.c\nindex 68a260c214..07a60fee35 100644\n--- a/date.c\n+++ b/date.c\n@@ -893,8 +893,11 @@ int parse_date_basic(const char *date, timestamp_t *timestamp, int *offset)\n \t\t}\n \t}\n \n-\tif (!tm_gmt)\n+\tif (!tm_gmt) {\n+\t\tif (*timestamp < *offset * 60)\n+\t\t\treturn -1;\n \t\t*timestamp -= *offset * 60;\n+\t}\n \treturn 0; /* success */\n }\n \n\nWe should probably check for overflow there, as well, though I think in\npractice it would get caught earlier. The timestamp variable comes from\ntm_to_time_t(), which already has some limits.\n\nOf course the right fix is for us to handle negative timestamps. I have\nsome patches working towards that, but...\n\n> The commit-graph could certainly warn better here to\n> say we do not have enough date bits to store this\n> timestamp (the same would happen with a date beyond\n> 2138 or something like that).\n\nRight. By the time it gets to the commit-graph, it's a gigantic positive\ntime. But either way, yes, it should realize it's not representable and\nomit the commit from the graph file.\n\nThat's something I haven't looked at for my negative timestamp patches\nat all. I think it's OK for the commit-graph to simply punt on them, as\nlong as it doesn't die or produce wrong answers.\n\n-Peff\n"}]}