RE: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick

[David Turner <David.Turner@twosigma.com>]

> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Monday, November 07, 2016 2:14 PM
> To: David Turner
> Cc: git@vger.kernel.org
> 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@twosigma.com>
> 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@twosigma.com>
> > ---
> >  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.