{"thread":{"id":"47208","subject":"[PATCH] merge-recursive: Handle addition of submodule on our side of history","startedAt":"2017-11-14T17:31:37Z","lastAt":"2017-11-14T20:51:45Z","messageCount":3,"participants":["Elijah Newren","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"332556","messageId":"20171114173124.25982-1-newren@gmail.com","threadId":"47208","inReplyTo":"ABPp-BHDrw_dAESic3xK7kC3jMgKeNQuPQF69OpbVYhRkbhJsw@mail.gmail.com","subject":"[PATCH] merge-recursive: Handle addition of submodule on our side of history","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2017-11-14T17:31:24Z","receivedAt":"2017-11-14T17:31:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The code for a newly added path assumed that the path was a normal file,\nand thus checked for there being a directory still being in the way of\nthe file. Note that since unpack_trees() does path-in-the-way checks\nalready, the only way for there to be a directory in the way at this\npoint in the code, is if there is some kind of D/F conflict in the merge.\n\nFor a submodule addition on HEAD's side of history, the submodule would\nhave already been present. This means that we do expect there to be a\ndirectory present but should not consider it to be \"in the way\"; instead,\nit's the expected submodule. So, when there's a submodule addition from\nHEAD's side, don't bother checking the working copy for a directory in\nthe way.\n\nSigned-off-by: Elijah Newren \n---\nThis commit is based on top of sb/test-cherry-pick-submodule-getting-in-a-way.\n\n merge-recursive.c | 5 +++--\n t/t3512-cherry-pick-submodule.sh | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1d3f8f0d22..9fb0b9f8fd 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1901,8 +1901,9 @@ static int process_entry(struct merge_options *o,\n \t\t\toid = b_oid;\n \t\t\tconf = _(\"directory/file\");\n \t\t}\n-\t\tif (dir_in_way(path, !o->call_depth,\n-\t\t\t S_ISGITLINK(a_mode))) {\n+\t\tif (dir_in_way(path,\n+\t\t\t !o->call_depth && !S_ISGITLINK(a_mode),\n+\t\t\t 0)) {\n \t\t\tchar *new_path = unique_path(o, path, add_branch);\n \t\t\tclean_merge = 0;\n \t\t\toutput(o, 1, _(\"CONFLICT (%s): There is a directory with name %s in %s. \"\ndiff --git a/t/t3512-cherry-pick-submodule.sh b/t/t3512-cherry-pick-submodule.sh\nindex 1b1e31100f..ce48c4fcca 100755\n--- a/t/t3512-cherry-pick-submodule.sh\n+++ b/t/t3512-cherry-pick-submodule.sh\n@@ -10,7 +10,7 @@ KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1\n test_submodule_switch \"git cherry-pick\"\n \n-test_expect_failure 'unrelated submodule/file conflict is ignored' '\n+test_expect_success 'unrelated submodule/file conflict is ignored' '\n \ttest_create_repo sub &&\n \n \ttouch sub/file &&\n-- \n2.15.0.2.g63e86ab1a0\n\n"},{"id":"332562","messageId":"CAGZ79kbdC8fYx503=YKBvEE62=T5WYST0QSNuQjPp5QG1cx1aw@mail.gmail.com","threadId":"47208","inReplyTo":"20171114173124.25982-1-newren@gmail.com","subject":"Re: [PATCH] merge-recursive: Handle addition of submodule on our side of history","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-14T18:48:04Z","receivedAt":"2017-11-14T18:48:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+cc Ефимов Василий who reported the issue at\nhttps://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8fd63@ispras.ru/\n\nOn Tue, Nov 14, 2017 at 9:31 AM, Elijah Newren wrote:\n> The code for a newly added path assumed that the path was a normal file,\n> and thus checked for there being a directory still being in the way of\n> the file. Note that since unpack_trees() does path-in-the-way checks\n> already, the only way for there to be a directory in the way at this\n> point in the code, is if there is some kind of D/F conflict in the merge.\n>\n> For a submodule addition on HEAD's side of history, the submodule would\n> have already been present. This means that we do expect there to be a\n> directory present but should not consider it to be \"in the way\"; instead,\n> it's the expected submodule. So, when there's a submodule addition from\n> HEAD's side, don't bother checking the working copy for a directory in\n> the way.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> This commit is based on top of sb/test-cherry-pick-submodule-getting-in-a-way.\n\nThanks for getting the discussion started here (and fixing the bug),\nbased on your input in\nhttps://public-inbox.org/git/CABPp-BHDrw_dAESic3xK7kC3jMgKeNQuPQF69OpbVYhRkbhJsw@mail.gmail.com/\nI adapted the test case locally to have two tests one file/submodule\nand a submodule/file conflict, after the setup which boroows a lot of\ncode from the\nexisting test, we'll have:\n\n test_expect_success 'unrelated submodule/file conflict is ignored' '\n (\n cd a_repo &&\n git checkout with_sub^0 &&\n git cherry-pick with_file^0\n )\n '\n\n test_expect_success 'unrelated file/submodule conflict is ignored' '\n (\n cd a_repo &&\n git checkout with_file^0 &&\n git cherry-pick with_sub^0\n )\n '\n\nand the other case now fails. I'll take a look into that. I think we'd need to\ncheck either a_mode or b_mode to be a submodule depending on which side\nthe submodule occured.\n\nSo I'll build on top of this patch to fix the other way, too.\n\n>\n> merge-recursive.c | 5 +++--\n> t/t3512-cherry-pick-submodule.sh | 2 +-\n> 2 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 1d3f8f0d22..9fb0b9f8fd 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1901,8 +1901,9 @@ static int process_entry(struct merge_options *o,\n> oid = b_oid;\n> conf = _(\"directory/file\");\n> }\n> - if (dir_in_way(path, !o->call_depth,\n> - S_ISGITLINK(a_mode))) {\n> + if (dir_in_way(path,\n> + !o->call_depth && !S_ISGITLINK(a_mode),\n> + 0)) {\n\nThe last flag is_empty_ok is ok to keep at 0, I think.\n\nThanks,\nStefan\n"},{"id":"332573","messageId":"CAGZ79kZApzOsjns+q7mU=ta7OPd1fF7r8dWKY3GKa4sRTSMZ+w@mail.gmail.com","threadId":"47208","inReplyTo":"CAGZ79kbdC8fYx503=YKBvEE62=T5WYST0QSNuQjPp5QG1cx1aw@mail.gmail.com","subject":"Re: [PATCH] merge-recursive: Handle addition of submodule on our side of history","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-14T20:51:38Z","receivedAt":"2017-11-14T20:51:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 14, 2017 at 10:48 AM, Stefan Beller wrote:\n\n> test_expect_success 'unrelated file/submodule conflict is ignored' '\n> (\n> cd a_repo &&\n> git checkout with_file^0 &&\n> git cherry-pick with_sub^0\n\nThis makes no sense, yet. Sorry about the noise.\nI think your patch is fine as is.\n"}]}