[PATCH v2 2/6] commit-graph: always parse before commit_graph_data_at()
- From
Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Feb 2, 2021, 03:01 UTC
- Message-ID
- <454b183b9ba502da7f40dc36aaa95cc3d12b5c2f.1612234883.git.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.850.v2.git.1612234883.gitgitgadget@gmail.com>
From: Derrick Stolee <dstolee@microsoft.com>
There is a subtle failure happening when computing corrected commit dates with --split enabled. It requires a base layer needing the generation_data_overflow chunk. Then, the next layer on top erroneously thinks it needs an overflow chunk due to a bug leading to recalculating all reachable generation numbers. The output of the failure is
BUG: commit-graph.c:1912: expected to write 8 bytes to chunk 47444f56, but wrote 0 instead
These "expected" 8 bytes are due to re-computing the corrected commit date for the lower layer but the new layer does not need any overflow.
Add a test to t5318-commit-graph.sh that demonstrates this bug. However, it does not trigger consistently with the existing code.
The generation number data is stored in a slab and accessed by commit_graph_data_at(). This data is initialized when parsing a commit, but is otherwise used assuming it has been populated. The loop in compute_generation_numbers() did not enforce that all reachable commits were parsed and had correct values. This could lead to some problems when writing a commit-graph with corrected commit dates based on a commit-graph without them.
It has been difficult to identify the issue here because it was so hard to reproduce. It relies on this uninitialized data having a non-zero value, but also on specifically in a way that overwrites the existing data.
This patch adds the extra parse to ensure the data is filled before we compute the generation number of a commit. This triggers the new test to fail because the generation number overflow count does not match between this computation and the write for that chunk.
The actual fix will follow as the next few changes.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- commit-graph.c | 16 ++++++++++++---- t/t5318-commit-graph.sh | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c index 03e5a987968..edbb3a0f2cc 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1193,7 +1193,9 @@ static int write_graph_chunk_generation_data(struct hashfile *f, for (i = 0; i < ctx->commits.nr; i++) { struct commit *c = ctx->commits.list[i]; - timestamp_t offset = commit_graph_data_at(c)->generation - c->date; + timestamp_t offset; + repo_parse_commit(ctx->r, c); + offset = commit_graph_data_at(c)->generation - c->date; display_progress(ctx->progress, ++ctx->progress_cnt); if (offset > GENERATION_NUMBER_V2_OFFSET_MAX) { @@ -1444,15 +1446,20 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) _("Computing commit graph generation numbers"), ctx->commits.nr); for (i = 0; i < ctx->commits.nr; i++) { - uint32_t level = *topo_level_slab_at(ctx->topo_levels, ctx->commits.list[i]); - timestamp_t corrected_commit_date = commit_graph_data_at(ctx->commits.list[i])->generation; + struct commit *c = ctx->commits.list[i]; + uint32_t level; + timestamp_t corrected_commit_date; + + repo_parse_commit(ctx->r, c); + level = *topo_level_slab_at(ctx->topo_levels, c); + corrected_commit_date = commit_graph_data_at(c)->generation; display_progress(ctx->progress, i + 1); if (level != GENERATION_NUMBER_ZERO && corrected_commit_date != GENERATION_NUMBER_ZERO) continue; - commit_list_insert(ctx->commits.list[i], &list); + commit_list_insert(c, &list); while (list) { struct commit *current = list->item; struct commit_list *parent; @@ -1461,6 +1468,7 @@ static void compute_generation_numbers(struct write_commit_graph_context *ctx) timestamp_t max_corrected_commit_date = 0; for (parent = current->parents; parent; parent = parent->next) { + repo_parse_commit(ctx->r, parent->item); level = *topo_level_slab_at(ctx->topo_levels, parent->item); corrected_commit_date = commit_graph_data_at(parent->item)->generation; diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index fa27df579a5..2cf29f425a0 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh @@ -446,6 +446,27 @@ test_expect_success 'warn on improper hash version' ' ) ' +test_expect_failure 'lower layers have overflow chunk' ' + cd "$TRASH_DIRECTORY/full" && + UNIX_EPOCH_ZERO="@0 +0000" && + FUTURE_DATE="@2147483646 +0000" && + rm -f .git/objects/info/commit-graph && + test_commit --date "$FUTURE_DATE" future-1 && + test_commit --date "$UNIX_EPOCH_ZERO" old-1 && + git commit-graph write --reachable && + test_commit --date "$FUTURE_DATE" future-2 && + test_commit --date "$UNIX_EPOCH_ZERO" old-2 && + git commit-graph write --reachable --split=no-merge && + test_commit extra && + git commit-graph write --reachable --split=no-merge && + git commit-graph write --reachable && + graph_read_expect 16 "generation_data generation_data_overflow extra_edges" && + mv .git/objects/info/commit-graph commit-graph-upgraded && + git commit-graph write --reachable && + graph_read_expect 16 "generation_data generation_data_overflow extra_edges" && + test_cmp .git/objects/info/commit-graph commit-graph-upgraded +' + # the verify tests below expect the commit-graph to contain # exactly the commits reachable from the commits/8 branch. # If the file changes the set of commits in the list, then the
-- gitgitgadget