"Abhishek Kumar via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> > > Comparing commits by generation has been independently defined twice, in > commit-reach and commit. Let's simplify the implementation by moving > compare_commits_by_gen() to commit-graph. All right, seems reasonable. Though it might be not obvious that the second repetition of code comparing commits by generation is part of commit.c's compare_commits_by_gen_then_commit_date(). Is't it micro-pessimization though, or can the compiler inline function across different files? On the other hand it reduces code duplication... > > Signed-off-by: Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> > Reviewed-by: Taylor Blau <me@xxxxxxxxxxxx> > Signed-off-by: Abhishek Kumar <abhishekkumar8222@xxxxxxxxx> > --- > commit-graph.c | 15 +++++++++++++++ > commit-graph.h | 2 ++ > commit-reach.c | 15 --------------- > commit.c | 9 +++------ > 4 files changed, 20 insertions(+), 21 deletions(-) > > diff --git a/commit-graph.c b/commit-graph.c > index af8d9cc45e..fb6e2bf18f 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -112,6 +112,21 @@ uint32_t commit_graph_generation(const struct commit *c) > return data->generation; > } > > +int compare_commits_by_gen(const void *_a, const void *_b) > +{ > + const struct commit *a = _a, *b = _b; > + const uint32_t generation_a = commit_graph_generation(a); > + const uint32_t generation_b = commit_graph_generation(b); All right, this function used protected access to generation number of a commit, that is it correctly handles the case where commit '_a' and/or '_b' are new enough to be not present in the commit graph. That is why we cannot simply use commit_gen_cmp(), that is the function used for sorting during `git commit-graph write --reachable --changed-paths`, because after 1st patch it access the slab directly. > + > + /* older commits first */ Nice! Thanks for adding this comment. Though it might be good idea to add this comment also to the header file, commit-graph.h, because the fact that compare_commits_by_gen() and compare_commits_by_gen_then_commit_date() sort in different order is not something that we can see from their names. Well, they have slightly different sigatures... > + if (generation_a < generation_b) > + return -1; > + else if (generation_a > generation_b) > + return 1; > + > + return 0; > +} > + > static struct commit_graph_data *commit_graph_data_at(const struct commit *c) > { > unsigned int i, nth_slab; > diff --git a/commit-graph.h b/commit-graph.h > index 09a97030dc..701e3d41aa 100644 > --- a/commit-graph.h > +++ b/commit-graph.h > @@ -146,4 +146,6 @@ struct commit_graph_data { > */ > uint32_t commit_graph_generation(const struct commit *); > uint32_t commit_graph_position(const struct commit *); > + > +int compare_commits_by_gen(const void *_a, const void *_b); All right. > #endif > diff --git a/commit-reach.c b/commit-reach.c > index efd5925cbb..c83cc291e7 100644 > --- a/commit-reach.c > +++ b/commit-reach.c > @@ -561,21 +561,6 @@ int commit_contains(struct ref_filter *filter, struct commit *commit, > return repo_is_descendant_of(the_repository, commit, list); > } > > -static int compare_commits_by_gen(const void *_a, const void *_b) > -{ > - const struct commit *a = *(const struct commit * const *)_a; > - const struct commit *b = *(const struct commit * const *)_b; > - > - uint32_t generation_a = commit_graph_generation(a); > - uint32_t generation_b = commit_graph_generation(b); > - > - if (generation_a < generation_b) > - return -1; > - if (generation_a > generation_b) > - return 1; > - return 0; > -} All right, commit-reach.c includes commit-graph.h, so now it simply uses compare_commits_by_gen() that was copied to commit-graph.c. > - > int can_all_from_reach_with_flag(struct object_array *from, > unsigned int with_flag, > unsigned int assign_flag, > diff --git a/commit.c b/commit.c > index 4ce8cb38d5..bd6d5e587f 100644 > --- a/commit.c > +++ b/commit.c > @@ -731,14 +731,11 @@ int compare_commits_by_author_date(const void *a_, const void *b_, > int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused) > { > const struct commit *a = a_, *b = b_; > - const uint32_t generation_a = commit_graph_generation(a), > - generation_b = commit_graph_generation(b); > + int ret_val = compare_commits_by_gen(a_, b_); > > /* newer commits first */ Maybe this comment should be put in the header file, near this functionn declaration? > - if (generation_a < generation_b) > - return 1; > - else if (generation_a > generation_b) > - return -1; > + if (ret_val) > + return -ret_val; All right, this handles reversed sorting order of compare_commits_by_gen(). > > /* use date as a heuristic when generations are equal */ > if (a->date < b->date) Best, -- Jakub Narębski