On Mon, Nov 7, 2016 at 10:31 AM, David Turner <dturner@xxxxxxxxxxxx> wrote: > When a submodule is being merged or cherry-picked into a working > tree that already contains a corresponding empty directory, do not > record a conflict. > > One situation where this bug appears is: > > - Commit 1 adds a submodule "... at sub1" as inferred by text below. > - Commit 2 removes that submodule and re-adds it into a subdirectory > (sub1 to sub1/sub1). > - Commit 3 adds an unrelated file. > > Now the user checks out commit 1 (first deinitializing the submodule), > and attempts to cherry-pick commit 3. Previously, this would fail, > because the incoming submodule sub1/sub1 would falsely conflict with > the empty sub1 directory. So you'd want to achieve: $ # on commit 3: git checkout <commit 1> git cherry-pick <commit 3> which essentially moves the gitlink back to its original place (from sub1/sub1 -> sub1). This sounds reasonable. But what if the submodule contains a (file/directory) named sub1? We'd first remove the sub1/sub1 submodule (and even delete the inner directory?), such that "sub1/" becomes an empty dir, which is perfect for having a submodule right there at "sub1/" > > This patch ignores the empty sub1 directory, fixing the bug. We only > ignore the empty directory if the object being emplaced is a > submodule, which expects an empty directory. > > Signed-off-by: David Turner <dturner@xxxxxxxxxxxx> > --- > merge-recursive.c | 21 +++++++++++++++------ > t/t3030-merge-recursive.sh | 4 ++-- > t/t3426-rebase-submodule.sh | 3 --- > 3 files changed, 17 insertions(+), 11 deletions(-) > > Note that there are four calls to dir_in_way, and only two of them > have changed their semantics. This is because the merge code is quite > complicated, and I don't fully understand it. A good approach. I was trying to haggle with unpack-trees.c and the merging code and put way more on my plate than I could eat in one sitting. Trying to get the mess sorted now to prepare a patch series this week. > So I did not have time > to analyze the remaining calls to see whether they, too, should be > changed. The call in line 1205 (in handle_file, which is only called from conflict_rename_rename_1to2) may be relevant if we move around submodules on the same level and modifying it in different branches. However I think preserving current behavior is ok. The other one in handle_change_delete also doesn't look obvious one way or another, so I'd stick with current behavior. >For me, there are no test failures either way, indicating > that probably these cases are rare. The tests have to be crafted for this specific code pattern, > > The reason behind the empty_ok parameter (as opposed to just always > allowing empy directories to be blown away) is found in t6022's 'pair > rename to parent of other (D/F conflicts) w/ untracked dir'. This > test would fail with an unconditional rename, because it wouldn't > generate the conflict file. Or the submodule from your commit message contains a "sub1/..." itself. > It's not clear how important that > behavior is (I do not recall ever noticing the file~branch thing > before), but it seemed better to preserve it in case it was important. > > diff --git a/merge-recursive.c b/merge-recursive.c > index 9041c2f..e64b48b 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -664,7 +664,13 @@ static char *unique_path(struct merge_options *o, const char *path, const char * > return strbuf_detach(&newpath, NULL); > } > > -static int dir_in_way(const char *path, int check_working_copy) > +/** > + * Check whether a directory in the index is in the way of an incoming > + * file. Return 1 if so. If check_working_copy is non-zero, also > + * check the working directory. If empty_ok is non-zero, also return > + * 0 in the case where the working-tree dir exists but is empty. > + */ Thanks for the documenting comment! This is probably fine as is with just two boolean parameters. If we'd add more, we might have thought about adding a flags parameter with bits for each flag. Looks good to me, Thanks, Stefan