{"thread":{"id":"44437","subject":"[PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick","startedAt":"2016-11-07T18:31:51Z","lastAt":"2016-11-18T04:48:02Z","messageCount":5,"participants":["David Turner","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"305495","messageId":"1478543491-6286-1-git-send-email-dturner@twosigma.com","threadId":"44437","inReplyTo":null,"subject":"[PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2016-11-07T18:31:31Z","receivedAt":"2016-11-07T18:31:51Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"When a submodule is being merged or cherry-picked into a working\ntree that already contains a corresponding empty directory, do not\nrecord a conflict.\n\nOne situation where this bug appears is:\n\n- Commit 1 adds a submodule\n- Commit 2 removes that submodule and re-adds it into a subdirectory\n       (sub1 to sub1/sub1).\n- Commit 3 adds an unrelated file.\n\nNow the user checks out commit 1 (first deinitializing the submodule),\nand attempts to cherry-pick commit 3.  Previously, this would fail,\nbecause the incoming submodule sub1/sub1 would falsely conflict with\nthe empty sub1 directory.\n\nThis patch ignores the empty sub1 directory, fixing the bug.  We only\nignore the empty directory if the object being emplaced is a\nsubmodule, which expects an empty directory.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n merge-recursive.c           | 21 +++++++++++++++------\n t/t3030-merge-recursive.sh  |  4 ++--\n t/t3426-rebase-submodule.sh |  3 ---\n 3 files changed, 17 insertions(+), 11 deletions(-)\n\nNote that there are four calls to dir_in_way, and only two of them\nhave changed their semantics.  This is because the merge code is quite\ncomplicated, and I don't fully understand it.  So I did not have time\nto analyze the remaining calls to see whether they, too, should be\nchanged.  For me, there are no test failures either way, indicating\nthat probably these cases are rare.\n\nThe reason behind the empty_ok parameter (as opposed to just always\nallowing empy directories to be blown away) is found in t6022's 'pair\nrename to parent of other (D/F conflicts) w/ untracked dir'.  This\ntest would fail with an unconditional rename, because it wouldn't\ngenerate the conflict file.  It's not clear how important that\nbehavior is (I do not recall ever noticing the file~branch thing\nbefore), but it seemed better to preserve it in case it was important.\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 9041c2f..e64b48b 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -664,7 +664,13 @@ static char *unique_path(struct merge_options *o, const char *path, const char *\n \treturn strbuf_detach(&newpath, NULL);\n }\n \n-static int dir_in_way(const char *path, int check_working_copy)\n+/**\n+ * Check whether a directory in the index is in the way of an incoming\n+ * file.  Return 1 if so.  If check_working_copy is non-zero, also\n+ * check the working directory.  If empty_ok is non-zero, also return\n+ * 0 in the case where the working-tree dir exists but is empty.\n+ */\n+static int dir_in_way(const char *path, int check_working_copy, int empty_ok)\n {\n \tint pos;\n \tstruct strbuf dirpath = STRBUF_INIT;\n@@ -684,7 +690,8 @@ static int dir_in_way(const char *path, int check_working_copy)\n \t}\n \n \tstrbuf_release(&dirpath);\n-\treturn check_working_copy && !lstat(path, &st) && S_ISDIR(st.st_mode);\n+\treturn check_working_copy && !lstat(path, &st) && S_ISDIR(st.st_mode) &&\n+\t\t!(empty_ok && is_empty_dir(path));\n }\n \n static int was_tracked(const char *path)\n@@ -1062,7 +1069,7 @@ static int handle_change_delete(struct merge_options *o,\n {\n \tchar *renamed = NULL;\n \tint ret = 0;\n-\tif (dir_in_way(path, !o->call_depth)) {\n+\tif (dir_in_way(path, !o->call_depth, 0)) {\n \t\trenamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);\n \t}\n \n@@ -1195,7 +1202,7 @@ static int handle_file(struct merge_options *o,\n \t\tremove_file(o, 0, rename->path, 0);\n \t\tdst_name = unique_path(o, rename->path, cur_branch);\n \t} else {\n-\t\tif (dir_in_way(rename->path, !o->call_depth)) {\n+\t\tif (dir_in_way(rename->path, !o->call_depth, 0)) {\n \t\t\tdst_name = unique_path(o, rename->path, cur_branch);\n \t\t\toutput(o, 1, _(\"%s is a directory in %s adding as %s instead\"),\n \t\t\t       rename->path, other_branch, dst_name);\n@@ -1704,7 +1711,8 @@ static int merge_content(struct merge_options *o,\n \t\t\t o->branch2 == rename_conflict_info->branch1) ?\n \t\t\tpair1->two->path : pair1->one->path;\n \n-\t\tif (dir_in_way(path, !o->call_depth))\n+\t\tif (dir_in_way(path, !o->call_depth,\n+\t\t\t       S_ISGITLINK(pair1->two->mode)))\n \t\t\tdf_conflict_remains = 1;\n \t}\n \tif (merge_file_special_markers(o, &one, &a, &b,\n@@ -1862,7 +1870,8 @@ 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\tif (dir_in_way(path, !o->call_depth,\n+\t\t\t       S_ISGITLINK(a_mode))) {\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/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 470f334..be074a1 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -575,13 +575,13 @@ test_expect_success 'merge removes empty directories' '\n \ttest_must_fail test -d d\n '\n \n-test_expect_failure 'merge-recursive simple w/submodule' '\n+test_expect_success 'merge-recursive simple w/submodule' '\n \n \tgit checkout submod &&\n \tgit merge remove\n '\n \n-test_expect_failure 'merge-recursive simple w/submodule result' '\n+test_expect_sucess 'merge-recursive simple w/submodule result' '\n \n \tgit ls-files -s >actual &&\n \t(\ndiff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh\nindex d5b896d..ebf4f5e 100755\n--- a/t/t3426-rebase-submodule.sh\n+++ b/t/t3426-rebase-submodule.sh\n@@ -38,9 +38,6 @@ git_rebase_interactive () {\n \tgit rebase -i \"$1\"\n }\n \n-KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1\n-# The real reason \"replace directory with submodule\" fails is because a\n-# directory \"sub1\" exists, but we reuse the suppression added for merge here\n test_submodule_switch \"git_rebase_interactive\"\n \n test_done\n-- \n2.8.0.rc4.22.g8ae061a\n\n"},{"id":"305503","messageId":"CAGZ79kbp82VssqutEg5=TnciQrGkDRQ3iNm_vUo9eBBGMXQRNg@mail.gmail.com","threadId":"44437","inReplyTo":"1478543491-6286-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-07T19:13:43Z","receivedAt":"2016-11-07T19:20:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 7, 2016 at 10:31 AM, David Turner <dturner@twosigma.com> wrote:\n> When a submodule is being merged or cherry-picked into a working\n> tree that already contains a corresponding empty directory, do not\n> record a conflict.\n>\n> One situation where this bug appears is:\n>\n> - Commit 1 adds a submodule\n\n\"... at sub1\" as inferred by text below.\n\n> - Commit 2 removes that submodule and re-adds it into a subdirectory\n>        (sub1 to sub1/sub1).\n> - Commit 3 adds an unrelated file.\n>\n> Now the user checks out commit 1 (first deinitializing the submodule),\n> and attempts to cherry-pick commit 3.  Previously, this would fail,\n> because the incoming submodule sub1/sub1 would falsely conflict with\n> the empty sub1 directory.\n\nSo you'd want to achieve:\n  $ # on commit 3:\n  git checkout <commit 1>\n  git cherry-pick <commit 3>\n\nwhich essentially moves the gitlink back to its original place\n(from sub1/sub1 -> sub1).  This sounds reasonable.\nBut what if the submodule contains a (file/directory)\nnamed sub1? We'd first remove the sub1/sub1 submodule\n(and even delete the inner directory?), such that \"sub1/\"\nbecomes an empty dir, which is perfect for having a\nsubmodule right there at \"sub1/\"\n\n>\n> This patch ignores the empty sub1 directory, fixing the bug.  We only\n> ignore the empty directory if the object being emplaced is a\n> submodule, which expects an empty directory.\n>\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> ---\n>  merge-recursive.c           | 21 +++++++++++++++------\n>  t/t3030-merge-recursive.sh  |  4 ++--\n>  t/t3426-rebase-submodule.sh |  3 ---\n>  3 files changed, 17 insertions(+), 11 deletions(-)\n>\n> Note that there are four calls to dir_in_way, and only two of them\n> have changed their semantics.  This is because the merge code is quite\n> complicated, and I don't fully understand it.\n\nA good approach. I was trying to haggle with unpack-trees.c and\nthe merging code and put way more on my plate than I could eat\nin one sitting. Trying to get the mess sorted now to prepare a patch\nseries this week.\n\n> So I did not have time\n> to analyze the remaining calls to see whether they, too, should be\n> changed.\n\nThe call in line 1205 (in handle_file, which is only called from\nconflict_rename_rename_1to2) may be relevant if we move around\nsubmodules on the same level and modifying it in different branches.\nHowever I think preserving current behavior is ok.\n\nThe other one in handle_change_delete also doesn't look obvious\none way or another, so I'd stick with current behavior.\n\n>For me, there are no test failures either way, indicating\n> that probably these cases are rare.\n\nThe tests have to be crafted for this specific code pattern,\n\n>\n> The reason behind the empty_ok parameter (as opposed to just always\n> allowing empy directories to be blown away) is found in t6022's 'pair\n> rename to parent of other (D/F conflicts) w/ untracked dir'.  This\n> test would fail with an unconditional rename, because it wouldn't\n> generate the conflict file.\n\nOr the submodule from your commit message contains a \"sub1/...\" itself.\n\n>  It's not clear how important that\n> behavior is (I do not recall ever noticing the file~branch thing\n> before), but it seemed better to preserve it in case it was important.\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 9041c2f..e64b48b 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -664,7 +664,13 @@ static char *unique_path(struct merge_options *o, const char *path, const char *\n>         return strbuf_detach(&newpath, NULL);\n>  }\n>\n> -static int dir_in_way(const char *path, int check_working_copy)\n> +/**\n> + * Check whether a directory in the index is in the way of an incoming\n> + * file.  Return 1 if so.  If check_working_copy is non-zero, also\n> + * check the working directory.  If empty_ok is non-zero, also return\n> + * 0 in the case where the working-tree dir exists but is empty.\n> + */\n\nThanks for the documenting comment! This is probably fine as is with just\ntwo boolean parameters. If we'd add more, we might have thought about\nadding a flags parameter with bits for each flag.\n\nLooks good to me,\nThanks,\nStefan\n"},{"id":"305507","messageId":"378e63aa70e54fe9b839acf90680917a@exmbdft7.ad.twosigma.com","threadId":"44437","inReplyTo":"CAGZ79kbp82VssqutEg5=TnciQrGkDRQ3iNm_vUo9eBBGMXQRNg@mail.gmail.com","subject":"RE: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-11-07T20:38:18Z","receivedAt":"2016-11-07T20:44:28Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> -----Original Message-----\n> From: Stefan Beller [mailto:sbeller@google.com]\n> Sent: Monday, November 07, 2016 2:14 PM\n> To: David Turner\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH] submodules: allow empty working-tree dirs in\n> merge/cherry-pick\n> \n> On Mon, Nov 7, 2016 at 10:31 AM, David Turner <dturner@twosigma.com>\n> wrote:\n> > When a submodule is being merged or cherry-picked into a working tree\n> > that already contains a corresponding empty directory, do not record a\n> > conflict.\n> >\n> > One situation where this bug appears is:\n> >\n> > - Commit 1 adds a submodule\n> \n> \"... at sub1\" as inferred by text below.\n> \n> > - Commit 2 removes that submodule and re-adds it into a subdirectory\n> >        (sub1 to sub1/sub1).\n> > - Commit 3 adds an unrelated file.\n> >\n> > Now the user checks out commit 1 (first deinitializing the submodule),\n> > and attempts to cherry-pick commit 3.  Previously, this would fail,\n> > because the incoming submodule sub1/sub1 would falsely conflict with\n> > the empty sub1 directory.\n> \n> So you'd want to achieve:\n>   $ # on commit 3:\n>   git checkout <commit 1>\n>   git cherry-pick <commit 3>\n> \n> which essentially moves the gitlink back to its original place (from\n> sub1/sub1 -> sub1).  This sounds reasonable.\n> But what if the submodule contains a (file/directory) named sub1? We'd\n> first remove the sub1/sub1 submodule (and even delete the inner\n> directory?), such that \"sub1/\"\n> becomes an empty dir, which is perfect for having a submodule right there\n> at \"sub1/\"\n\nI'm confused about the \"what if\" here.\n\nIn our particular situation, the submodule in question was not initialized.  Basically, the submodule move by developer A messed up developer B's rebase, where developers A and B had been working on completely disjoint sets of submodules.  If it had been initialized, that might be a different story.  It would be somewhat less surprising, and thus probably OK.  The \"first deinitializing the submodule\" bit above, I think, describes the situation.\n\nIf the \"what if\" you are worried about is corruption caused the move of sub1/sub1 into sub1, don't worry about it.  sub1/ would still contain the .git file, and so would not be empty.  Even if this patch were really wacky, the worst it could do is delete already-empty directories.\n\n> > This patch ignores the empty sub1 directory, fixing the bug.  We only\n> > ignore the empty directory if the object being emplaced is a\n> > submodule, which expects an empty directory.\n> >\n> > Signed-off-by: David Turner <dturner@twosigma.com>\n> > ---\n> >  merge-recursive.c           | 21 +++++++++++++++------\n> >  t/t3030-merge-recursive.sh  |  4 ++--  t/t3426-rebase-submodule.sh |\n> > 3 ---\n> >  3 files changed, 17 insertions(+), 11 deletions(-)\n> >\n> > Note that there are four calls to dir_in_way, and only two of them\n> > have changed their semantics.  This is because the merge code is quite\n> > complicated, and I don't fully understand it.\n> \n> A good approach. I was trying to haggle with unpack-trees.c and the\n> merging code and put way more on my plate than I could eat in one sitting.\n> Trying to get the mess sorted now to prepare a patch series this week.\n\nIf your approach also fixes the same tests that mine fixes, then I am happy to use your series over mine.  Please CC me so I can take a peek.\n\n> > So I did not have time\n> > to analyze the remaining calls to see whether they, too, should be\n> > changed.\n> \n> The call in line 1205 (in handle_file, which is only called from\n> conflict_rename_rename_1to2) may be relevant if we move around submodules\n> on the same level and modifying it in different branches.\n> However I think preserving current behavior is ok.\n\nSo, the case there would be moving sub1 to sub2, where sub2 was previously a different submodule?  It appears that this works at least after my patch, if not before.  But I gather from the name rename_1to2 that I actually need to copy the submodule not move it?  This seems like such a rare case that I don't actually need to handle it; basically nobody needs two copies of one submodule in the same repo.  I think that case fails for other reasons anyway.\n\n> The other one in handle_change_delete also doesn't look obvious one way or\n> another, so I'd stick with current behavior.\n\nThis appears to be implicated in the t6022 test that I mentioned -- if I change empty_ok unconditionally to 1, the test fails.\n\n> >For me, there are no test failures either way, indicating  that\n> >probably these cases are rare.\n> \n> The tests have to be crafted for this specific code pattern,\n> \n> >\n> > The reason behind the empty_ok parameter (as opposed to just always\n> > allowing empy directories to be blown away) is found in t6022's 'pair\n> > rename to parent of other (D/F conflicts) w/ untracked dir'.  This\n> > test would fail with an unconditional rename, because it wouldn't\n> > generate the conflict file.\n> \n> Or the submodule from your commit message contains a \"sub1/...\" itself.\n\nSee above.\n\n"},{"id":"305510","messageId":"CAGZ79kb8XM_j37H-j3fUz9a0DFXn_v+S4P4r-QyuQOKM3xqhRw@mail.gmail.com","threadId":"44437","inReplyTo":"378e63aa70e54fe9b839acf90680917a@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-07T20:48:22Z","receivedAt":"2016-11-07T20:48:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 7, 2016 at 12:38 PM, David Turner <David.Turner@twosigma.com> wrote:\n>> -----Original Message-----\n>> From: Stefan Beller [mailto:sbeller@google.com]\n>> Sent: Monday, November 07, 2016 2:14 PM\n>> To: David Turner\n>> Cc: git@vger.kernel.org\n>> Subject: Re: [PATCH] submodules: allow empty working-tree dirs in\n>> merge/cherry-pick\n>>\n>> On Mon, Nov 7, 2016 at 10:31 AM, David Turner <dturner@twosigma.com>\n>> wrote:\n>> > When a submodule is being merged or cherry-picked into a working tree\n>> > that already contains a corresponding empty directory, do not record a\n>> > conflict.\n>> >\n>> > One situation where this bug appears is:\n>> >\n>> > - Commit 1 adds a submodule\n>>\n>> \"... at sub1\" as inferred by text below.\n>>\n>> > - Commit 2 removes that submodule and re-adds it into a subdirectory\n>> >        (sub1 to sub1/sub1).\n>> > - Commit 3 adds an unrelated file.\n>> >\n>> > Now the user checks out commit 1 (first deinitializing the submodule),\n>> > and attempts to cherry-pick commit 3.  Previously, this would fail,\n>> > because the incoming submodule sub1/sub1 would falsely conflict with\n>> > the empty sub1 directory.\n>>\n>> So you'd want to achieve:\n>>   $ # on commit 3:\n>>   git checkout <commit 1>\n>>   git cherry-pick <commit 3>\n>>\n>> which essentially moves the gitlink back to its original place (from\n>> sub1/sub1 -> sub1).  This sounds reasonable.\n>> But what if the submodule contains a (file/directory) named sub1? We'd\n>> first remove the sub1/sub1 submodule (and even delete the inner\n>> directory?), such that \"sub1/\"\n>> becomes an empty dir, which is perfect for having a submodule right there\n>> at \"sub1/\"\n>\n> I'm confused about the \"what if\" here.\n>\n> In our particular situation, the submodule in question was not initialized.\n\noops. That explains it. I somehow assumed we were talking about\ninitialized submodules.\n\n>\n> If your approach also fixes the same tests that mine fixes, then I am happy to use your series over mine.  Please CC me so I can take a peek.\n\nNo, my series seems to be orthogonal to this one. I plan\non cc'ing you nevertheless as it is still nearby.\n\n> basically nobody needs two copies of one submodule in the same repo.\n\nIIRC this is how gitlinks were used in very early days :/\n(kernel people were using gitlinks to track different kernel versions\nand see if they were interoperable or working at all.\ne.g. see d92a39590d1126e195f1bbccf182a2cdb79218e7, which\nonly makes sense (for the update command) if the referenced repository\ncontains references of all submodules, which either means a huge reference\npile that contains different projects at the same time, or the same project\nat different versions.\n\n>  I think that case fails for other reasons anyway.\n>\n\nYes. I agree that the patch as-is is applicable. I did not try to oppose\nyour approach, but rather give some thoughts I had.\n\nStefan\n"},{"id":"306137","messageId":"xmqqwpg1mm11.fsf@gitster.mtv.corp.google.com","threadId":"44437","inReplyTo":"1478543491-6286-1-git-send-email-dturner@twosigma.com","subject":"Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-18T04:47:54Z","receivedAt":"2016-11-18T04:48:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twosigma.com> writes:\n\n> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\n> index 470f334..be074a1 100755\n> --- a/t/t3030-merge-recursive.sh\n> +++ b/t/t3030-merge-recursive.sh\n> @@ -575,13 +575,13 @@ test_expect_success 'merge removes empty directories' '\n>  \ttest_must_fail test -d d\n>  '\n>  \n> -test_expect_failure 'merge-recursive simple w/submodule' '\n> +test_expect_success 'merge-recursive simple w/submodule' '\n>  \n>  \tgit checkout submod &&\n>  \tgit merge remove\n>  '\n>  \n> -test_expect_failure 'merge-recursive simple w/submodule result' '\n> +test_expect_sucess 'merge-recursive simple w/submodule result' '\n\nHere is a typo.  I wonder if we want to do \"set -e\" at the end of\ntest-lib.sh to catch a breakage like this.  I only caught it by\nbeing lucky (I was staring \"make test\" output as it flew by).\n\nI've already amended the copy I have, but in case you are going to\nreroll in the future, please do not forget to update your copy.\n"}]}