Re: [PATCH v3 04/11] commit-graph: consolidate compare_commits_by_gen

[jnareb@gmail.com (Jakub Narębski)]

"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,
-- 
Jakub Narębski