{"thread":{"id":"57564","subject":"[PATCH] Use reverse_commit_list helper function for in-place list reversal","startedAt":"2022-03-15T19:42:01Z","lastAt":"2022-03-16T19:44:32Z","messageCount":5,"participants":["Jayati Shrivastava via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"451422","messageId":"pull.1177.git.1647373314457.gitgitgadget@gmail.com","threadId":"57564","inReplyTo":null,"subject":"[PATCH] Use reverse_commit_list helper function for in-place list reversal","fromName":"Jayati Shrivastava via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-15T19:41:54Z","receivedAt":"2022-03-15T19:42:01Z","isPatch":true,"sender":{"key":"gaurijove@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48182195?v=4"},"body":"From: JAYATI SHRIVASTAVA \n\nHere, a reverse copy of a list is being created by iterating\nover the list after which the original list is discarded.\nInstead of creating a new allocation, we can reverse the\noriginal list in-place using the reverse_commit_list helper\nfunction.\n\nSigned-off-by: Jayati Shrivastava \n---\n Use reverse_commit_list helper function for in-place list reversal\n \n This patch addresses https://github.com/gitgitgadget/git/issues/1156 . I\n have left builtin/merge.c unmodified since in its case, the original\n list is needed separately from the reverse copy.\n \n (Please excuse if you are receiving this patch again. I had previously\n sent it using git send-email but for some reason the patches are not\n getting delivered to the mailing list despite correctly passing the\n --to/--cc/--in-reply-to options.)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1177%2Fvictorphoenix3%2Freverse-list-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1177/victorphoenix3/reverse-list-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1177\n\n sequencer.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 35006c0cea6..bccbb9e3522 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3749,7 +3749,7 @@ static int do_merge(struct repository *r,\n \tint run_commit_flags = 0;\n \tstruct strbuf ref_name = STRBUF_INIT;\n \tstruct commit *head_commit, *merge_commit, *i;\n-\tstruct commit_list *bases, *j, *reversed = NULL;\n+\tstruct commit_list *bases, *j;\n \tstruct commit_list *to_merge = NULL, **tail = &to_merge;\n \tconst char *strategy = !opts->xopts_nr &&\n \t\t(!opts->strategy ||\n@@ -3984,9 +3984,7 @@ static int do_merge(struct repository *r,\n \t\t git_path_merge_head(r), 0);\n \twrite_message(\"no-ff\", 5, git_path_merge_mode(r), 0);\n \n-\tfor (j = bases; j; j = j->next)\n-\t\tcommit_list_insert(j->item, &reversed);\n-\tfree_commit_list(bases);\n+\tbases = reverse_commit_list(bases);\n \n \trepo_read_index(r);\n \tinit_merge_options(&o, r);\n@@ -4002,10 +4000,10 @@ static int do_merge(struct repository *r,\n \t\t * update the index and working copy immediately.\n \t\t */\n \t\tret = merge_ort_recursive(&o,\n-\t\t\t\t\t head_commit, merge_commit, reversed,\n+\t\t\t\t\t head_commit, merge_commit, bases,\n \t\t\t\t\t &i);\n \t} else {\n-\t\tret = merge_recursive(&o, head_commit, merge_commit, reversed,\n+\t\tret = merge_recursive(&o, head_commit, merge_commit, bases,\n \t\t\t\t &i);\n \t}\n \tif (ret <= 0)\n\nbase-commit: b896f729e240d250cf56899e6a0073f6aa469f5d\n-- \ngitgitgadget\n"},{"id":"451451","messageId":"xmqq4k3yddz4.fsf@gitster.g","threadId":"57564","inReplyTo":"pull.1177.git.1647373314457.gitgitgadget@gmail.com","subject":"Re: [PATCH] Use reverse_commit_list helper function for in-place list reversal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-15T22:53:51Z","receivedAt":"2022-03-15T22:53:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jayati Shrivastava via GitGitGadget\" \nwrites:\n\n> Subject: Re: [PATCH] Use reverse_commit_list helper function for in-place list reversal\n\nPerhaps\n\n Subject: [PATCH] sequencer: use reverse_commit_list_helper()\n\ncf. Documentation/SubmittingPatches::[[summary-section]]\n\n> From: JAYATI SHRIVASTAVA \n\nThis name should match what is on Signed-off-by: line. Check your\nuser.name configuration, perhaps?\n\n> Here, a reverse copy of a list is being created by iterating\n> over the list after which the original list is discarded.\n> Instead of creating a new allocation, we can reverse the\n> original list in-place using the reverse_commit_list helper\n> function.\n\nPerhaps\n\n Instead of creating a new allocation, reverse the list in-place\n by calling the reverse_commit_list() helper.\n\ncf. Documentation/SubmittingPatches::[[imperative-mood]]\n>\n> Signed-off-by: Jayati Shrivastava \n> ---\n\n> @@ -3984,9 +3984,7 @@ static int do_merge(struct repository *r,\n> \t\t git_path_merge_head(r), 0);\n> \twrite_message(\"no-ff\", 5, git_path_merge_mode(r), 0);\n> \n> -\tfor (j = bases; j; j = j->next)\n> -\t\tcommit_list_insert(j->item, &reversed);\n> -\tfree_commit_list(bases);\n> +\tbases = reverse_commit_list(bases);\n\nIf the code in the original code that followed from here used both\n\"bases\" and \"reversed\", this would not have worked, but because the\noriginal gets rid of \"bases\", we can just reverse the list in-place\nwith reverse_commit_list() helper and reuse the same variable.\n\n> \trepo_read_index(r);\n> \tinit_merge_options(&o, r);\n> @@ -4002,10 +4000,10 @@ static int do_merge(struct repository *r,\n> \t\t * update the index and working copy immediately.\n> \t\t */\n> \t\tret = merge_ort_recursive(&o,\n> -\t\t\t\t\t head_commit, merge_commit, reversed,\n> +\t\t\t\t\t head_commit, merge_commit, bases,\n> \t\t\t\t\t &i);\n> \t} else {\n> -\t\tret = merge_recursive(&o, head_commit, merge_commit, reversed,\n> +\t\tret = merge_recursive(&o, head_commit, merge_commit, bases,\n> \t\t\t\t &i);\n> \t}\n\nLooks good.\n\nThanks.\n"},{"id":"451462","messageId":"pull.1177.v2.git.1647427356490.gitgitgadget@gmail.com","threadId":"57564","inReplyTo":"pull.1177.git.1647373314457.gitgitgadget@gmail.com","subject":"[PATCH v2] sequencer.c: use reverse_commit_list() helper","fromName":"Jayati Shrivastava via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-16T10:42:36Z","receivedAt":"2022-03-16T10:42:42Z","isPatch":true,"sender":{"key":"gaurijove@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48182195?v=4"},"body":"From: Jayati Shrivastava \n\nInstead of creating a new allocation, reverse the original\nlist in-place by calling the reverse_commit_list() helper.\nThe original code discards the list \"bases\" after storing\nits reverse copy in a newly created list \"reversed\". If the\ncode that followed from here used both \"bases\" and \"reversed\",\nthe modification would not have worked, but since the original\nlist \"bases\" gets discarded, we can simply reverse \"bases\"\nin-place with the reverse_commit_list() helper and reuse the\nsame variable in the code that follows.\n\nSigned-off-by: Jayati Shrivastava \n---\n Use reverse_commit_list helper function for in-place list reversal\n \n This patch addresses https://github.com/gitgitgadget/git/issues/1156 . I\n have left builtin/merge.c unmodified since in its case, the original\n list is needed separately from the reverse copy.\n \n (Please excuse if you are receiving this patch again. I had previously\n sent it using git send-email but for some reason the patches are not\n getting delivered to the mailing list despite correctly passing the\n --to/--cc/--in-reply-to options.)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1177%2Fvictorphoenix3%2Freverse-list-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1177/victorphoenix3/reverse-list-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1177\n\nRange-diff vs v1:\n\n 1: 8c3c6c9ed4c ! 1: f794bcd8bda Use reverse_commit_list helper function for in-place list reversal\n @@\n ## Metadata ##\n -Author: JAYATI SHRIVASTAVA \n +Author: Jayati Shrivastava \n \n ## Commit message ##\n - Use reverse_commit_list helper function for in-place list reversal\n + sequencer.c: use reverse_commit_list() helper\n \n - Here, a reverse copy of a list is being created by iterating\n - over the list after which the original list is discarded.\n - Instead of creating a new allocation, we can reverse the\n - original list in-place using the reverse_commit_list helper\n - function.\n + Instead of creating a new allocation, reverse the original\n + list in-place by calling the reverse_commit_list() helper.\n + The original code discards the list \"bases\" after storing\n + its reverse copy in a newly created list \"reversed\". If the\n + code that followed from here used both \"bases\" and \"reversed\",\n + the modification would not have worked, but since the original\n + list \"bases\" gets discarded, we can simply reverse \"bases\"\n + in-place with the reverse_commit_list() helper and reuse the\n + same variable in the code that follows.\n \n Signed-off-by: Jayati Shrivastava \n \n\n\n sequencer.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 35006c0cea6..bccbb9e3522 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3749,7 +3749,7 @@ static int do_merge(struct repository *r,\n \tint run_commit_flags = 0;\n \tstruct strbuf ref_name = STRBUF_INIT;\n \tstruct commit *head_commit, *merge_commit, *i;\n-\tstruct commit_list *bases, *j, *reversed = NULL;\n+\tstruct commit_list *bases, *j;\n \tstruct commit_list *to_merge = NULL, **tail = &to_merge;\n \tconst char *strategy = !opts->xopts_nr &&\n \t\t(!opts->strategy ||\n@@ -3984,9 +3984,7 @@ static int do_merge(struct repository *r,\n \t\t git_path_merge_head(r), 0);\n \twrite_message(\"no-ff\", 5, git_path_merge_mode(r), 0);\n \n-\tfor (j = bases; j; j = j->next)\n-\t\tcommit_list_insert(j->item, &reversed);\n-\tfree_commit_list(bases);\n+\tbases = reverse_commit_list(bases);\n \n \trepo_read_index(r);\n \tinit_merge_options(&o, r);\n@@ -4002,10 +4000,10 @@ static int do_merge(struct repository *r,\n \t\t * update the index and working copy immediately.\n \t\t */\n \t\tret = merge_ort_recursive(&o,\n-\t\t\t\t\t head_commit, merge_commit, reversed,\n+\t\t\t\t\t head_commit, merge_commit, bases,\n \t\t\t\t\t &i);\n \t} else {\n-\t\tret = merge_recursive(&o, head_commit, merge_commit, reversed,\n+\t\tret = merge_recursive(&o, head_commit, merge_commit, bases,\n \t\t\t\t &i);\n \t}\n \tif (ret <= 0)\n\nbase-commit: b896f729e240d250cf56899e6a0073f6aa469f5d\n-- \ngitgitgadget\n"},{"id":"451464","messageId":"pull.1177.v3.git.1647429623830.gitgitgadget@gmail.com","threadId":"57564","inReplyTo":"pull.1177.v2.git.1647427356490.gitgitgadget@gmail.com","subject":"[PATCH v3] sequencer.c: use reverse_commit_list() helper","fromName":"Jayati Shrivastava via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-16T11:20:23Z","receivedAt":"2022-03-16T11:20:33Z","isPatch":true,"sender":{"key":"gaurijove@gmail.com","avatar":"https://avatars.githubusercontent.com/u/48182195?v=4"},"body":"From: Jayati Shrivastava \n\nInstead of creating a new allocation, reverse the\noriginal list in-place by calling the\nreverse_commit_list() helper. The original code\ndiscards the list \"bases\" after storing its\nreverse copy in a newly created list \"reversed\".\nIf the code that followed from here used both\n\"bases\" and \"reversed\", the modification would not\nhave worked, but since the original list \"bases\"\ngets discarded, we can simply reverse \"bases\"\nin-place with the reverse_commit_list() helper and\nreuse the same variable in the code that follows.\n\nbuiltin/merge.c has been left unmodified, since in\nits case, the original list is needed separately\nfrom its reverse copy by the code.\n\nSigned-off-by: Jayati Shrivastava \n---\n Use reverse_commit_list helper function for in-place list reversal\n \n This patch address https://github.com/gitgitgadget/git/issues/1156\n Changes since v2:\n \n * updated the commit message to explain why builtin/merge.c has not\n been modified\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1177%2Fvictorphoenix3%2Freverse-list-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1177/victorphoenix3/reverse-list-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1177\n\nRange-diff vs v2:\n\n 1: f794bcd8bda ! 1: 6dd0d92096b sequencer.c: use reverse_commit_list() helper\n @@ Metadata\n ## Commit message ##\n sequencer.c: use reverse_commit_list() helper\n \n - Instead of creating a new allocation, reverse the original\n - list in-place by calling the reverse_commit_list() helper.\n - The original code discards the list \"bases\" after storing\n - its reverse copy in a newly created list \"reversed\". If the\n - code that followed from here used both \"bases\" and \"reversed\",\n - the modification would not have worked, but since the original\n - list \"bases\" gets discarded, we can simply reverse \"bases\"\n - in-place with the reverse_commit_list() helper and reuse the\n - same variable in the code that follows.\n + Instead of creating a new allocation, reverse the\n + original list in-place by calling the\n + reverse_commit_list() helper. The original code\n + discards the list \"bases\" after storing its\n + reverse copy in a newly created list \"reversed\".\n + If the code that followed from here used both\n + \"bases\" and \"reversed\", the modification would not\n + have worked, but since the original list \"bases\"\n + gets discarded, we can simply reverse \"bases\"\n + in-place with the reverse_commit_list() helper and\n + reuse the same variable in the code that follows.\n +\n + builtin/merge.c has been left unmodified, since in\n + its case, the original list is needed separately\n + from its reverse copy by the code.\n \n Signed-off-by: Jayati Shrivastava \n \n\n\n sequencer.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 35006c0cea6..bccbb9e3522 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3749,7 +3749,7 @@ static int do_merge(struct repository *r,\n \tint run_commit_flags = 0;\n \tstruct strbuf ref_name = STRBUF_INIT;\n \tstruct commit *head_commit, *merge_commit, *i;\n-\tstruct commit_list *bases, *j, *reversed = NULL;\n+\tstruct commit_list *bases, *j;\n \tstruct commit_list *to_merge = NULL, **tail = &to_merge;\n \tconst char *strategy = !opts->xopts_nr &&\n \t\t(!opts->strategy ||\n@@ -3984,9 +3984,7 @@ static int do_merge(struct repository *r,\n \t\t git_path_merge_head(r), 0);\n \twrite_message(\"no-ff\", 5, git_path_merge_mode(r), 0);\n \n-\tfor (j = bases; j; j = j->next)\n-\t\tcommit_list_insert(j->item, &reversed);\n-\tfree_commit_list(bases);\n+\tbases = reverse_commit_list(bases);\n \n \trepo_read_index(r);\n \tinit_merge_options(&o, r);\n@@ -4002,10 +4000,10 @@ static int do_merge(struct repository *r,\n \t\t * update the index and working copy immediately.\n \t\t */\n \t\tret = merge_ort_recursive(&o,\n-\t\t\t\t\t head_commit, merge_commit, reversed,\n+\t\t\t\t\t head_commit, merge_commit, bases,\n \t\t\t\t\t &i);\n \t} else {\n-\t\tret = merge_recursive(&o, head_commit, merge_commit, reversed,\n+\t\tret = merge_recursive(&o, head_commit, merge_commit, bases,\n \t\t\t\t &i);\n \t}\n \tif (ret <= 0)\n\nbase-commit: b896f729e240d250cf56899e6a0073f6aa469f5d\n-- \ngitgitgadget\n"},{"id":"451505","messageId":"xmqqee31lm1w.fsf@gitster.g","threadId":"57564","inReplyTo":"pull.1177.v3.git.1647429623830.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] sequencer.c: use reverse_commit_list() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-16T19:44:27Z","receivedAt":"2022-03-16T19:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jayati Shrivastava via GitGitGadget\" \nwrites:\n\n> From: Jayati Shrivastava \n>\n> Instead of creating a new allocation, reverse the\n> original list in-place by calling the\n> reverse_commit_list() helper.\n\nOK. I recall that v2 wrapped with longer lines that was easier to\nread.\n\n> The original code\n> discards the list \"bases\" after storing its\n> reverse copy in a newly created list \"reversed\".\n> If the code that followed from here used both\n> \"bases\" and \"reversed\", the modification would not\n> have worked, but since the original list \"bases\"\n> gets discarded, we can simply reverse \"bases\"\n> in-place with the reverse_commit_list() helper and\n> reuse the same variable in the code that follows.\n\nI am 30% surprised to see this in the log message ;-). There is\nnothing incorrect in the description per-se, but it is something I\nwould expect in a review that reads a patch and follows along\nthinking aloud to see if the code makes sense, or below \"---\", but\nit does explain why the patch chose to lose the extra variable, so\nprobably it is OK.\n\n> builtin/merge.c has been left unmodified, since in its case, the\n> original list is needed separately from its reverse copy by the\n> code.\n\nGood.\n\nThe patch text looks good, too.\n\nThanks.\n"}]}