Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 2, 2023, 23:06 UTC
- Message-ID
- <xmqq1qn7sm49.fsf@gitster.g>
- In-Reply-To
- <20230202165137.118741-1-five231003@gmail.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?
Show 32 quoted lines
> 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"))