diff mbox series

[5/5] commit-graph: prepare commit graph

Message ID dddeec30ebfc6d8403cc10395a9d0c331d1f7ad3.1612199707.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Generation Number v2: Fix a tricky split graph bug | expand

Commit Message

Derrick Stolee Feb. 1, 2021, 5:15 p.m. UTC 
From: Derrick Stolee <dstolee@microsoft.com>

Before checking if the repository has a commit-graph loaded, be sure
to run prepare_commit_graph(). This is necessary because without this
instance we would not initialize the topo_levels slab for each of the
struct commit_graphs in the chain before we start to parse the
commits. This leads to possibly recomputing the topological levels for
commits in lower layers even when we are adding a small number of
commits on top.

By properly initializing the topo_slab, we fix the previously broken
case of a split commit graph where a base layer has the
generation_data_overflow chunk.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit-graph.c          | 10 ++--------
 t/t5318-commit-graph.sh |  2 +-
 2 files changed, 3 insertions(+), 9 deletions(-)

Comments

Taylor Blau Feb. 1, 2021, 6:25 p.m. UTC | #1 
On Mon, Feb 01, 2021 at 05:15:07PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Before checking if the repository has a commit-graph loaded, be sure
> to run prepare_commit_graph(). This is necessary because without this
> instance we would not initialize the topo_levels slab for each of the
> struct commit_graphs in the chain before we start to parse the
> commits. This leads to possibly recomputing the topological levels for
> commits in lower layers even when we are adding a small number of
> commits on top.

I think that the situation arises as follows:

  - Prior to this patch, we didn't always prepare the commit-graph, so
    the topo_levels slab wasn't initialized either.

  - Then we try and compute the topo levels for new commits (which are
    likely to be decendants of older ones that are in commit-graphs and
    have their topo-levels already computed).

  - But in the course of computing topo-levels for the new commits, we
    have to recur on their ancestors, which *look* like they have
    uncomputed topo levels.

That all makes sense, but it may be worth cutting and pasting what I
wrote into your patch to make it clearer.

> By properly initializing the topo_slab, we fix the previously broken
> case of a split commit graph where a base layer has the
> generation_data_overflow chunk.

Makes sense.

> -test_expect_failure 'lower layers have overflow chunk' '
> +test_expect_success 'lower layers have overflow chunk' '
>  	cd "$TRASH_DIRECTORY/full" &&
>  	UNIX_EPOCH_ZERO="@0 +0000" &&
>  	FUTURE_DATE="@2147483646 +0000" &&

Terrific :-).

Thanks,
Taylor
diff mbox series

Patch

diff --git a/commit-graph.c b/commit-graph.c
index 08148dd17f1..858fa5594e3 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2309,6 +2309,7 @@  int write_commit_graph(struct object_directory *odb,
 	init_topo_level_slab(&topo_levels);
 	ctx->topo_levels = &topo_levels;
 
+	prepare_commit_graph(ctx->r);
 	if (ctx->r->objects->commit_graph) {
 		struct commit_graph *g = ctx->r->objects->commit_graph;
 
@@ -2322,7 +2323,6 @@  int write_commit_graph(struct object_directory *odb,
 		ctx->changed_paths = 1;
 	if (!(flags & COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS)) {
 		struct commit_graph *g;
-		prepare_commit_graph_one(ctx->r, ctx->odb);
 
 		g = ctx->r->objects->commit_graph;
 
@@ -2334,10 +2334,7 @@  int write_commit_graph(struct object_directory *odb,
 	}
 
 	if (ctx->split) {
-		struct commit_graph *g;
-		prepare_commit_graph(ctx->r);
-
-		g = ctx->r->objects->commit_graph;
+		struct commit_graph *g = ctx->r->objects->commit_graph;
 
 		while (g) {
 			ctx->num_commit_graphs_before++;
@@ -2361,9 +2358,6 @@  int write_commit_graph(struct object_directory *odb,
 
 	ctx->approx_nr_objects = approximate_object_count();
 
-	if (ctx->append)
-		prepare_commit_graph_one(ctx->r, ctx->odb);
-
 	if (ctx->append && ctx->r->objects->commit_graph) {
 		struct commit_graph *g = ctx->r->objects->commit_graph;
 		for (i = 0; i < g->num_commits; i++) {
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 2cf29f425a0..5b15f1aa0f6 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -446,7 +446,7 @@  test_expect_success 'warn on improper hash version' '
 	)
 '
 
-test_expect_failure 'lower layers have overflow chunk' '
+test_expect_success 'lower layers have overflow chunk' '
 	cd "$TRASH_DIRECTORY/full" &&
 	UNIX_EPOCH_ZERO="@0 +0000" &&
 	FUTURE_DATE="@2147483646 +0000" &&