> -----Original Message----- > From: Stefan Beller [mailto:sbeller@xxxxxxxxxx] > Sent: Monday, November 07, 2016 2:14 PM > To: David Turner > Cc: git@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] submodules: allow empty working-tree dirs in > merge/cherry-pick > > 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/" I'm confused about the "what if" here. In 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. If 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. > > 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. 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. > > 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. So, 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. > The other one in handle_change_delete also doesn't look obvious one way or > another, so I'd stick with current behavior. This appears to be implicated in the t6022 test that I mentioned -- if I change empty_ok unconditionally to 1, the test fails. > >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. See above.