{"thread":{"id":"58236","subject":"[PATCH] merge-ort: clean up after failed merge","startedAt":"2022-07-29T08:52:37Z","lastAt":"2022-07-31T18:44:24Z","messageCount":7,"participants":["Johannes Schindelin via GitGitGadget","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"460189","messageId":"pull.1307.git.1659084748350.gitgitgadget@gmail.com","threadId":"58236","inReplyTo":null,"subject":"[PATCH] merge-ort: clean up after failed merge","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-29T08:52:27Z","receivedAt":"2022-07-29T08:52:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 9fefce68dc8 (merge-ort: basic outline for merge_switch_to_result(),\n2020-12-13), we added functionality to lay down the result of a merge on\ndisk. But we forgot to release the data structures in case\n`unpack_trees()` failed to run properly.\n\nThis was pointed out by the `linux-leaks` job in our CI runs.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort: clean up after failed merge\n \n I was investigating why seen's CI runs fail, and came up with this fix.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1307%2Fdscho%2Fmerge-ort-impl-leakfix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1307/dscho/merge-ort-impl-leakfix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1307\n\n merge-ort.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex ee7fbe71404..61b9e90018b 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1002,6 +1002,7 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\tif (checkout(opt, head, result->tree)) {\n \t\t\t/* failure to function */\n \t\t\tresult->clean = -1;\n+\t\t\tmerge_finalize(opt, result);\n \t\t\treturn;\n \t\t}\n \n@@ -1010,6 +1011,7 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\t\t\t\t\t &opti->conflicted)) {\n \t\t\t/* failure to function */\n \t\t\tresult->clean = -1;\n+\t\t\tmerge_finalize(opt, result);\n \t\t\treturn;\n \t\t}\n \t}\n\nbase-commit: 9fefce68dc85d96781090f86c067d83f7c50b617\n-- \ngitgitgadget\n"},{"id":"460223","messageId":"CABPp-BEBVO+O8bJaQRwVpt1VC0dC+sMJduAtmHpYev9uhnF-Rw@mail.gmail.com","threadId":"58236","inReplyTo":"pull.1307.git.1659084748350.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-ort: clean up after failed merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-07-29T15:31:30Z","receivedAt":"2022-07-29T15:31:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 29, 2022 at 1:52 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> From: Johannes Schindelin \n>\n> In 9fefce68dc8 (merge-ort: basic outline for merge_switch_to_result(),\n> 2020-12-13), we added functionality to lay down the result of a merge on\n> disk. But we forgot to release the data structures in case\n> `unpack_trees()` failed to run properly.\n>\n> This was pointed out by the `linux-leaks` job in our CI runs.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> merge-ort: clean up after failed merge\n>\n> I was investigating why seen's CI runs fail, and came up with this fix.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1307%2Fdscho%2Fmerge-ort-impl-leakfix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1307/dscho/merge-ort-impl-leakfix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1307\n>\n> merge-ort.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index ee7fbe71404..61b9e90018b 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -1002,6 +1002,7 @@ void merge_switch_to_result(struct merge_options *opt,\n> if (checkout(opt, head, result->tree)) {\n> /* failure to function */\n> result->clean = -1;\n> + merge_finalize(opt, result);\n> return;\n> }\n>\n> @@ -1010,6 +1011,7 @@ void merge_switch_to_result(struct merge_options *opt,\n> &opti->conflicted)) {\n> /* failure to function */\n> result->clean = -1;\n> + merge_finalize(opt, result);\n> return;\n> }\n> }\n>\n> base-commit: 9fefce68dc85d96781090f86c067d83f7c50b617\n> --\n> gitgitgadget\n\nGood catch. Can you rebase on to a slightly newer commit? I think we\nalso need a trace2_region_leave() call in each block as well, which is\nonly clear if you look at newer versions.\n"},{"id":"460236","messageId":"pull.1307.v2.git.1659114727.gitgitgadget@gmail.com","threadId":"58236","inReplyTo":"pull.1307.git.1659084748350.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] merge-ort: clean up after failed merge","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-29T17:12:05Z","receivedAt":"2022-07-29T17:12:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I was investigating why seen's CI runs fail, and came up with this fix.\n\nChanges since v1:\n\n * Rebased onto en/merge-ort-perf.\n * Now we're not only cleaning up the merge data structure, but also leaving\n the Trace2 region when returning early from merge_switch_to_result().\n\nJohannes Schindelin (2):\n merge-ort: clean up after failed merge\n merge-ort: do leave Trace2 region even if checkout fails\n\n merge-ort.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\n\nbase-commit: 557ac0350d9efa1f59c708779ca3fb3aee121131\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1307%2Fdscho%2Fmerge-ort-impl-leakfix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1307/dscho/merge-ort-impl-leakfix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1307\n\nRange-diff vs v1:\n\n 1: 128f77f7f34 ! 1: 082c7ffa41f merge-ort: clean up after failed merge\n @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,\n +\t\t\tmerge_finalize(opt, result);\n \t\t\treturn;\n \t\t}\n - \n + \t\ttrace2_region_leave(\"merge\", \"checkout\", opt->repo);\n @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,\n \t\t\t\t\t\t &opti->conflicted)) {\n \t\t\t/* failure to function */\n @@ merge-ort.c: void merge_switch_to_result(struct merge_options *opt,\n +\t\t\tmerge_finalize(opt, result);\n \t\t\treturn;\n \t\t}\n - \t}\n + \t\ttrace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n -: ----------- > 2: d2e1af0f922 merge-ort: do leave Trace2 region even if checkout fails\n\n-- \ngitgitgadget\n"},{"id":"460237","messageId":"082c7ffa41f241c61ae2fe5ab763fb09c680a17b.1659114727.git.gitgitgadget@gmail.com","threadId":"58236","inReplyTo":"pull.1307.v2.git.1659114727.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] merge-ort: clean up after failed merge","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-29T17:12:06Z","receivedAt":"2022-07-29T17:12:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 9fefce68dc8 (merge-ort: basic outline for merge_switch_to_result(),\n2020-12-13), we added functionality to lay down the result of a merge on\ndisk. But we forgot to release the data structures in case\n`unpack_trees()` failed to run properly.\n\nThis was pointed out by the `linux-leaks` job in our CI runs.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 931b91438cf..e820e45a8e8 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3157,6 +3157,7 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\tif (checkout(opt, head, result->tree)) {\n \t\t\t/* failure to function */\n \t\t\tresult->clean = -1;\n+\t\t\tmerge_finalize(opt, result);\n \t\t\treturn;\n \t\t}\n \t\ttrace2_region_leave(\"merge\", \"checkout\", opt->repo);\n@@ -3167,6 +3168,7 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\t\t\t\t\t &opti->conflicted)) {\n \t\t\t/* failure to function */\n \t\t\tresult->clean = -1;\n+\t\t\tmerge_finalize(opt, result);\n \t\t\treturn;\n \t\t}\n \t\ttrace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n-- \ngitgitgadget\n\n"},{"id":"460238","messageId":"d2e1af0f922ea5316074dd0867c66650161b0ab5.1659114727.git.gitgitgadget@gmail.com","threadId":"58236","inReplyTo":"pull.1307.v2.git.1659114727.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] merge-ort: do leave Trace2 region even if checkout fails","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-29T17:12:07Z","receivedAt":"2022-07-29T17:12:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 557ac0350d9 (merge-ort: begin performance work; instrument with\ntrace2_region_* calls, 2021-01-23), we added Trace2 instrumentation, but\nin the error path that returns early, we forgot to tell Trace2 that\nwe're leaving the region. Let's fix that.\n\nPointed-out-by: Elijah Newren \nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex e820e45a8e8..eb0296902ad 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3158,6 +3158,7 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\t\t/* failure to function */\n \t\t\tresult->clean = -1;\n \t\t\tmerge_finalize(opt, result);\n+\t\t\ttrace2_region_leave(\"merge\", \"checkout\", opt->repo);\n \t\t\treturn;\n \t\t}\n \t\ttrace2_region_leave(\"merge\", \"checkout\", opt->repo);\n@@ -3169,6 +3170,8 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\t\t/* failure to function */\n \t\t\tresult->clean = -1;\n \t\t\tmerge_finalize(opt, result);\n+\t\t\ttrace2_region_leave(\"merge\", \"record_conflicted\",\n+\t\t\t\t\t opt->repo);\n \t\t\treturn;\n \t\t}\n \t\ttrace2_region_leave(\"merge\", \"record_conflicted\", opt->repo);\n-- \ngitgitgadget\n"},{"id":"460295","messageId":"CABPp-BGKUkVKJK87jZn-7A3AuxDjKwttVpez9UVZrTJZyCFodw@mail.gmail.com","threadId":"58236","inReplyTo":"pull.1307.v2.git.1659114727.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] merge-ort: clean up after failed merge","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-07-30T00:50:30Z","receivedAt":"2022-07-30T00:50:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 29, 2022 at 10:12 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> I was investigating why seen's CI runs fail, and came up with this fix.\n>\n> Changes since v1:\n>\n> * Rebased onto en/merge-ort-perf.\n> * Now we're not only cleaning up the merge data structure, but also leaving\n> the Trace2 region when returning early from merge_switch_to_result().\n>\n> Johannes Schindelin (2):\n> merge-ort: clean up after failed merge\n> merge-ort: do leave Trace2 region even if checkout fails\n>\n> merge-ort.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n\nThanks, series looks good to me:\n\nReviewed-by: Elijah Newren \n"},{"id":"460334","messageId":"xmqqles915rx.fsf@gitster.g","threadId":"58236","inReplyTo":"CABPp-BGKUkVKJK87jZn-7A3AuxDjKwttVpez9UVZrTJZyCFodw@mail.gmail.com","subject":"Re: [PATCH v2 0/2] merge-ort: clean up after failed merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-31T18:44:18Z","receivedAt":"2022-07-31T18:44:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Fri, Jul 29, 2022 at 10:12 AM Johannes Schindelin via GitGitGadget\n> wrote:\n>>\n>> I was investigating why seen's CI runs fail, and came up with this fix.\n>>\n>> Changes since v1:\n>>\n>> * Rebased onto en/merge-ort-perf.\n>> * Now we're not only cleaning up the merge data structure, but also leaving\n>> the Trace2 region when returning early from merge_switch_to_result().\n>>\n>> Johannes Schindelin (2):\n>> merge-ort: clean up after failed merge\n>> merge-ort: do leave Trace2 region even if checkout fails\n>>\n>> merge-ort.c | 5 +++++\n>> 1 file changed, 5 insertions(+)\n>\n> Thanks, series looks good to me:\n>\n> Reviewed-by: Elijah Newren \n\nThanks, both.\n\nThe new \"leave\" calls immediately next to the existing ones that\nlook identical appear duplicated, but correcting the logic first\nin an obvious way like the posted patch, leaving any clean-up to\nlater, is a prudent thing to do.\n\nQueued.\n"}]}