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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 15, 2023, 22:58 UTC
Message-ID: <20230315225815.170437-1-jonathantanmy@google.com>
In-Reply-To: <abd3e7a67beb80ef687253ddb8bb5fe7a769357f.1678902343.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
> 
> 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).

Show 11 quoted lines

> 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).