{"thread":{"id":"59182","subject":"[GSoC][PATCH] merge: use reverse_commit_list() for list reversal","startedAt":"2023-02-02T16:53:23Z","lastAt":"2023-02-05T18:13:06Z","messageCount":10,"participants":["Kousik Sanagavarapu","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471372","messageId":"20230202165137.118741-1-five231003@gmail.com","threadId":"59182","inReplyTo":null,"subject":"[GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-02T16:51:41Z","receivedAt":"2023-02-02T16:53:23Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Instead of manually doing an in-place list reversal, use the helper\nfunction reverse_commit_list(), hence improving code readability.\n\nSigned-off-by: Kousik Sanagavarapu \n---\n\nThis patch addresses the issue #1156(Use reverse_commit_list() in more\nplaces) on gitgitgadget. I also would like to submit this patch as the microproject for\nGSoC 2023.\n\n builtin/merge.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5900b81729..4503dbfeb3 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -736,7 +736,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\tstruct commit *result;\n \t\tstruct commit_list *reversed = NULL;\n \t\tstruct merge_options o;\n-\t\tstruct commit_list *j;\n \n \t\tif (remoteheads->next) {\n \t\t\terror(_(\"Not handling anything other than two heads merge.\"));\n@@ -757,8 +756,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\to.branch1 = head_arg;\n \t\to.branch2 = merge_remote_util(remoteheads->item)->name;\n \n-\t\tfor (j = common; j; j = j->next)\n-\t\t\tcommit_list_insert(j->item, &reversed);\n+\t\treversed = reverse_commit_list(common);\n \n \t\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \t\tif (!strcmp(strategy, \"ort\"))\n-- \n2.25.1\n\n"},{"id":"471403","messageId":"xmqq1qn7sm49.fsf@gitster.g","threadId":"59182","inReplyTo":"20230202165137.118741-1-five231003@gmail.com","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-02T23:06:46Z","receivedAt":"2023-02-02T23:06:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu writes:\n\n> Instead of manually doing an in-place list reversal, use the helper\n> function reverse_commit_list(), hence improving code readability.\n\nBut isn't reverse_commit_list() destructive in that it reuses the\nelements on the original list to create the reversed list? The\noriginal code creates a new and separate list, so even when\ntry_merge_strategy() is called many times, its \"common\" parameter\ngiven by the caller stays the same.\n\nWhat happens in the second and subsequent call to\ntry_merge_strategy() in your updated version? The element pointed\nat by the \"common\" variable, which is the first element of the list\nin the first call, gets its .next member NULLed by calling\nreverse_commit_list(). The rest of the try_merge_strategy()\nfunction in its first call might work the same way as before (as\nlong as it does not use \"common\" and only uses \"reversed\"; I did not\ncheck), but because the \"common\" the caller passed is now a single\nelement list that consists of itself, and all the other elements are\nlost. The caller uses that same \"common\" to call try_merge_strategy()\nagain, with a different strategy. This second call is getting a common\nancestor list that was already broken by the first call, no?\n\n> Signed-off-by: Kousik Sanagavarapu \n> ---\n>\n> This patch addresses the issue #1156(Use reverse_commit_list() in more\n> places) on gitgitgadget. I also would like to submit this patch as the microproject for\n> GSoC 2023.\n>\n> builtin/merge.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 5900b81729..4503dbfeb3 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -736,7 +736,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n> \t\tstruct commit *result;\n> \t\tstruct commit_list *reversed = NULL;\n> \t\tstruct merge_options o;\n> -\t\tstruct commit_list *j;\n> \n> \t\tif (remoteheads->next) {\n> \t\t\terror(_(\"Not handling anything other than two heads merge.\"));\n> @@ -757,8 +756,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n> \t\to.branch1 = head_arg;\n> \t\to.branch2 = merge_remote_util(remoteheads->item)->name;\n> \n> -\t\tfor (j = common; j; j = j->next)\n> -\t\t\tcommit_list_insert(j->item, &reversed);\n> +\t\treversed = reverse_commit_list(common);\n> \n> \t\thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n> \t\tif (!strcmp(strategy, \"ort\"))\n"},{"id":"471407","messageId":"CABPp-BHE=zGT_vPW8+TZn-wqmufhVdGQT-=LXoLQkto6TMrnrA@mail.gmail.com","threadId":"59182","inReplyTo":"20230202165137.118741-1-five231003@gmail.com","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-02-02T23:46:45Z","receivedAt":"2023-02-02T23:47:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Feb 2, 2023 at 9:10 AM Kousik Sanagavarapu wrote:\n>\n> Instead of manually doing an in-place list reversal, use the helper\n> function reverse_commit_list(), hence improving code readability.\n>\n> Signed-off-by: Kousik Sanagavarapu \n> ---\n>\n> This patch addresses the issue #1156(Use reverse_commit_list() in more\n> places) on gitgitgadget. I also would like to submit this patch as the microproject for\n> GSoC 2023.\n\nLooks like this was a suggestion from me, so I'm to blame. But\nlooking at it again, I'm not sure builtin/merge.c is actually a good\ncandidate because...\n\n> builtin/merge.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 5900b81729..4503dbfeb3 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -736,7 +736,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n> struct commit *result;\n> struct commit_list *reversed = NULL;\n> struct merge_options o;\n> - struct commit_list *j;\n>\n> if (remoteheads->next) {\n> error(_(\"Not handling anything other than two heads merge.\"));\n> @@ -757,8 +756,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n> o.branch1 = head_arg;\n> o.branch2 = merge_remote_util(remoteheads->item)->name;\n>\n> - for (j = common; j; j = j->next)\n> - commit_list_insert(j->item, &reversed);\n\nThis is not an in-place list reversal; it's creating a new list that\nis reversed. \"common\" can continue to be used separately from\n\"reversed\" after this point. (And the new list, \"reversed\" is\nconsumed by merge_recursive()/merge_ort_recursive(), so there's\nnothing to free.)\n\n> + reversed = reverse_commit_list(common);\n\nThis is an in-place list reversal. If there's only one merge strategy\nbeing attempted, this may work, since \"common\" probably won't be used\nafterwards.\n\nHowever, if we have multiple merge strategies that expect to be passed\nthe common commits -- a situation we probably don't have a testcase\nfor in the testsuite -- then I think this code change will result in a\nuse-after-free error.\n\n\n> hold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n> if (!strcmp(strategy, \"ort\"))\n> --\n> 2.25.1\n"},{"id":"471445","messageId":"20230203171142.122983-1-five231003@gmail.com","threadId":"59182","inReplyTo":"xmqq1qn7sm49.fsf@gitster.g","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-03T17:11:42Z","receivedAt":"2023-02-03T17:11:49Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"\n"},{"id":"471448","messageId":"20230203173006.123249-1-five231003@gmail.com","threadId":"59182","inReplyTo":"xmqq1qn7sm49.fsf@gitster.g","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-03T17:30:06Z","receivedAt":"2023-02-03T17:30:15Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"When I submitted the patch, I didn't realize that reverse_commit_list() helper\nfunction had such behavior. It was very different from what I had expected.\nMaybe I should have been more careful and looked into the details before\nsubmitting the patch.\n\nThanks for your explanation, it was valuable. Especially on the part where you\nexplained about the try_merge_strategy() function, it helped me gain more\ninsight on the whole situation. Sorry for this patch, I will be more careful next time.\n\nP.S. Please ignore the empty email, it was me trying to figure out git-send-email\nand how could to reply to specific lines of the email by referencing them. The\nemail got sent by accident.\n"},{"id":"471450","messageId":"20230203174910.123441-1-five231003@gmail.com","threadId":"59182","inReplyTo":"CABPp-BHE=zGT_vPW8+TZn-wqmufhVdGQT-=LXoLQkto6TMrnrA@mail.gmail.com","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-03T17:49:10Z","receivedAt":"2023-02-03T17:49:36Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Now, I think I understand the mistake that I did. Even if it did work\nfor one merge strategy, the code would not be good as the helper function\nis not doing what it is intended to do. In any case, I should have been\nmore careful submitting the patch.\n\nOn a side note, I think we can now close the issue #1156 on gitgitgadget? As\nwith builtin/merge.c out of the way, the only other case is in revision.c\nand the use of the helper function there is inapproriate.\n\nThanks for the explanation.\n"},{"id":"471452","messageId":"CABPp-BHnXy+Hpv4y83znMxDGOTCZQfYhnDon=ehBDGOxAnW1vQ@mail.gmail.com","threadId":"59182","inReplyTo":"20230203174910.123441-1-five231003@gmail.com","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-02-03T18:02:36Z","receivedAt":"2023-02-03T18:02:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Feb 3, 2023 at 9:49 AM Kousik Sanagavarapu wrote:\n>\n> Now, I think I understand the mistake that I did. Even if it did work\n> for one merge strategy, the code would not be good as the helper function\n> is not doing what it is intended to do. In any case, I should have been\n> more careful submitting the patch.\n>\n> On a side note, I think we can now close the issue #1156 on gitgitgadget? As\n> with builtin/merge.c out of the way, the only other case is in revision.c\n> and the use of the helper function there is inapproriate.\n>\n> Thanks for the explanation.\n\nYeah, it should have been closed back when\nhttps://lore.kernel.org/git/CANsrJQd0v2V9H8HPkiH2179C1c-NOSTRRB8YXt8v6R0YAbFPDQ@mail.gmail.com/\nwas submitted. But none of us caught it.\n\nAlso, gitgitgadget #1156 was opened because of my suggestion to do\nthis as a \"leftoverbit\", i.e. I was suggesting it as a micro-project\nto new people. I should have checked at the time that it was a valid\nmicro-project, but neglected to do so. You merely came along and\nstarted implementing what was suggested.\n\nAnyway, the point of the GSoC microprojects are to make sure you are\nfamiliar with how to format and submit patches to the mailing list and\nrespond; having the code you contribute in a microproject be accepted\nis not required, just a bonus. And you clearly managed to send the\npatch to the list, had a correctly formatted commit message (short\nsummary with area and correct lack of capitalization, good\ndescriptions, signed-off-by), got the additional notes for reviewers\n(very helpful!) in the correct spot, etc., so I still see this as a\nsuccessful microproject for you. I apologize for not doing my due\ndiligence when I suggested it, and for us not catching that it should\nhave been closed when someone implemented the valid half of the\nsuggestion last year.\n"},{"id":"471456","messageId":"xmqqmt5uo9ea.fsf@gitster.g","threadId":"59182","inReplyTo":"CABPp-BHnXy+Hpv4y83znMxDGOTCZQfYhnDon=ehBDGOxAnW1vQ@mail.gmail.com","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-03T19:07:25Z","receivedAt":"2023-02-03T19:07:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Also, gitgitgadget #1156 was opened because of my suggestion to do\n> this as a \"leftoverbit\", i.e. I was suggesting it as a micro-project\n> to new people. I should have checked at the time that it was a valid\n> micro-project, but neglected to do so. You merely came along and\n> started implementing what was suggested.\n>\n> Anyway, the point of the GSoC microprojects are to make sure you are\n> familiar with how to format and submit patches to the mailing list and\n> respond; having the code you contribute in a microproject be accepted\n> is not required, just a bonus. And you clearly managed to send the\n> patch to the list, had a correctly formatted commit message (short\n> summary with area and correct lack of capitalization, good\n> descriptions, signed-off-by), got the additional notes for reviewers\n> (very helpful!) in the correct spot, etc., so I still see this as a\n> successful microproject for you. I apologize for not doing my due\n> diligence when I suggested it, and for us not catching that it should\n> have been closed when someone implemented the valid half of the\n> suggestion last year.\n\nOne possible action item for us may be to rename or give comment to\nhighlight the in-place destructive nature of the function to make it\neasier for developers to use (or avoid misusing) it.\n"},{"id":"471531","messageId":"20230205173237.159371-1-five231003@gmail.com","threadId":"59182","inReplyTo":"CABPp-BHnXy+Hpv4y83znMxDGOTCZQfYhnDon=ehBDGOxAnW1vQ@mail.gmail.com","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-05T17:32:37Z","receivedAt":"2023-02-05T17:32:44Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Fri, 3 Feb 2023 at 23:32, Elijah Newren wrote:\n\n> Anyway, the point of the GSoC microprojects are to make sure you are\n> familiar with how to format and submit patches to the mailing list and\n> respond; having the code you contribute in a microproject be accepted\n> is not required, just a bonus.  And you clearly managed to send the\n> patch to the list, had a correctly formatted commit message (short\n> summary with area and correct lack of capitalization, good\n> descriptions, signed-off-by), got the additional notes for reviewers\n> (very helpful!) in the correct spot, etc., so I still see this as a\n> successful microproject for you.\n\nI still need to get used to the codebase a bit, but the process\nitself is fun. I also realized how helpful these notes and discussions\nare. Thanks.\n"},{"id":"471535","messageId":"20230205181257.159652-1-five231003@gmail.com","threadId":"59182","inReplyTo":"xmqqmt5uo9ea.fsf@gitster.g","subject":"Re: [GSoC][PATCH] merge: use reverse_commit_list() for list reversal","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-02-05T18:12:57Z","receivedAt":"2023-02-05T18:13:06Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Sat, 4 Feb 2023 at 00:37, Junio C Hamano wrote:\n\n> One possible action item for us may be to rename or give comment to\n> highlight the in-place destructive nature of the function to make it\n> easier for developers to use (or avoid misusing) it.\n\nThere is a comment describing what the function does in commit.h but\nit doesn't _explicitly_ warn about this destructive behavior (although,\nwith a little thinking, developers can infer that the function can cause\nthis behavior after reading the comment). So, maybe we should go with the\ncomment being a bit more descriptive?\n\nThanks\n"}]}