Re: [PATCH v3 04/11] commit-graph: consolidate compare_commits_by_gen
From: Jakub Narębski <hidden>
Date: 2020-08-21 11:05:24
"Abhishek Kumar via GitGitGadget" [off-list ref] writes:
From: Abhishek Kumar <redacted> 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...
quoted hunk
Signed-off-by: Abhishek Kumar <redacted> Reviewed-by: Taylor Blau <redacted> Signed-off-by: Abhishek Kumar <redacted> --- 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...
quoted hunk
+ 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.
quoted hunk
#endifdiff --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.
quoted hunk
- 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