"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
