Use reverse_commit_list helper function for in-place list reversal

Commit Message

Jayati Shrivastava March 15, 2022, 7:41 p.m. UTC

From: JAYATI SHRIVASTAVA <[e-mail]>

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.

Signed-off-by: Jayati Shrivastava <[e-mail]>
---
    Use reverse_commit_list helper function for in-place list reversal
    
    This patch addresses https://github.com/gitgitgadget/git/issues/1156 . I
    have left builtin/merge.c unmodified since in its case, the original
    list is needed separately from the reverse copy.
    
    (Please excuse if you are receiving this patch again. I had previously
    sent it using git send-email but for some reason the patches are not
    getting delivered to the mailing list despite correctly passing the
    --to/--cc/--in-reply-to options.)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1177%2Fvictorphoenix3%2Freverse-list-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1177/victorphoenix3/reverse-list-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1177

 sequencer.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)


base-commit: b896f729e240d250cf56899e6a0073f6aa469f5d

Comments

Junio C Hamano March 15, 2022, 10:53 p.m. UTC | #1

"Jayati Shrivastava via GitGitGadget" <[e-mail]>
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 <[e-mail]>

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

> 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 <[e-mail]>
> ---

> @@ -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.

>  	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.

Patch

diff --git a/sequencer.c b/sequencer.c
index 35006c0cea6..bccbb9e3522 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3749,7 +3749,7 @@  static int do_merge(struct repository *r,
 	int run_commit_flags = 0;
 	struct strbuf ref_name = STRBUF_INIT;
 	struct commit *head_commit, *merge_commit, *i;
-	struct commit_list *bases, *j, *reversed = NULL;
+	struct commit_list *bases, *j;
 	struct commit_list *to_merge = NULL, **tail = &to_merge;
 	const char *strategy = !opts->xopts_nr &&
 		(!opts->strategy ||
@@ -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);
 
 	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);
 	}
 	if (ret <= 0)