Re: [PATCH 1/6] commit-graph: fix regression when computing bloom filter
flat view
From: Taylor Blau <hidden>
Date: 2020-07-28 15:28:50
On Tue, Jul 28, 2020 at 09:13:46AM +0000, Abhishek Kumar via GitGitGadget wrote:
From: Abhishek Kumar <redacted> With 3d112755 (commit-graph: examine commits by generation number), Git knew to sort by generation number before examining the diff when not using pack order. c49c82aa (commit: move members graph_pos, generation to a slab, 2020-06-17) moved generation number into a slab and introduced a helper which returns GENERATION_NUMBER_INFINITY when writing the graph. Sorting is no longer useful and essentially reverts the earlier commit.
This last sentence is slightly confusing. Do you think it would be more clear if you said elaborated a bit? Perhaps something like: [...] commit_gen_cmp is used when writing a commit-graph to sort commits in generation order before computing Bloom filters. Since c49c82aa 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. I think the above would be a good extra paragraph in the commit message provided that you remove the sentence beginning with "Sorting is no longer useful..."
quoted hunk ↗ jump to hunk
Let's fix this by accessing generation number directly through the slab. Signed-off-by: Abhishek Kumar <redacted> --- commit-graph.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)diff --git a/commit-graph.c b/commit-graph.c index 1af68c297d..5d3c9bd23c 100644 --- a/commit-graph.c +++ b/commit-graph.c@@ -144,8 +144,9 @@ 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; +
Nit; this whitespace diff is extraneous, but it's not hurting anything either. Since it looks like you're rerolling anyway, it would be good to just get rid of it. Otherwise this fix makes sense to me.
/* lower generation commits first */ if (generation_a < generation_b) return -1; -- gitgitgadget
Thanks, Taylor