{"thread":{"id":"51535","subject":"BUG() during criss-cross merge with directory rename and deleted file","startedAt":"2019-07-26T22:09:36Z","lastAt":"2019-08-08T11:00:26Z","messageCount":11,"participants":["Emily Shaffer","Elijah Newren","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"379411","messageId":"20190726220928.GG113966@google.com","threadId":"51535","inReplyTo":null,"subject":"BUG() during criss-cross merge with directory rename and deleted file","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-07-26T22:09:28Z","receivedAt":"2019-07-26T22:09:36Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Hi all,\n\nAs mentioned during the standup in #git-devel today, we encountered a\nBUG() during certain circumstances of a criss-cross merge. Timing has\nprevented me from getting the repro case together to send just yet, but\nit appears that the issue is as follows:\n\n - During a merge in a repo with fairly complicated, merge-y history,\n the following BUG() is seen:\n \"BUG: There are unmerged index entries:\n BUG: 2 baz/bar.txt\n BUG: merge-recursive.c:429: unmerged index entries in\n merge-recursive.c\n Aborted\"\n - But when the user then runs `git status` (after clearing\n .git/index.lock) the directory is clean.\n - The repo does not contain a 'baz/bar.txt' (although 'baz/' exists).\n - In the repo's history a 'foo/bar.txt' can be found (that is the only\n 'bar.txt' to ever exist in the project).\n\nDigging further shows:\n\n - The merge had multiple closest shared ancestors (criss-cross\n merge)\n - Directory 'foo/' was renamed on one side to 'baz/'...\n - ...and 'foo/bar.txt' was deleted on the other side.\n - When the virtual ancestor is generated, the directory rename can't be\n resolved and leaves a conflict.\n - The virtual ancestor being written to disk is in a conflicted state.\n - This causes the top-level merge to fail, printing the BUG() above\n which references a 'baz/bar.txt' that never really lived there.\n\nFurthermore...\n\n - If merge.directoryrenames is set to any value besides \"conflict\", the\n merge succeeds (no BUG() generated).\n\nThis seems to have been introduced in 8c8e5bd6eb331d055aa7fa6345f6dcdadd658979\nalthough I haven't bisected it yet.\n\nIt seems like the solution is to watch for whether we're making a\nvirtual ancestor during a recursive merge, and if so, to treat\nmerge.directoryrenames = \"conflict\" as \"true\" or \"false\" instead.\n\nI plan to modify the tests added in 8c8e5bd6 to highlight this issue\n(just haven't had time, and probably won't til next week), and send a\npatch doing the special treatment on merge.directoryrenames.\n\nHappy to hear other ideas folks have.\n\n - Emily\n"},{"id":"379960","messageId":"20190805223350.27504-1-newren@gmail.com","threadId":"51535","inReplyTo":"20190726220928.GG113966@google.com","subject":"[PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-05T22:33:50Z","receivedAt":"2019-08-05T22:33:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Ever since commit 8c8e5bd6eb33 (\"merge-recursive: switch directory\nrename detection default\", 2019-04-05), the default handling with\ndirectory rename detection was to report a conflict and leave unstaged\nentries in the index. However, when creating a virtual merge base in\nthe recursive case, we absolutely need a tree, and the only way a tree\ncan be written is if we have no unstaged entries -- otherwise we hit a\nBUG().\n\nThere are a few fixes possible here which at least fix the BUG(), but\nnone of them seem optimal for other reasons; see the comments with the\nnew testcase 13e in t6043 for details (which testcase triggered a BUG()\nprior to this patch). As such, just opt for a very conservative and\nsimple choice that is still relatively reasonable: have the recursive\ncase treat 'conflict' as 'false' for opt->detect_directory_renames.\n\nReported-by: Emily Shaffer \nSigned-off-by: Elijah Newren \n---\n\nI really should introduce constants like\n DETECT_DIRECTORY_RENAMES_NEVER = 0\n DETECT_DIRECTORY_RENAMES_CONFLICT = 1\n DETECT_DIRECTORY_RENAMES_YES = 2\nand then use them in the code to make it clearer, but I wanted to make\nthe code change as simple and contained as possible given that this is\nbuilt on maint, fixes a BUG() and we're already in -rc1.\n\nI know this bug doesn't satisfy the normal criteria for making it into\n2.23 (it's a bug that was present in 2.22 rather than a regression in\n2.23), but given that it's a BUG() condition, I was hoping it is\nimportant and safe enough to include anyway.\n\n(This fix does merge down cleanly to master, next, and pu.)\n\n merge-recursive.c | 3 +-\n t/t6043-merge-rename-directories.sh | 111 ++++++++++++++++++++++++++++\n 2 files changed, 113 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d2e380b7ed..c7691d9b54 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2856,7 +2856,8 @@ static int detect_and_process_renames(struct merge_options *opt,\n \thead_pairs = get_diffpairs(opt, common, head);\n \tmerge_pairs = get_diffpairs(opt, common, merge);\n \n-\tif (opt->detect_directory_renames) {\n+\tif ((opt->detect_directory_renames == 2) ||\n+\t (opt->detect_directory_renames == 1 && !opt->call_depth)) {\n \t\tdir_re_head = get_directory_renames(head_pairs);\n \t\tdir_re_merge = get_directory_renames(merge_pairs);\n \ndiff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh\nindex 50b7543483..c966147d5d 100755\n--- a/t/t6043-merge-rename-directories.sh\n+++ b/t/t6043-merge-rename-directories.sh\n@@ -4403,4 +4403,115 @@ test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual\n \t)\n '\n \n+# Testcase 13e, directory rename in virtual merge base\n+#\n+# This testcase has a slightly different setup than all the above cases, in\n+# order to include a recursive case:\n+#\n+# A C\n+# o - o\n+# / \\ / \\\n+# O o X ?\n+# \\ / \\ /\n+# o o\n+# B D\n+#\n+# Commit O: a/{z,y}\n+# Commit A: b/{z,y}\n+# Commit B: a/{z,y,x}\n+# Commit C: b/{z,y,x}\n+# Commit D: b/{z,y}, a/x\n+# Expected: b/{z,y,x} (sort of; see below for why this might not be expected)\n+#\n+# NOTES: 'X' represents a virtual merge base. With the default of\n+# directory rename detection yielding conflicts, merging A and B\n+# results in a conflict complaining about whether 'x' should be\n+# under 'a/' or 'b/'. However, when creating the virtual merge\n+# base 'X', since virtual merge bases need to be written out as a\n+# tree, we cannot have a conflict, so some resolution has to be\n+# picked.\n+#\n+# In choosing the right resolution, it's worth noting here that\n+# commits C & D are merges of A & B that choose different\n+# locations for 'x' (i.e. they resolve the conflict differently),\n+# and so it would be nice when merging C & D if git could detect\n+# this difference of opinion and report a conflict. But the only\n+# way to do so that I can think of would be to have the virtual\n+# merge base place 'x' in some directory other than either 'a/' or\n+# 'b/', which seems a little weird -- especially since it'd result\n+# in a rename/rename(1to2) conflict with a source path that never\n+# existed in any version.\n+#\n+# So, for now, when directory rename detection is set to\n+# 'conflict' just avoid doing directory rename detection at all in\n+# the recursive case. This will not allow us to detect a conflict\n+# in the outer merge for this special kind of setup, but it at\n+# least avoids hitting a BUG().\n+#\n+test_expect_success '13e-setup: directory rename detection in recursive case' '\n+\ttest_create_repo 13e &&\n+\t(\n+\t\tcd 13e &&\n+\n+\t\tmkdir a &&\n+\t\techo z >a/z &&\n+\t\techo y >a/y &&\n+\t\tgit add a &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout A &&\n+\t\tgit mv a/ b/ &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"A\" &&\n+\n+\t\tgit checkout B &&\n+\t\techo x >a/x &&\n+\t\tgit add a &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"B\" &&\n+\n+\t\tgit branch C A &&\n+\t\tgit branch D B &&\n+\n+\t\tgit checkout C &&\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge B &&\n+\t\tgit add b/x &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"C\" &&\n+\n+\n+\t\tgit checkout D &&\n+\t\ttest_must_fail git -c merge.directoryRenames=conflict merge A &&\n+\t\tgit add b/x &&\n+\t\tmkdir a &&\n+\t\tgit mv b/x a/x &&\n+\t\ttest_tick &&\n+\t\tgit commit -m \"D\"\n+\t)\n+'\n+\n+test_expect_success '13e-check: directory rename detection in recursive case' '\n+\t(\n+\t\tcd 13e &&\n+\n+\t\tgit checkout --quiet D^0 &&\n+\n+\t\tgit -c merge.directoryRenames=conflict merge -s recursive C^0 >out 2>err &&\n+\n+\t\ttest_i18ngrep ! CONFLICT out &&\n+\t\ttest_i18ngrep ! BUG: err &&\n+\t\ttest_i18ngrep ! core.dumped err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tgit ls-files >paths &&\n+\t\t! grep a/x paths &&\n+\t\tgrep b/x paths\n+\t)\n+'\n+\n test_done\n-- \n2.22.0.246.g5ddf3d502a\n\n"},{"id":"380016","messageId":"xmqq1rxykz91.fsf@gitster-ct.c.googlers.com","threadId":"51535","inReplyTo":"20190805223350.27504-1-newren@gmail.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-06T16:57:46Z","receivedAt":"2019-08-06T16:57:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Ever since commit 8c8e5bd6eb33 (\"merge-recursive: switch directory\n> rename detection default\", 2019-04-05), the default handling with\n> directory rename detection was to report a conflict and leave unstaged\n> entries in the index. However, when creating a virtual merge base in\n> the recursive case, we absolutely need a tree, and the only way a tree\n> can be written is if we have no unstaged entries -- otherwise we hit a\n> BUG().\n>\n> There are a few fixes possible here which at least fix the BUG(), but\n> none of them seem optimal for other reasons; see the comments with the\n> new testcase 13e in t6043 for details (which testcase triggered a BUG()\n> prior to this patch). As such, just opt for a very conservative and\n> simple choice that is still relatively reasonable: have the recursive\n> case treat 'conflict' as 'false' for opt->detect_directory_renames.\n>\n> Reported-by: Emily Shaffer \n> Signed-off-by: Elijah Newren \n> ---\n>\n> I really should introduce constants like\n> DETECT_DIRECTORY_RENAMES_NEVER = 0\n> DETECT_DIRECTORY_RENAMES_CONFLICT = 1\n> DETECT_DIRECTORY_RENAMES_YES = 2\n\nHow would they compare with MERGE_DIRECTORY_RENAMES_* macros\nI see at the tip of 'pu'? init_merge_options() seems to read\none of those values from the \"repo settings\" and copies it to\nthe detect_directory_renames field, so I am reading that they\nmust be identical.\n\n> and then use them in the code to make it clearer, but I wanted to make\n> the code change as simple and contained as possible given that this is\n> built on maint, fixes a BUG() and we're already in -rc1.\n>\n> I know this bug doesn't satisfy the normal criteria for making it into\n> 2.23 (it's a bug that was present in 2.22 rather than a regression in\n> 2.23), but given that it's a BUG() condition, I was hoping it is\n> important and safe enough to include anyway.\n\nI do agree that it is sensible to avoid doing any funky thing during\nthe virtual base merges, whose result is much less observable (hence\nharder to form the right mental model in end user's head) than the\noutermost merge. Do we want to allow this for inner merges when the\nsetting is 2? Wouldn't that hit the same BUG()?\n\n> (This fix does merge down cleanly to master, next, and pu.)\n>\n> merge-recursive.c | 3 +-\n> t/t6043-merge-rename-directories.sh | 111 ++++++++++++++++++++++++++++\n> 2 files changed, 113 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d2e380b7ed..c7691d9b54 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2856,7 +2856,8 @@ static int detect_and_process_renames(struct merge_options *opt,\n> \thead_pairs = get_diffpairs(opt, common, head);\n> \tmerge_pairs = get_diffpairs(opt, common, merge);\n> \n> -\tif (opt->detect_directory_renames) {\n> +\tif ((opt->detect_directory_renames == 2) ||\n> +\t (opt->detect_directory_renames == 1 && !opt->call_depth)) {\n> \t\tdir_re_head = get_directory_renames(head_pairs);\n> \t\tdir_re_merge = get_directory_renames(merge_pairs);\n> \n> diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh\n> index 50b7543483..c966147d5d 100755\n> --- a/t/t6043-merge-rename-directories.sh\n> +++ b/t/t6043-merge-rename-directories.sh\n> @@ -4403,4 +4403,115 @@ test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual\n> \t)\n> '\n> \n> +# Testcase 13e, directory rename in virtual merge base\n> +#\n> +# This testcase has a slightly different setup than all the above cases, in\n> +# order to include a recursive case:\n> +#\n> +# A C\n> +# o - o\n> +# / \\ / \\\n> +# O o X ?\n> +# \\ / \\ /\n> +# o o\n> +# B D\n> +#\n> +# Commit O: a/{z,y}\n> +# Commit A: b/{z,y}\n> +# Commit B: a/{z,y,x}\n> +# Commit C: b/{z,y,x}\n> +# Commit D: b/{z,y}, a/x\n> +# Expected: b/{z,y,x} (sort of; see below for why this might not be expected)\n> +#\n> +# NOTES: 'X' represents a virtual merge base. With the default of\n> +# directory rename detection yielding conflicts, merging A and B\n> +# results in a conflict complaining about whether 'x' should be\n> +# under 'a/' or 'b/'. However, when creating the virtual merge\n> +# base 'X', since virtual merge bases need to be written out as a\n> +# tree, we cannot have a conflict, so some resolution has to be\n> +# picked.\n> +#\n> +# In choosing the right resolution, it's worth noting here that\n> +# commits C & D are merges of A & B that choose different\n> +# locations for 'x' (i.e. they resolve the conflict differently),\n> +# and so it would be nice when merging C & D if git could detect\n> +# this difference of opinion and report a conflict. But the only\n> +# way to do so that I can think of would be to have the virtual\n> +# merge base place 'x' in some directory other than either 'a/' or\n> +# 'b/', which seems a little weird -- especially since it'd result\n> +# in a rename/rename(1to2) conflict with a source path that never\n> +# existed in any version.\n> +#\n> +# So, for now, when directory rename detection is set to\n> +# 'conflict' just avoid doing directory rename detection at all in\n> +# the recursive case. This will not allow us to detect a conflict\n> +# in the outer merge for this special kind of setup, but it at\n> +# least avoids hitting a BUG().\n> +#\n> +test_expect_success '13e-setup: directory rename detection in recursive case' '\n> +\ttest_create_repo 13e &&\n> +\t(\n> +\t\tcd 13e &&\n> +\n> +\t\tmkdir a &&\n> +\t\techo z >a/z &&\n> +\t\techo y >a/y &&\n> +\t\tgit add a &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"O\" &&\n> +\n> +\t\tgit branch O &&\n> +\t\tgit branch A &&\n> +\t\tgit branch B &&\n> +\n> +\t\tgit checkout A &&\n> +\t\tgit mv a/ b/ &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"A\" &&\n> +\n> +\t\tgit checkout B &&\n> +\t\techo x >a/x &&\n> +\t\tgit add a &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"B\" &&\n> +\n> +\t\tgit branch C A &&\n> +\t\tgit branch D B &&\n> +\n> +\t\tgit checkout C &&\n> +\t\ttest_must_fail git -c merge.directoryRenames=conflict merge B &&\n> +\t\tgit add b/x &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"C\" &&\n> +\n> +\n> +\t\tgit checkout D &&\n> +\t\ttest_must_fail git -c merge.directoryRenames=conflict merge A &&\n> +\t\tgit add b/x &&\n> +\t\tmkdir a &&\n> +\t\tgit mv b/x a/x &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"D\"\n> +\t)\n> +'\n> +\n> +test_expect_success '13e-check: directory rename detection in recursive case' '\n> +\t(\n> +\t\tcd 13e &&\n> +\n> +\t\tgit checkout --quiet D^0 &&\n> +\n> +\t\tgit -c merge.directoryRenames=conflict merge -s recursive C^0 >out 2>err &&\n> +\n> +\t\ttest_i18ngrep ! CONFLICT out &&\n> +\t\ttest_i18ngrep ! BUG: err &&\n> +\t\ttest_i18ngrep ! core.dumped err &&\n> +\t\ttest_must_be_empty err &&\n> +\n> +\t\tgit ls-files >paths &&\n> +\t\t! grep a/x paths &&\n> +\t\tgrep b/x paths\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"380018","messageId":"xmqqwofqjjdh.fsf@gitster-ct.c.googlers.com","threadId":"51535","inReplyTo":"20190805223350.27504-1-newren@gmail.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-06T17:26:02Z","receivedAt":"2019-08-06T17:26:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I know this bug doesn't satisfy the normal criteria for making it into\n> 2.23 (it's a bug that was present in 2.22 rather than a regression in\n> 2.23), but given that it's a BUG() condition, I was hoping it is\n> important and safe enough to include anyway.\n\nFor maintenance and upcoming release, a safer \"fix\" to do might be\nto also (in addition to this patch) flip the default to no to revert\nto the stable state before \"directory renames\" was introduced, while\nstill allowing those who want to help can explore the right fix to\nthis codepath.\n\n> (This fix does merge down cleanly to master, next, and pu.)\n>\n> merge-recursive.c | 3 +-\n> t/t6043-merge-rename-directories.sh | 111 ++++++++++++++++++++++++++++\n> 2 files changed, 113 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d2e380b7ed..c7691d9b54 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2856,7 +2856,8 @@ static int detect_and_process_renames(struct merge_options *opt,\n> \thead_pairs = get_diffpairs(opt, common, head);\n> \tmerge_pairs = get_diffpairs(opt, common, merge);\n> \n> -\tif (opt->detect_directory_renames) {\n> +\tif ((opt->detect_directory_renames == 2) ||\n> +\t (opt->detect_directory_renames == 1 && !opt->call_depth)) {\n\n\n"},{"id":"380019","messageId":"CABPp-BHpbmZQyO72N7i_wn9RpE-bA-YGLZCR-9CDt-QGfExmGA@mail.gmail.com","threadId":"51535","inReplyTo":"xmqq1rxykz91.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-06T17:26:24Z","receivedAt":"2019-08-06T17:26:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 6, 2019 at 9:57 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > Ever since commit 8c8e5bd6eb33 (\"merge-recursive: switch directory\n> > rename detection default\", 2019-04-05), the default handling with\n> > directory rename detection was to report a conflict and leave unstaged\n> > entries in the index. However, when creating a virtual merge base in\n> > the recursive case, we absolutely need a tree, and the only way a tree\n> > can be written is if we have no unstaged entries -- otherwise we hit a\n> > BUG().\n> >\n> > There are a few fixes possible here which at least fix the BUG(), but\n> > none of them seem optimal for other reasons; see the comments with the\n> > new testcase 13e in t6043 for details (which testcase triggered a BUG()\n> > prior to this patch). As such, just opt for a very conservative and\n> > simple choice that is still relatively reasonable: have the recursive\n> > case treat 'conflict' as 'false' for opt->detect_directory_renames.\n> >\n> > Reported-by: Emily Shaffer \n> > Signed-off-by: Elijah Newren \n> > ---\n> >\n> > I really should introduce constants like\n> > DETECT_DIRECTORY_RENAMES_NEVER = 0\n> > DETECT_DIRECTORY_RENAMES_CONFLICT = 1\n> > DETECT_DIRECTORY_RENAMES_YES = 2\n>\n> How would they compare with MERGE_DIRECTORY_RENAMES_* macros\n> I see at the tip of 'pu'? init_merge_options() seems to read\n> one of those values from the \"repo settings\" and copies it to\n> the detect_directory_renames field, so I am reading that they\n> must be identical.\n\nIndeed, it looks like Stolee has done my work for me -- though I would\nsuspect that his change won't be merged down to maint, and I am\nassuming that my patch might be. If you don't want to merge it down\nto maint, I can rebase on Stolee's patch; just let me know. (And if\nyou do want to merge it down to maint, I can send a subsequent patch\nto modify and adopt Stolee's naming.)\n\n> > and then use them in the code to make it clearer, but I wanted to make\n> > the code change as simple and contained as possible given that this is\n> > built on maint, fixes a BUG() and we're already in -rc1.\n> >\n> > I know this bug doesn't satisfy the normal criteria for making it into\n> > 2.23 (it's a bug that was present in 2.22 rather than a regression in\n> > 2.23), but given that it's a BUG() condition, I was hoping it is\n> > important and safe enough to include anyway.\n>\n> I do agree that it is sensible to avoid doing any funky thing during\n> the virtual base merges, whose result is much less observable (hence\n> harder to form the right mental model in end user's head) than the\n> outermost merge. Do we want to allow this for inner merges when the\n> setting is 2? Wouldn't that hit the same BUG()?\n\nNo, it doesn't hit the same BUG(). Hitting the BUG() only came from\nhaving unstaged entries at the end of an inner merge, which only came\nfrom the 'conflict' setting for directory rename detection. Having\ndirectory rename detection completely on (detect the rename and accept\nit as the resolution), or completely off (don't detect directory\nrenames, i.e. leave old pathname as the resolution) both have well\ndefined resolutions.\n\nBut that doesn't completely answer your question about whether we want\nto have directory rename detection for the inner merges, it just\nsuggests we have to avoid 'conflict' as a setting in that case. Let\nme look at the same testcase:\n\n> > +# Testcase 13e, directory rename in virtual merge base\n> > +#\n> > +# This testcase has a slightly different setup than all the above cases, in\n> > +# order to include a recursive case:\n> > +#\n> > +# A C\n> > +# o - o\n> > +# / \\ / \\\n> > +# O o X ?\n> > +# \\ / \\ /\n> > +# o o\n> > +# B D\n> > +#\n> > +# Commit O: a/{z,y}\n> > +# Commit A: b/{z,y}\n> > +# Commit B: a/{z,y,x}\n> > +# Commit C: b/{z,y,x}\n> > +# Commit D: b/{z,y}, a/x\n\nHere, if the user has directory rename detection fully on\n(opt->detect_directory_renames == 2), then the fact that commits C and\nD resolved the merge differently is perhaps more surprising, because\nthe default would be to just accept the rename. That means one side\n(commit C) is likely to have just taken the default resolution, but\nsomeone (whoever created commit D) made a manual effort to undo the\ndirectory rename. Now, if the virtual merge base doesn't do directory\nrename detection, then it'll match commit D which undid the directory\nrename detection, and result in commit C winning and using the\ndirectory rename. That seems like the wrong choice to me.\n\nSo, I think that if opt->detect_directory_renames == 2, then we really\ndo want to use directory rename detection on the virtual merge base so\nthat we don't silently miss someone manually undoing the directory\nrename detection on one side of history.\n\nThe trickier choice comes when opt->detect_directory_renames == 1 (or\n'conflict') because then we don't have a sane\navoid-accidentally-matching-one-side solution. I'm not sure there's a\n\"best\" choice for this case, but we should certainly at least avoid a\nBUG(). The simple ways to do that were to translate 'conflict' to\neither 'false' or 'true'. I can't give a good rationale for why I\npicked 'false' over 'true' in that case with this patch; either seem\nequally good, but 'false' has a slight performance advantage over\n'true' so I went with it.\n"},{"id":"380020","messageId":"CABPp-BEW4C__h+MVibmSNz3n3CaVDeDOxxYSfTgXj5NR_HfULw@mail.gmail.com","threadId":"51535","inReplyTo":"xmqqwofqjjdh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-06T17:29:00Z","receivedAt":"2019-08-06T17:29:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 6, 2019 at 10:26 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > I know this bug doesn't satisfy the normal criteria for making it into\n> > 2.23 (it's a bug that was present in 2.22 rather than a regression in\n> > 2.23), but given that it's a BUG() condition, I was hoping it is\n> > important and safe enough to include anyway.\n>\n> For maintenance and upcoming release, a safer \"fix\" to do might be\n> to also (in addition to this patch) flip the default to no to revert\n> to the stable state before \"directory renames\" was introduced, while\n> still allowing those who want to help can explore the right fix to\n> this codepath.\n\nThat might be a bit more jarring. Directory rename detection did not\nhave this bug in git 2.18, 2.19, 2.20, or 2.21. It was new to 2.22\nwith the 'conflict' setting\n"},{"id":"380030","messageId":"xmqqsgqejiad.fsf@gitster-ct.c.googlers.com","threadId":"51535","inReplyTo":"xmqq1rxykz91.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-06T17:49:30Z","receivedAt":"2019-08-06T17:49:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I do agree that it is sensible to avoid doing any funky thing during\n> the virtual base merges, whose result is much less observable (hence\n> harder to form the right mental model in end user's head) than the\n> outermost merge. Do we want to allow this for inner merges when the\n> setting is 2? Wouldn't that hit the same BUG()?\n\nAh, actually no. 2 would make a choice of committing to one\npossible merge result and does not leave options to be resolved as\nconflicted, so we should be able to write out its decision as a\ntree. If we cannot form a tree object when operating with 2, we do\nhave a bug and we do want to know about it.\n\n"},{"id":"380042","messageId":"20190806202829.GB26909@google.com","threadId":"51535","inReplyTo":"20190805223350.27504-1-newren@gmail.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-08-06T20:28:29Z","receivedAt":"2019-08-06T20:28:37Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Aug 05, 2019 at 03:33:50PM -0700, Elijah Newren wrote:\n> Ever since commit 8c8e5bd6eb33 (\"merge-recursive: switch directory\n> rename detection default\", 2019-04-05), the default handling with\n> directory rename detection was to report a conflict and leave unstaged\n> entries in the index. However, when creating a virtual merge base in\n> the recursive case, we absolutely need a tree, and the only way a tree\n> can be written is if we have no unstaged entries -- otherwise we hit a\n> BUG().\n> \n> There are a few fixes possible here which at least fix the BUG(), but\n> none of them seem optimal for other reasons; see the comments with the\n> new testcase 13e in t6043 for details (which testcase triggered a BUG()\n> prior to this patch). As such, just opt for a very conservative and\n> simple choice that is still relatively reasonable: have the recursive\n> case treat 'conflict' as 'false' for opt->detect_directory_renames.\n> \n> Reported-by: Emily Shaffer \n> Signed-off-by: Elijah Newren \n> ---\n> \n> I really should introduce constants like\n> DETECT_DIRECTORY_RENAMES_NEVER = 0\n> DETECT_DIRECTORY_RENAMES_CONFLICT = 1\n> DETECT_DIRECTORY_RENAMES_YES = 2\n> and then use them in the code to make it clearer, but I wanted to make\n> the code change as simple and contained as possible given that this is\n> built on maint, fixes a BUG() and we're already in -rc1.\n> \n> I know this bug doesn't satisfy the normal criteria for making it into\n> 2.23 (it's a bug that was present in 2.22 rather than a regression in\n> 2.23), but given that it's a BUG() condition, I was hoping it is\n> important and safe enough to include anyway.\n> \n> (This fix does merge down cleanly to master, next, and pu.)\n\nThanks for picking this up and sorry I didn't end up sending anything -\npriority shifts on this end. :)\n\n> \n> merge-recursive.c | 3 +-\n> t/t6043-merge-rename-directories.sh | 111 ++++++++++++++++++++++++++++\n> 2 files changed, 113 insertions(+), 1 deletion(-)\n> \n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d2e380b7ed..c7691d9b54 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2856,7 +2856,8 @@ static int detect_and_process_renames(struct merge_options *opt,\n> \thead_pairs = get_diffpairs(opt, common, head);\n> \tmerge_pairs = get_diffpairs(opt, common, merge);\n> \n> -\tif (opt->detect_directory_renames) {\nSo we used to say, \"If you want to apply directory renames, or mark\ndirectory renames as merge conflicts, we will go look for renames no\nmatter what.\"\n\n> +\tif ((opt->detect_directory_renames == 2) ||\n> +\t (opt->detect_directory_renames == 1 && !opt->call_depth)) {\nBut now we say, \"If you want to apply directory renames always, or if\nyou want to mark them as conflicts AND you aren't below the first layer\nof a recursive merge, then we will go look for renames.\"\n\n> \t\tdir_re_head = get_directory_renames(head_pairs);\n> \t\tdir_re_merge = get_directory_renames(merge_pairs);\nThat means that when we usually prefer to mark directory renames as\nconflicts and we are putting together a virtual ancestor, we\ndon't try to detect renames at all, which I imagine leaves it to the top\nlevel merge to mark as conflicts for the user to resolve much later.\n> \n> diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh\n> index 50b7543483..c966147d5d 100755\n> --- a/t/t6043-merge-rename-directories.sh\n> +++ b/t/t6043-merge-rename-directories.sh\n> @@ -4403,4 +4403,115 @@ test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual\n> \t)\n> '\n> \n> +# Testcase 13e, directory rename in virtual merge base\n> +#\n> +# This testcase has a slightly different setup than all the above cases, in\n> +# order to include a recursive case:\n> +#\n> +# A C\n> +# o - o\n> +# / \\ / \\\n> +# O o X ?\n> +# \\ / \\ /\n> +# o o\n> +# B D\n> +#\n> +# Commit O: a/{z,y}\n> +# Commit A: b/{z,y}\n> +# Commit B: a/{z,y,x}\n> +# Commit C: b/{z,y,x}\n> +# Commit D: b/{z,y}, a/x\n> +# Expected: b/{z,y,x} (sort of; see below for why this might not be expected)\n> +#\n> +# NOTES: 'X' represents a virtual merge base. With the default of\n> +# directory rename detection yielding conflicts, merging A and B\n> +# results in a conflict complaining about whether 'x' should be\n> +# under 'a/' or 'b/'. However, when creating the virtual merge\n> +# base 'X', since virtual merge bases need to be written out as a\n> +# tree, we cannot have a conflict, so some resolution has to be\n> +# picked.\n> +#\n> +# In choosing the right resolution, it's worth noting here that\n> +# commits C & D are merges of A & B that choose different\n> +# locations for 'x' (i.e. they resolve the conflict differently),\n> +# and so it would be nice when merging C & D if git could detect\n> +# this difference of opinion and report a conflict. But the only\n> +# way to do so that I can think of would be to have the virtual\n> +# merge base place 'x' in some directory other than either 'a/' or\n> +# 'b/', which seems a little weird -- especially since it'd result\n> +# in a rename/rename(1to2) conflict with a source path that never\n> +# existed in any version.\n> +#\n> +# So, for now, when directory rename detection is set to\n> +# 'conflict' just avoid doing directory rename detection at all in\n> +# the recursive case. This will not allow us to detect a conflict\n> +# in the outer merge for this special kind of setup, but it at\n> +# least avoids hitting a BUG().\n> +#\n> +test_expect_success '13e-setup: directory rename detection in recursive case' '\n> +\ttest_create_repo 13e &&\n> +\t(\n> +\t\tcd 13e &&\n> +\n> +\t\tmkdir a &&\n> +\t\techo z >a/z &&\n> +\t\techo y >a/y &&\n> +\t\tgit add a &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"O\" &&\n> +\n> +\t\tgit branch O &&\n> +\t\tgit branch A &&\n> +\t\tgit branch B &&\n> +\n> +\t\tgit checkout A &&\n> +\t\tgit mv a/ b/ &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"A\" &&\nSo we do a directory rename in branch A...\n\n> +\n> +\t\tgit checkout B &&\n> +\t\techo x >a/x &&\n> +\t\tgit add a &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"B\" &&\n... And we add a new file to the directory in question in branch B...\n\nSo A and B will be the ones combined to make a virtual ancestor.\n> +\n> +\t\tgit branch C A &&\n> +\t\tgit branch D B &&\n> +\n> +\t\tgit checkout C &&\n> +\t\ttest_must_fail git -c merge.directoryRenames=conflict merge B &&\n> +\t\tgit add b/x &&\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"C\" &&\n\nThen we merge C with B which places B as a mutual ancestor of D as well\nas C.\n\n> +\n> +\n> +\t\tgit checkout D &&\n> +\t\ttest_must_fail git -c merge.directoryRenames=conflict merge A &&\n\nNow we do the same thing merging A with D, which means that D has\nancestors B from branching and A from merge, and C has ancestors A from\nbranching and B from merge. So D and C have two closest ancestors\n(criss-cross merge).\n\n> +\t\tgit add b/x &&\n> +\t\tmkdir a &&\n> +\t\tgit mv b/x a/x &&\n\nNow D adds contention over a/x and b/x (which were both mentioned in the\nancestry too) to induce a conflict... or is this adding a resolution\nwhich can be decided on automatically? I guess later you are looking to\nmake sure no CONFLICT still exists in the output, so you must be\nresolving the conflict here?\n\n> +\t\ttest_tick &&\n> +\t\tgit commit -m \"D\"\n> +\t)\n> +'\n> +\n> +test_expect_success '13e-check: directory rename detection in recursive case' '\n> +\t(\n> +\t\tcd 13e &&\n> +\n> +\t\tgit checkout --quiet D^0 &&\n> +\n> +\t\tgit -c merge.directoryRenames=conflict merge -s recursive C^0 >out 2>err &&\n\nNow we finally do the recursive merge - C and D have equally likely\nancestors A and B, and A and B have a rename conflict.\n\n> +\n> +\t\ttest_i18ngrep ! CONFLICT out &&\n> +\t\ttest_i18ngrep ! BUG: err &&\n\nThe BUG is gone. But should it not use i18ngrep? BUG() isn't localized.\n\n> +\t\ttest_i18ngrep ! core.dumped err &&\n> +\t\ttest_must_be_empty err &&\n> +\n> +\t\tgit ls-files >paths &&\n> +\t\t! grep a/x paths &&\nFinally, make sure that a/x has been truly disappeared...\n\n> +\t\tgrep b/x paths\n...and b/x is the only x left standing.\n> +\t)\n> +'\n> +\n> test_done\n> -- \n> 2.22.0.246.g5ddf3d502a\n> \n"},{"id":"380046","messageId":"CABPp-BFZig73jXKkKNaz=YpT4enB3_ARQ1KTz_ttPYobkY6Bhg@mail.gmail.com","threadId":"51535","inReplyTo":"20190806202829.GB26909@google.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-06T21:16:25Z","receivedAt":"2019-08-06T21:16:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 6, 2019 at 1:28 PM Emily Shaffer wrote:\n>\n> On Mon, Aug 05, 2019 at 03:33:50PM -0700, Elijah Newren wrote:\n> > Ever since commit 8c8e5bd6eb33 (\"merge-recursive: switch directory\n> > rename detection default\", 2019-04-05), the default handling with\n> > directory rename detection was to report a conflict and leave unstaged\n> > entries in the index. However, when creating a virtual merge base in\n> > the recursive case, we absolutely need a tree, and the only way a tree\n> > can be written is if we have no unstaged entries -- otherwise we hit a\n> > BUG().\n...\n> Thanks for picking this up and sorry I didn't end up sending anything -\n> priority shifts on this end. :)\n\nI totally understand.\n\n...\n> > + git checkout C &&\n> > + test_must_fail git -c merge.directoryRenames=conflict merge B &&\n> > + git add b/x &&\n> > + test_tick &&\n> > + git commit -m \"C\" &&\n>\n> Then we merge C with B which places B as a mutual ancestor of D as well\n> as C.\n>\n> > +\n> > +\n> > + git checkout D &&\n> > + test_must_fail git -c merge.directoryRenames=conflict merge A &&\n>\n> Now we do the same thing merging A with D, which means that D has\n> ancestors B from branching and A from merge, and C has ancestors A from\n> branching and B from merge. So D and C have two closest ancestors\n> (criss-cross merge).\n>\n> > + git add b/x &&\n> > + mkdir a &&\n> > + git mv b/x a/x &&\n>\n> Now D adds contention over a/x and b/x (which were both mentioned in the\n> ancestry too) to induce a conflict... or is this adding a resolution\n> which can be decided on automatically? I guess later you are looking to\n> make sure no CONFLICT still exists in the output, so you must be\n> resolving the conflict here?\n\nYes, we are resolving the conflict for D by choosing to reject the\ndirectory rename, placing 'x' in a/x instead of b/x.\n\nWhen merge.directoryRenames=conflict, it'll make only b/x be present\nin the working directory and mark it as conflicted (i.e. assume the\ndirectory rename is probably the right resolution, but print a warning\nand mark the index as needing to be updated to verify -- this allows\ne.g. \"git add -u\" to do the \"right thing\"). For commit C, we just did\na \"git add b/x\" to accept the directory rename. For commit D, we\nwanted to say we didn't want the directory rename which you'd first\nguess would be \"git mv b/x a/x\" BUT: (1) a/x has unstaged entries\nwhich will cause git-mv to fail, and (2) directory a/ didn't exist --\nboth of these issues had to be corrected before running git-mv.\n\n> > + test_tick &&\n> > + git commit -m \"D\"\n> > + )\n> > +'\n> > +\n> > +test_expect_success '13e-check: directory rename detection in recursive case' '\n> > + (\n> > + cd 13e &&\n> > +\n> > + git checkout --quiet D^0 &&\n> > +\n> > + git -c merge.directoryRenames=conflict merge -s recursive C^0 >out 2>err &&\n>\n> Now we finally do the recursive merge - C and D have equally likely\n> ancestors A and B, and A and B have a rename conflict.\n>\n> > +\n> > + test_i18ngrep ! CONFLICT out &&\n> > + test_i18ngrep ! BUG: err &&\n>\n> The BUG is gone. But should it not use i18ngrep? BUG() isn't localized.\n\nTechnically, yes, you're right. However, this line's purpose isn't\ncorrectness of the test but documentation for the person reading the\ntestcase about what it's real original purpose was; my real test was\nthe \"test_must_be_empty err\" check I have below it, but I added this\nline just to document the intent better. I kind of like the\n\"CONFLICT\" and \"BUG\" lines looking similar just so the reader of the\ntestcase doesn't have to try to reason through why they are different,\nalthough I guess it does present the problem that more careful readers\nlike yourself might do a double take.\n\nIf folks find it more readable to use regular grep instead of\ntest_i18ngrep, I can change this line as well as the \"core dumped\"\ncheck immediately below over to regular grep.\n\n> > + test_i18ngrep ! core.dumped err &&\n> > + test_must_be_empty err &&\n> > +\n> > + git ls-files >paths &&\n> > + ! grep a/x paths &&\n> Finally, make sure that a/x has been truly disappeared...\n>\n> > + grep b/x paths\n> ...and b/x is the only x left standing.\n\n\nThanks for taking a look. :-)\n"},{"id":"380047","messageId":"20190806215446.GA59629@google.com","threadId":"51535","inReplyTo":"CABPp-BFZig73jXKkKNaz=YpT4enB3_ARQ1KTz_ttPYobkY6Bhg@mail.gmail.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-08-06T21:54:46Z","receivedAt":"2019-08-06T21:54:56Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"> > > +\n> > > + test_i18ngrep ! CONFLICT out &&\n> > > + test_i18ngrep ! BUG: err &&\n> >\n> > The BUG is gone. But should it not use i18ngrep? BUG() isn't localized.\n> \n> Technically, yes, you're right. However, this line's purpose isn't\n> correctness of the test but documentation for the person reading the\n> testcase about what it's real original purpose was; my real test was\n> the \"test_must_be_empty err\" check I have below it, but I added this\n> line just to document the intent better. I kind of like the\n> \"CONFLICT\" and \"BUG\" lines looking similar just so the reader of the\n> testcase doesn't have to try to reason through why they are different,\n> although I guess it does present the problem that more careful readers\n> like yourself might do a double take.\n> \n> If folks find it more readable to use regular grep instead of\n> test_i18ngrep, I can change this line as well as the \"core dumped\"\n> check immediately below over to regular grep.\n> \n\nIt's fine by me, I see your point.\n\n> Thanks for taking a look. :-)\n\nReviewed-by: Emily Shaffer \n"},{"id":"380116","messageId":"20190808110023.GB14189@sigill.intra.peff.net","threadId":"51535","inReplyTo":"CABPp-BFZig73jXKkKNaz=YpT4enB3_ARQ1KTz_ttPYobkY6Bhg@mail.gmail.com","subject":"Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-08T11:00:24Z","receivedAt":"2019-08-08T11:00:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 06, 2019 at 02:16:25PM -0700, Elijah Newren wrote:\n\n> > > + test_i18ngrep ! CONFLICT out &&\n> > > + test_i18ngrep ! BUG: err &&\n> >\n> > The BUG is gone. But should it not use i18ngrep? BUG() isn't localized.\n> \n> Technically, yes, you're right. However, this line's purpose isn't\n> correctness of the test but documentation for the person reading the\n> testcase about what it's real original purpose was; my real test was\n> the \"test_must_be_empty err\" check I have below it, but I added this\n> line just to document the intent better. I kind of like the\n> \"CONFLICT\" and \"BUG\" lines looking similar just so the reader of the\n> testcase doesn't have to try to reason through why they are different,\n> although I guess it does present the problem that more careful readers\n> like yourself might do a double take.\n\nI think it would be better to drop the grep for BUG entirely.\n\nNot BUG()-ing should be something we implicitly assume for all commands,\nand checking the exit code already covers that[1]. I don't think we\nshould be cluttering up every test, even ones that are in response to a\nBUG(), with redundant checks. If you really want to document it further,\na comment can do that without incurring extra run-time overhead. But I\nthink in this case that your existing comments and commit message cover\nit quite well.\n\n-Peff\n\n[1] There are cases where there's a crash in a sub-process, but in that\n case the failure should be surfaced in the way the test is written.\n It is here, and I'd argue that any case where it isn't probably\n ought to be rewritten (because you're missing not just BUG()s, but\n probably die()).\n"}]}