Use reverse_commit_list helper function for in-place list reversal

From: JAYATI SHRIVASTAVA <[email protected]>

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.

---
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(-)

index 35006c0cea6..bccbb9e3522 100644
@@ -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)

base-commit: b896f729e240d250cf56899e6a0073f6aa469f5d

gitgitgadget

"Jayati Shrivastava via GitGitGadget" <[email protected]>

Perhaps

Subject: [PATCH] sequencer: use reverse_commit_list_helper()

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

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

Perhaps

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

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

From: Jayati Shrivastava <[email protected]>

Instead of creating a new allocation, reverse the original list in-place by calling the reverse_commit_list() helper. The original code discards the list "bases" after storing its reverse copy in a newly created list "reversed". If the code that followed from here used both "bases" and "reversed", the modification would not have worked, but since the original list "bases" gets discarded, we can simply reverse "bases" in-place with the reverse_commit_list() helper and reuse the same variable in the code that follows.

---
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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1177/victorphoenix3/reverse-list-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1177

Range-diff vs v1:

1: 8c3c6c9ed4c ! 1: f794bcd8bda Use reverse_commit_list helper function for in-place list reversal
@@
## Metadata ##
-Author: JAYATI SHRIVASTAVA <[email protected]>
+Author: Jayati Shrivastava <[email protected]>

## Commit message ##
- Use reverse_commit_list helper function for in-place list reversal
+ sequencer.c: use reverse_commit_list() helper

- 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.
+ Instead of creating a new allocation, reverse the original
+ list in-place by calling the reverse_commit_list() helper.
+ The original code discards the list "bases" after storing
+ its reverse copy in a newly created list "reversed". If the
+ code that followed from here used both "bases" and "reversed",
+ the modification would not have worked, but since the original
+ list "bases" gets discarded, we can simply reverse "bases"
+ in-place with the reverse_commit_list() helper and reuse the
+ same variable in the code that follows.

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

index 35006c0cea6..bccbb9e3522 100644
@@ -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)

base-commit: b896f729e240d250cf56899e6a0073f6aa469f5d

gitgitgadget

From: Jayati Shrivastava <[email protected]>

Instead of creating a new allocation, reverse the original list in-place by calling the reverse_commit_list() helper. The original code discards the list "bases" after storing its reverse copy in a newly created list "reversed". If the code that followed from here used both "bases" and "reversed", the modification would not have worked, but since the original list "bases" gets discarded, we can simply reverse "bases" in-place with the reverse_commit_list() helper and reuse the same variable in the code that follows.

builtin/merge.c has been left unmodified, since in its case, the original list is needed separately from its reverse copy by the code.

---
Use reverse_commit_list helper function for in-place list reversal

This patch address https://github.com/gitgitgadget/git/issues/1156
Changes since v2:

* updated the commit message to explain why builtin/merge.c has not been modified

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

Range-diff vs v2:

1: f794bcd8bda ! 1: 6dd0d92096b sequencer.c: use reverse_commit_list() helper
@@ Metadata
## Commit message ##
sequencer.c: use reverse_commit_list() helper

- Instead of creating a new allocation, reverse the original
- list in-place by calling the reverse_commit_list() helper.
- The original code discards the list "bases" after storing
- its reverse copy in a newly created list "reversed". If the
- code that followed from here used both "bases" and "reversed",
- the modification would not have worked, but since the original
- list "bases" gets discarded, we can simply reverse "bases"
- in-place with the reverse_commit_list() helper and reuse the
- same variable in the code that follows.
+ Instead of creating a new allocation, reverse the
+ original list in-place by calling the
+ reverse_commit_list() helper. The original code
+ discards the list "bases" after storing its
+ reverse copy in a newly created list "reversed".
+ If the code that followed from here used both
+ "bases" and "reversed", the modification would not
+ have worked, but since the original list "bases"
+ gets discarded, we can simply reverse "bases"
+ in-place with the reverse_commit_list() helper and
+ reuse the same variable in the code that follows.
+
+ builtin/merge.c has been left unmodified, since in
+ its case, the original list is needed separately
+ from its reverse copy by the code.

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

index 35006c0cea6..bccbb9e3522 100644
@@ -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)

base-commit: b896f729e240d250cf56899e6a0073f6aa469f5d

gitgitgadget

"Jayati Shrivastava via GitGitGadget" <[email protected]>

OK. I recall that v2 wrapped with longer lines that was easier to read.

I am 30% surprised to see this in the log message ;-). There is nothing incorrect in the description per-se, but it is something I would expect in a review that reads a patch and follows along thinking aloud to see if the code makes sense, or below "---", but it does explain why the patch chose to lose the extra variable, so probably it is OK.

Good.

The patch text looks good, too.

Thanks.