On Tue, Jul 28, 2020 at 11:28:44AM -0400, Taylor Blau wrote: > On Tue, Jul 28, 2020 at 09:13:46AM +0000, Abhishek Kumar via GitGitGadget wrote: > > From: Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> > > > > 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. > Thanks! That is clearer. Will change. > 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..." > > > Let's fix this by accessing generation number directly through the slab. > > > > Signed-off-by: Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> > > --- > > 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
