{"thread":{"id":"57158","subject":"[PATCH] merge-ort: fix bug with renormalization and rename/delete conflicts","startedAt":"2021-12-28T00:20:50Z","lastAt":"2021-12-30T23:35:25Z","messageCount":5,"participants":["Elijah Newren via GitGitGadget","Derrick Stolee","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"445036","messageId":"pull.1174.git.git.1640650846612.gitgitgadget@gmail.com","threadId":"57158","inReplyTo":null,"subject":"[PATCH] merge-ort: fix bug with renormalization and rename/delete conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-28T00:20:46Z","receivedAt":"2021-12-28T00:20:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nEver since commit a492d5331c (\"merge-ort: ensure we consult df_conflict\nand path_conflicts\", 2021-06-30), when renormalization is active AND a\nfile is involved in a rename/delete conflict BUT the file is unmodified\n(either before or after renormalization), merge-ort was running into an\nassertion failure. Prior to that commit (or if assertions were compiled\nout), merge-ort would mis-merge instead, ignoring the rename/delete\nconflict and just deleting the file.\n\nRemove the assertions, fix the code appropriately, leave some good\ncomments in the code, and add a testcase for this situation.\n\nReported-by: Ralf Thielow \nSigned-off-by: Elijah Newren \n---\n merge-ort: fix bug with renormalization and rename/delete conflicts\n \n Original report:\n https://lore.kernel.org/git/CAN0XMOK8iHZnbtYw7CPAQGJcmuVSDxQoFNFEwiaa41V89F1rzA@mail.gmail.com/\n \n Built in v2.34.1, but rebases onto and/or merges cleanly with newer\n versions.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1174%2Fnewren%2Fmerge-ort-rename-delete-renormalization-bug-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1174/newren/merge-ort-rename-delete-renormalization-bug-v1\nPull-Request: https://github.com/git/git/pull/1174\n\n merge-ort.c | 19 ++++++++++++++++---\n t/t6418-merge-text-auto.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 0342f104836..c3197970219 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3841,9 +3841,22 @@ static void process_entry(struct merge_options *opt,\n \t\tif (opt->renormalize &&\n \t\t blob_unchanged(opt, &ci->stages[0], &ci->stages[side],\n \t\t\t\t path)) {\n-\t\t\tci->merged.is_null = 1;\n-\t\t\tci->merged.clean = 1;\n-\t\t\tassert(!ci->df_conflict && !ci->path_conflict);\n+\t\t\tif (!ci->path_conflict) {\n+\t\t\t\t/*\n+\t\t\t\t * Blob unchanged after renormalization, so\n+\t\t\t\t * there's no modify/delete conflict after all;\n+\t\t\t\t * we can just remove the file.\n+\t\t\t\t */\n+\t\t\t\tci->merged.is_null = 1;\n+\t\t\t\tci->merged.clean = 1;\n+\t\t\t\t /*\n+\t\t\t\t * file goes away => even if there was a\n+\t\t\t\t * directory/file conflict there isn't one now.\n+\t\t\t\t */\n+\t\t\t\tci->df_conflict = 0;\n+\t\t\t} else {\n+\t\t\t\t/* rename/delete, so conflict remains */\n+\t\t\t}\n \t\t} else if (ci->path_conflict &&\n \t\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n \t\t\t/*\ndiff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh\nindex 1e0296dd172..41288a60ceb 100755\n--- a/t/t6418-merge-text-auto.sh\n+++ b/t/t6418-merge-text-auto.sh\n@@ -204,4 +204,30 @@ test_expect_success 'Test delete/normalize conflict' '\n \ttest_path_is_missing file\n '\n \n+test_expect_success 'rename/delete vs. renormalization' '\n+\tgit init subrepo &&\n+\t(\n+\t\tcd subrepo &&\n+\t\techo foo >oldfile &&\n+\t\tgit add oldfile &&\n+\t\tgit commit -m original &&\n+\n+\t\tgit branch rename &&\n+\t\tgit branch nuke &&\n+\n+\t\tgit checkout rename &&\n+\t\tgit mv oldfile newfile &&\n+\t\tgit commit -m renamed &&\n+\n+\t\tgit checkout nuke &&\n+\t\tgit rm oldfile &&\n+\t\tgit commit -m deleted &&\n+\n+\t\tgit checkout rename^0 &&\n+\t\ttest_must_fail git -c merge.renormalize=true merge nuke >out &&\n+\n+\t\tgrep \"rename/delete\" out\n+\t)\n+'\n+\n test_done\n\nbase-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n-- \ngitgitgadget\n"},{"id":"445078","messageId":"78ba0d99-246b-d90c-b725-c084c8304f02@gmail.com","threadId":"57158","inReplyTo":"pull.1174.git.git.1640650846612.gitgitgadget@gmail.com","subject":"Re: [PATCH] merge-ort: fix bug with renormalization and rename/delete conflicts","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-12-28T13:55:16Z","receivedAt":"2021-12-28T13:55:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/27/2021 7:20 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> Ever since commit a492d5331c (\"merge-ort: ensure we consult df_conflict\n> and path_conflicts\", 2021-06-30), when renormalization is active AND a\n> file is involved in a rename/delete conflict BUT the file is unmodified\n> (either before or after renormalization), merge-ort was running into an\n> assertion failure. \n\nThis \"the file is unmodified\" is critical, as when I looked at the test,\nit seemed too simple. I asked myself, \"Why does renormalization matter\nhere?\" Turns out it is just an artifact of the carefully organized cases.\n\n> \t\tif (opt->renormalize &&\n> \t\t blob_unchanged(opt, &ci->stages[0], &ci->stages[side],\n> \t\t\t\t path)) {\n> -\t\t\tci->merged.is_null = 1;\n> -\t\t\tci->merged.clean = 1;\n> -\t\t\tassert(!ci->df_conflict && !ci->path_conflict);\n> +\t\t\tif (!ci->path_conflict) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Blob unchanged after renormalization, so\n> +\t\t\t\t * there's no modify/delete conflict after all;\n> +\t\t\t\t * we can just remove the file.\n> +\t\t\t\t */\n> +\t\t\t\tci->merged.is_null = 1;\n> +\t\t\t\tci->merged.clean = 1;\n> +\t\t\t\t /*\n> +\t\t\t\t * file goes away => even if there was a\n> +\t\t\t\t * directory/file conflict there isn't one now.\n> +\t\t\t\t */\n> +\t\t\t\tci->df_conflict = 0;\n> +\t\t\t} else {\n> +\t\t\t\t/* rename/delete, so conflict remains */\n> +\t\t\t}\n\nThis breakdown of the cases is informative, and I like how self-contained\nthe change is.\n\n> +test_expect_success 'rename/delete vs. renormalization' '\n> +\tgit init subrepo &&\n> +\t(\n> +\t\tcd subrepo &&\n> +\t\techo foo >oldfile &&\n> +\t\tgit add oldfile &&\n> +\t\tgit commit -m original &&\n> +\n> +\t\tgit branch rename &&\n> +\t\tgit branch nuke &&\n> +\n> +\t\tgit checkout rename &&\n> +\t\tgit mv oldfile newfile &&\n> +\t\tgit commit -m renamed &&\n> +\n> +\t\tgit checkout nuke &&\n> +\t\tgit rm oldfile &&\n> +\t\tgit commit -m deleted &&\n> +\n> +\t\tgit checkout rename^0 &&\n> +\t\ttest_must_fail git -c merge.renormalize=true merge nuke >out &&\n> +\n> +\t\tgrep \"rename/delete\" out\n> +\t)\n> +'\n> +\n> test_done\n\nI tested this on the latest 'master' and saw the following:\n\n git: merge-ort.c:3846: process_entry: Assertion `!ci->df_conflict && !ci->path_conflict' failed\n\nso it indeed hits this case.\n\nThis patch looks good to me. Thanks!\n\nReviewed-by: Derrick Stolee \n\n"},{"id":"445242","messageId":"pull.1174.v2.git.git.1640902135926.gitgitgadget@gmail.com","threadId":"57158","inReplyTo":"pull.1174.git.git.1640650846612.gitgitgadget@gmail.com","subject":"[PATCH v2] merge-ort: fix bug with renormalization and rename/delete conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-30T22:08:55Z","receivedAt":"2021-12-30T22:09:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nEver since commit a492d5331c (\"merge-ort: ensure we consult df_conflict\nand path_conflicts\", 2021-06-30), when renormalization is active AND a\nfile is involved in a rename/delete conflict BUT the file is unmodified\n(either before or after renormalization), merge-ort was running into an\nassertion failure. Prior to that commit (or if assertions were compiled\nout), merge-ort would mis-merge instead, ignoring the rename/delete\nconflict and just deleting the file.\n\nRemove the assertions, fix the code appropriately, leave some good\ncomments in the code, and add a testcase for this situation.\n\nReported-by: Ralf Thielow \nReviewed-by: Derrick Stolee \nSigned-off-by: Elijah Newren \n---\n merge-ort: fix bug with renormalization and rename/delete conflicts\n \n Original report:\n https://lore.kernel.org/git/CAN0XMOK8iHZnbtYw7CPAQGJcmuVSDxQoFNFEwiaa41V89F1rzA@mail.gmail.com/\n \n Built in v2.34.1, but rebases onto and/or merges cleanly with newer\n versions.\n \n Changes since v1:\n \n * Added Stolee's Reviewed-by\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1174%2Fnewren%2Fmerge-ort-rename-delete-renormalization-bug-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1174/newren/merge-ort-rename-delete-renormalization-bug-v2\nPull-Request: https://github.com/git/git/pull/1174\n\nRange-diff vs v1:\n\n 1: 5841f3d901d ! 1: 72876b9c106 merge-ort: fix bug with renormalization and rename/delete conflicts\n @@ Commit message\n comments in the code, and add a testcase for this situation.\n \n Reported-by: Ralf Thielow \n + Reviewed-by: Derrick Stolee \n Signed-off-by: Elijah Newren \n \n ## merge-ort.c ##\n\n\n merge-ort.c | 19 ++++++++++++++++---\n t/t6418-merge-text-auto.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 0342f104836..c3197970219 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3841,9 +3841,22 @@ static void process_entry(struct merge_options *opt,\n \t\tif (opt->renormalize &&\n \t\t blob_unchanged(opt, &ci->stages[0], &ci->stages[side],\n \t\t\t\t path)) {\n-\t\t\tci->merged.is_null = 1;\n-\t\t\tci->merged.clean = 1;\n-\t\t\tassert(!ci->df_conflict && !ci->path_conflict);\n+\t\t\tif (!ci->path_conflict) {\n+\t\t\t\t/*\n+\t\t\t\t * Blob unchanged after renormalization, so\n+\t\t\t\t * there's no modify/delete conflict after all;\n+\t\t\t\t * we can just remove the file.\n+\t\t\t\t */\n+\t\t\t\tci->merged.is_null = 1;\n+\t\t\t\tci->merged.clean = 1;\n+\t\t\t\t /*\n+\t\t\t\t * file goes away => even if there was a\n+\t\t\t\t * directory/file conflict there isn't one now.\n+\t\t\t\t */\n+\t\t\t\tci->df_conflict = 0;\n+\t\t\t} else {\n+\t\t\t\t/* rename/delete, so conflict remains */\n+\t\t\t}\n \t\t} else if (ci->path_conflict &&\n \t\t\t oideq(&ci->stages[0].oid, &ci->stages[side].oid)) {\n \t\t\t/*\ndiff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh\nindex 1e0296dd172..41288a60ceb 100755\n--- a/t/t6418-merge-text-auto.sh\n+++ b/t/t6418-merge-text-auto.sh\n@@ -204,4 +204,30 @@ test_expect_success 'Test delete/normalize conflict' '\n \ttest_path_is_missing file\n '\n \n+test_expect_success 'rename/delete vs. renormalization' '\n+\tgit init subrepo &&\n+\t(\n+\t\tcd subrepo &&\n+\t\techo foo >oldfile &&\n+\t\tgit add oldfile &&\n+\t\tgit commit -m original &&\n+\n+\t\tgit branch rename &&\n+\t\tgit branch nuke &&\n+\n+\t\tgit checkout rename &&\n+\t\tgit mv oldfile newfile &&\n+\t\tgit commit -m renamed &&\n+\n+\t\tgit checkout nuke &&\n+\t\tgit rm oldfile &&\n+\t\tgit commit -m deleted &&\n+\n+\t\tgit checkout rename^0 &&\n+\t\ttest_must_fail git -c merge.renormalize=true merge nuke >out &&\n+\n+\t\tgrep \"rename/delete\" out\n+\t)\n+'\n+\n test_done\n\nbase-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n-- \ngitgitgadget\n"},{"id":"445248","messageId":"xmqqa6ghog5x.fsf@gitster.g","threadId":"57158","inReplyTo":"78ba0d99-246b-d90c-b725-c084c8304f02@gmail.com","subject":"Re: [PATCH] merge-ort: fix bug with renormalization and rename/delete conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-30T22:56:42Z","receivedAt":"2021-12-30T22:56:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> This breakdown of the cases is informative, and I like how self-contained\n> the change is.\n> ....\n>\n> This patch looks good to me. Thanks!\n>\n> Reviewed-by: Derrick Stolee \n\nThanks, both.\n\nA related tangent, but I was looking at the data structure involved\nand noticed that the casting between structure types \"merged_info\"\nand \"conflict_info\" looked a bit ugly. It might be worth cleaning\nthem up into \n\n (A) a union with two struct, with \"clean\" member in the union to\n switch between the two structures; or\n\n (B) a single structure that looks like \"conflict_info\" but inlines\n members of \"merged_info\" into it.\n\nThe latter may be cleaner and simpler, and the unified data type\nwould be the \"merge info\", which may be representing cleanly merged\npath, or conflicted path, and would justify conditional use of some\nmembers based on the value of the .clean member.\n\n"},{"id":"445251","messageId":"CABPp-BHDsd66zE4GwNB45HSCffSD5gryUYtdL+9t4LdVCDk0Vw@mail.gmail.com","threadId":"57158","inReplyTo":"xmqqa6ghog5x.fsf@gitster.g","subject":"Re: [PATCH] merge-ort: fix bug with renormalization and rename/delete conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-12-30T23:35:09Z","receivedAt":"2021-12-30T23:35:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Dec 30, 2021 at 2:56 PM Junio C Hamano wrote:\n>\n> Derrick Stolee writes:\n>\n> > This breakdown of the cases is informative, and I like how self-contained\n> > the change is.\n> > ....\n> >\n> > This patch looks good to me. Thanks!\n> >\n> > Reviewed-by: Derrick Stolee \n>\n> Thanks, both.\n>\n> A related tangent, but I was looking at the data structure involved\n> and noticed that the casting between structure types \"merged_info\"\n> and \"conflict_info\" looked a bit ugly.\n\nYes, that's true.\n\n> It might be worth cleaning them up into\n>\n> (A) a union with two struct, with \"clean\" member in the union to\n> switch between the two structures; or\n>\n> (B) a single structure that looks like \"conflict_info\" but inlines\n> members of \"merged_info\" into it.\n>\n> The latter may be cleaner and simpler, and the unified data type\n> would be the \"merge info\", which may be representing cleanly merged\n> path, or conflicted path, and would justify conditional use of some\n> members based on the value of the .clean member.\n\nThese are heavily used data structures. Note that:\n sizeof(struct conflict_info) = 216\n sizeof(struct merged_info) = 64\nIn particular, we have to allocate one or the other of these for every\npath (both file and directory) involved in the merge. Since the\nformer is 3.375 times bigger than the latter, and the vast majority of\npaths involved in a merge usually do not conflict (think of files only\nchanged on one side), using just one combined struct would require\nmore than 3x the amount of memory. So I'd rather avoid (B).\n\n(A) may work, but I'd still have to allocate merged_info instead of\nthe union type to avoid the memory increase. And since we have an\namount of memory allocated that is smaller than the union, when\naccessing it via the union, Stolee would probably still want all the\nsame casting safeguards (as a safety check to avoid out-of-bounds\naccesses) that I think you're complaining about.\n"}]}