{"thread":{"id":"55086","subject":"[PATCH 1/5] commit-graph: use repo_parse_commit","startedAt":"2021-02-01T17:15:56Z","lastAt":"2021-02-11T04:45:11Z","messageCount":37,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Derrick Stolee","Junio C Hamano","Jonathan Nieder","Eric Sunshine","SZEDER Gábor","Abhishek Kumar"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"415768","messageId":"9c605c99f66cae3c66bcdd4cbeefbfa1ec2be192.1612199707.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.git.1612199707.gitgitgadget@gmail.com","subject":"[PATCH 1/5] commit-graph: use repo_parse_commit","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-01T17:15:03Z","receivedAt":"2021-02-01T17:15:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe write_commit_graph_context has a repository pointer, so use it.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 10 +++++-----\n commit.h | 5 +++--\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f3bde2ad95a..03e5a987968 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1098,7 +1098,7 @@ static int write_graph_chunk_data(struct hashfile *f,\n \t\tuint32_t packedDate[2];\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n \n-\t\tif (parse_commit_no_graph(*list))\n+\t\tif (repo_parse_commit_no_graph(ctx->r, *list))\n \t\t\tdie(_(\"unable to parse commit %s\"),\n \t\t\t\toid_to_hex(&(*list)->object.oid));\n \t\ttree = get_commit_tree_oid(*list);\n@@ -1411,11 +1411,11 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n \t\tif (!commit)\n \t\t\tcontinue;\n \t\tif (ctx->split) {\n-\t\t\tif ((!parse_commit(commit) &&\n+\t\t\tif ((!repo_parse_commit(ctx->r, commit) &&\n \t\t\t commit_graph_position(commit) == COMMIT_NOT_FROM_GRAPH) ||\n \t\t\t flags == COMMIT_GRAPH_SPLIT_REPLACE)\n \t\t\t\tadd_missing_parents(ctx, commit);\n-\t\t} else if (!parse_commit_no_graph(commit))\n+\t\t} else if (!repo_parse_commit_no_graph(ctx->r, commit))\n \t\t\tadd_missing_parents(ctx, commit);\n \t}\n \tstop_progress(&ctx->progress);\n@@ -1710,9 +1710,9 @@ static void copy_oids_to_commits(struct write_commit_graph_context *ctx)\n \t\t\tcontinue;\n \n \t\tif (ctx->split && flags == COMMIT_GRAPH_SPLIT_REPLACE)\n-\t\t\tparse_commit(ctx->commits.list[ctx->commits.nr]);\n+\t\t\trepo_parse_commit(ctx->r, ctx->commits.list[ctx->commits.nr]);\n \t\telse\n-\t\t\tparse_commit_no_graph(ctx->commits.list[ctx->commits.nr]);\n+\t\t\trepo_parse_commit_no_graph(ctx->r, ctx->commits.list[ctx->commits.nr]);\n \n \t\tnum_parents = commit_list_count(ctx->commits.list[ctx->commits.nr]->parents);\n \t\tif (num_parents > 2)\ndiff --git a/commit.h b/commit.h\nindex 251d877fcf6..b05ab558ce2 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -89,9 +89,10 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)\n \treturn repo_parse_commit_gently(r, item, 0);\n }\n \n-static inline int parse_commit_no_graph(struct commit *commit)\n+static inline int repo_parse_commit_no_graph(struct repository *r,\n+\t\t\t\t\t struct commit *commit)\n {\n-\treturn repo_parse_commit_internal(the_repository, commit, 0, 0);\n+\treturn repo_parse_commit_internal(r, commit, 0, 0);\n }\n \n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n-- \ngitgitgadget\n\n"},{"id":"415769","messageId":"d554fa306601c9e5e0e804d10b7a73b6eece6b04.1612199707.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.git.1612199707.gitgitgadget@gmail.com","subject":"[PATCH 3/5] commit-graph: validate layers for generation data","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-01T17:15:05Z","receivedAt":"2021-02-01T17:15:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWe need to be extra careful that we don't use corrected\ncommit dates from any layer of a commit-graph chain if there is a\nsingle commit-graph file that is missing the generation_data chunk.\nUpdate validate_mixed_generation_chain() to correctly update each\nlayer to ignore the generation_data chunk in this case. It now also\nreturns 1 if all layers have a generation_data chunk. This return\nvalue will be used in the next change.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex edbb3a0f2cc..13992137dd0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -614,19 +614,26 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \treturn graph_chain;\n }\n \n-static void validate_mixed_generation_chain(struct commit_graph *g)\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;\n+\tint read_generation_data = 1;\n+\tstruct commit_graph *p = g;\n \n-\tif (!g)\n-\t\treturn;\n-\n-\tread_generation_data = !!g->chunk_generation_data;\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 \twhile (g) {\n \t\tg->read_generation_data = read_generation_data;\n \t\tg = g->base_graph;\n \t}\n+\n+\treturn read_generation_data;\n }\n \n struct commit_graph *read_commit_graph_one(struct repository *r,\n-- \ngitgitgadget\n\n"},{"id":"415770","messageId":"b267a9653a7560d1e59708f20106ef054d140a9f.1612199707.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.git.1612199707.gitgitgadget@gmail.com","subject":"[PATCH 4/5] commit-graph: be extra careful about mixed generations","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-01T17:15:06Z","receivedAt":"2021-02-01T17:16:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen upgrading to a commit-graph with corrected commit dates from\none without, there are a few things that need to be considered.\n\nWhen computing generation numbers for the new commit-graph file that\nexpects to add the generation_data chunk with corrected commit\ndates, we need to ensure that the 'generation' member of the\ncommit_graph_data struct is set to zero for these commits.\n\nUnfortunately, the fallback to use topological level for generation\nnumber when corrected commit dates are not available are causing us\nharm here: parsing commits notices that read_generation_data is\nfalse and populates 'generation' with the topological level.\n\nThe solution is to iterate through the commits, parse the commits\nto populate initial values, then reset the generation values to\nzero to trigger recalculation. This loop only occurs when the\nexisting commit-graph data has no corrected commit dates.\n\nWhile this improves our situation somewhat, we have not completely\nsolved the issue for correctly computing generation numbers for mixes\nlayers. That follows in the next change.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 32 +++++++++++++++++++++++---------\n 1 file changed, 23 insertions(+), 9 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 13992137dd0..08148dd17f1 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1033,7 +1033,8 @@ struct write_commit_graph_context {\n \t\t split:1,\n \t\t changed_paths:1,\n \t\t order_by_pack:1,\n-\t\t write_generation_data:1;\n+\t\t write_generation_data:1,\n+\t\t trust_generation_numbers:1;\n \n \tstruct topo_level_slab *topo_levels;\n \tconst struct commit_graph_opts *opts;\n@@ -1452,6 +1453,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\tctx->progress = start_delayed_progress(\n \t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n \t\t\t\t\tctx->commits.nr);\n+\n+\tif (ctx->write_generation_data && !ctx->trust_generation_numbers) {\n+\t\tfor (i = 0; i < ctx->commits.nr; i++) {\n+\t\t\tstruct commit *c = ctx->commits.list[i];\n+\t\t\trepo_parse_commit(ctx->r, c);\n+\t\t\tcommit_graph_data_at(c)->generation = GENERATION_NUMBER_ZERO;\n+\t\t}\n+\t}\n+\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.list[i];\n \t\tuint32_t level;\n@@ -1480,7 +1490,8 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n \n \t\t\t\tif (level == GENERATION_NUMBER_ZERO ||\n-\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO) {\n+\t\t\t\t (ctx->write_generation_data &&\n+\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO)) {\n \t\t\t\t\tall_parents_computed = 0;\n \t\t\t\t\tcommit_list_insert(parent->item, &list);\n \t\t\t\t\tbreak;\n@@ -1500,12 +1511,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\t\t\t\tmax_level = GENERATION_NUMBER_V1_MAX - 1;\n \t\t\t\t*topo_level_slab_at(ctx->topo_levels, current) = max_level + 1;\n \n-\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n-\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n-\t\t\t\tcommit_graph_data_at(current)->generation = max_corrected_commit_date + 1;\n-\n-\t\t\t\tif (commit_graph_data_at(current)->generation - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n-\t\t\t\t\tctx->num_generation_data_overflows++;\n+\t\t\t\tif (ctx->write_generation_data) {\n+\t\t\t\t\ttimestamp_t cur_g;\n+\t\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n+\t\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n+\t\t\t\t\tcur_g = commit_graph_data_at(current)->generation\n+\t\t\t\t\t = max_corrected_commit_date + 1;\n+\t\t\t\t\tif (cur_g - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n+\t\t\t\t\t\tctx->num_generation_data_overflows++;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n \t}\n@@ -2396,7 +2410,7 @@ int write_commit_graph(struct object_directory *odb,\n \t} else\n \t\tctx->num_commit_graphs_after = 1;\n \n-\tvalidate_mixed_generation_chain(ctx->r->objects->commit_graph);\n+\tctx->trust_generation_numbers = validate_mixed_generation_chain(ctx->r->objects->commit_graph);\n \n \tcompute_generation_numbers(ctx);\n \n-- \ngitgitgadget\n\n"},{"id":"415771","messageId":"pull.850.git.1612199707.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":null,"subject":"[PATCH 0/5] Generation Number v2: Fix a tricky split graph bug","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-01T17:15:02Z","receivedAt":"2021-02-01T17:16:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Here is a bugfix for the recently-landed-in-next generation v2 topic\n(ak/corrected-commit-date).\n\nThis was occasionally hitting us when computing new commit-graphs on\nexisting repositories with the new bits. It was very hard to reproduce, and\nit turns out to be due to not parsing commits before accessing generation\nnumber data. Doing so in the right place demonstrates the bug of recomputing\nthe corrected commit date even for commits in lower layers with computed\nvalues.\n\nThe fix is split into these steps:\n\n 1. Parse commits more often before accessing their data. (This allows the\n bug to be demonstrated in the test suite.)\n 2. Check the full commit-graph chain for generation data chunks.\n 3. Don't compute corrected commit dates if the lower layers do not support\n them.\n 4. Parse the commit-graph file more often.\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n commit-graph: use repo_parse_commit\n commit-graph: always parse before commit_graph_data_at()\n commit-graph: validate layers for generation data\n commit-graph: be extra careful about mixed generations\n commit-graph: prepare commit graph\n\n commit-graph.c | 87 ++++++++++++++++++++++++++---------------\n commit.h | 5 ++-\n t/t5318-commit-graph.sh | 21 ++++++++++\n 3 files changed, 79 insertions(+), 34 deletions(-)\n\n\nbase-commit: 5a3b130cad0d5c770f766e3af6d32b41766374c0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-850%2Fderrickstolee%2Fgen-v2-upgrade-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-850/derrickstolee/gen-v2-upgrade-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/850\n-- \ngitgitgadget\n"},{"id":"415772","messageId":"dddeec30ebfc6d8403cc10395a9d0c331d1f7ad3.1612199707.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.git.1612199707.gitgitgadget@gmail.com","subject":"[PATCH 5/5] commit-graph: prepare commit graph","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-01T17:15:07Z","receivedAt":"2021-02-01T17:16:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nBefore checking if the repository has a commit-graph loaded, be sure\nto run prepare_commit_graph(). This is necessary because without this\ninstance we would not initialize the topo_levels slab for each of the\nstruct commit_graphs in the chain before we start to parse the\ncommits. This leads to possibly recomputing the topological levels for\ncommits in lower layers even when we are adding a small number of\ncommits on top.\n\nBy properly initializing the topo_slab, we fix the previously broken\ncase of a split commit graph where a base layer has the\ngeneration_data_overflow chunk.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 10 ++--------\n t/t5318-commit-graph.sh | 2 +-\n 2 files changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 08148dd17f1..858fa5594e3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2309,6 +2309,7 @@ int write_commit_graph(struct object_directory *odb,\n \tinit_topo_level_slab(&topo_levels);\n \tctx->topo_levels = &topo_levels;\n \n+\tprepare_commit_graph(ctx->r);\n \tif (ctx->r->objects->commit_graph) {\n \t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n \n@@ -2322,7 +2323,6 @@ int write_commit_graph(struct object_directory *odb,\n \t\tctx->changed_paths = 1;\n \tif (!(flags & COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS)) {\n \t\tstruct commit_graph *g;\n-\t\tprepare_commit_graph_one(ctx->r, ctx->odb);\n \n \t\tg = ctx->r->objects->commit_graph;\n \n@@ -2334,10 +2334,7 @@ int write_commit_graph(struct object_directory *odb,\n \t}\n \n \tif (ctx->split) {\n-\t\tstruct commit_graph *g;\n-\t\tprepare_commit_graph(ctx->r);\n-\n-\t\tg = ctx->r->objects->commit_graph;\n+\t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n \n \t\twhile (g) {\n \t\t\tctx->num_commit_graphs_before++;\n@@ -2361,9 +2358,6 @@ int write_commit_graph(struct object_directory *odb,\n \n \tctx->approx_nr_objects = approximate_object_count();\n \n-\tif (ctx->append)\n-\t\tprepare_commit_graph_one(ctx->r, ctx->odb);\n-\n \tif (ctx->append && ctx->r->objects->commit_graph) {\n \t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n \t\tfor (i = 0; i < g->num_commits; i++) {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 2cf29f425a0..5b15f1aa0f6 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -446,7 +446,7 @@ test_expect_success 'warn on improper hash version' '\n \t)\n '\n \n-test_expect_failure 'lower layers have overflow chunk' '\n+test_expect_success 'lower layers have overflow chunk' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tUNIX_EPOCH_ZERO=\"@0 +0000\" &&\n \tFUTURE_DATE=\"@2147483646 +0000\" &&\n-- \ngitgitgadget\n"},{"id":"415773","messageId":"82afa811dffa3f7b3c371de5c220b0529d96ed87.1612199707.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.git.1612199707.gitgitgadget@gmail.com","subject":"[PATCH 2/5] commit-graph: always parse before commit_graph_data_at()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-01T17:15:04Z","receivedAt":"2021-02-01T17:16:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThere is a subtle failure happening when computing corrected commit\ndates with --split enabled. It requires a base layer needing the\ngeneration_data_overflow chunk. Then, the next layer on top\nerroneously thinks it needs an overflow chunk due to a bug leading\nto recalculating all reachable generation numbers. The output of\nthe failure is\n\n BUG: commit-graph.c:1912: expected to write 8 bytes to\n chunk 47444f56, but wrote 0 instead\n\nThese \"expected\" 8 bytes are due to re-computing the corrected\ncommit date for the lower layer but the new layer does not need\nany overflow.\n\nAdd a test to t5318-commit-graph.sh that demonstrates this bug. However,\nit does not trigger consistently with the existing code.\n\nThe generation number data is stored in a slab and accessed by\ncommit_graph_data_at(). This data is initialized when parsing a commit,\nbut is otherwise used assuming it has been populated. The loop in\ncompute_generation_numbers() did not enforce that all reachable\ncommits were parsed and had correct values. This could lead to some\nproblems when writing a commit-graph with corrected commit dates based\non a commit-graph without them.\n\nIt has been difficult to identify the issue here becaus it was so hard\nto reproduce. It relies on this uninitialized data having a non-zero\nvalue, but also on specifically in a way that overwrites the existing\ndata.\n\nThis patch adds the extra parse to ensure the data is filled before we\ncompute the generation number of a commit. This triggers the new test\nto fail because the generation number overflow count does not match\nbetween this computation and the write for that chunk.\n\nThe actual fix will follow as the next few changes.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 16 ++++++++++++----\n t/t5318-commit-graph.sh | 21 +++++++++++++++++++++\n 2 files changed, 33 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 03e5a987968..edbb3a0f2cc 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1193,7 +1193,9 @@ static int write_graph_chunk_generation_data(struct hashfile *f,\n \n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.list[i];\n-\t\ttimestamp_t offset = commit_graph_data_at(c)->generation - c->date;\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@@ -1444,15 +1446,20 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n \t\t\t\t\tctx->commits.nr);\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n-\t\tuint32_t level = *topo_level_slab_at(ctx->topo_levels, ctx->commits.list[i]);\n-\t\ttimestamp_t corrected_commit_date = commit_graph_data_at(ctx->commits.list[i])->generation;\n+\t\tstruct commit *c = ctx->commits.list[i];\n+\t\tuint32_t level;\n+\t\ttimestamp_t corrected_commit_date;\n+\n+\t\trepo_parse_commit(ctx->r, c);\n+\t\tlevel = *topo_level_slab_at(ctx->topo_levels, c);\n+\t\tcorrected_commit_date = commit_graph_data_at(c)->generation;\n \n \t\tdisplay_progress(ctx->progress, i + 1);\n \t\tif (level != GENERATION_NUMBER_ZERO &&\n \t\t corrected_commit_date != GENERATION_NUMBER_ZERO)\n \t\t\tcontinue;\n \n-\t\tcommit_list_insert(ctx->commits.list[i], &list);\n+\t\tcommit_list_insert(c, &list);\n \t\twhile (list) {\n \t\t\tstruct commit *current = list->item;\n \t\t\tstruct commit_list *parent;\n@@ -1461,6 +1468,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\t\ttimestamp_t max_corrected_commit_date = 0;\n \n \t\t\tfor (parent = current->parents; parent; parent = parent->next) {\n+\t\t\t\trepo_parse_commit(ctx->r, parent->item);\n \t\t\t\tlevel = *topo_level_slab_at(ctx->topo_levels, parent->item);\n \t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex fa27df579a5..2cf29f425a0 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -446,6 +446,27 @@ test_expect_success 'warn on improper hash version' '\n \t)\n '\n \n+test_expect_failure 'lower layers have overflow chunk' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tUNIX_EPOCH_ZERO=\"@0 +0000\" &&\n+\tFUTURE_DATE=\"@2147483646 +0000\" &&\n+\trm -f .git/objects/info/commit-graph &&\n+\ttest_commit --date \"$FUTURE_DATE\" future-1 &&\n+\ttest_commit --date \"$UNIX_EPOCH_ZERO\" old-1 &&\n+\tgit commit-graph write --reachable &&\n+\ttest_commit --date \"$FUTURE_DATE\" future-2 &&\n+\ttest_commit --date \"$UNIX_EPOCH_ZERO\" old-2 &&\n+\tgit commit-graph write --reachable --split=no-merge &&\n+\ttest_commit extra &&\n+\tgit commit-graph write --reachable --split=no-merge &&\n+\tgit commit-graph write --reachable &&\n+\tgraph_read_expect 16 \"generation_data generation_data_overflow extra_edges\" &&\n+\tmv .git/objects/info/commit-graph commit-graph-upgraded &&\n+\tgit commit-graph write --reachable &&\n+\tgraph_read_expect 16 \"generation_data generation_data_overflow extra_edges\" &&\n+\ttest_cmp .git/objects/info/commit-graph commit-graph-upgraded\n+'\n+\n # the verify tests below expect the commit-graph to contain\n # exactly the commits reachable from the commits/8 branch.\n # If the file changes the set of commits in the list, then the\n-- \ngitgitgadget\n\n"},{"id":"415774","messageId":"YBg7RAhkvoe/iV1q@nand.local","threadId":"55086","inReplyTo":"9c605c99f66cae3c66bcdd4cbeefbfa1ec2be192.1612199707.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] commit-graph: use repo_parse_commit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-01T17:32:52Z","receivedAt":"2021-02-01T17:34:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 01, 2021 at 05:15:03PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The write_commit_graph_context has a repository pointer, so use it.\n>\n> Signed-off-by: Derrick Stolee \n\nEverything here looks good to me, but...\n\n> ---\n> commit-graph.c | 10 +++++-----\n> commit.h | 5 +++--\n> 2 files changed, 8 insertions(+), 7 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f3bde2ad95a..03e5a987968 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1098,7 +1098,7 @@ static int write_graph_chunk_data(struct hashfile *f,\n> \t\tuint32_t packedDate[2];\n> \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n>\n> -\t\tif (parse_commit_no_graph(*list))\n> +\t\tif (repo_parse_commit_no_graph(ctx->r, *list))\n> \t\t\tdie(_(\"unable to parse commit %s\"),\n> \t\t\t\toid_to_hex(&(*list)->object.oid));\n> \t\ttree = get_commit_tree_oid(*list);\n> @@ -1411,11 +1411,11 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n> \t\tif (!commit)\n> \t\t\tcontinue;\n> \t\tif (ctx->split) {\n> -\t\t\tif ((!parse_commit(commit) &&\n> +\t\t\tif ((!repo_parse_commit(ctx->r, commit) &&\n\nI know that this has nothing to do with your patch, but it really would\nbe nice to unify all of these parse_commit() vs parse_commit_no_graph()\ncalls.\n\nAs I recall this dates back to 43d3561805 (commit-graph write: don't die\nif the existing graph is corrupt, 2019-03-25), and there was some\ndiscussion on the list at the time about revisiting this if we ever were\nable to write incremental commit graphs:\n\n https://lore.kernel.org/git/20190221223753.20070-8-avarab@gmail.com/\n\nNow may be a good time to revisit that, although I don't want to detract\nfrom another fix (that we really do need to land before v2.31.0).\n\nThanks,\nTaylor\n"},{"id":"415775","messageId":"YBg858G1eFy1if6o@nand.local","threadId":"55086","inReplyTo":"d554fa306601c9e5e0e804d10b7a73b6eece6b04.1612199707.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] commit-graph: validate layers for generation data","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-01T17:39:51Z","receivedAt":"2021-02-01T17:41:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 01, 2021 at 05:15:05PM +0000, Derrick Stolee via GitGitGadget wrote:\n> diff --git a/commit-graph.c b/commit-graph.c\n> index edbb3a0f2cc..13992137dd0 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -614,19 +614,26 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n> \treturn graph_chain;\n> }\n>\n> -static void validate_mixed_generation_chain(struct commit_graph *g)\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\nThanks for the comment. It does make me wonder if the function name\nneeds updating, though. I'm having some trouble coming up with a better\nalternative, though, so maybe it's fine...\n\n> {\n> -\tint read_generation_data;\n> +\tint read_generation_data = 1;\n\nOK, we might not ever enter the while loops below, so this needs\ninitializing.\n\n> +\tstruct commit_graph *p = g;\n>\n> -\tif (!g)\n> -\t\treturn;\n> -\n> -\tread_generation_data = !!g->chunk_generation_data;\n> +\twhile (read_generation_data && p) {\n> +\t\tread_generation_data = p->read_generation_data;\n> +\t\tp = p->base_graph;\n> +\t}\n\nThis could probably be guarded with an 'if !read_generation_data', since\nif the previous while loop always read '1' from\n'p->read_generation_data', then nothing needs updating, no?\n\n(If you do make this change, please don't add '!read_generation_data' to\nthe while expression, since it isn't a property of the loop.)\n\n> \twhile (g) {\n> \t\tg->read_generation_data = read_generation_data;\n> \t\tg = g->base_graph;\n> \t}\n> +\n> +\treturn read_generation_data;\n> }\n>\n> struct commit_graph *read_commit_graph_one(struct repository *r,\n> --\n> gitgitgadget\n>\n\nThanks,\nTaylor\n"},{"id":"415779","messageId":"YBhChR3ReDhAde87@nand.local","threadId":"55086","inReplyTo":"b267a9653a7560d1e59708f20106ef054d140a9f.1612199707.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] commit-graph: be extra careful about mixed generations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-01T18:04:14Z","receivedAt":"2021-02-01T18:05:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 01, 2021 at 05:15:06PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> When upgrading to a commit-graph with corrected commit dates from\n> one without, there are a few things that need to be considered.\n>\n> When computing generation numbers for the new commit-graph file that\n> expects to add the generation_data chunk with corrected commit\n> dates, we need to ensure that the 'generation' member of the\n> commit_graph_data struct is set to zero for these commits.\n>\n> Unfortunately, the fallback to use topological level for generation\n> number when corrected commit dates are not available are causing us\n> harm here: parsing commits notices that read_generation_data is\n> false and populates 'generation' with the topological level.\n>\n> The solution is to iterate through the commits, parse the commits\n> to populate initial values, then reset the generation values to\n> zero to trigger recalculation. This loop only occurs when the\n> existing commit-graph data has no corrected commit dates.\n>\n> While this improves our situation somewhat, we have not completely\n> solved the issue for correctly computing generation numbers for mixes\n> layers. That follows in the next change.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> commit-graph.c | 32 +++++++++++++++++++++++---------\n> 1 file changed, 23 insertions(+), 9 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 13992137dd0..08148dd17f1 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1033,7 +1033,8 @@ struct write_commit_graph_context {\n> \t\t split:1,\n> \t\t changed_paths:1,\n> \t\t order_by_pack:1,\n> -\t\t write_generation_data:1;\n> +\t\t write_generation_data:1,\n> +\t\t trust_generation_numbers:1;\n>\n> \tstruct topo_level_slab *topo_levels;\n> \tconst struct commit_graph_opts *opts;\n> @@ -1452,6 +1453,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n> \t\tctx->progress = start_delayed_progress(\n> \t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n> \t\t\t\t\tctx->commits.nr);\n> +\n> +\tif (ctx->write_generation_data && !ctx->trust_generation_numbers) {\n> +\t\tfor (i = 0; i < ctx->commits.nr; i++) {\n> +\t\t\tstruct commit *c = ctx->commits.list[i];\n> +\t\t\trepo_parse_commit(ctx->r, c);\n> +\t\t\tcommit_graph_data_at(c)->generation = GENERATION_NUMBER_ZERO;\n> +\t\t}\n> +\t}\n> +\n\nThis took me a while to figure out since I spent quite a lot of time\nthinking that you were setting the topological level to zero, _not_ the\ncorrected committer date.\n\nNow that I understand which is which, I agree that this is the right way\nto go forward.\n\nThat said, I do find it unnecessarily complex that we compute both the\ngeneration number and the topological level in the same loops in\ncompute_generation_numbers()...\n\n> \tfor (i = 0; i < ctx->commits.nr; i++) {\n> \t\tstruct commit *c = ctx->commits.list[i];\n> \t\tuint32_t level;\n> @@ -1480,7 +1490,8 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n> \t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n>\n> \t\t\t\tif (level == GENERATION_NUMBER_ZERO ||\n> -\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO) {\n> +\t\t\t\t (ctx->write_generation_data &&\n> +\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO)) {\n\n...for exactly reasons like this. It does make sense that they could be\ncomputed together since their computation is indeed quite similar. But\nin practice I think you end up spending a lot of time reasoning around\ncomplex conditionals like these.\n\nSo, I feel a little bit like we should spend some effort to split these\nup. I'm OK with a little bit of code duplication (though if we can\nfactor out some common routine, that would also be nice). But I think\nthere's a tradeoff between DRY-ness and understandability, and that we\nmight be on the wrong side of it here.\n\n> \t\t\t\t\tall_parents_computed = 0;\n> \t\t\t\t\tcommit_list_insert(parent->item, &list);\n> \t\t\t\t\tbreak;\n> @@ -1500,12 +1511,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n> \t\t\t\t\tmax_level = GENERATION_NUMBER_V1_MAX - 1;\n> \t\t\t\t*topo_level_slab_at(ctx->topo_levels, current) = max_level + 1;\n>\n> -\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n> -\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n> -\t\t\t\tcommit_graph_data_at(current)->generation = max_corrected_commit_date + 1;\n> -\n> -\t\t\t\tif (commit_graph_data_at(current)->generation - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n> -\t\t\t\t\tctx->num_generation_data_overflows++;\n> +\t\t\t\tif (ctx->write_generation_data) {\n> +\t\t\t\t\ttimestamp_t cur_g;\n> +\t\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n> +\t\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n> +\t\t\t\t\tcur_g = commit_graph_data_at(current)->generation\n> +\t\t\t\t\t = max_corrected_commit_date + 1;\n> +\t\t\t\t\tif (cur_g - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n> +\t\t\t\t\t\tctx->num_generation_data_overflows++;\n> +\t\t\t\t}\n\nLooks like two things happened here:\n\n - A new local variable was introduced to store the value of\n 'commit_graph_data_at(current)->generation' (now called 'cur_g'),\n and\n\n - All of this was guarded by a conditional on\n 'ctx->write_generation_data'.\n\nThe first one is a readability improvement, and the second is the\nsubstantive one, no?\n\n> \t\t\t}\n> \t\t}\n> \t}\n> @@ -2396,7 +2410,7 @@ int write_commit_graph(struct object_directory *odb,\n> \t} else\n> \t\tctx->num_commit_graphs_after = 1;\n>\n> -\tvalidate_mixed_generation_chain(ctx->r->objects->commit_graph);\n> +\tctx->trust_generation_numbers = validate_mixed_generation_chain(ctx->r->objects->commit_graph);\n>\n> \tcompute_generation_numbers(ctx);\n\nMakes sense.\n\nThanks,\nTaylor\n"},{"id":"415781","messageId":"a040a680-42ac-66cc-831b-c23a23dd9837@gmail.com","threadId":"55086","inReplyTo":"YBg858G1eFy1if6o@nand.local","subject":"Re: [PATCH 3/5] commit-graph: validate layers for generation data","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-01T18:10:30Z","receivedAt":"2021-02-01T18:11:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/1/2021 12:39 PM, Taylor Blau wrote:\n> On Mon, Feb 01, 2021 at 05:15:05PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> +\tstruct commit_graph *p = g;\n>>\n>> -\tif (!g)\n>> -\t\treturn;\n>> -\n>> -\tread_generation_data = !!g->chunk_generation_data;\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> This could probably be guarded with an 'if !read_generation_data', since\n> if the previous while loop always read '1' from\n> 'p->read_generation_data', then nothing needs updating, no?\n> \n> (If you do make this change, please don't add '!read_generation_data' to\n> the while expression, since it isn't a property of the loop.)\n\nTrue, we could do that. It's enough to add\n\n\tif (read_generation_data)\n\t\treturn 1;\n\n>> \twhile (g) {\n>> \t\tg->read_generation_data = read_generation_data;\n>> \t\tg = g->base_graph;\n>> \t}\n>> +\n>> +\treturn read_generation_data;\n\nthen this becomes\n\n\treturn 0;\n\nThanks,\n-Stolee\n"},{"id":"415782","messageId":"defb76f2-aa85-66be-7b3d-e6b741774f22@gmail.com","threadId":"55086","inReplyTo":"YBhChR3ReDhAde87@nand.local","subject":"Re: [PATCH 4/5] commit-graph: be extra careful about mixed generations","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-01T18:13:06Z","receivedAt":"2021-02-01T18:14:03Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/1/2021 1:04 PM, Taylor Blau wrote:\n> On Mon, Feb 01, 2021 at 05:15:06PM +0000, Derrick Stolee via GitGitGadget wrote:\n...\n>> \tstruct topo_level_slab *topo_levels;\n>> \tconst struct commit_graph_opts *opts;\n>> @@ -1452,6 +1453,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n>> \t\tctx->progress = start_delayed_progress(\n>> \t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n>> \t\t\t\t\tctx->commits.nr);\n>> +\n>> +\tif (ctx->write_generation_data && !ctx->trust_generation_numbers) {\n>> +\t\tfor (i = 0; i < ctx->commits.nr; i++) {\n>> +\t\t\tstruct commit *c = ctx->commits.list[i];\n>> +\t\t\trepo_parse_commit(ctx->r, c);\n>> +\t\t\tcommit_graph_data_at(c)->generation = GENERATION_NUMBER_ZERO;\n>> +\t\t}\n>> +\t}\n>> +\n> \n> This took me a while to figure out since I spent quite a lot of time\n> thinking that you were setting the topological level to zero, _not_ the\n> corrected committer date.\n> \n> Now that I understand which is which, I agree that this is the right way\n> to go forward.\n> \n> That said, I do find it unnecessarily complex that we compute both the\n> generation number and the topological level in the same loops in\n> compute_generation_numbers()...\n> \n>> \tfor (i = 0; i < ctx->commits.nr; i++) {\n>> \t\tstruct commit *c = ctx->commits.list[i];\n>> \t\tuint32_t level;\n>> @@ -1480,7 +1490,8 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n>> \t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n>>\n>> \t\t\t\tif (level == GENERATION_NUMBER_ZERO ||\n>> -\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO) {\n>> +\t\t\t\t (ctx->write_generation_data &&\n>> +\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO)) {\n> \n> ...for exactly reasons like this. It does make sense that they could be\n> computed together since their computation is indeed quite similar. But\n> in practice I think you end up spending a lot of time reasoning around\n> complex conditionals like these.\n> \n> So, I feel a little bit like we should spend some effort to split these\n> up. I'm OK with a little bit of code duplication (though if we can\n> factor out some common routine, that would also be nice). But I think\n> there's a tradeoff between DRY-ness and understandability, and that we\n> might be on the wrong side of it here.\n\nYou're probably right that it is valuable to split the computations.\nIt would allow us to skip all of the \"if (ctx->write_generation_data)\"\nchecks in this implementation and rely on the callers to make that\nchoice.\n\n>> \t\t\t\t\tall_parents_computed = 0;\n>> \t\t\t\t\tcommit_list_insert(parent->item, &list);\n>> \t\t\t\t\tbreak;\n>> @@ -1500,12 +1511,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n>> \t\t\t\t\tmax_level = GENERATION_NUMBER_V1_MAX - 1;\n>> \t\t\t\t*topo_level_slab_at(ctx->topo_levels, current) = max_level + 1;\n>>\n>> -\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n>> -\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n>> -\t\t\t\tcommit_graph_data_at(current)->generation = max_corrected_commit_date + 1;\n>> -\n>> -\t\t\t\tif (commit_graph_data_at(current)->generation - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n>> -\t\t\t\t\tctx->num_generation_data_overflows++;\n>> +\t\t\t\tif (ctx->write_generation_data) {\n>> +\t\t\t\t\ttimestamp_t cur_g;\n>> +\t\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n>> +\t\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n>> +\t\t\t\t\tcur_g = commit_graph_data_at(current)->generation\n>> +\t\t\t\t\t = max_corrected_commit_date + 1;\n>> +\t\t\t\t\tif (cur_g - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n>> +\t\t\t\t\t\tctx->num_generation_data_overflows++;\n>> +\t\t\t\t}\n> \n> Looks like two things happened here:\n> \n> - A new local variable was introduced to store the value of\n> 'commit_graph_data_at(current)->generation' (now called 'cur_g'),\n> and\n> \n> - All of this was guarded by a conditional on\n> 'ctx->write_generation_data'.\n> \n> The first one is a readability improvement, and the second is the\n> substantive one, no?\n\nYes. Adding these checks and tabs made things super-wide, so cur_g\nexists only for readability. If we split the computation, then this\nis no longer required.\n\nThanks,\n-Stolee\n"},{"id":"415783","messageId":"YBhHaW7JpiYeNQIp@nand.local","threadId":"55086","inReplyTo":"dddeec30ebfc6d8403cc10395a9d0c331d1f7ad3.1612199707.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] commit-graph: prepare commit graph","fromName":"Taylor Blau","fromEmail":"ttaylorr@github.com","sentAt":"2021-02-01T18:25:06Z","receivedAt":"2021-02-01T18:26:31Z","isPatch":true,"sender":{"key":"ttaylorr@github.com","avatar":"https://gravatar.com/avatar/d5f3476f26b6f99cbb6b467e7ed7482f5762c8157bc73f569196e428bdcbea25?d=mp&s=160"},"body":"On Mon, Feb 01, 2021 at 05:15:07PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> Before checking if the repository has a commit-graph loaded, be sure\n> to run prepare_commit_graph(). This is necessary because without this\n> instance we would not initialize the topo_levels slab for each of the\n> struct commit_graphs in the chain before we start to parse the\n> commits. This leads to possibly recomputing the topological levels for\n> commits in lower layers even when we are adding a small number of\n> commits on top.\n\nI think that the situation arises as follows:\n\n - Prior to this patch, we didn't always prepare the commit-graph, so\n the topo_levels slab wasn't initialized either.\n\n - Then we try and compute the topo levels for new commits (which are\n likely to be decendants of older ones that are in commit-graphs and\n have their topo-levels already computed).\n\n - But in the course of computing topo-levels for the new commits, we\n have to recur on their ancestors, which *look* like they have\n uncomputed topo levels.\n\nThat all makes sense, but it may be worth cutting and pasting what I\nwrote into your patch to make it clearer.\n\n> By properly initializing the topo_slab, we fix the previously broken\n> case of a split commit graph where a base layer has the\n> generation_data_overflow chunk.\n\nMakes sense.\n\n> -test_expect_failure 'lower layers have overflow chunk' '\n> +test_expect_success 'lower layers have overflow chunk' '\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> \tUNIX_EPOCH_ZERO=\"@0 +0000\" &&\n> \tFUTURE_DATE=\"@2147483646 +0000\" &&\n\nTerrific :-).\n\nThanks,\nTaylor\n"},{"id":"415786","messageId":"xmqqzh0nljbb.fsf@gitster.c.googlers.com","threadId":"55086","inReplyTo":"82afa811dffa3f7b3c371de5c220b0529d96ed87.1612199707.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] commit-graph: always parse before commit_graph_data_at()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-01T18:44:08Z","receivedAt":"2021-02-01T18:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> It has been difficult to identify the issue here becaus it was so hard\n\n\"because\" (will fix-up locally).\n\n> to reproduce. It relies on this uninitialized data having a non-zero\n> value, but also on specifically in a way that overwrites the existing\n> data.\n"},{"id":"415787","messageId":"xmqqv9bbliru.fsf@gitster.c.googlers.com","threadId":"55086","inReplyTo":"b267a9653a7560d1e59708f20106ef054d140a9f.1612199707.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] commit-graph: be extra careful about mixed generations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-01T18:55:49Z","receivedAt":"2021-02-01T18:56:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> When upgrading to a commit-graph with corrected commit dates from\n> one without, there are a few things that need to be considered.\n>\n> When computing generation numbers for the new commit-graph file that\n> expects to add the generation_data chunk with corrected commit\n> dates, we need to ensure that the 'generation' member of the\n> commit_graph_data struct is set to zero for these commits.\n>\n> Unfortunately, the fallback to use topological level for generation\n> number when corrected commit dates are not available are causing us\n> harm here: parsing commits notices that read_generation_data is\n> false and populates 'generation' with the topological level.\n>\n> The solution is to iterate through the commits, parse the commits\n> to populate initial values, then reset the generation values to\n> zero to trigger recalculation. This loop only occurs when the\n> existing commit-graph data has no corrected commit dates.\n>\n> While this improves our situation somewhat, we have not completely\n> solved the issue for correctly computing generation numbers for mixes\n\n\"mixed\"? (will not touch locally).\n\n> layers. That follows in the next change.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> commit-graph.c | 32 +++++++++++++++++++++++---------\n> 1 file changed, 23 insertions(+), 9 deletions(-)\n\nThe hunks that mention trust_generation_numbers match what the\ncommit log message says what the patch does, but other two hunks\nthat make sure that some code that used to run regardless of the\nvalue of \"ctx->write_generation_data\" are now run only when the\nmember is nonzero smells like an unrelated change.\n\nThanks.\n\n"},{"id":"415865","messageId":"9c605c99f66cae3c66bcdd4cbeefbfa1ec2be192.1612234883.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","subject":"[PATCH v2 1/6] commit-graph: use repo_parse_commit","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-02T03:01:18Z","receivedAt":"2021-02-02T03:02:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe write_commit_graph_context has a repository pointer, so use it.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 10 +++++-----\n commit.h | 5 +++--\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f3bde2ad95a..03e5a987968 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1098,7 +1098,7 @@ static int write_graph_chunk_data(struct hashfile *f,\n \t\tuint32_t packedDate[2];\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n \n-\t\tif (parse_commit_no_graph(*list))\n+\t\tif (repo_parse_commit_no_graph(ctx->r, *list))\n \t\t\tdie(_(\"unable to parse commit %s\"),\n \t\t\t\toid_to_hex(&(*list)->object.oid));\n \t\ttree = get_commit_tree_oid(*list);\n@@ -1411,11 +1411,11 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n \t\tif (!commit)\n \t\t\tcontinue;\n \t\tif (ctx->split) {\n-\t\t\tif ((!parse_commit(commit) &&\n+\t\t\tif ((!repo_parse_commit(ctx->r, commit) &&\n \t\t\t commit_graph_position(commit) == COMMIT_NOT_FROM_GRAPH) ||\n \t\t\t flags == COMMIT_GRAPH_SPLIT_REPLACE)\n \t\t\t\tadd_missing_parents(ctx, commit);\n-\t\t} else if (!parse_commit_no_graph(commit))\n+\t\t} else if (!repo_parse_commit_no_graph(ctx->r, commit))\n \t\t\tadd_missing_parents(ctx, commit);\n \t}\n \tstop_progress(&ctx->progress);\n@@ -1710,9 +1710,9 @@ static void copy_oids_to_commits(struct write_commit_graph_context *ctx)\n \t\t\tcontinue;\n \n \t\tif (ctx->split && flags == COMMIT_GRAPH_SPLIT_REPLACE)\n-\t\t\tparse_commit(ctx->commits.list[ctx->commits.nr]);\n+\t\t\trepo_parse_commit(ctx->r, ctx->commits.list[ctx->commits.nr]);\n \t\telse\n-\t\t\tparse_commit_no_graph(ctx->commits.list[ctx->commits.nr]);\n+\t\t\trepo_parse_commit_no_graph(ctx->r, ctx->commits.list[ctx->commits.nr]);\n \n \t\tnum_parents = commit_list_count(ctx->commits.list[ctx->commits.nr]->parents);\n \t\tif (num_parents > 2)\ndiff --git a/commit.h b/commit.h\nindex 251d877fcf6..b05ab558ce2 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -89,9 +89,10 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)\n \treturn repo_parse_commit_gently(r, item, 0);\n }\n \n-static inline int parse_commit_no_graph(struct commit *commit)\n+static inline int repo_parse_commit_no_graph(struct repository *r,\n+\t\t\t\t\t struct commit *commit)\n {\n-\treturn repo_parse_commit_internal(the_repository, commit, 0, 0);\n+\treturn repo_parse_commit_internal(r, commit, 0, 0);\n }\n \n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n-- \ngitgitgadget\n\n"},{"id":"415866","messageId":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.git.1612199707.gitgitgadget@gmail.com","subject":"[PATCH v2 0/6] Generation Number v2: Fix a tricky split graph bug","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-02T03:01:17Z","receivedAt":"2021-02-02T03:02:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Here is a bugfix for the recently-landed-in-next generation v2 topic\n(ak/corrected-commit-date).\n\nThis was occasionally hitting us when computing new commit-graphs on\nexisting repositories with the new bits. It was very hard to reproduce, and\nit turns out to be due to not parsing commits before accessing generation\nnumber data. Doing so in the right place demonstrates the bug of recomputing\nthe corrected commit date even for commits in lower layers with computed\nvalues.\n\nThe fix is split into these steps:\n\n 1. Parse commits more often before accessing their data. (This allows the\n bug to be demonstrated in the test suite.)\n 2. Check the full commit-graph chain for generation data chunks.\n 3. Don't compute corrected commit dates if the lower layers do not support\n them.\n 4. Parse the commit-graph file more often.\n\nThanks, -Stolee\n\n\nUpdates in v2\n=============\n\n * Fixed some typos or other clarifications in commit messages.\n\n * The loop assigning read_generation_data is skipped if they already all\n agree with value 1.\n\n * I split compute_generation_numbers into two methods. This essentially\n splits the previous patch 4 into patches 4 & 5 here. The new patch 4 just\n splits the logic as-is, then the new patch 5 does the re-initialization\n of generation values when in the upgrade scenario.\n\nDerrick Stolee (6):\n commit-graph: use repo_parse_commit\n commit-graph: always parse before commit_graph_data_at()\n commit-graph: validate layers for generation data\n commit-graph: compute generations separately\n commit-graph: be extra careful about mixed generations\n commit-graph: prepare commit graph\n\n commit-graph.c | 138 +++++++++++++++++++++++++++++-----------\n commit.h | 5 +-\n t/t5318-commit-graph.sh | 21 ++++++\n 3 files changed, 125 insertions(+), 39 deletions(-)\n\n\nbase-commit: 5a3b130cad0d5c770f766e3af6d32b41766374c0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-850%2Fderrickstolee%2Fgen-v2-upgrade-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-850/derrickstolee/gen-v2-upgrade-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/850\n\nRange-diff vs v1:\n\n 1: 9c605c99f66 = 1: 9c605c99f66 commit-graph: use repo_parse_commit\n 2: 82afa811dff ! 2: 454b183b9ba commit-graph: always parse before commit_graph_data_at()\n @@ Commit message\n problems when writing a commit-graph with corrected commit dates based\n on a commit-graph without them.\n \n - It has been difficult to identify the issue here becaus it was so hard\n + It has been difficult to identify the issue here because it was so hard\n to reproduce. It relies on this uninitialized data having a non-zero\n value, but also on specifically in a way that overwrites the existing\n data.\n 3: d554fa30660 ! 3: 3d223fa2156 commit-graph: validate layers for generation data\n @@ commit-graph.c: static struct commit_graph *load_commit_graph_chain(struct repos\n \n -\tif (!g)\n -\t\treturn;\n --\n --\tread_generation_data = !!g->chunk_generation_data;\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 +-\tread_generation_data = !!g->chunk_generation_data;\n ++\tif (read_generation_data)\n ++\t\treturn 1;\n + \n \twhile (g) {\n - \t\tg->read_generation_data = read_generation_data;\n +-\t\tg->read_generation_data = read_generation_data;\n ++\t\tg->read_generation_data = 0;\n \t\tg = g->base_graph;\n \t}\n +\n -+\treturn read_generation_data;\n ++\treturn 0;\n }\n \n struct commit_graph *read_commit_graph_one(struct repository *r,\n -: ----------- > 4: 05248ff222f commit-graph: compute generations separately\n 4: b267a9653a7 ! 5: 9bccee8fb63 commit-graph: be extra careful about mixed generations\n @@ Commit message\n existing commit-graph data has no corrected commit dates.\n \n While this improves our situation somewhat, we have not completely\n - solved the issue for correctly computing generation numbers for mixes\n + solved the issue for correctly computing generation numbers for mixed\n layers. That follows in the next change.\n \n Signed-off-by: Derrick Stolee \n @@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph\n \t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n \t\t\t\t\tctx->commits.nr);\n +\n -+\tif (ctx->write_generation_data && !ctx->trust_generation_numbers) {\n ++\tif (!ctx->trust_generation_numbers) {\n +\t\tfor (i = 0; i < ctx->commits.nr; i++) {\n +\t\t\tstruct commit *c = ctx->commits.list[i];\n +\t\t\trepo_parse_commit(ctx->r, c);\n @@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph\n +\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.list[i];\n - \t\tuint32_t level;\n -@@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n - \t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n - \n - \t\t\t\tif (level == GENERATION_NUMBER_ZERO ||\n --\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO) {\n -+\t\t\t\t (ctx->write_generation_data &&\n -+\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO)) {\n - \t\t\t\t\tall_parents_computed = 0;\n - \t\t\t\t\tcommit_list_insert(parent->item, &list);\n - \t\t\t\t\tbreak;\n -@@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n - \t\t\t\t\tmax_level = GENERATION_NUMBER_V1_MAX - 1;\n - \t\t\t\t*topo_level_slab_at(ctx->topo_levels, current) = max_level + 1;\n - \n --\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n --\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n --\t\t\t\tcommit_graph_data_at(current)->generation = max_corrected_commit_date + 1;\n --\n --\t\t\t\tif (commit_graph_data_at(current)->generation - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n --\t\t\t\t\tctx->num_generation_data_overflows++;\n -+\t\t\t\tif (ctx->write_generation_data) {\n -+\t\t\t\t\ttimestamp_t cur_g;\n -+\t\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n -+\t\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n -+\t\t\t\t\tcur_g = commit_graph_data_at(current)->generation\n -+\t\t\t\t\t = max_corrected_commit_date + 1;\n -+\t\t\t\t\tif (cur_g - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n -+\t\t\t\t\t\tctx->num_generation_data_overflows++;\n -+\t\t\t\t}\n - \t\t\t}\n - \t\t}\n - \t}\n + \t\ttimestamp_t corrected_commit_date;\n @@ commit-graph.c: int write_commit_graph(struct object_directory *odb,\n \t} else\n \t\tctx->num_commit_graphs_after = 1;\n @@ commit-graph.c: int write_commit_graph(struct object_directory *odb,\n -\tvalidate_mixed_generation_chain(ctx->r->objects->commit_graph);\n +\tctx->trust_generation_numbers = validate_mixed_generation_chain(ctx->r->objects->commit_graph);\n \n - \tcompute_generation_numbers(ctx);\n - \n + \tcompute_topological_levels(ctx);\n + \tif (ctx->write_generation_data)\n 5: dddeec30ebf ! 6: 38086c85b52 commit-graph: prepare commit graph\n @@ Commit message\n commit-graph: prepare commit graph\n \n Before checking if the repository has a commit-graph loaded, be sure\n - to run prepare_commit_graph(). This is necessary because without this\n - instance we would not initialize the topo_levels slab for each of the\n - struct commit_graphs in the chain before we start to parse the\n - commits. This leads to possibly recomputing the topological levels for\n - commits in lower layers even when we are adding a small number of\n - commits on top.\n + to run prepare_commit_graph(). This is necessary because otherwise\n + the topo_levels slab is not initialized. As we compute topo_levels for\n + the new commits, we iterate further into the lower layers since the\n + first visit to each commit looks as though the topo_level is not\n + populated.\n \n By properly initializing the topo_slab, we fix the previously broken\n case of a split commit graph where a base layer has the\n\n-- \ngitgitgadget\n"},{"id":"415867","messageId":"3d223fa2156530f67997be8fa7e254e599f3b13a.1612234883.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","subject":"[PATCH v2 3/6] commit-graph: validate layers for generation data","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-02T03:01:20Z","receivedAt":"2021-02-02T03:02:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWe need to be extra careful that we don't use corrected\ncommit dates from any layer of a commit-graph chain if there is a\nsingle commit-graph file that is missing the generation_data chunk.\nUpdate validate_mixed_generation_chain() to correctly update each\nlayer to ignore the generation_data chunk in this case. It now also\nreturns 1 if all layers have a generation_data chunk. This return\nvalue will be used in the next change.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 22 ++++++++++++++++------\n 1 file changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex edbb3a0f2cc..b3f7c3bbcb3 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -614,19 +614,29 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,\n \treturn graph_chain;\n }\n \n-static void validate_mixed_generation_chain(struct commit_graph *g)\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;\n+\tint read_generation_data = 1;\n+\tstruct commit_graph *p = g;\n \n-\tif (!g)\n-\t\treturn;\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-\tread_generation_data = !!g->chunk_generation_data;\n+\tif (read_generation_data)\n+\t\treturn 1;\n \n \twhile (g) {\n-\t\tg->read_generation_data = read_generation_data;\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-- \ngitgitgadget\n\n"},{"id":"415868","messageId":"454b183b9ba502da7f40dc36aaa95cc3d12b5c2f.1612234883.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","subject":"[PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-02T03:01:19Z","receivedAt":"2021-02-02T03:02:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThere is a subtle failure happening when computing corrected commit\ndates with --split enabled. It requires a base layer needing the\ngeneration_data_overflow chunk. Then, the next layer on top\nerroneously thinks it needs an overflow chunk due to a bug leading\nto recalculating all reachable generation numbers. The output of\nthe failure is\n\n BUG: commit-graph.c:1912: expected to write 8 bytes to\n chunk 47444f56, but wrote 0 instead\n\nThese \"expected\" 8 bytes are due to re-computing the corrected\ncommit date for the lower layer but the new layer does not need\nany overflow.\n\nAdd a test to t5318-commit-graph.sh that demonstrates this bug. However,\nit does not trigger consistently with the existing code.\n\nThe generation number data is stored in a slab and accessed by\ncommit_graph_data_at(). This data is initialized when parsing a commit,\nbut is otherwise used assuming it has been populated. The loop in\ncompute_generation_numbers() did not enforce that all reachable\ncommits were parsed and had correct values. This could lead to some\nproblems when writing a commit-graph with corrected commit dates based\non a commit-graph without them.\n\nIt has been difficult to identify the issue here because it was so hard\nto reproduce. It relies on this uninitialized data having a non-zero\nvalue, but also on specifically in a way that overwrites the existing\ndata.\n\nThis patch adds the extra parse to ensure the data is filled before we\ncompute the generation number of a commit. This triggers the new test\nto fail because the generation number overflow count does not match\nbetween this computation and the write for that chunk.\n\nThe actual fix will follow as the next few changes.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 16 ++++++++++++----\n t/t5318-commit-graph.sh | 21 +++++++++++++++++++++\n 2 files changed, 33 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 03e5a987968..edbb3a0f2cc 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1193,7 +1193,9 @@ static int write_graph_chunk_generation_data(struct hashfile *f,\n \n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.list[i];\n-\t\ttimestamp_t offset = commit_graph_data_at(c)->generation - c->date;\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@@ -1444,15 +1446,20 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n \t\t\t\t\tctx->commits.nr);\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n-\t\tuint32_t level = *topo_level_slab_at(ctx->topo_levels, ctx->commits.list[i]);\n-\t\ttimestamp_t corrected_commit_date = commit_graph_data_at(ctx->commits.list[i])->generation;\n+\t\tstruct commit *c = ctx->commits.list[i];\n+\t\tuint32_t level;\n+\t\ttimestamp_t corrected_commit_date;\n+\n+\t\trepo_parse_commit(ctx->r, c);\n+\t\tlevel = *topo_level_slab_at(ctx->topo_levels, c);\n+\t\tcorrected_commit_date = commit_graph_data_at(c)->generation;\n \n \t\tdisplay_progress(ctx->progress, i + 1);\n \t\tif (level != GENERATION_NUMBER_ZERO &&\n \t\t corrected_commit_date != GENERATION_NUMBER_ZERO)\n \t\t\tcontinue;\n \n-\t\tcommit_list_insert(ctx->commits.list[i], &list);\n+\t\tcommit_list_insert(c, &list);\n \t\twhile (list) {\n \t\t\tstruct commit *current = list->item;\n \t\t\tstruct commit_list *parent;\n@@ -1461,6 +1468,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\t\ttimestamp_t max_corrected_commit_date = 0;\n \n \t\t\tfor (parent = current->parents; parent; parent = parent->next) {\n+\t\t\t\trepo_parse_commit(ctx->r, parent->item);\n \t\t\t\tlevel = *topo_level_slab_at(ctx->topo_levels, parent->item);\n \t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex fa27df579a5..2cf29f425a0 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -446,6 +446,27 @@ test_expect_success 'warn on improper hash version' '\n \t)\n '\n \n+test_expect_failure 'lower layers have overflow chunk' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tUNIX_EPOCH_ZERO=\"@0 +0000\" &&\n+\tFUTURE_DATE=\"@2147483646 +0000\" &&\n+\trm -f .git/objects/info/commit-graph &&\n+\ttest_commit --date \"$FUTURE_DATE\" future-1 &&\n+\ttest_commit --date \"$UNIX_EPOCH_ZERO\" old-1 &&\n+\tgit commit-graph write --reachable &&\n+\ttest_commit --date \"$FUTURE_DATE\" future-2 &&\n+\ttest_commit --date \"$UNIX_EPOCH_ZERO\" old-2 &&\n+\tgit commit-graph write --reachable --split=no-merge &&\n+\ttest_commit extra &&\n+\tgit commit-graph write --reachable --split=no-merge &&\n+\tgit commit-graph write --reachable &&\n+\tgraph_read_expect 16 \"generation_data generation_data_overflow extra_edges\" &&\n+\tmv .git/objects/info/commit-graph commit-graph-upgraded &&\n+\tgit commit-graph write --reachable &&\n+\tgraph_read_expect 16 \"generation_data generation_data_overflow extra_edges\" &&\n+\ttest_cmp .git/objects/info/commit-graph commit-graph-upgraded\n+'\n+\n # the verify tests below expect the commit-graph to contain\n # exactly the commits reachable from the commits/8 branch.\n # If the file changes the set of commits in the list, then the\n-- \ngitgitgadget\n\n"},{"id":"415869","messageId":"05248ff222fe7f2a7976e7a7cf2ee272eec6edbb.1612234883.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","subject":"[PATCH v2 4/6] commit-graph: compute generations separately","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-02T03:01:21Z","receivedAt":"2021-02-02T03:02:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe compute_generation_numbers() method was introduced by 3258c663\n(commit-graph: compute generation numbers, 2018-05-01) to compute what\nis now known as \"topological levels\". These are still stored in the\ncommit-graph file for compatibility sake while c1a09119 (commit-graph:\nimplement corrected commit date, 2021-01-16) updated the method to also\ncompute the new version of generation numbers: corrected commit date.\n\nIt makes sense why these are grouped. They perform very similar walks of\nthe necessary commits and compute similar maximums over each parent.\nHowever, having these two together conflates them in subtle ways that is\nhard to separate.\n\nIn particular, the topo_level slab is used to store the topological\nlevels in all cases, but the commit_graph_data_at(c)->generation member\nstores different values depending on the state of the existing\ncommit-graph file.\n\n* If the existing commit-graph file has a \"GDAT\" chunk, then these\n values represent corrected commit dates.\n\n* If the existing commit-graph file doesn't have a \"GDAT\" chunk, then\n these values are actually the topological levels.\n\nThis issue only occurs only when upgrading an existing commit-graph file\ninto one that has the \"GDAT\" chunk. The current change does not resolve\nthis upgrade problem, but splitting the implementation into two pieces\nhere helps with that process, which will follow in the next change.\n\nThe important thing this helps with is the case where the\nnum_generation_data_overflows was being incremented incorrectly,\ntriggering a write of the overflow chunk.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 70 ++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 56 insertions(+), 14 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b3f7c3bbcb3..2790f70d113 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1446,27 +1446,24 @@ static void close_reachable(struct write_commit_graph_context *ctx)\n \tstop_progress(&ctx->progress);\n }\n \n-static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n+static void compute_topological_levels(struct write_commit_graph_context *ctx)\n {\n \tint i;\n \tstruct commit_list *list = NULL;\n \n \tif (ctx->report_progress)\n \t\tctx->progress = start_delayed_progress(\n-\t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n+\t\t\t\t\t_(\"Computing commit graph topological levels\"),\n \t\t\t\t\tctx->commits.nr);\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.list[i];\n \t\tuint32_t level;\n-\t\ttimestamp_t corrected_commit_date;\n \n \t\trepo_parse_commit(ctx->r, c);\n \t\tlevel = *topo_level_slab_at(ctx->topo_levels, c);\n-\t\tcorrected_commit_date = commit_graph_data_at(c)->generation;\n \n \t\tdisplay_progress(ctx->progress, i + 1);\n-\t\tif (level != GENERATION_NUMBER_ZERO &&\n-\t\t corrected_commit_date != GENERATION_NUMBER_ZERO)\n+\t\tif (level != GENERATION_NUMBER_ZERO)\n \t\t\tcontinue;\n \n \t\tcommit_list_insert(c, &list);\n@@ -1475,15 +1472,12 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\t\tstruct commit_list *parent;\n \t\t\tint all_parents_computed = 1;\n \t\t\tuint32_t max_level = 0;\n-\t\t\ttimestamp_t max_corrected_commit_date = 0;\n \n \t\t\tfor (parent = current->parents; parent; parent = parent->next) {\n \t\t\t\trepo_parse_commit(ctx->r, parent->item);\n \t\t\t\tlevel = *topo_level_slab_at(ctx->topo_levels, parent->item);\n-\t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n \n-\t\t\t\tif (level == GENERATION_NUMBER_ZERO ||\n-\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO) {\n+\t\t\t\tif (level == GENERATION_NUMBER_ZERO) {\n \t\t\t\t\tall_parents_computed = 0;\n \t\t\t\t\tcommit_list_insert(parent->item, &list);\n \t\t\t\t\tbreak;\n@@ -1491,9 +1485,6 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \n \t\t\t\tif (level > max_level)\n \t\t\t\t\tmax_level = level;\n-\n-\t\t\t\tif (corrected_commit_date > max_corrected_commit_date)\n-\t\t\t\t\tmax_corrected_commit_date = corrected_commit_date;\n \t\t\t}\n \n \t\t\tif (all_parents_computed) {\n@@ -1502,6 +1493,55 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\t\t\tif (max_level > GENERATION_NUMBER_V1_MAX - 1)\n \t\t\t\t\tmax_level = GENERATION_NUMBER_V1_MAX - 1;\n \t\t\t\t*topo_level_slab_at(ctx->topo_levels, current) = max_level + 1;\n+\t\t\t}\n+\t\t}\n+\t}\n+\tstop_progress(&ctx->progress);\n+}\n+\n+static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n+{\n+\tint i;\n+\tstruct commit_list *list = NULL;\n+\n+\tif (ctx->report_progress)\n+\t\tctx->progress = start_delayed_progress(\n+\t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n+\t\t\t\t\tctx->commits.nr);\n+\tfor (i = 0; i < ctx->commits.nr; i++) {\n+\t\tstruct commit *c = ctx->commits.list[i];\n+\t\ttimestamp_t corrected_commit_date;\n+\n+\t\trepo_parse_commit(ctx->r, c);\n+\t\tcorrected_commit_date = commit_graph_data_at(c)->generation;\n+\n+\t\tdisplay_progress(ctx->progress, i + 1);\n+\t\tif (corrected_commit_date != GENERATION_NUMBER_ZERO)\n+\t\t\tcontinue;\n+\n+\t\tcommit_list_insert(c, &list);\n+\t\twhile (list) {\n+\t\t\tstruct commit *current = list->item;\n+\t\t\tstruct commit_list *parent;\n+\t\t\tint all_parents_computed = 1;\n+\t\t\ttimestamp_t max_corrected_commit_date = 0;\n+\n+\t\t\tfor (parent = current->parents; parent; parent = parent->next) {\n+\t\t\t\trepo_parse_commit(ctx->r, parent->item);\n+\t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n+\n+\t\t\t\tif (corrected_commit_date == GENERATION_NUMBER_ZERO) {\n+\t\t\t\t\tall_parents_computed = 0;\n+\t\t\t\t\tcommit_list_insert(parent->item, &list);\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\n+\t\t\t\tif (corrected_commit_date > max_corrected_commit_date)\n+\t\t\t\t\tmax_corrected_commit_date = corrected_commit_date;\n+\t\t\t}\n+\n+\t\t\tif (all_parents_computed) {\n+\t\t\t\tpop_commit(&list);\n \n \t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n \t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n@@ -2401,7 +2441,9 @@ int write_commit_graph(struct object_directory *odb,\n \n \tvalidate_mixed_generation_chain(ctx->r->objects->commit_graph);\n \n-\tcompute_generation_numbers(ctx);\n+\tcompute_topological_levels(ctx);\n+\tif (ctx->write_generation_data)\n+\t\tcompute_generation_numbers(ctx);\n \n \tif (ctx->changed_paths)\n \t\tcompute_bloom_filters(ctx);\n-- \ngitgitgadget\n\n"},{"id":"415870","messageId":"38086c85b529f033d243746984227faf87e363d9.1612234883.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","subject":"[PATCH v2 6/6] commit-graph: prepare commit graph","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-02T03:01:23Z","receivedAt":"2021-02-02T03:03:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nBefore checking if the repository has a commit-graph loaded, be sure\nto run prepare_commit_graph(). This is necessary because otherwise\nthe topo_levels slab is not initialized. As we compute topo_levels for\nthe new commits, we iterate further into the lower layers since the\nfirst visit to each commit looks as though the topo_level is not\npopulated.\n\nBy properly initializing the topo_slab, we fix the previously broken\ncase of a split commit graph where a base layer has the\ngeneration_data_overflow chunk.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 10 ++--------\n t/t5318-commit-graph.sh | 2 +-\n 2 files changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ee8d5a0cdb4..94d3ab4a04f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2348,6 +2348,7 @@ int write_commit_graph(struct object_directory *odb,\n \tinit_topo_level_slab(&topo_levels);\n \tctx->topo_levels = &topo_levels;\n \n+\tprepare_commit_graph(ctx->r);\n \tif (ctx->r->objects->commit_graph) {\n \t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n \n@@ -2361,7 +2362,6 @@ int write_commit_graph(struct object_directory *odb,\n \t\tctx->changed_paths = 1;\n \tif (!(flags & COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS)) {\n \t\tstruct commit_graph *g;\n-\t\tprepare_commit_graph_one(ctx->r, ctx->odb);\n \n \t\tg = ctx->r->objects->commit_graph;\n \n@@ -2373,10 +2373,7 @@ int write_commit_graph(struct object_directory *odb,\n \t}\n \n \tif (ctx->split) {\n-\t\tstruct commit_graph *g;\n-\t\tprepare_commit_graph(ctx->r);\n-\n-\t\tg = ctx->r->objects->commit_graph;\n+\t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n \n \t\twhile (g) {\n \t\t\tctx->num_commit_graphs_before++;\n@@ -2400,9 +2397,6 @@ int write_commit_graph(struct object_directory *odb,\n \n \tctx->approx_nr_objects = approximate_object_count();\n \n-\tif (ctx->append)\n-\t\tprepare_commit_graph_one(ctx->r, ctx->odb);\n-\n \tif (ctx->append && ctx->r->objects->commit_graph) {\n \t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n \t\tfor (i = 0; i < g->num_commits; i++) {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 2cf29f425a0..5b15f1aa0f6 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -446,7 +446,7 @@ test_expect_success 'warn on improper hash version' '\n \t)\n '\n \n-test_expect_failure 'lower layers have overflow chunk' '\n+test_expect_success 'lower layers have overflow chunk' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tUNIX_EPOCH_ZERO=\"@0 +0000\" &&\n \tFUTURE_DATE=\"@2147483646 +0000\" &&\n-- \ngitgitgadget\n"},{"id":"415871","messageId":"9bccee8fb6381102d543d6b28e90959307ed08eb.1612234883.git.gitgitgadget@gmail.com","threadId":"55086","inReplyTo":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","subject":"[PATCH v2 5/6] commit-graph: be extra careful about mixed generations","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-02T03:01:22Z","receivedAt":"2021-02-02T03:03:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen upgrading to a commit-graph with corrected commit dates from\none without, there are a few things that need to be considered.\n\nWhen computing generation numbers for the new commit-graph file that\nexpects to add the generation_data chunk with corrected commit\ndates, we need to ensure that the 'generation' member of the\ncommit_graph_data struct is set to zero for these commits.\n\nUnfortunately, the fallback to use topological level for generation\nnumber when corrected commit dates are not available are causing us\nharm here: parsing commits notices that read_generation_data is\nfalse and populates 'generation' with the topological level.\n\nThe solution is to iterate through the commits, parse the commits\nto populate initial values, then reset the generation values to\nzero to trigger recalculation. This loop only occurs when the\nexisting commit-graph data has no corrected commit dates.\n\nWhile this improves our situation somewhat, we have not completely\nsolved the issue for correctly computing generation numbers for mixed\nlayers. That follows in the next change.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2790f70d113..ee8d5a0cdb4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1036,7 +1036,8 @@ struct write_commit_graph_context {\n \t\t split:1,\n \t\t changed_paths:1,\n \t\t order_by_pack:1,\n-\t\t write_generation_data:1;\n+\t\t write_generation_data:1,\n+\t\t trust_generation_numbers:1;\n \n \tstruct topo_level_slab *topo_levels;\n \tconst struct commit_graph_opts *opts;\n@@ -1508,6 +1509,15 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n \t\tctx->progress = start_delayed_progress(\n \t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n \t\t\t\t\tctx->commits.nr);\n+\n+\tif (!ctx->trust_generation_numbers) {\n+\t\tfor (i = 0; i < ctx->commits.nr; i++) {\n+\t\t\tstruct commit *c = ctx->commits.list[i];\n+\t\t\trepo_parse_commit(ctx->r, c);\n+\t\t\tcommit_graph_data_at(c)->generation = GENERATION_NUMBER_ZERO;\n+\t\t}\n+\t}\n+\n \tfor (i = 0; i < ctx->commits.nr; i++) {\n \t\tstruct commit *c = ctx->commits.list[i];\n \t\ttimestamp_t corrected_commit_date;\n@@ -2439,7 +2449,7 @@ int write_commit_graph(struct object_directory *odb,\n \t} else\n \t\tctx->num_commit_graphs_after = 1;\n \n-\tvalidate_mixed_generation_chain(ctx->r->objects->commit_graph);\n+\tctx->trust_generation_numbers = validate_mixed_generation_chain(ctx->r->objects->commit_graph);\n \n \tcompute_topological_levels(ctx);\n \tif (ctx->write_generation_data)\n-- \ngitgitgadget\n\n"},{"id":"415874","messageId":"YBjCHxaMCYyUZ2t5@nand.local","threadId":"55086","inReplyTo":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/6] Generation Number v2: Fix a tricky split graph bug","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-02T03:08:15Z","receivedAt":"2021-02-02T03:09:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 02, 2021 at 03:01:17AM +0000, Derrick Stolee via GitGitGadget wrote:\n> Updates in v2\n> =============\n>\n> * Fixed some typos or other clarifications in commit messages.\n>\n> * The loop assigning read_generation_data is skipped if they already all\n> agree with value 1.\n>\n> * I split compute_generation_numbers into two methods. This essentially\n> splits the previous patch 4 into patches 4 & 5 here. The new patch 4 just\n> splits the logic as-is, then the new patch 5 does the re-initialization\n> of generation values when in the upgrade scenario.\n\nThanks for addressing my nits. I'm much happier with patch 5 as a result\nof your changes, so I think this series was a positive step forward not\nonly for fixing a correctness bug, but also for improving the\nreadability of commit-graph.c.\n\nBoth the range-diff and the new patches look good to me, so this series\nhas my:\n\n Reviewed-by: Taylor Blau \n\n\nThanks,\nTaylor\n"},{"id":"415984","messageId":"YBn3fxFe978Up5Ly@google.com","threadId":"55086","inReplyTo":"454b183b9ba502da7f40dc36aaa95cc3d12b5c2f.1612234883.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-02-03T01:08:15Z","receivedAt":"2021-02-03T01:09:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Derrick Stolee wrote:\n\n> There is a subtle failure happening when computing corrected commit\n> dates with --split enabled. It requires a base layer needing the\n> generation_data_overflow chunk. Then, the next layer on top\n> erroneously thinks it needs an overflow chunk due to a bug leading\n> to recalculating all reachable generation numbers. The output of\n> the failure is\n>\n> BUG: commit-graph.c:1912: expected to write 8 bytes to\n> chunk 47444f56, but wrote 0 instead\n\nAt Google, we're running into a commit-graph issue that appears to\nhave also arrived as part of this last week's rollout. This one is a\nbit worse --- it is reproducible for affected users and stops them\nfrom being able to do day-to-day development:\n\n $ git pull\n remote: Finding sources: 100% (33/33)\n remote: Total 33 (delta 18), reused 33 (delta 18)\n Unpacking objects: 100% (33/33), 27.44 KiB | 460.00 KiB/s, done.\n From https://example.com/path/to/repo\n 05ba0d775..279e4e6d0 master -> origin/master\n BUG: commit-reach.c:64: bad generation skip 29e3 > 628 at 62abdabd1be00ebadbf73061ecf72b35042338e3\n error: merge died of signal 6\n\n\"git commit-graph verify\" agrees that the generation numbers are wrong:\n\n $ git commit-graph verify\n commit-graph generation for commit 4290b2214cdf50263118322735347d151715a272 is 3 != 1586\n Verifying commits in commit graph: 100% (1/1), done.\n commit-graph generation for commit b6c73a8472c7cb503cce0668849150a4b4329230 is 1576 != 10724\n Verifying commits in commit graph: 100% (10/10), done.\n Verifying commits in commit graph: 100% (88/88), done.\n Verifying commits in commit graph: 100% (208/208), done.\n Verifying commits in commit graph: 100% (592/592), done.\n Verifying commits in commit graph: 100% (1567/1567), done.\n Verifying commits in commit graph: 100% (8358/8358), done.\n\nWe have some examples of repositories that were corrupted this way,\nbut we didn't catch them in the act of corruption --- it started\nhappening to several users with this release so we immediately rolled\nback.\n\nQuestions:\n\n- is this likely to be due to the same cause, or is it orthogonal?\n\n- what is the recommended way to recover from this state? \"git fsck\"\n shows the repositories to have no problems. \"git help commit-graph\"\n doesn't show a command for users to use; is\n `rm -fr .git/objects/info/commit-graphs/` the recommended recovery\n command?\n\n- is there configuration or a patch we can roll out to help affected\n users recover from this state?\n\nThanks,\nJonathan\n"},{"id":"415988","messageId":"1dab0bf0-9a7f-370a-c807-25d67ac7a0a0@gmail.com","threadId":"55086","inReplyTo":"YBn3fxFe978Up5Ly@google.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-03T01:35:00Z","receivedAt":"2021-02-03T01:35:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/2/2021 8:08 PM, Jonathan Nieder wrote:\n> Derrick Stolee wrote:\n> \n>> There is a subtle failure happening when computing corrected commit\n>> dates with --split enabled. It requires a base layer needing the\n>> generation_data_overflow chunk. Then, the next layer on top\n>> erroneously thinks it needs an overflow chunk due to a bug leading\n>> to recalculating all reachable generation numbers. The output of\n>> the failure is\n>>\n>> BUG: commit-graph.c:1912: expected to write 8 bytes to\n>> chunk 47444f56, but wrote 0 instead\n> \n> At Google, we're running into a commit-graph issue that appears to\n> have also arrived as part of this last week's rollout. This one is a\n> bit worse --- it is reproducible for affected users and stops them\n> from being able to do day-to-day development:\n\nYou're shipping 'next' widely? I appreciate the extra eyes on\nearly bits, so we can find more issues and get them resolved.\n\n> $ git pull\n> remote: Finding sources: 100% (33/33)\n> remote: Total 33 (delta 18), reused 33 (delta 18)\n> Unpacking objects: 100% (33/33), 27.44 KiB | 460.00 KiB/s, done.\n> From https://example.com/path/to/repo\n> 05ba0d775..279e4e6d0 master -> origin/master\n> BUG: commit-reach.c:64: bad generation skip 29e3 > 628 at 62abdabd1be00ebadbf73061ecf72b35042338e3\n> error: merge died of signal 6\n> \n> \"git commit-graph verify\" agrees that the generation numbers are wrong:\n> \n> $ git commit-graph verify\n> commit-graph generation for commit 4290b2214cdf50263118322735347d151715a272 is 3 != 1586\n> Verifying commits in commit graph: 100% (1/1), done.\n> commit-graph generation for commit b6c73a8472c7cb503cce0668849150a4b4329230 is 1576 != 10724\n> Verifying commits in commit graph: 100% (10/10), done.\n> Verifying commits in commit graph: 100% (88/88), done.\n> Verifying commits in commit graph: 100% (208/208), done.\n> Verifying commits in commit graph: 100% (592/592), done.\n> Verifying commits in commit graph: 100% (1567/1567), done.\n> Verifying commits in commit graph: 100% (8358/8358), done.\n> \n> We have some examples of repositories that were corrupted this way,\n> but we didn't catch them in the act of corruption --- it started\n> happening to several users with this release so we immediately rolled\n> back.\n\nIt is definitely related to the split commit-graph during the\nupgrade scenario. Your verify output shows that you are using\nthe --split option heavily (possibly with fetch.writeCommitGraph?\nor are you using 'git maintenance run --task=commit-graph'?)\n\n> Questions:\n> \n> - is this likely to be due to the same cause, or is it orthogonal?\n\nMy guess is that the reason is the same. I think that you might\nhave some strangeness of a commit-graph layer with corrected commit\ndates being below a commit-graph layer without it.\n\n> - what is the recommended way to recover from this state? \"git fsck\"\n> shows the repositories to have no problems. \"git help commit-graph\"\n> doesn't show a command for users to use; is\n> `rm -fr .git/objects/info/commit-graphs/` the recommended recovery\n> command?\n\nThat, followed by `git commit-graph write --reachable [--changed-paths]`\ndepending on what they want.\n\n> - is there configuration or a patch we can roll out to help affected\n> users recover from this state?\n\nIf you are willing, then take v2 of this series and follow through by\nclearing the commit-graph files of affected users. Note that you can\nbe proactive using `git commit-graph verify` to see who needs rewrites.\n\nThanks,\n-Stolee\n"},{"id":"415989","messageId":"YBoBBie2t1EhcLAN@google.com","threadId":"55086","inReplyTo":"1dab0bf0-9a7f-370a-c807-25d67ac7a0a0@gmail.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-02-03T01:48:54Z","receivedAt":"2021-02-03T01:49:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDerrick Stolee wrote:\n> On 2/2/2021 8:08 PM, Jonathan Nieder wrote:\n\n>> At Google, we're running into a commit-graph issue that appears to\n>> have also arrived as part of this last week's rollout. This one is a\n>> bit worse --- it is reproducible for affected users and stops them\n>> from being able to do day-to-day development:\n>\n> You're shipping 'next' widely? I appreciate the extra eyes on\n> early bits, so we can find more issues and get them resolved.\n\nYes. Changes in 'next' have already gotten all the vetting via code\nreview that they're going to get; the difference between changes in\n'next' and 'master' is that the latter have had some production\nexposure among users of 'next' with the ability to get help from a\nlocal expert, roll back quickly when there's a problem, and so on. I\nrecommend that anyone with an installation with that ability use\n'next', to improve the quality of code that ultimately is released\nfrom 'master'.\n\nIt also helps us get the chance to use our experience to affect the\ndirection of a topic before it's too late.\n\n[...]\n>> We have some examples of repositories that were corrupted this way,\n>> but we didn't catch them in the act of corruption --- it started\n>> happening to several users with this release so we immediately rolled\n>> back.\n>\n> It is definitely related to the split commit-graph during the\n> upgrade scenario. Your verify output shows that you are using\n> the --split option heavily (possibly with fetch.writeCommitGraph?\n> or are you using 'git maintenance run --task=commit-graph'?)\n\nYep, the splits come from fetch.writeCommitGraph.\n\n[...]\n>> - what is the recommended way to recover from this state? \"git fsck\"\n>> shows the repositories to have no problems. \"git help commit-graph\"\n>> doesn't show a command for users to use; is\n>> `rm -fr .git/objects/info/commit-graphs/` the recommended recovery\n>> command?\n>\n> That, followed by `git commit-graph write --reachable [--changed-paths]`\n> depending on what they want.\n\nCan we package this as something more user-friendly? E.g.\n\n\tgit commit-graph clear\n\tgit commit-graph write --reachable\n\nIf that makes sense to you, I'm happy to send a patch (or to review\none if someone else gets to it first). I'm mostly asking to find out\nwhether this matches your idea of what the UI should be like.\n\n>> - is there configuration or a patch we can roll out to help affected\n>> users recover from this state?\n>\n> If you are willing, then take v2 of this series and follow through by\n> clearing the commit-graph files of affected users. Note that you can\n> be proactive using `git commit-graph verify` to see who needs rewrites.\n\nDoes this mean we should change the BUG error message to help affected\nusers discover how they can recover for themselves (for example, using\ncommands like the above)?\n\nAlso, should \"git fsck\" call \"git commit-graph verify\" to make the\nlatter more discoverable?\n\nThanks,\nJonathan\n"},{"id":"415990","messageId":"xmqq7dnpewg4.fsf@gitster.c.googlers.com","threadId":"55086","inReplyTo":"1dab0bf0-9a7f-370a-c807-25d67ac7a0a0@gmail.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-03T02:06:51Z","receivedAt":"2021-02-03T02:07:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> - what is the recommended way to recover from this state? \"git fsck\"\n>> shows the repositories to have no problems. \"git help commit-graph\"\n>> doesn't show a command for users to use; is\n>> `rm -fr .git/objects/info/commit-graphs/` the recommended recovery\n>> command?\n\n\"rm -f .git/objects/info/commit-graph\" as well, no?\n\n> That, followed by `git commit-graph write --reachable [--changed-paths]`\n> depending on what they want.\n\nJust out of curiosity, how important is \"--reachable\"? It only\ntraverses from the tips of refs and unlike fsck and repack, not from\nreflog entries (or the index for that matter, but that shouldn't\nmake much difference as there is no _commit_ in the index).\n\n>> - is there configuration or a patch we can roll out to help affected\n>> users recover from this state?\n>\n> If you are willing, then take v2 of this series and follow through by\n> clearing the commit-graph files of affected users. Note that you can\n> be proactive using `git commit-graph verify` to see who needs rewrites.\n\nFYI, today's 368b6599 (Merge branch 'ds/commit-graph-genno-fix' into\njch, 2021-02-01) has this stuff.\n"},{"id":"415994","messageId":"6dc1520f-8130-75e1-6617-67b54cb03933@gmail.com","threadId":"55086","inReplyTo":"YBoBBie2t1EhcLAN@google.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-03T03:07:32Z","receivedAt":"2021-02-03T03:08:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/2/2021 8:48 PM, Jonathan Nieder wrote:\n> Hi,\n> \n> Derrick Stolee wrote:\n>> On 2/2/2021 8:08 PM, Jonathan Nieder wrote:\n> \n>>> At Google, we're running into a commit-graph issue that appears to\n>>> have also arrived as part of this last week's rollout. This one is a\n>>> bit worse --- it is reproducible for affected users and stops them\n>>> from being able to do day-to-day development:\n>>\n>> You're shipping 'next' widely? I appreciate the extra eyes on\n>> early bits, so we can find more issues and get them resolved.\n> \n> Yes. Changes in 'next' have already gotten all the vetting via code\n> review that they're going to get; the difference between changes in\n> 'next' and 'master' is that the latter have had some production\n> exposure among users of 'next' with the ability to get help from a\n> local expert, roll back quickly when there's a problem, and so on. I\n> recommend that anyone with an installation with that ability use\n> 'next', to improve the quality of code that ultimately is released\n> from 'master'.\n> \n> It also helps us get the chance to use our experience to affect the\n> direction of a topic before it's too late.\n\nThis is a good practice. It's also how I found the issues fixed\nin this series, but that's because I install it locally for my own\nextra additional testing before shipping it to users.\n\n> [...]\n>>> We have some examples of repositories that were corrupted this way,\n>>> but we didn't catch them in the act of corruption --- it started\n>>> happening to several users with this release so we immediately rolled\n>>> back.\n>>\n>> It is definitely related to the split commit-graph during the\n>> upgrade scenario. Your verify output shows that you are using\n>> the --split option heavily (possibly with fetch.writeCommitGraph?\n>> or are you using 'git maintenance run --task=commit-graph'?)\n> \n> Yep, the splits come from fetch.writeCommitGraph.\n> \n> [...]\n>>> - what is the recommended way to recover from this state? \"git fsck\"\n>>> shows the repositories to have no problems. \"git help commit-graph\"\n>>> doesn't show a command for users to use; is\n>>> `rm -fr .git/objects/info/commit-graphs/` the recommended recovery\n>>> command?\n>>\n>> That, followed by `git commit-graph write --reachable [--changed-paths]`\n>> depending on what they want.\n> \n> Can we package this as something more user-friendly? E.g.\n> \n> \tgit commit-graph clear\n> \tgit commit-graph write --reachable\n> \n> If that makes sense to you, I'm happy to send a patch (or to review\n> one if someone else gets to it first). I'm mostly asking to find out\n> whether this matches your idea of what the UI should be like.\n\n'clear' is probably fine. I was thinking it might be good to have\nan option to the 'write' subcommand to clear the existing data, but\nit's probably better as separate steps.\n\n>>> - is there configuration or a patch we can roll out to help affected\n>>> users recover from this state?\n>>\n>> If you are willing, then take v2 of this series and follow through by\n>> clearing the commit-graph files of affected users. Note that you can\n>> be proactive using `git commit-graph verify` to see who needs rewrites.\n> \n> Does this mean we should change the BUG error message to help affected\n> users discover how they can recover for themselves (for example, using\n> commands like the above)?\n\nIt _is_ a bug that led to this, but it's more about incorrect\ncommit-graph data which could be caused by anything. Better to\nhave a better message such as \"your commit-graph file is\nprobably corrupt\".\n\n> Also, should \"git fsck\" call \"git commit-graph verify\" to make the\n> latter more discoverable?\n\nYes. I thought it did, but I must be incorrect.\n\nThanks,\n-Stolee\n"},{"id":"415995","messageId":"247b0056-8358-f71f-22ee-0bacfbc4239d@gmail.com","threadId":"55086","inReplyTo":"xmqq7dnpewg4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-03T03:09:00Z","receivedAt":"2021-02-03T03:09:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/2/2021 9:06 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>>> - what is the recommended way to recover from this state? \"git fsck\"\n>>> shows the repositories to have no problems. \"git help commit-graph\"\n>>> doesn't show a command for users to use; is\n>>> `rm -fr .git/objects/info/commit-graphs/` the recommended recovery\n>>> command?\n> \n> \"rm -f .git/objects/info/commit-graph\" as well, no?\n\nIn this case, that won't be necessary since they are using a\nsplit commit-graph. However, the following is what I do to\nbe extra sure:\n\n\trm -rf .git/objects/info/commit-graph*\n\nDeletes the singleton file and the directory.\n\n>> That, followed by `git commit-graph write --reachable [--changed-paths]`\n>> depending on what they want.\n> \n> Just out of curiosity, how important is \"--reachable\"? It only\n> traverses from the tips of refs and unlike fsck and repack, not from\n> reflog entries (or the index for that matter, but that shouldn't\n> make much difference as there is no _commit_ in the index).\n\nI just like to focus on the reachable commits starting at refs\ninstead of scanning all packed objects to see which are commits\nor not.\n\nThanks,\n-stolee\n\n"},{"id":"416056","messageId":"YBrCli7AR/XrB3Pr@nand.local","threadId":"55086","inReplyTo":"6dc1520f-8130-75e1-6617-67b54cb03933@gmail.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-03T15:34:46Z","receivedAt":"2021-02-03T15:54:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 02, 2021 at 10:07:32PM -0500, Derrick Stolee wrote:\n> > Can we package this as something more user-friendly? E.g.\n> >\n> > \tgit commit-graph clear\n> > \tgit commit-graph write --reachable\n> >\n> > If that makes sense to you, I'm happy to send a patch (or to review\n> > one if someone else gets to it first). I'm mostly asking to find out\n> > whether this matches your idea of what the UI should be like.\n>\n> 'clear' is probably fine. I was thinking it might be good to have\n> an option to the 'write' subcommand to clear the existing data, but\n> it's probably better as separate steps.\n\nWouldn't 'git commit-graph write --split=replace --reachable' do the\nsame thing? I know that you changed some of the spots where we load an\nexisting commit graph, so my claim might be out-of-date, but I'm pretty\nsure that this would get you out of a broken state.\n\nThinking aloud, I'm not totally sure that we should be exposing \"git\ncommit-graph clear\" to users. The only time that you'd want to run this\nis if you were trying to remove a corrupted commit-graph, so I'd rather\nsee guidance on how to do that safely show up in\nDocumentation/git-commit-graph.txt.\n\nOn the other hand, now I'm encouraging running \"rm -fr\n$GIT_DIR/objects/info/commit-graph*\", which feels dangerous.\n\nSomewhere in the middle would be something like:\n\n git -c core.commitGraph=false commit-graph write --reachable\n\nwhich would disable reading existing commit-graph files. Since\n85102ac71b (commit-graph: don't write commit-graph when disabled,\n2020-10-09), that causes us to exit immediately.\n\nI think that reverting that patch and advertising setting\n'core.commitGraph=false' in the documentation makes the most sense.\n\nThanks,\nTaylor\n"},{"id":"416070","messageId":"CAPig+cTMcNjJVyTb1mio+Pof46G3fXxurrtMiwO4h8DWi8Eu2A@mail.gmail.com","threadId":"55086","inReplyTo":"YBrCli7AR/XrB3Pr@nand.local","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-02-03T17:37:21Z","receivedAt":"2021-02-03T17:38:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 3, 2021 at 10:55 AM Taylor Blau wrote:\n> On Tue, Feb 02, 2021 at 10:07:32PM -0500, Derrick Stolee wrote:\n> > 'clear' is probably fine. I was thinking it might be good to have\n> > an option to the 'write' subcommand to clear the existing data, but\n> > it's probably better as separate steps.\n>\n> Wouldn't 'git commit-graph write --split=replace --reachable' do the\n> same thing? I know that you changed some of the spots where we load an\n> existing commit graph, so my claim might be out-of-date, but I'm pretty\n> sure that this would get you out of a broken state.\n>\n> Thinking aloud, I'm not totally sure that we should be exposing \"git\n> commit-graph clear\" to users. The only time that you'd want to run this\n> is if you were trying to remove a corrupted commit-graph, so I'd rather\n> see guidance on how to do that safely show up in\n> Documentation/git-commit-graph.txt.\n\nThrowing one more idea into the mix, git-worktree recently got a\n`repair` subcommand. Even though it presently repairs a small set of\nproblems, the subcommand name is intentionally generic so as to\nprovide room for growth. One could imagine `git commit-graph repair`\nbeing added to provide a user-friendly mechanism for recovering from\ncommit-graph damage.\n"},{"id":"416074","messageId":"xmqqk0rpc7uj.fsf@gitster.c.googlers.com","threadId":"55086","inReplyTo":"YBrCli7AR/XrB3Pr@nand.local","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-03T18:41:08Z","receivedAt":"2021-02-03T18:42:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Thinking aloud, I'm not totally sure that we should be exposing \"git\n> commit-graph clear\" to users. The only time that you'd want to run this\n> is if you were trying to remove a corrupted commit-graph, so I'd rather\n> see guidance on how to do that safely show up in\n> Documentation/git-commit-graph.txt.\n>\n> On the other hand, now I'm encouraging running \"rm -fr\n> $GIT_DIR/objects/info/commit-graph*\", which feels dangerous.\n\nTrue.\n\nAs this is, like pack .idx file, supposed to be \"precomputed cached\ndata that can be fully recreated using primary information\" [*], I\nam perfectly fine to say \"commit-graph may have unexplored corners,\nand when you hit a BUG(), you can safely use 'commit-graph clear'\nand recreate it from scratch, or operate without it if you feel you\ndo not yet want to trust your data to it for now.\" Giving safer and\neasier way to opt out for those who need to get today's release\ndone, with enough performance incentive to re-enable it when the\ncrunch is over, would be an honest thing to do, I would think.\n\n\tSide note: the index file also used to be considered to hold\n\tsuch cached data, that can be recreated from the working\n\ttree data and the tip commit. We no longer treat it that\n\tway, though.\n\n> Somewhere in the middle would be something like:\n>\n> git -c core.commitGraph=false commit-graph write --reachable\n\nI am a bit worried about the thinking along this line, because it\ngives the users an impression that there is no escaping from\ntrusting commit-graph---the one that was created from scratch is\nbug-free and they only need to be cautious about incrementals.\n\nBut (1) we do not know that, and (2) it is an unconvincing message\nto somebody who just got hit by a BUG().\n\n> which would disable reading existing commit-graph files. Since\n> 85102ac71b (commit-graph: don't write commit-graph when disabled,\n> 2020-10-09), that causes us to exit immediately.\n\nMeaning the three command sequence\n\n\tgit commit-graph clear\n\tgit commit-graph write --reachable\n git config core.commitGraph false\n\nto force a clean build of a graph and forbid further updates until\nthe bug is squashed??? But should't core.commitGraph forbid reading\nand using the data in the existing files, too? In which case, shouldn't\nit be equivalent to \"git commit-graph clear\"?\n\n> I think that reverting that patch and advertising setting\n> 'core.commitGraph=false' in the documentation makes the most sense.\n\n"},{"id":"416092","messageId":"YBsQ5WhmSPNPyDDs@nand.local","threadId":"55086","inReplyTo":"xmqqk0rpc7uj.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-02-03T21:08:53Z","receivedAt":"2021-02-03T21:10:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 03, 2021 at 10:41:08AM -0800, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Thinking aloud, I'm not totally sure that we should be exposing \"git\n> > commit-graph clear\" to users. The only time that you'd want to run this\n> > is if you were trying to remove a corrupted commit-graph, so I'd rather\n> > see guidance on how to do that safely show up in\n> > Documentation/git-commit-graph.txt.\n> >\n> > On the other hand, now I'm encouraging running \"rm -fr\n> > $GIT_DIR/objects/info/commit-graph*\", which feels dangerous.\n>\n> True.\n>\n> As this is, like pack .idx file, supposed to be \"precomputed cached\n> data that can be fully recreated using primary information\" [*], I\n> am perfectly fine to say \"commit-graph may have unexplored corners,\n> and when you hit a BUG(), you can safely use 'commit-graph clear'\n> and recreate it from scratch, or operate without it if you feel you\n> do not yet want to trust your data to it for now.\" Giving safer and\n> easier way to opt out for those who need to get today's release\n> done, with enough performance incentive to re-enable it when the\n> crunch is over, would be an honest thing to do, I would think.\n>\n> \tSide note: the index file also used to be considered to hold\n> \tsuch cached data, that can be recreated from the working\n> \ttree data and the tip commit. We no longer treat it that\n> \tway, though.\n>\n> > Somewhere in the middle would be something like:\n> >\n> > git -c core.commitGraph=false commit-graph write --reachable\n>\n> I am a bit worried about the thinking along this line, because it\n> gives the users an impression that there is no escaping from\n> trusting commit-graph---the one that was created from scratch is\n> bug-free and they only need to be cautious about incrementals.\n>\n> But (1) we do not know that, and (2) it is an unconvincing message\n> to somebody who just got hit by a BUG().\n\nThis is a convincing counter-point to my proposal. Yeah, I agree that we\nshouldn't be advertising that commit-graph is completely trustworthy.\n\n> > which would disable reading existing commit-graph files. Since\n> > 85102ac71b (commit-graph: don't write commit-graph when disabled,\n> > 2020-10-09), that causes us to exit immediately.\n>\n> Meaning the three command sequence\n>\n> \tgit commit-graph clear\n> \tgit commit-graph write --reachable\n> git config core.commitGraph false\n>\n> to force a clean build of a graph and forbid further updates until\n> the bug is squashed??? But should't core.commitGraph forbid reading\n> and using the data in the existing files, too? In which case, shouldn't\n> it be equivalent to \"git commit-graph clear\"?\n\nI think we may be saying the same thing. I was suggesting that if we\nreverted 85102ac71b, that 'git -c core.commitGraph=false commit-graph\nwrite ...' would rewrite your commit-graph from scratch (without opening\nup existing ones and propagating corruption).\n\nSo I was saying that that *would* be a viable \"git commit-grpah clear\"\n(if 85102ac71b were reverted).\n\nThanks,\nTaylor\n"},{"id":"416373","messageId":"20210207190415.GB1015009@szeder.dev","threadId":"55086","inReplyTo":"xmqq7dnpewg4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-02-07T19:04:15Z","receivedAt":"2021-02-07T19:05:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Feb 02, 2021 at 06:06:51PM -0800, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n> >> - what is the recommended way to recover from this state? \"git fsck\"\n> >> shows the repositories to have no problems. \"git help commit-graph\"\n> >> doesn't show a command for users to use; is\n> >> `rm -fr .git/objects/info/commit-graphs/` the recommended recovery\n> >> command?\n> \n> \"rm -f .git/objects/info/commit-graph\" as well, no?\n> \n> > That, followed by `git commit-graph write --reachable [--changed-paths]`\n> > depending on what they want.\n> \n> Just out of curiosity, how important is \"--reachable\"? It only\n> traverses from the tips of refs and unlike fsck and repack, not from\n> reflog entries (or the index for that matter, but that shouldn't\n> make much difference as there is no _commit_ in the index).\n\nScanning all objects in all packfiles is a very inefficient way to\nfind the commits to be recorded in the commit-graph, and depending on\nthe repository's shape and size can have several times higher runtime\nand memory footprint.\n\n"},{"id":"416382","messageId":"xmqqh7mn7i3m.fsf@gitster.c.googlers.com","threadId":"55086","inReplyTo":"20210207190415.GB1015009@szeder.dev","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-07T20:12:13Z","receivedAt":"2021-02-07T20:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Tue, Feb 02, 2021 at 06:06:51PM -0800, Junio C Hamano wrote:\n>> Derrick Stolee writes:\n>> \n>> >> - what is the recommended way to recover from this state? \"git fsck\"\n>> >> shows the repositories to have no problems. \"git help commit-graph\"\n>> >> doesn't show a command for users to use; is\n>> >> `rm -fr .git/objects/info/commit-graphs/` the recommended recovery\n>> >> command?\n>> \n>> \"rm -f .git/objects/info/commit-graph\" as well, no?\n>> \n>> > That, followed by `git commit-graph write --reachable [--changed-paths]`\n>> > depending on what they want.\n>> \n>> Just out of curiosity, how important is \"--reachable\"? It only\n>> traverses from the tips of refs and unlike fsck and repack, not from\n>> reflog entries (or the index for that matter, but that shouldn't\n>> make much difference as there is no _commit_ in the index).\n>\n> Scanning all objects in all packfiles is a very inefficient way to\n> find the commits to be recorded in the commit-graph, and depending on\n> the repository's shape and size can have several times higher runtime\n> and memory footprint.\n\nBut wouldn't it make the resulting graph file not very useful for\nthe purpose of say deciding what object to pack when running \"gc\" or\n\"repack\" or \"prune\"? The fact that it ignores the index and the reflog\nentries as roots of traversal with \"--reachable\" bothers me.\n"},{"id":"416389","messageId":"74f1eeab-3211-71e5-c83c-19c0b7c0257d@gmail.com","threadId":"55086","inReplyTo":"xmqqh7mn7i3m.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-02-08T02:01:44Z","receivedAt":"2021-02-08T02:02:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/7/2021 3:12 PM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>> Scanning all objects in all packfiles is a very inefficient way to\n>> find the commits to be recorded in the commit-graph, and depending on\n>> the repository's shape and size can have several times higher runtime\n>> and memory footprint.\n> \n> But wouldn't it make the resulting graph file not very useful for\n> the purpose of say deciding what object to pack when running \"gc\" or\n> \"repack\" or \"prune\"? The fact that it ignores the index and the reflog\n> entries as roots of traversal with \"--reachable\" bothers me.\n\nThe reflog _might_ have something of value there, but the hope is\nthat very few commits are actually being force-pushed away. The focus\nis to prioritize the deep history, and it would definitely be an\nanti-pattern if the commits in the reflog are so numerous that they\nmust be tracked by the commit-graph. Of course, skipping the --reachable\noption enables a way to gather these commits as necessary.\n\nThe index won't have commit oids that matter (yes, for submodules they\nwill exist, but those are not commits for the super repo).\n\nThanks,\n-Stolee\n"},{"id":"416396","messageId":"xmqq8s7z6r3c.fsf@gitster.c.googlers.com","threadId":"55086","inReplyTo":"74f1eeab-3211-71e5-c83c-19c0b7c0257d@gmail.com","subject":"Re: [PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-08T05:55:35Z","receivedAt":"2021-02-08T05:56:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> The reflog _might_ have something of value there, but the hope is\n> that very few commits are actually being force-pushed away. The focus\n> is to prioritize the deep history, and it would definitely be an\n> anti-pattern if the commits in the reflog are so numerous that they\n> must be tracked by the commit-graph. Of course, skipping the --reachable\n> option enables a way to gather these commits as necessary.\n\nSure. As long as gaps in commit-graph coverage only affect\nperformance and never correctness (e.g. even if the things that are\nonly reachable from reflog entries are not covered by commit-graph,\nwhen 'git prune' wants to know if an object can safely pruned, we\nwill correctly say \"no, do not prune it yet\"), that is perfectly fine.\n"},{"id":"416712","messageId":"YCS2N8WC5jQW1jCZ@Abhishek-Arch","threadId":"55086","inReplyTo":"pull.850.v2.git.1612234883.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/6] Generation Number v2: Fix a tricky split graph bug","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2021-02-11T04:44:39Z","receivedAt":"2021-02-11T04:45:11Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"On Tue, Feb 02, 2021 at 03:01:17AM +0000, Derrick Stolee via GitGitGadget wrote:\n> Here is a bugfix for the recently-landed-in-next generation v2 topic\n> (ak/corrected-commit-date).\n> \n> This was occasionally hitting us when computing new commit-graphs on\n> existing repositories with the new bits. It was very hard to reproduce, and\n> it turns out to be due to not parsing commits before accessing generation\n> number data. Doing so in the right place demonstrates the bug of recomputing\n> the corrected commit date even for commits in lower layers with computed\n> values.\n> \n> The fix is split into these steps:\n> \n> 1. Parse commits more often before accessing their data. (This allows the\n> bug to be demonstrated in the test suite.)\n> 2. Check the full commit-graph chain for generation data chunks.\n> 3. Don't compute corrected commit dates if the lower layers do not support\n> them.\n> 4. Parse the commit-graph file more often.\n> \n> Thanks, -Stolee\n> \n\nThanks for the fast bug-fix. It must have been hard to track down why as\nwe weren't able to reproduce this behaviour before. As Taylor said\nbefore, this patch is a clear improvement in readability and correctness.\n\nThanks\n- Abhishek\n\n> \n> Updates in v2\n> =============\n> \n> * Fixed some typos or other clarifications in commit messages.\n> \n> * The loop assigning read_generation_data is skipped if they already all\n> agree with value 1.\n> \n> * I split compute_generation_numbers into two methods. This essentially\n> splits the previous patch 4 into patches 4 & 5 here. The new patch 4 just\n> splits the logic as-is, then the new patch 5 does the re-initialization\n> of generation values when in the upgrade scenario.\n> \n> Derrick Stolee (6):\n> commit-graph: use repo_parse_commit\n> commit-graph: always parse before commit_graph_data_at()\n> commit-graph: validate layers for generation data\n> commit-graph: compute generations separately\n> commit-graph: be extra careful about mixed generations\n> commit-graph: prepare commit graph\n> \n> commit-graph.c | 138 +++++++++++++++++++++++++++++-----------\n> commit.h | 5 +-\n> t/t5318-commit-graph.sh | 21 ++++++\n> 3 files changed, 125 insertions(+), 39 deletions(-)\n> \n> \n> base-commit: 5a3b130cad0d5c770f766e3af6d32b41766374c0\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-850%2Fderrickstolee%2Fgen-v2-upgrade-fix-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-850/derrickstolee/gen-v2-upgrade-fix-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/850\n> \n> Range-diff vs v1:\n> \n> 1: 9c605c99f66 = 1: 9c605c99f66 commit-graph: use repo_parse_commit\n> 2: 82afa811dff ! 2: 454b183b9ba commit-graph: always parse before commit_graph_data_at()\n> @@ Commit message\n> problems when writing a commit-graph with corrected commit dates based\n> on a commit-graph without them.\n> \n> - It has been difficult to identify the issue here becaus it was so hard\n> + It has been difficult to identify the issue here because it was so hard\n> to reproduce. It relies on this uninitialized data having a non-zero\n> value, but also on specifically in a way that overwrites the existing\n> data.\n> 3: d554fa30660 ! 3: 3d223fa2156 commit-graph: validate layers for generation data\n> @@ commit-graph.c: static struct commit_graph *load_commit_graph_chain(struct repos\n> \n> -\tif (!g)\n> -\t\treturn;\n> --\n> --\tread_generation_data = !!g->chunk_generation_data;\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> +-\tread_generation_data = !!g->chunk_generation_data;\n> ++\tif (read_generation_data)\n> ++\t\treturn 1;\n> + \n> \twhile (g) {\n> - \t\tg->read_generation_data = read_generation_data;\n> +-\t\tg->read_generation_data = read_generation_data;\n> ++\t\tg->read_generation_data = 0;\n> \t\tg = g->base_graph;\n> \t}\n> +\n> -+\treturn read_generation_data;\n> ++\treturn 0;\n> }\n> \n> struct commit_graph *read_commit_graph_one(struct repository *r,\n> -: ----------- > 4: 05248ff222f commit-graph: compute generations separately\n> 4: b267a9653a7 ! 5: 9bccee8fb63 commit-graph: be extra careful about mixed generations\n> @@ Commit message\n> existing commit-graph data has no corrected commit dates.\n> \n> While this improves our situation somewhat, we have not completely\n> - solved the issue for correctly computing generation numbers for mixes\n> + solved the issue for correctly computing generation numbers for mixed\n> layers. That follows in the next change.\n> \n> Signed-off-by: Derrick Stolee \n> @@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph\n> \t\t\t\t\t_(\"Computing commit graph generation numbers\"),\n> \t\t\t\t\tctx->commits.nr);\n> +\n> -+\tif (ctx->write_generation_data && !ctx->trust_generation_numbers) {\n> ++\tif (!ctx->trust_generation_numbers) {\n> +\t\tfor (i = 0; i < ctx->commits.nr; i++) {\n> +\t\t\tstruct commit *c = ctx->commits.list[i];\n> +\t\t\trepo_parse_commit(ctx->r, c);\n> @@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph\n> +\n> \tfor (i = 0; i < ctx->commits.nr; i++) {\n> \t\tstruct commit *c = ctx->commits.list[i];\n> - \t\tuint32_t level;\n> -@@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n> - \t\t\t\tcorrected_commit_date = commit_graph_data_at(parent->item)->generation;\n> - \n> - \t\t\t\tif (level == GENERATION_NUMBER_ZERO ||\n> --\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO) {\n> -+\t\t\t\t (ctx->write_generation_data &&\n> -+\t\t\t\t corrected_commit_date == GENERATION_NUMBER_ZERO)) {\n> - \t\t\t\t\tall_parents_computed = 0;\n> - \t\t\t\t\tcommit_list_insert(parent->item, &list);\n> - \t\t\t\t\tbreak;\n> -@@ commit-graph.c: static void compute_generation_numbers(struct write_commit_graph_context *ctx)\n> - \t\t\t\t\tmax_level = GENERATION_NUMBER_V1_MAX - 1;\n> - \t\t\t\t*topo_level_slab_at(ctx->topo_levels, current) = max_level + 1;\n> - \n> --\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n> --\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n> --\t\t\t\tcommit_graph_data_at(current)->generation = max_corrected_commit_date + 1;\n> --\n> --\t\t\t\tif (commit_graph_data_at(current)->generation - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n> --\t\t\t\t\tctx->num_generation_data_overflows++;\n> -+\t\t\t\tif (ctx->write_generation_data) {\n> -+\t\t\t\t\ttimestamp_t cur_g;\n> -+\t\t\t\t\tif (current->date && current->date > max_corrected_commit_date)\n> -+\t\t\t\t\t\tmax_corrected_commit_date = current->date - 1;\n> -+\t\t\t\t\tcur_g = commit_graph_data_at(current)->generation\n> -+\t\t\t\t\t = max_corrected_commit_date + 1;\n> -+\t\t\t\t\tif (cur_g - current->date > GENERATION_NUMBER_V2_OFFSET_MAX)\n> -+\t\t\t\t\t\tctx->num_generation_data_overflows++;\n> -+\t\t\t\t}\n> - \t\t\t}\n> - \t\t}\n> - \t}\n> + \t\ttimestamp_t corrected_commit_date;\n> @@ commit-graph.c: int write_commit_graph(struct object_directory *odb,\n> \t} else\n> \t\tctx->num_commit_graphs_after = 1;\n> @@ commit-graph.c: int write_commit_graph(struct object_directory *odb,\n> -\tvalidate_mixed_generation_chain(ctx->r->objects->commit_graph);\n> +\tctx->trust_generation_numbers = validate_mixed_generation_chain(ctx->r->objects->commit_graph);\n> \n> - \tcompute_generation_numbers(ctx);\n> - \n> + \tcompute_topological_levels(ctx);\n> + \tif (ctx->write_generation_data)\n> 5: dddeec30ebf ! 6: 38086c85b52 commit-graph: prepare commit graph\n> @@ Commit message\n> commit-graph: prepare commit graph\n> \n> Before checking if the repository has a commit-graph loaded, be sure\n> - to run prepare_commit_graph(). This is necessary because without this\n> - instance we would not initialize the topo_levels slab for each of the\n> - struct commit_graphs in the chain before we start to parse the\n> - commits. This leads to possibly recomputing the topological levels for\n> - commits in lower layers even when we are adding a small number of\n> - commits on top.\n> + to run prepare_commit_graph(). This is necessary because otherwise\n> + the topo_levels slab is not initialized. As we compute topo_levels for\n> + the new commits, we iterate further into the lower layers since the\n> + first visit to each commit looks as though the topo_level is not\n> + populated.\n> \n> By properly initializing the topo_slab, we fix the previously broken\n> case of a split commit graph where a base layer has the\n> \n> -- \n> gitgitgadget\n"}]}