[PATCHv2 0/2] Fix resolvable rename + D/F conflict testcases

[PATCHv2 0/2] Fix resolvable rename + D/F conflict testcases

[Elijah Newren]

This fixes an issue reported by Nick (no lastname given) on the
mailing list, as well as a closely related issue in the handling of
rename + directory/file conflicts, namely where a filename on one side
of the rename is a directory name on the other side of the merge.

Change since v1:
  * Split unrelated 2/3 patch out into a separate submission

Elijah Newren (2):
  t3509: Add rename + D/F conflict testcase that recursive strategy
    fails
  merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dir

 merge-recursive.c               |   16 +++++-----
 t/t3509-cherry-pick-merge-df.sh |   66 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 8 deletions(-)

-- 
1.7.3.rc0.8.g2ec3f

[PATCHv2 1/2] t3509: Add rename + D/F conflict testcase that recursive strategy fails

[Elijah Newren]

When one side of a file rename matches a directory name on the other side,
the recursive merge strategy will fail.  This is true even if the merge is
trivially resolvable.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t3509-cherry-pick-merge-df.sh |   66 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/t/t3509-cherry-pick-merge-df.sh b/t/t3509-cherry-pick-merge-df.sh
index a5ccdbf..eb5826f 100755
--- a/t/t3509-cherry-pick-merge-df.sh
+++ b/t/t3509-cherry-pick-merge-df.sh
@@ -32,4 +32,70 @@ test_expect_success SYMLINKS 'Cherry-pick succeeds with rename across D/F confli
 	git cherry-pick branch
 '
 
+test_expect_success 'Setup rename with file on one side matching directory name on other' '
+	git checkout --orphan nick-testcase &&
+	git rm -rf . &&
+
+	>empty &&
+	git add empty &&
+	git commit -m "Empty file" &&
+
+	git checkout -b simple &&
+	mv empty file &&
+	mkdir empty &&
+	mv file empty &&
+	git add empty/file &&
+	git commit -m "Empty file under empty dir" &&
+
+	echo content >newfile &&
+	git add newfile &&
+	git commit -m "New file"
+'
+
+test_expect_success 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (resolve)' '
+	git reset --hard &&
+	git checkout -q nick-testcase^0 &&
+	git cherry-pick --strategy=resolve simple
+'
+
+test_expect_failure 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (recursive)' '
+	git reset --hard &&
+	git checkout -q nick-testcase^0 &&
+	git cherry-pick --strategy=recursive simple
+'
+
+test_expect_success 'Setup rename with file on one side matching different dirname on other' '
+	git reset --hard &&
+	git checkout --orphan mergeme &&
+	git rm -rf . &&
+
+	mkdir sub &&
+	mkdir othersub &&
+	echo content > sub/file &&
+	echo foo > othersub/whatever &&
+	git add -A &&
+	git commit -m "Common commmit" &&
+
+	git rm -rf othersub &&
+	git mv sub/file othersub &&
+	git commit -m "Commit to merge" &&
+
+	git checkout -b newhead mergeme~1 &&
+	>independent-change &&
+	git add independent-change &&
+	git commit -m "Completely unrelated change"
+'
+
+test_expect_success 'Cherry-pick with rename to different D/F conflict succeeds (resolve)' '
+	git reset --hard &&
+	git checkout -q newhead^0 &&
+	git cherry-pick --strategy=resolve mergeme
+'
+
+test_expect_failure 'Cherry-pick with rename to different D/F conflict succeeds (recursive)' '
+	git reset --hard &&
+	git checkout -q newhead^0 &&
+	git cherry-pick --strategy=recursive mergeme
+'
+
 test_done
-- 
1.7.3.rc0.8.g2ec3f

[PATCHv2 2/2] merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dir

[Elijah Newren]

In merge-recursive.c, whenever there was a rename where a file name on one
side of the rename matches a directory name on the other side of the merge,
then the very first check that
  string_list_has_string(&o->current_directory_set, ren1_dst)
would trigger forcing it into marking it as a rename/directory conflict.

However, if the path is only renamed on one side and a simple three-way
merge between the separate files resolves cleanly, then we don't need to
mark it as a rename/directory conflict.  So, we can simply move the check
for rename/directory conflicts after we've verified that there isn't a
rename/rename conflict and that a threeway content merge doesn't work.

This changes the particular error message one gets in the case where the
directory name that a file on one side of the rename matches is not also
part of the rename pair.  For example, with commits containing the files:
  COMMON    -> (HEAD,           MERGE )
  ---------    ---------------  -------
  sub/file1 -> (sub/file1,      newsub)
  <NULL>    -> (newsub/newfile, <NULL>)
then previously when one tried to merge MERGE into HEAD, one would get
  CONFLICT (rename/directory): Rename sub/file1->newsub in HEAD  directory newsub added in merge
  Renaming sub/file1 to newsub~HEAD instead
  Adding newsub/newfile
  Automatic merge failed; fix conflicts and then commit the result.
