{"thread":{"id":"48837","subject":"[PATCH 0/3] Add testcases showing suboptimal submodule/path conflict handling","startedAt":"2018-07-07T20:44:17Z","lastAt":"2018-07-11T03:57:18Z","messageCount":12,"participants":["Elijah Newren","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"351854","messageId":"20180707204404.7208-1-newren@gmail.com","threadId":"48837","inReplyTo":null,"subject":"[PATCH 0/3] Add testcases showing suboptimal submodule/path conflict handling","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-07T20:44:01Z","receivedAt":"2018-07-07T20:44:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Last November, Stefan asked me about an issue with submodules. In\naddition to providing a fix for that issue, I mentioned a few other\nproblems I noticed with submodules and merging[1]. Turn those issues\ninto testcases (as I probably should have done back then).\n\n[1] https://public-inbox.org/git/CABPp-BHDrw_dAESic3xK7kC3jMgKeNQuPQF69OpbVYhRkbhJsw@mail.gmail.com/\n\nElijah Newren (3):\n t7405: add a file/submodule conflict\n t7405: add a directory/submodule conflict\n t7405: verify 'merge --abort' works after submodule/path conflicts\n\n t/t7405-submodule-merge.sh | 173 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 173 insertions(+)\n\n-- \n2.18.0.134.gafc206209.dirty\n\n"},{"id":"351855","messageId":"20180707204404.7208-2-newren@gmail.com","threadId":"48837","inReplyTo":"20180707204404.7208-1-newren@gmail.com","subject":"[PATCH 1/3] t7405: add a file/submodule conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-07T20:44:02Z","receivedAt":"2018-07-07T20:44:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In the case of a file/submodule conflict, although both cannot exist at\nthe same path, we expect both to be present somewhere for the user to be\nable to resolve the conflict with. Add a testcase for this.\n\nSigned-off-by: Elijah Newren \n---\n t/t7405-submodule-merge.sh | 56 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 56 insertions(+)\n\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 7bfb2f498..95fd05d83 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' '\n \t grep \"$(cat expect3)\" actual > /dev/null)\n '\n \n+# File/submodule conflict\n+# Commit O: \n+# Commit A: path (submodule)\n+# Commit B: path\n+# Expected: path/ is submodule and file contents for B's path are somewhere\n+\n+test_expect_success 'setup file/submodule conflict' '\n+\ttest_create_repo file-submodule &&\n+\t(\n+\t\tcd file-submodule &&\n+\n+\t\tgit commit --allow-empty -m O &&\n+\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout B &&\n+\t\techo contents >path &&\n+\t\tgit add path &&\n+\t\tgit commit -m B &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_create_repo path &&\n+\t\t(\n+\t\t\tcd path &&\n+\n+\t\t\techo hello >world &&\n+\t\t\tgit add world &&\n+\t\t\tgit commit -m \"submodule\"\n+\t\t) &&\n+\t\tgit add path &&\n+\t\tgit commit -m A\n+\t)\n+'\n+\n+test_expect_failure 'file/submodule conflict' '\n+\ttest_when_finished \"git -C file-submodule reset --hard\" &&\n+\t(\n+\t\tcd file-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B^0 >out 2>err &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 2 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 2 out &&\n+\n+\t\t# path/ is still a submodule\n+\t\ttest_path_is_dir path/.git &&\n+\n+\t\techo Checking if contents from B:path showed up anywhere &&\n+\t\tgrep -q content * 2>/dev/null\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.134.gafc206209.dirty\n\n"},{"id":"351856","messageId":"20180707204404.7208-4-newren@gmail.com","threadId":"48837","inReplyTo":"20180707204404.7208-1-newren@gmail.com","subject":"[PATCH 3/3] t7405: verify 'merge --abort' works after submodule/path conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-07T20:44:04Z","receivedAt":"2018-07-07T20:44:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n t/t7405-submodule-merge.sh | 26 ++++++++++++++++++++++++++\n 1 file changed, 26 insertions(+)\n\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 6cb51c966..9f71a4859 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -335,6 +335,19 @@ test_expect_failure 'file/submodule conflict' '\n \t)\n '\n \n+test_expect_success 'file/submodule conflict; merge --abort works afterward' '\n+\ttest_when_finished \"git -C file-submodule reset --hard\" &&\n+\t(\n+\t\tcd file-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B^0 >out 2>err &&\n+\n+\t\ttest_path_is_file .git/MERGE_HEAD &&\n+\t\tgit merge --abort\n+\t)\n+'\n+\n # Directory/submodule conflict\n # Commit O: \n # Commit A: path (submodule), with sole tracked file named 'world'\n@@ -422,7 +435,20 @@ test_expect_failure 'directory/submodule conflict; should not treat submodule fi\n \t\t# merge from starting; we should not be writing to such paths\n \t\t# anyway.\n \t\ttest_i18ngrep ! \"refusing to lose untracked file at\" err\n+\t)\n+'\n+\n+test_expect_failure 'directory/submodule conflict; merge --abort works afterward' '\n+\ttest_when_finished \"git -C directory-submodule/path reset --hard\" &&\n+\ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n+\t(\n+\t\tcd directory-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B2^0 >out 2>err &&\n \n+\t\ttest_path_is_file .git/MERGE_HEAD &&\n+\t\tgit merge --abort\n \t)\n '\n \n-- \n2.18.0.134.gafc206209.dirty\n\n"},{"id":"351857","messageId":"20180707204404.7208-3-newren@gmail.com","threadId":"48837","inReplyTo":"20180707204404.7208-1-newren@gmail.com","subject":"[PATCH 2/3] t7405: add a directory/submodule conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-07T20:44:03Z","receivedAt":"2018-07-07T20:44:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For a directory/submodule conflict, we want contents from both the\ndirectory and the submodule to be present for the user to use to resolve\nthe conflict, but we do not want paths under the directory being written\ninto the submodule and we do not want the merge being confused by paths\nunder the submodule being in the way. Add testcases for these situations.\n\nSigned-off-by: Elijah Newren \n---\n t/t7405-submodule-merge.sh | 91 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 91 insertions(+)\n\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 95fd05d83..6cb51c966 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -335,4 +335,95 @@ test_expect_failure 'file/submodule conflict' '\n \t)\n '\n \n+# Directory/submodule conflict\n+# Commit O: \n+# Commit A: path (submodule), with sole tracked file named 'world'\n+# Commit B1: path/file\n+# Commit B2: path/world\n+#\n+# Expected from merge of A & B1:\n+# Contents under path/ from commit B1 are renamed elsewhere; we do not\n+# want to write files from one of our tracked directories into a submodule\n+#\n+# Expected from merge of A & B2:\n+# Similar to last merge, but with a slight twist: we don't want paths\n+# under the submodule to be treated as untracked or in the way.\n+\n+test_expect_success 'setup directory/submodule conflict' '\n+\ttest_create_repo directory-submodule &&\n+\t(\n+\t\tcd directory-submodule &&\n+\n+\t\tgit commit --allow-empty -m O &&\n+\n+\t\tgit branch A &&\n+\t\tgit branch B1 &&\n+\t\tgit branch B2 &&\n+\n+\t\tgit checkout B1 &&\n+\t\tmkdir path &&\n+\t\techo contents >path/file &&\n+\t\tgit add path/file &&\n+\t\tgit commit -m B1 &&\n+\n+\t\tgit checkout B2 &&\n+\t\tmkdir path &&\n+\t\techo contents >path/world &&\n+\t\tgit add path/world &&\n+\t\tgit commit -m B2 &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_create_repo path &&\n+\t\t(\n+\t\t\tcd path &&\n+\n+\t\t\techo hello >world &&\n+\t\t\tgit add world &&\n+\t\t\tgit commit -m \"submodule\"\n+\t\t) &&\n+\t\tgit add path &&\n+\t\tgit commit -m A\n+\t)\n+'\n+\n+test_expect_failure 'directory/submodule conflict; keep submodule clean' '\n+\ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n+\t(\n+\t\tcd directory-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B1^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 2 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 1 out &&\n+\n+\t\t# path/ is still a submodule\n+\t\ttest_path_is_dir path/.git &&\n+\n+\t\techo Checking if contents from B1:path/file showed up &&\n+\t\t# Would rather use grep -r, but that is GNU extension...\n+\t\tgit ls-files -co | xargs grep -q contents 2>/dev/null &&\n+\t\ttest_path_is_missing path/file\n+\t)\n+'\n+\n+test_expect_failure 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '\n+\ttest_when_finished \"git -C directory-submodule/path reset --hard\" &&\n+\ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n+\t(\n+\t\tcd directory-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B2^0 >out 2>err &&\n+\n+\t\t# We do not want files within the submodule to prevent the\n+\t\t# merge from starting; we should not be writing to such paths\n+\t\t# anyway.\n+\t\ttest_i18ngrep ! \"refusing to lose untracked file at\" err\n+\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.134.gafc206209.dirty\n\n"},{"id":"352029","messageId":"CAGZ79kZ0xyHMT8SngGCUSAEf31NXdPvzkvdaPb8Khm27xRjgTg@mail.gmail.com","threadId":"48837","inReplyTo":"20180707204404.7208-2-newren@gmail.com","subject":"Re: [PATCH 1/3] t7405: add a file/submodule conflict","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-09T21:11:47Z","receivedAt":"2018-07-09T21:12:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Jul 7, 2018 at 1:44 PM Elijah Newren wrote:\n>\n> In the case of a file/submodule conflict, although both cannot exist at\n> the same path, we expect both to be present somewhere for the user to be\n> able to resolve the conflict with. Add a testcase for this.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> t/t7405-submodule-merge.sh | 56 ++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 56 insertions(+)\n>\n> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\n> index 7bfb2f498..95fd05d83 100755\n> --- a/t/t7405-submodule-merge.sh\n> +++ b/t/t7405-submodule-merge.sh\n> @@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' '\n> grep \"$(cat expect3)\" actual > /dev/null)\n> '\n>\n> +# File/submodule conflict\n> +# Commit O: \n> +# Commit A: path (submodule)\n> +# Commit B: path\n> +# Expected: path/ is submodule and file contents for B's path are somewhere\n> +\n> +test_expect_success 'setup file/submodule conflict' '\n> + test_create_repo file-submodule &&\n> + (\n> + cd file-submodule &&\n> +\n> + git commit --allow-empty -m O &&\n> +\n> + git branch A &&\n> + git branch B &&\n> +\n> + git checkout B &&\n> + echo contents >path &&\n> + git add path &&\n> + git commit -m B &&\n> +\n> + git checkout A &&\n> + test_create_repo path &&\n> + (\n> + cd path &&\n> +\n> + echo hello >world &&\n\n test_commit -C path &&\n\nor do we need a dirty submodule specifically?\nIf so what is important, the untracked file or\nwould changes in the index be sufficient?\n\n> + git add world &&\n\nwhen observing this one in verbose mode , you may be\nasked to use 'git submodule add' instead\nhttps://github.com/git/git/blob/master/builtin/add.c#L323\n\n> + git commit -m \"submodule\"\n\nNot sure if we'd need the shell it is only test_commit,\nthe [submodule] add and commit, so maybe we can get away with\n3 times -C ?\n\n> +test_expect_failure 'file/submodule conflict' '\n\nWhich part fails here?\n\n> + test_when_finished \"git -C file-submodule reset --hard\" &&\n> + (\n> + cd file-submodule &&\n> +\n> + git checkout A^0 &&\n> + test_must_fail git merge B^0 >out 2>err &&\n> +\n> + git ls-files -s >out &&\n> + test_line_count = 2 out &&\n> + git ls-files -u >out &&\n> + test_line_count = 2 out &&\n> +\n> + # path/ is still a submodule\n> + test_path_is_dir path/.git &&\n> +\n> + echo Checking if contents from B:path showed up anywhere &&\n\nThis could be a comment instead?\n\n> + grep -q content * 2>/dev/null\n\nAh that is why we needed the specific echo above.\n\nSorry for being dense here, I am not quite following the last part of the test,\nas it is unclear what ought to fail in this test.\n\nThanks,\nStefan\n"},{"id":"352113","messageId":"CABPp-BFjZZZ9uGXYe0gAA_VvcFmTsnurAknVrr-CytP2hT6wLg@mail.gmail.com","threadId":"48837","inReplyTo":"CAGZ79kZ0xyHMT8SngGCUSAEf31NXdPvzkvdaPb8Khm27xRjgTg@mail.gmail.com","subject":"Re: [PATCH 1/3] t7405: add a file/submodule conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-10T15:28:57Z","receivedAt":"2018-07-10T15:29:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jul 9, 2018 at 2:11 PM, Stefan Beller wrote:\n> On Sat, Jul 7, 2018 at 1:44 PM Elijah Newren wrote:\n>>\n>> In the case of a file/submodule conflict, although both cannot exist at\n>> the same path, we expect both to be present somewhere for the user to be\n>> able to resolve the conflict with. Add a testcase for this.\n>>\n>> Signed-off-by: Elijah Newren \n>> ---\n>> t/t7405-submodule-merge.sh | 56 ++++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 56 insertions(+)\n>>\n>> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\n>> index 7bfb2f498..95fd05d83 100755\n>> --- a/t/t7405-submodule-merge.sh\n>> +++ b/t/t7405-submodule-merge.sh\n>> @@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' '\n>> grep \"$(cat expect3)\" actual > /dev/null)\n>> '\n>>\n>> +# File/submodule conflict\n>> +# Commit O: \n>> +# Commit A: path (submodule)\n>> +# Commit B: path\n>> +# Expected: path/ is submodule and file contents for B's path are somewhere\n>> +\n>> +test_expect_success 'setup file/submodule conflict' '\n>> + test_create_repo file-submodule &&\n>> + (\n>> + cd file-submodule &&\n>> +\n>> + git commit --allow-empty -m O &&\n>> +\n>> + git branch A &&\n>> + git branch B &&\n>> +\n>> + git checkout B &&\n>> + echo contents >path &&\n\nThis should have been 'content' rather than 'contents', given my grep below...\n\n>> + git add path &&\n>> + git commit -m B &&\n>> +\n>> + git checkout A &&\n>> + test_create_repo path &&\n>> + (\n>> + cd path &&\n>> +\n>> + echo hello >world &&\n>\n> test_commit -C path &&\n>\n> or do we need a dirty submodule specifically?\n> If so what is important, the untracked file or\n> would changes in the index be sufficient?\n\nDo you mean\n test_commit -C path hello\nbecause test_commit needs at least one positional argument (which will\nserve as contents, message, filename, and tag)? Anyway, yeah, doing\nthis would remove the whole innermost subshell -- the cd, the echo,\nthe git add, and the git commit.\n\n>\n>> + git add world &&\n>\n> when observing this one in verbose mode , you may be\n> asked to use 'git submodule add' instead\n> https://github.com/git/git/blob/master/builtin/add.c#L323\n\nUm, at this point, I'm adding a regular file -- not a submodule. Also,\nthis git add will disappear if I use test_commit. Perhaps you meant\none of the other 'git add's?\n\n>\n>> + git commit -m \"submodule\"\n>\n> Not sure if we'd need the shell it is only test_commit,\n> the [submodule] add and commit, so maybe we can get away with\n> 3 times -C ?\n\nThis also disappears with test_commit.\n\n>\n>> +test_expect_failure 'file/submodule conflict' '\n>\n> Which part fails here?\n\nAddressed below...\n\n>> + test_when_finished \"git -C file-submodule reset --hard\" &&\n>> + (\n>> + cd file-submodule &&\n>> +\n>> + git checkout A^0 &&\n>> + test_must_fail git merge B^0 >out 2>err &&\n\nI probably shouldn't have redirected stdout and stderr here; makes it\nharder for you to see what's happening, especially since I don't test\neither in any way.\n\n>> +\n>> + git ls-files -s >out &&\n>> + test_line_count = 2 out &&\n>> + git ls-files -u >out &&\n>> + test_line_count = 2 out &&\n>> +\n>> + # path/ is still a submodule\n>> + test_path_is_dir path/.git &&\n>> +\n>> + echo Checking if contents from B:path showed up anywhere &&\n>\n> This could be a comment instead?\n>\n>> + grep -q content * 2>/dev/null\n>\n> Ah that is why we needed the specific echo above.\n\nYeah, if there was a handy\ntest_string_exists_in_some_file_in_current_directory I could have used\nthat, but seems like an oddly specific special test function, so I\njust added an echo so someone watching the test output under --verbose\ncould see how far the test got.\n\n> Sorry for being dense here, I am not quite following the last part of the test,\n> as it is unclear what ought to fail in this test.\n\nHmm, I obviously wasn't nearly as clear as I thought I was. Thinking\nit over, two things may have thrown you off:\n\n1) I had a typo ('content' vs. 'contents')\n2) I didn't just check what was currently failing but other things\nthat should be true for this test.\n\nFor item 2, I've had multiple cases in the past where I created a\nminimal test, only to find that if I had more carefully checked the\nexpected results that it would have prevented a future bug. Also, it\nwas harder in the future to figure out, because I no longer remembered\nthe context for the test setup. So, in this test I check the contents\nof the index, make sure that the submodule is still present, and then\nI finally check for the thing that currently fails:\n\ncommit B added a file called 'path'; its contents should appear\nsomewhere in the directory for the user to use when trying to resolve\nthe failed merge. It cannot appear at 'path' (there's a submodule in\nthe way from commit A), but it should be present somewhere, and in\nparticular I'd expect it in the same directory. So, I simply grep all\nfiles in the current directory for the string (and ignore errors about\n'grep: path is a directory').\n\nDoes that help? If so, I'll submit a reroll with the changes and a\nfew extra comments.\n"},{"id":"352121","messageId":"CAGZ79kYK4uwUeDECoabrXJesVzgFBKNejV3reJOXhMyYxUjXyw@mail.gmail.com","threadId":"48837","inReplyTo":"CABPp-BFjZZZ9uGXYe0gAA_VvcFmTsnurAknVrr-CytP2hT6wLg@mail.gmail.com","subject":"Re: [PATCH 1/3] t7405: add a file/submodule conflict","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-10T15:53:25Z","receivedAt":"2018-07-10T15:53:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jul 10, 2018 at 8:28 AM Elijah Newren wrote:\n>\n> On Mon, Jul 9, 2018 at 2:11 PM, Stefan Beller wrote:\n> > On Sat, Jul 7, 2018 at 1:44 PM Elijah Newren wrote:\n> >>\n> >> In the case of a file/submodule conflict, although both cannot exist at\n> >> the same path, we expect both to be present somewhere for the user to be\n> >> able to resolve the conflict with. Add a testcase for this.\n> >>\n> >> Signed-off-by: Elijah Newren \n> >> ---\n> >> t/t7405-submodule-merge.sh | 56 ++++++++++++++++++++++++++++++++++++++\n> >> 1 file changed, 56 insertions(+)\n> >>\n> >> diff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\n> >> index 7bfb2f498..95fd05d83 100755\n> >> --- a/t/t7405-submodule-merge.sh\n> >> +++ b/t/t7405-submodule-merge.sh\n> >> @@ -279,4 +279,60 @@ test_expect_success 'recursive merge with submodule' '\n> >> grep \"$(cat expect3)\" actual > /dev/null)\n> >> '\n> >>\n> >> +# File/submodule conflict\n> >> +# Commit O: \n> >> +# Commit A: path (submodule)\n> >> +# Commit B: path\n> >> +# Expected: path/ is submodule and file contents for B's path are somewhere\n> >> +\n> >> +test_expect_success 'setup file/submodule conflict' '\n> >> + test_create_repo file-submodule &&\n> >> + (\n> >> + cd file-submodule &&\n> >> +\n> >> + git commit --allow-empty -m O &&\n> >> +\n> >> + git branch A &&\n> >> + git branch B &&\n> >> +\n> >> + git checkout B &&\n> >> + echo contents >path &&\n>\n> This should have been 'content' rather than 'contents', given my grep below...\n>\n> >> + git add path &&\n> >> + git commit -m B &&\n> >> +\n> >> + git checkout A &&\n> >> + test_create_repo path &&\n> >> + (\n> >> + cd path &&\n> >> +\n> >> + echo hello >world &&\n> >\n> > test_commit -C path &&\n> >\n> > or do we need a dirty submodule specifically?\n> > If so what is important, the untracked file or\n> > would changes in the index be sufficient?\n>\n> Do you mean\n> test_commit -C path hello\n> because test_commit needs at least one positional argument (which will\n> serve as contents, message, filename, and tag)? Anyway, yeah, doing\n> this would remove the whole innermost subshell -- the cd, the echo,\n> the git add, and the git commit.\n>\n> >\n> >> + git add world &&\n> >\n> > when observing this one in verbose mode , you may be\n> > asked to use 'git submodule add' instead\n> > https://github.com/git/git/blob/master/builtin/add.c#L323\n>\n> Um, at this point, I'm adding a regular file -- not a submodule. Also,\n> this git add will disappear if I use test_commit. Perhaps you meant\n> one of the other 'git add's?\n>\n> >\n> >> + git commit -m \"submodule\"\n> >\n> > Not sure if we'd need the shell it is only test_commit,\n> > the [submodule] add and commit, so maybe we can get away with\n> > 3 times -C ?\n>\n> This also disappears with test_commit.\n\nSorry, I got ahead of my thinking as normally in submodule tests a\nshell+commit (in the superproject) is the rough pattern of its setup.\n\nI was wondering if you want to test *repo* vs file or *submodule* vs file\nconflict and if we want to treat them differently?\n(A submodule really belongs to the project, so we could expect users\nto have moved a tree into a submodule or vice versa. But if a repo\nappears, we might have a hard time caring as we (or I at the moment)\nreally have no clue what the setup is intended for.\n(With these comments, I think I meant to annotate the part\n\n+ git add path &&\n+ git commit -m A\n\nto use \"git submodule add ./path\")\n\n\n> >> + test_must_fail git merge B^0 >out 2>err &&\n>\n> I probably shouldn't have redirected stdout and stderr here; makes it\n> harder for you to see what's happening, especially since I don't test\n> either in any way.\n>\n\n\n> >> + grep -q content * 2>/dev/null\n> >\n> > Ah that is why we needed the specific echo above.\n>\n> Yeah, if there was a handy\n> test_string_exists_in_some_file_in_current_directory I could have used\n> that, but seems like an oddly specific special test function, so I\n> just added an echo so someone watching the test output under --verbose\n> could see how far the test got.\n>\n> > Sorry for being dense here, I am not quite following the last part of the test,\n> > as it is unclear what ought to fail in this test.\n>\n> Hmm, I obviously wasn't nearly as clear as I thought I was. Thinking\n> it over, two things may have thrown you off:\n>\n> 1) I had a typo ('content' vs. 'contents')\n\nI understood that part, or rather I did not spot the typo.\n\n> 2) I didn't just check what was currently failing but other things\n> that should be true for this test.\n>\n> For item 2, I've had multiple cases in the past where I created a\n> minimal test, only to find that if I had more carefully checked the\n> expected results that it would have prevented a future bug. Also, it\n> was harder in the future to figure out, because I no longer remembered\n> the context for the test setup. So, in this test I check the contents\n> of the index, make sure that the submodule is still present, and then\n> I finally check for the thing that currently fails:\n>\n> commit B added a file called 'path'; its contents should appear\n> somewhere in the directory for the user to use when trying to resolve\n> the failed merge. It cannot appear at 'path' (there's a submodule in\n> the way from commit A), but it should be present somewhere, and in\n> particular I'd expect it in the same directory. So, I simply grep all\n> files in the current directory for the string (and ignore errors about\n> 'grep: path is a directory').\n>\n> Does that help? If so, I'll submit a reroll with the changes and a\n> few extra comments.\n\nThat does help; yes, I thought\n* we want to check for the file content of the file to be somewhere\n (maybe path.file?)\n* equally we'd want to have the submodule somewhere; you seem\n to expect it at path (we could have moved it to path.sub or path.git,\n but that involves extra work as we have to update the .gitmodules\n file to have the correct path <->name mapping)\n* good call with the index, I skimmed over the ls-files calls not thinking\n what they'd check.\n\nSo for now only the \"file content is missing\" part is failing the test,\nwhereas the rest is successful.\n\nThanks for the explanation!\nStefan\n"},{"id":"352153","messageId":"CABPp-BE1kqjiGPqJfrsj8AiFnWg0CUZErUnDb8dAvXWkt7cDMA@mail.gmail.com","threadId":"48837","inReplyTo":"CAGZ79kYK4uwUeDECoabrXJesVzgFBKNejV3reJOXhMyYxUjXyw@mail.gmail.com","subject":"Re: [PATCH 1/3] t7405: add a file/submodule conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-10T17:30:13Z","receivedAt":"2018-07-10T18:32:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jul 10, 2018 at 8:53 AM, Stefan Beller wrote:\n> On Tue, Jul 10, 2018 at 8:28 AM Elijah Newren wrote:\n\n>> 2) I didn't just check what was currently failing but other things\n>> that should be true for this test.\n>>\n>> For item 2, I've had multiple cases in the past where I created a\n>> minimal test, only to find that if I had more carefully checked the\n>> expected results that it would have prevented a future bug. Also, it\n>> was harder in the future to figure out, because I no longer remembered\n>> the context for the test setup. So, in this test I check the contents\n>> of the index, make sure that the submodule is still present, and then\n>> I finally check for the thing that currently fails:\n>>\n>> commit B added a file called 'path'; its contents should appear\n>> somewhere in the directory for the user to use when trying to resolve\n>> the failed merge. It cannot appear at 'path' (there's a submodule in\n>> the way from commit A), but it should be present somewhere, and in\n>> particular I'd expect it in the same directory. So, I simply grep all\n>> files in the current directory for the string (and ignore errors about\n>> 'grep: path is a directory').\n>>\n>> Does that help? If so, I'll submit a reroll with the changes and a\n>> few extra comments.\n>\n> That does help; yes, I thought\n> * we want to check for the file content of the file to be somewhere\n> (maybe path.file?)\n\nYes. I wanted to avoid nailing down the expected pathname until the\ncode was in place that handled this case. merge-recursive currently\nelsewhere uses path~$BRANCH to workaround conflicting paths, but I had\na (half-baked-so-far) idea for changing some of the path-conflict\nstuff which would involve different names. So, I just left it\nundecided until implemented.\n\n> * equally we'd want to have the submodule somewhere; you seem\n> to expect it at path (we could have moved it to path.sub or path.git,\n> but that involves extra work as we have to update the .gitmodules\n> file to have the correct path <->name mapping)\n\nYes I wanted it at path for a specific reason: git can detect renames\nof files, and now of directories, but not of submodules, so it seems\nmore important to keep the submodule where it is when possible.\n(merge_file_1() has code running counter to this, but that pre-dates\nsubmodules and should be updated, IMO.)\n\n> * good call with the index, I skimmed over the ls-files calls not thinking\n> what they'd check.\n>\n> So for now only the \"file content is missing\" part is failing the test,\n> whereas the rest is successful.\n>\n> Thanks for the explanation!\n> Stefan\n\n:-)\n"},{"id":"352193","messageId":"20180711035659.27352-1-newren@gmail.com","threadId":"48837","inReplyTo":"20180707204404.7208-1-newren@gmail.com","subject":"[PATCH v2 0/3] Add testcases showing suboptimal submodule/path conflict handling","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-11T03:56:56Z","receivedAt":"2018-07-11T03:57:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This patch series documents a few problems with submodules and merging,\nfirst mentioned at [1].\n\nChanges since v1 (full range-diff below):\n - Incorporate suggestions from Stefan: use test_commit and git\n submodule add (note that git submodule add means there will also\n be a .gitmodules file, so the count of tracked files goes up by\n 1)\n - Explain the tests a little better\n - Test the merge --abort case a little more thoroughly.\n\n[1] https://public-inbox.org/git/CABPp-BHDrw_dAESic3xK7kC3jMgKeNQuPQF69OpbVYhRkbhJsw@mail.gmail.com/\n\nElijah Newren (3):\n t7405: add a file/submodule conflict\n t7405: add a directory/submodule conflict\n t7405: verify 'merge --abort' works after submodule/path conflicts\n\n t/t7405-submodule-merge.sh | 173 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 173 insertions(+)\n\n1: 44bc2a05f ! 1: 16f6622ce t7405: add a file/submodule conflict\n @@ -32,20 +32,14 @@\n +\t\tgit branch B &&\n +\n +\t\tgit checkout B &&\n -+\t\techo contents >path &&\n ++\t\techo content >path &&\n +\t\tgit add path &&\n +\t\tgit commit -m B &&\n +\n +\t\tgit checkout A &&\n +\t\ttest_create_repo path &&\n -+\t\t(\n -+\t\t\tcd path &&\n -+\n -+\t\t\techo hello >world &&\n -+\t\t\tgit add world &&\n -+\t\t\tgit commit -m \"submodule\"\n -+\t\t) &&\n -+\t\tgit add path &&\n ++\t\ttest_commit -C path world &&\n ++\t\tgit submodule add ./path &&\n +\t\tgit commit -m A\n +\t)\n +'\n @@ -56,16 +50,20 @@\n +\t\tcd file-submodule &&\n +\n +\t\tgit checkout A^0 &&\n -+\t\ttest_must_fail git merge B^0 >out 2>err &&\n ++\t\ttest_must_fail git merge B^0 &&\n +\n +\t\tgit ls-files -s >out &&\n -+\t\ttest_line_count = 2 out &&\n ++\t\ttest_line_count = 3 out &&\n +\t\tgit ls-files -u >out &&\n +\t\ttest_line_count = 2 out &&\n +\n +\t\t# path/ is still a submodule\n +\t\ttest_path_is_dir path/.git &&\n +\n ++\t\t# There is a submodule at \"path\", so B:path cannot be written\n ++\t\t# there. We expect it to be written somewhere in the same\n ++\t\t# directory, though, so just grep for its content in all\n ++\t\t# files, and ignore \"grep: path: Is a directory\" message\n +\t\techo Checking if contents from B:path showed up anywhere &&\n +\t\tgrep -q content * 2>/dev/null\n +\t)\n2: 2fa139d3b ! 2: 31dc3bc4e t7405: add a directory/submodule conflict\n @@ -56,14 +56,8 @@\n +\n +\t\tgit checkout A &&\n +\t\ttest_create_repo path &&\n -+\t\t(\n -+\t\t\tcd path &&\n -+\n -+\t\t\techo hello >world &&\n -+\t\t\tgit add world &&\n -+\t\t\tgit commit -m \"submodule\"\n -+\t\t) &&\n -+\t\tgit add path &&\n ++\t\ttest_commit -C path hello world &&\n ++\t\tgit submodule add ./path &&\n +\t\tgit commit -m A\n +\t)\n +'\n @@ -77,7 +71,7 @@\n +\t\ttest_must_fail git merge B1^0 &&\n +\n +\t\tgit ls-files -s >out &&\n -+\t\ttest_line_count = 2 out &&\n ++\t\ttest_line_count = 3 out &&\n +\t\tgit ls-files -u >out &&\n +\t\ttest_line_count = 1 out &&\n +\n @@ -87,6 +81,9 @@\n +\t\techo Checking if contents from B1:path/file showed up &&\n +\t\t# Would rather use grep -r, but that is GNU extension...\n +\t\tgit ls-files -co | xargs grep -q contents 2>/dev/null &&\n ++\n ++\t\t# However, B1:path/file should NOT have shown up at path/file,\n ++\t\t# because we should not write into the submodule\n +\t\ttest_path_is_missing path/file\n +\t)\n +'\n3: b2a44bedf ! 3: 49719952b t7405: verify 'merge --abort' works after submodule/path conflicts\n @@ -39,12 +39,17 @@\n +\ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n +\t(\n +\t\tcd directory-submodule &&\n -+\n -+\t\tgit checkout A^0 &&\n -+\t\ttest_must_fail git merge B2^0 >out 2>err &&\n \n ++\t\tgit checkout A^0 &&\n ++\t\ttest_must_fail git merge B2^0 &&\n +\t\ttest_path_is_file .git/MERGE_HEAD &&\n -+\t\tgit merge --abort\n ++\n ++\t\t# merge --abort should succeed, should clear .git/MERGE_HEAD,\n ++\t\t# and should not leave behind any conflicted files\n ++\t\tgit merge --abort &&\n ++\t\ttest_path_is_missing .git/MERGE_HEAD &&\n ++\t\tgit ls-files -u >conflicts &&\n ++\t\ttest_must_be_empty conflicts\n \t)\n '\n \n-- \n2.18.0.132.g6e63b23f4\n"},{"id":"352194","messageId":"20180711035659.27352-2-newren@gmail.com","threadId":"48837","inReplyTo":"20180711035659.27352-1-newren@gmail.com","subject":"[PATCH v2 1/3] t7405: add a file/submodule conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-11T03:56:57Z","receivedAt":"2018-07-11T03:57:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In the case of a file/submodule conflict, although both cannot exist at\nthe same path, we expect both to be present somewhere for the user to be\nable to resolve the conflict with. Add a testcase for this.\n\nSigned-off-by: Elijah Newren \n---\n t/t7405-submodule-merge.sh | 54 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 54 insertions(+)\n\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 7bfb2f498..62888c2c5 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -279,4 +279,58 @@ test_expect_success 'recursive merge with submodule' '\n \t grep \"$(cat expect3)\" actual > /dev/null)\n '\n \n+# File/submodule conflict\n+# Commit O: \n+# Commit A: path (submodule)\n+# Commit B: path\n+# Expected: path/ is submodule and file contents for B's path are somewhere\n+\n+test_expect_success 'setup file/submodule conflict' '\n+\ttest_create_repo file-submodule &&\n+\t(\n+\t\tcd file-submodule &&\n+\n+\t\tgit commit --allow-empty -m O &&\n+\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit checkout B &&\n+\t\techo content >path &&\n+\t\tgit add path &&\n+\t\tgit commit -m B &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_create_repo path &&\n+\t\ttest_commit -C path world &&\n+\t\tgit submodule add ./path &&\n+\t\tgit commit -m A\n+\t)\n+'\n+\n+test_expect_failure 'file/submodule conflict' '\n+\ttest_when_finished \"git -C file-submodule reset --hard\" &&\n+\t(\n+\t\tcd file-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 3 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 2 out &&\n+\n+\t\t# path/ is still a submodule\n+\t\ttest_path_is_dir path/.git &&\n+\n+\t\t# There is a submodule at \"path\", so B:path cannot be written\n+\t\t# there. We expect it to be written somewhere in the same\n+\t\t# directory, though, so just grep for its content in all\n+\t\t# files, and ignore \"grep: path: Is a directory\" message\n+\t\techo Checking if contents from B:path showed up anywhere &&\n+\t\tgrep -q content * 2>/dev/null\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.132.g6e63b23f4\n\n"},{"id":"352195","messageId":"20180711035659.27352-3-newren@gmail.com","threadId":"48837","inReplyTo":"20180711035659.27352-1-newren@gmail.com","subject":"[PATCH v2 2/3] t7405: add a directory/submodule conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-11T03:56:58Z","receivedAt":"2018-07-11T03:57:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For a directory/submodule conflict, we want contents from both the\ndirectory and the submodule to be present for the user to use to resolve\nthe conflict, but we do not want paths under the directory being written\ninto the submodule and we do not want the merge being confused by paths\nunder the submodule being in the way. Add testcases for these situations.\n\nSigned-off-by: Elijah Newren \n---\n t/t7405-submodule-merge.sh | 88 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 88 insertions(+)\n\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 62888c2c5..45d1779d2 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -333,4 +333,92 @@ test_expect_failure 'file/submodule conflict' '\n \t)\n '\n \n+# Directory/submodule conflict\n+# Commit O: \n+# Commit A: path (submodule), with sole tracked file named 'world'\n+# Commit B1: path/file\n+# Commit B2: path/world\n+#\n+# Expected from merge of A & B1:\n+# Contents under path/ from commit B1 are renamed elsewhere; we do not\n+# want to write files from one of our tracked directories into a submodule\n+#\n+# Expected from merge of A & B2:\n+# Similar to last merge, but with a slight twist: we don't want paths\n+# under the submodule to be treated as untracked or in the way.\n+\n+test_expect_success 'setup directory/submodule conflict' '\n+\ttest_create_repo directory-submodule &&\n+\t(\n+\t\tcd directory-submodule &&\n+\n+\t\tgit commit --allow-empty -m O &&\n+\n+\t\tgit branch A &&\n+\t\tgit branch B1 &&\n+\t\tgit branch B2 &&\n+\n+\t\tgit checkout B1 &&\n+\t\tmkdir path &&\n+\t\techo contents >path/file &&\n+\t\tgit add path/file &&\n+\t\tgit commit -m B1 &&\n+\n+\t\tgit checkout B2 &&\n+\t\tmkdir path &&\n+\t\techo contents >path/world &&\n+\t\tgit add path/world &&\n+\t\tgit commit -m B2 &&\n+\n+\t\tgit checkout A &&\n+\t\ttest_create_repo path &&\n+\t\ttest_commit -C path hello world &&\n+\t\tgit submodule add ./path &&\n+\t\tgit commit -m A\n+\t)\n+'\n+\n+test_expect_failure 'directory/submodule conflict; keep submodule clean' '\n+\ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n+\t(\n+\t\tcd directory-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B1^0 &&\n+\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 3 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 1 out &&\n+\n+\t\t# path/ is still a submodule\n+\t\ttest_path_is_dir path/.git &&\n+\n+\t\techo Checking if contents from B1:path/file showed up &&\n+\t\t# Would rather use grep -r, but that is GNU extension...\n+\t\tgit ls-files -co | xargs grep -q contents 2>/dev/null &&\n+\n+\t\t# However, B1:path/file should NOT have shown up at path/file,\n+\t\t# because we should not write into the submodule\n+\t\ttest_path_is_missing path/file\n+\t)\n+'\n+\n+test_expect_failure 'directory/submodule conflict; should not treat submodule files as untracked or in the way' '\n+\ttest_when_finished \"git -C directory-submodule/path reset --hard\" &&\n+\ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n+\t(\n+\t\tcd directory-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B2^0 >out 2>err &&\n+\n+\t\t# We do not want files within the submodule to prevent the\n+\t\t# merge from starting; we should not be writing to such paths\n+\t\t# anyway.\n+\t\ttest_i18ngrep ! \"refusing to lose untracked file at\" err\n+\n+\t)\n+'\n+\n test_done\n-- \n2.18.0.132.g6e63b23f4\n\n"},{"id":"352196","messageId":"20180711035659.27352-4-newren@gmail.com","threadId":"48837","inReplyTo":"20180711035659.27352-1-newren@gmail.com","subject":"[PATCH v2 3/3] t7405: verify 'merge --abort' works after submodule/path conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-11T03:56:59Z","receivedAt":"2018-07-11T03:57:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n t/t7405-submodule-merge.sh | 31 +++++++++++++++++++++++++++++++\n 1 file changed, 31 insertions(+)\n\ndiff --git a/t/t7405-submodule-merge.sh b/t/t7405-submodule-merge.sh\nindex 45d1779d2..7855bd864 100755\n--- a/t/t7405-submodule-merge.sh\n+++ b/t/t7405-submodule-merge.sh\n@@ -333,6 +333,19 @@ test_expect_failure 'file/submodule conflict' '\n \t)\n '\n \n+test_expect_success 'file/submodule conflict; merge --abort works afterward' '\n+\ttest_when_finished \"git -C file-submodule reset --hard\" &&\n+\t(\n+\t\tcd file-submodule &&\n+\n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B^0 >out 2>err &&\n+\n+\t\ttest_path_is_file .git/MERGE_HEAD &&\n+\t\tgit merge --abort\n+\t)\n+'\n+\n # Directory/submodule conflict\n # Commit O: \n # Commit A: path (submodule), with sole tracked file named 'world'\n@@ -417,7 +430,25 @@ test_expect_failure 'directory/submodule conflict; should not treat submodule fi\n \t\t# merge from starting; we should not be writing to such paths\n \t\t# anyway.\n \t\ttest_i18ngrep ! \"refusing to lose untracked file at\" err\n+\t)\n+'\n+\n+test_expect_failure 'directory/submodule conflict; merge --abort works afterward' '\n+\ttest_when_finished \"git -C directory-submodule/path reset --hard\" &&\n+\ttest_when_finished \"git -C directory-submodule reset --hard\" &&\n+\t(\n+\t\tcd directory-submodule &&\n \n+\t\tgit checkout A^0 &&\n+\t\ttest_must_fail git merge B2^0 &&\n+\t\ttest_path_is_file .git/MERGE_HEAD &&\n+\n+\t\t# merge --abort should succeed, should clear .git/MERGE_HEAD,\n+\t\t# and should not leave behind any conflicted files\n+\t\tgit merge --abort &&\n+\t\ttest_path_is_missing .git/MERGE_HEAD &&\n+\t\tgit ls-files -u >conflicts &&\n+\t\ttest_must_be_empty conflicts\n \t)\n '\n \n-- \n2.18.0.132.g6e63b23f4\n\n"}]}