Re: [PATCH v3 01/11] commit-graph: fix regression when computing bloom filter

[jnareb@gmail.com (Jakub Narębski)]

"Abhishek Kumar via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: [PATCH v3 01/11] commit-graph: fix regression when computing bloom filter

s/bloom filter/Bloom filters/

> From: Abhishek Kumar <abhishekkumar8222@gmail.com>
>
> commit_gen_cmp is used when writing a commit-graph to sort commits in
> generation order before computing Bloom filters. Since c49c82aa (commit:
> move members graph_pos, generation to a slab, 2020-06-17) made it so
> that 'commit_graph_generation()' returns 'GENERATION_NUMBER_INFINITY'
> during writing, we cannot call it within this function. Instead, access
> the generation number directly through the slab (i.e., by calling
> 'commit_graph_data_at(c)->generation') in order to access it while
> writing.

Two things that might not be obvious from the commit message:

- Is commit_gen_cmp in commit-graph.c used by anything but writing
  Bloom filters for changed paths?

- That the generation number is computed during `commit-graph write`
  before computing Bloom filters.

Also, after this series 'generation' would be generation number v2, that
is corrected commit date, and not v1, that is topological levels.  We
should check, just in case, that it does not lead to significant
performance regression for `git commit-graph write --reachable <...>`
case (the one that uses commit_gen_cmp sort).

>
> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
> ---
>  commit-graph.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index e51c91dd5b..ace7400a1a 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -144,8 +144,8 @@ static int commit_gen_cmp(const void *va, const void *vb)
>  	const struct commit *a = *(const struct commit **)va;
>  	const struct commit *b = *(const struct commit **)vb;
>
> -	uint32_t generation_a = commit_graph_generation(a);
> -	uint32_t generation_b = commit_graph_generation(b);
> +	uint32_t generation_a = commit_graph_data_at(a)->generation;
> +	uint32_t generation_b = commit_graph_data_at(b)->generation;

Nice and easy.

>  	/* lower generation commits first */
>  	if (generation_a < generation_b)
>  		return -1;

Best,
-- 
Jakub Narębski