| Message ID | 6a0cde983d9ed20f043a4977313d714154602012.1597509583.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement Corrected Commit Date | expand |
On 8/15/2020 12:39 PM, Abhishek Kumar via GitGitGadget wrote: > From: Abhishek Kumar <abhishekkumar8222@gmail.com> > > 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. > > Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> > Reviewed-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> Whoops! Be sure to add the "Reviewed-by: above the "Signed-off-by" line(s) so re-signing off doesn't add a duplicate like this. Thanks, -Stolee
"Abhishek Kumar via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Abhishek Kumar <abhishekkumar8222@gmail.com> > > 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@gmail.com> > Reviewed-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com> > --- > 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,
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); + + /* older commits first */ + 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); #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; -} - 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 */ - if (generation_a < generation_b) - return 1; - else if (generation_a > generation_b) - return -1; + if (ret_val) + return -ret_val; /* use date as a heuristic when generations are equal */ if (a->date < b->date)