Re: [PATCH] Use reverse_commit_list helper function for in-place list reversal

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2022, 22:53 UTC
Message-ID: <xmqq4k3yddz4.fsf@gitster.g>
In-Reply-To: <pull.1177.git.1647373314457.gitgitgadget@gmail.com>

"Jayati Shrivastava via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH] Use reverse_commit_list helper function for in-place list reversal

Perhaps

    Subject: [PATCH] sequencer: use reverse_commit_list_helper()

cf. Documentation/SubmittingPatches::[[summary-section]]

> From: JAYATI SHRIVASTAVA <gaurijove@gmail.com>

This name should match what is on Signed-off-by: line. Check your user.name configuration, perhaps?

Show 5 quoted lines

> Here, a reverse copy of a list is being created by iterating
> over the list after which the original list is discarded.
> Instead of creating a new allocation, we can reverse the
> original list in-place using the reverse_commit_list helper
> function.

Perhaps

    Instead of creating a new allocation, reverse the list in-place
    by calling the reverse_commit_list() helper.

cf. Documentation/SubmittingPatches::[[imperative-mood]]

>
> Signed-off-by: Jayati Shrivastava <gaurijove@gmail.com>
> ---

Show 8 quoted lines

> @@ -3984,9 +3984,7 @@ static int do_merge(struct repository *r,
>  		      git_path_merge_head(r), 0);
>  	write_message("no-ff", 5, git_path_merge_mode(r), 0);
>  
> -	for (j = bases; j; j = j->next)
> -		commit_list_insert(j->item, &reversed);
> -	free_commit_list(bases);
> +	bases = reverse_commit_list(bases);

If the code in the original code that followed from here used both "bases" and "reversed", this would not have worked, but because the original gets rid of "bases", we can just reverse the list in-place with reverse_commit_list() helper and reuse the same variable.

Show 14 quoted lines

>  	repo_read_index(r);
>  	init_merge_options(&o, r);
> @@ -4002,10 +4000,10 @@ static int do_merge(struct repository *r,
>  		 * update the index and working copy immediately.
>  		 */
>  		ret = merge_ort_recursive(&o,
> -					  head_commit, merge_commit, reversed,
> +					  head_commit, merge_commit, bases,
>  					  &i);
>  	} else {
> -		ret = merge_recursive(&o, head_commit, merge_commit, reversed,
> +		ret = merge_recursive(&o, head_commit, merge_commit, bases,
>  				      &i);
>  	}

Looks good.

Thanks.