[v3,4/8] commit-graph: return generation from memory

Commit Message

Derrick Stolee March 15, 2023, 5:45 p.m. UTC

From: Derrick Stolee <[e-mail]>

The commit_graph_generation() method used to report a value of
GENERATION_NUMBER_INFINITY if the commit_graph_data_slab had an instance
for the given commit but the graph_pos indicated the commit was not in
the commit-graph file.

Instead, trust the 'generation' member if the commit has a value in the
slab _and_ the 'generation' member is non-zero. Otherwise, treat it as
GENERATION_NUMBER_INFINITY.

This only makes a difference for a very old case for the commit-graph:
the very first Git release to write commit-graph files wrote zeroes in
the topological level positions. If we are parsing a commit-graph with
all zeroes, those commits will now appear to have
GENERATION_NUMBER_INFINITY (as if they were not parsed from the
commit-graph).

I attempted several variations to work around the need for providing an
uninitialized 'generation' member, but this was the best one I found. It
does require a change to a verification test in t5318 because it reports
a different error than the one about non-zero generation numbers.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 commit-graph.c          | 8 +++-----
 t/t5318-commit-graph.sh | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

Comments

Jonathan Tan March 15, 2023, 10:58 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:
> From: Derrick Stolee <[e-mail]>
> 
> The commit_graph_generation() method used to report a value of
> GENERATION_NUMBER_INFINITY if the commit_graph_data_slab had an instance
> for the given commit but the graph_pos indicated the commit was not in
> the commit-graph file.
> 
> Instead, trust the 'generation' member if the commit has a value in the
> slab _and_ the 'generation' member is non-zero. Otherwise, treat it as
> GENERATION_NUMBER_INFINITY.

I would replace "Instead" with "However, a future commit intends to
compute and use commit generation numbers even for commits that are
not in the commit-graph file (and thus have no graph_pos). Therefore,
we need a new criterion for deciding if a generation number can be
trusted:" (or something to that effect).

> This only makes a difference for a very old case for the commit-graph:
> the very first Git release to write commit-graph files wrote zeroes in
> the topological level positions. If we are parsing a commit-graph with
> all zeroes, those commits will now appear to have
> GENERATION_NUMBER_INFINITY (as if they were not parsed from the
> commit-graph).
> 
> I attempted several variations to work around the need for providing an
> uninitialized 'generation' member, but this was the best one I found. It
> does require a change to a verification test in t5318 because it reports
> a different error than the one about non-zero generation numbers.

Thanks for investigating, and I think the method in this patch would
work. As you have stated, this only affects the commit-graph files that
once upon a time were written with no generation numbers, and this patch
makes those behave as if there were no generation numbers in the first
place (which is exactly what happened).

Patch

diff --git a/commit-graph.c b/commit-graph.c
index deccf984a0d..b4da4e05067 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -116,12 +116,10 @@  timestamp_t commit_graph_generation(const struct commit *c)
 	struct commit_graph_data *data =
 		commit_graph_data_slab_peek(&commit_graph_data_slab, c);
 
-	if (!data)
-		return GENERATION_NUMBER_INFINITY;
-	else if (data->graph_pos == COMMIT_NOT_FROM_GRAPH)
-		return GENERATION_NUMBER_INFINITY;
+	if (data && data->generation)
+		return data->generation;
 
-	return data->generation;
+	return GENERATION_NUMBER_INFINITY;
 }
 
 static struct commit_graph_data *commit_graph_data_at(const struct commit *c)
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 049c5fc8ead..b6e12115786 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -630,7 +630,7 @@  test_expect_success 'detect incorrect generation number' '
 
 test_expect_success 'detect incorrect generation number' '
 	corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_GENERATION "\01" \
-		"non-zero generation number"
+		"commit-graph generation for commit"
 '
 
 test_expect_success 'detect incorrect commit date' '