Re: [PATCH] commit-reach: fix sorting commits by generation

["René Scharfe" <l.s.r@web.de>]

Am 22.10.2018 um 23:10 schrieb Thomas Gummerer:
> compare_commit_by_gen is used to sort a list of pointers to 'struct
> commit'.  The comparison function for qsort is called with pointers to
> the objects it needs to compare, so when sorting a list of 'struct
> commit *', the arguments are of type 'struct commit **'.  However,
> currently the comparison function casts it's arguments to 'struct
> commit *' and uses those, leading to out of bounds memory access and
> potentially to wrong results.  Fix that.
> 
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> 
> I noticed this by running the test suite through valgrind.  I'm not
> familiar with this code, so I'm not sure why this didn't cause any
> issues or how they would manifest, but this seems like the right fix
> for this function either way.

Right; I sent a similar patch a while ago, but it seems to have fallen
through the cracks:

https://public-inbox.org/git/d1b58614-989f-5998-6c53-c19eee409a2f@web.de/

Anyway, your implied question was discussed back then.  Derrick wrote:

   The reason to sort is to hopefully minimize the amount we walk by 
   exploring the "lower" commits first. This is a performance-only thing, 
   not a correctness issue (which is why the bug exists). Even then, it is 
   just a heuristic.

Does b6723e4671 in pu (commit-reach: fix first-parent heuristic) change
that picture?  Did a quick test and found no performance difference with
and without the fix on top, i.e. proper sorting didn't seem to matter.

>  commit-reach.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/commit-reach.c b/commit-reach.c
> index bc522d6840..9efddfd7a0 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -516,8 +516,8 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,
>  
>  static int compare_commits_by_gen(const void *_a, const void *_b)
>  {
> -	const struct commit *a = (const struct commit *)_a;
> -	const struct commit *b = (const struct commit *)_b;
> +	const struct commit *a = *(const struct commit **)_a;
> +	const struct commit *b = *(const struct commit **)_b;
>  
>  	if (a->generation < b->generation)
>  		return -1;
> 

Looks good to me.

René