After this patch, the error message will instead become:
  Removing newsub
  Adding newsub/newfile
  CONFLICT (file/directory): There is a directory with name newsub in merge. Adding newsub as newsub~HEAD
  Automatic merge failed; fix conflicts and then commit the result.
That makes more sense to me, because git can't know that there's a conflict
until after it's tried resolving paths involving newsub/newfile to see if
they are still in the way at the end (and if newsub/newfile is not in the
way at the end, there should be no conflict at all, which did not hold with
git previously).

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 merge-recursive.c               |   16 ++++++++--------
 t/t3509-cherry-pick-merge-df.sh |    4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 20e1779..5ea6d11 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -935,14 +935,7 @@ static int process_renames(struct merge_options *o,
 
 			try_merge = 0;
 
-			if (string_list_has_string(&o->current_directory_set, ren1_dst)) {
-				clean_merge = 0;
-				output(o, 1, "CONFLICT (rename/directory): Rename %s->%s in %s "
-				       " directory %s added in %s",
-				       ren1_src, ren1_dst, branch1,
-				       ren1_dst, branch2);
-				conflict_rename_dir(o, ren1, branch1);
-			} else if (sha_eq(src_other.sha1, null_sha1)) {
+			if (sha_eq(src_other.sha1, null_sha1)) {
 				clean_merge = 0;
 				output(o, 1, "CONFLICT (rename/delete): Rename %s->%s in %s "
 				       "and deleted in %s",
@@ -1034,6 +1027,13 @@ static int process_renames(struct merge_options *o,
 					if (!ren1->dst_entry->stages[2].mode !=
 					    !ren1->dst_entry->stages[3].mode)
 						ren1->dst_entry->processed = 0;
+				} else if (string_list_has_string(&o->current_directory_set, ren1_dst)) {
+					clean_merge = 0;
+					output(o, 1, "CONFLICT (rename/directory): Rename %s->%s in %s "
+					       " directory %s added in %s",
+					       ren1_src, ren1_dst, branch1,
+					       ren1_dst, branch2);
+					conflict_rename_dir(o, ren1, branch1);
 				} else {
 					if (mfi.merge || !mfi.clean)
 						output(o, 1, "Renaming %s => %s", ren1_src, ren1_dst);
diff --git a/t/t3509-cherry-pick-merge-df.sh b/t/t3509-cherry-pick-merge-df.sh
index eb5826f..948ca1b 100755
--- a/t/t3509-cherry-pick-merge-df.sh
+++ b/t/t3509-cherry-pick-merge-df.sh
@@ -58,7 +58,7 @@ test_expect_success 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (reso
 	git cherry-pick --strategy=resolve simple
 '
 
-test_expect_failure 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (recursive)' '
+test_expect_success 'Cherry-pick succeeds with was_a_dir/file -> was_a_dir (recursive)' '
 	git reset --hard &&
 	git checkout -q nick-testcase^0 &&
 	git cherry-pick --strategy=recursive simple
@@ -92,7 +92,7 @@ test_expect_success 'Cherry-pick with rename to different D/F conflict succeeds
 	git cherry-pick --strategy=resolve mergeme
 '
 
-test_expect_failure 'Cherry-pick with rename to different D/F conflict succeeds (recursive)' '
+test_expect_success 'Cherry-pick with rename to different D/F conflict succeeds (recursive)' '
 	git reset --hard &&
 	git checkout -q newhead^0 &&
 	git cherry-pick --strategy=recursive mergeme
-- 
1.7.3.rc0.8.g2ec3f

Re: [PATCHv2 0/2] Fix resolvable rename + D/F conflict testcases

[Elijah Newren]

On Wed, Sep 8, 2010 at 12:40 AM, Elijah Newren <newren@gmail.com> wrote:
> This fixes an issue reported by Nick (no lastname given) on the
> mailing list, as well as a closely related issue in the handling of
> rename + directory/file conflicts, namely where a filename on one side
> of the rename is a directory name on the other side of the merge.
>
> Change since v1:
>  * Split unrelated 2/3 patch out into a separate submission

...oh, and now the series is based on master instead of next.  (I
would base it on maint, but since I was adding another testcase to
t3509 and t3509 does not yet exist in maint, I couldn't as easily put
it there.)

Re: [PATCHv2 2/2] merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dir

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> ... git can't know that there's a conflict
> until after it's tried resolving paths involving newsub/newfile to see if
> they are still in the way at the end (and if newsub/newfile is not in the
> way at the end, there should be no conflict at all, which did not hold with
> git previously).

I'll queue this patch to 'pu', but anybody who wrote the above to
correctly arriave at the crux of the issue in his analysis would know that
this is another band-aid on top of band-aid.  The approach merge-recursive
takes to first grab the set of directories and files fundamentally is
wrong---it should be resolving the paths in-core first and then look at
the result to ignore a directory that has become empty.