Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal

[Junio C Hamano <gitster@pobox.com>]

Kousik Sanagavarapu <five231003@gmail.com> writes:

> Instead of manually doing an in-place list reversal, use the helper
> function reverse_commit_list(), hence improving code readability.

But isn't reverse_commit_list() destructive in that it reuses the
elements on the original list to create the reversed list?  The
original code creates a new and separate list, so even when
try_merge_strategy() is called many times, its "common" parameter
given by the caller stays the same.

What happens in the second and subsequent call to
try_merge_strategy() in your updated version?  The element pointed
at by the "common" variable, which is the first element of the list
in the first call, gets its .next member NULLed by calling
reverse_commit_list().  The rest of the try_merge_strategy()
function in its first call might work the same way as before (as
long as it does not use "common" and only uses "reversed"; I did not
check), but because the "common" the caller passed is now a single
element list that consists of itself, and all the other elements are
lost.  The caller uses that same "common" to call try_merge_strategy()
again, with a different strategy.  This second call is getting a common
ancestor list that was already broken by the first call, no?

> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
>
> This patch addresses the issue #1156(Use reverse_commit_list() in more
> places) on gitgitgadget. I also would like to submit this patch as the microproject for
> GSoC 2023.
>
>  builtin/merge.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 5900b81729..4503dbfeb3 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -736,7 +736,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>  		struct commit *result;
>  		struct commit_list *reversed = NULL;
>  		struct merge_options o;
> -		struct commit_list *j;
>  
>  		if (remoteheads->next) {
>  			error(_("Not handling anything other than two heads merge."));
> @@ -757,8 +756,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>  		o.branch1 = head_arg;
>  		o.branch2 = merge_remote_util(remoteheads->item)->name;
>  
> -		for (j = common; j; j = j->next)
> -			commit_list_insert(j->item, &reversed);
> +		reversed = reverse_commit_list(common);
>  
>  		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
>  		if (!strcmp(strategy, "ort"))