Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick
From: Stefan Beller <hidden>
Date: 2016-11-07 19:20:56
On Mon, Nov 7, 2016 at 10:31 AM, David Turner [off-list ref] 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 <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.
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.
quoted hunk ↗ jump to hunk
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