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

-----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 [off-list ref]
wrote:

quoted

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.

quoted

- 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.

quoted

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 <redacted>
---
 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.

quoted

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.

quoted

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,

quoted

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.