cherry-picking a commit clobbers a file which is a directory in the target commit

This was seen on git version 1.7.0.4 on Ubuntu Lucid.

Basically, cherry-picking a commit from a branch where a file in the current
branch has been replaced by a directory, git clobbers the file and the
cherry-pick fails reporting conflicts.

To replicate:

   $ mkdir clobber
   $ cd clobber/
   $ git init
   $ touch sausage
   $ git add sausage
   $ git commit -m "added sausage"
   $ git checkout -b branch1
   $ mv sausage sausage1
   $ mkdir sausage
   $ mv sausage1 sausage/roll
   $ git add sausage/roll
   $ git commit -m "renamed sausage as sausage/roll"
   $ touch falafel
   $ git add falafel
   $ git commit -m "added falafel"
   $ git checkout master

Now if you try to cherry pick the commit just added to branch1 onto master:

    $ git cherry-pick branch1
    Automatic cherry-pick failed.  After resolving the conflicts,
    mark the corrected paths with 'git add <paths>' or 'git rm <paths>'
    and commit the result with:

            git commit -c branch1

    $ git status
    # On branch master
    # Changes to be committed:
    #   (use "git reset HEAD <file>..." to unstage)
    #
    #       renamed:    sausage -> falafel
    #
    # Untracked files:
    #   (use "git add <file>..." to include in what will be committed)
    #
    #       sausage~HEAD

Not what I expected at all. I'd not expect the file 'sausage' to be modified,
just the new file 'falafel' added, as I did in the original commit.

I know I can recover from this by moving sausage~HEAD back to sausage, or delete
it and check out sausage again, but I suspect it just shouldn't happen at all.


Is this a bug?

N

I've been Warnocked.  Can anyone point me in the right direction?

On Mon, Sep 6, 2010 at 3:50 AM, Nick [off-list ref] wrote:

I've been Warnocked.  Can anyone point me in the right direction?

It's a bug in the recursive merge strategy (i.e. the default one), and
affects current master as well as 1.7.0.4.  The resolve strategy
(which can be used with cherry-pick since 1.7.2) handles this
correctly:

$ git cherry-pick --strategy=resolve branch1
Trying simple merge.
Simple merge failed, trying Automatic merge.
[master e95e377] added falafel
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 falafel

I'll investigate.


Elijah

This fixes an issue reported by Nick, as well as a closely related
issue in the handling of rename + directory/file conflicts,
particularly where a file on one side of the rename is a directory
name on the other side of the merge.

Elijah Newren (3):
  t3509: Add rename + D/F conflict testcases that recursive strategy
    fails
  merge-recursive: Small code cleanup
  merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dir

 merge-recursive.c               |   50 ++++++++++++++++-------------
 t/t3509-cherry-pick-merge-df.sh |   66 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+), 22 deletions(-)

-- 
1.7.3.rc0.170.g5cfb0.dirty

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 <redacted>
---
 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.170.g5cfb0.dirty

process_renames() had a variable named "stage" and derived variables
src_other and dst_other whose purpose was not entirely clear to me.  Make
the name of stage slightly more descriptive and add a brief comment
explaining what is occurring.

Also, in d5af510 (RE: [PATCH] Avoid rename/add conflict when contents are
identical 2010-09-01), a separate if-block was added to provide a special
case for the rename/add conflict case that can be resolved (namely when
the contents on the destination side are identical).  However, as a
separate if block, it's not immediately obvious that its code is related to
the subsequent code checking for a rename/add conflict.  We can combine and
simplify the check slightly.

Signed-off-by: Elijah Newren <redacted>
---
 merge-recursive.c |   34 ++++++++++++++++++++--------------
 1 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index c574698..5e2886a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -923,15 +923,26 @@ static int process_renames(struct merge_options *o,
 			/* Renamed in 1, maybe changed in 2 */
 			struct string_list_item *item;
 			/* we only use sha1 and mode of these */
-			struct diff_filespec src_other, dst_other;
-			int try_merge, stage = a_renames == renames1 ? 3: 2;
+			struct diff_filespec src_other, dst_other, dst_renamed;
+			int try_merge;
 
-			remove_file(o, 1, ren1_src, o->call_depth || stage == 3);
+			/*
+			 * unpack_trees loads entries from common-commit
+			 * into stage 1, from head-commit into stage 2, and
+			 * from merge-commit into stage 3.  We keep track
+			 * of which side corresponds to the rename.
+			 */
+			int renamed_stage = a_renames == renames1 ? 2 : 3;
+			int other_stage =   a_renames == renames1 ? 3 : 2;
+
+			remove_file(o, 1, ren1_src, o->call_depth || renamed_stage == 2);
 
-			hashcpy(src_other.sha1, ren1->src_entry->stages[stage].sha);
-			src_other.mode = ren1->src_entry->stages[stage].mode;
-			hashcpy(dst_other.sha1, ren1->dst_entry->stages[stage].sha);
-			dst_other.mode = ren1->dst_entry->stages[stage].mode;
+			hashcpy(src_other.sha1, ren1->src_entry->stages[other_stage].sha);
+			src_other.mode = ren1->src_entry->stages[other_stage].mode;
+			hashcpy(dst_other.sha1, ren1->dst_entry->stages[other_stage].sha);
+			dst_other.mode = ren1->dst_entry->stages[other_stage].mode;
+			hashcpy(dst_renamed.sha1, ren1->dst_entry->stages[renamed_stage].sha);
+			dst_renamed.mode = ren1->dst_entry->stages[renamed_stage].mode;
 
 			try_merge = 0;
 

@@ -955,13 +966,8 @@ static int process_renames(struct merge_options *o,
 							ren1->pair->two : NULL,
 							branch1 == o->branch1 ?
 							NULL : ren1->pair->two, 1);
-			} else if ((dst_other.mode == ren1->pair->two->mode) &&
-				   sha_eq(dst_other.sha1, ren1->pair->two->sha1)) {
-				/* Added file on the other side
-				   identical to the file being
-				   renamed: clean merge */
-				update_file(o, 1, ren1->pair->two->sha1, ren1->pair->two->mode, ren1_dst);
-			} else if (!sha_eq(dst_other.sha1, null_sha1)) {
+			} else if (!sha_eq(dst_other.sha1, null_sha1) &&
+				   !sha_eq(dst_other.sha1, dst_renamed.sha1)) {
 				const char *new_path;
 				clean_merge = 0;
 				try_merge = 1;

-- 
1.7.3.rc0.170.g5cfb0.dirty

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 <redacted>
---
 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 5e2886a..645a79d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -946,14 +946,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",

@@ -1046,6 +1039,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.170.g5cfb0.dirty

On Mon, Sep 6, 2010 at 2:47 PM, Elijah Newren [off-list ref] wrote:

This fixes an issue reported by Nick, as well as a closely related
issue in the handling of rename + directory/file conflicts,
particularly where a file on one side of the rename is a directory
name on the other side of the merge.

I forgot to mention; this patch series is based on next, since it
touches some of the code from ks/recursive-rename-add-identical.

Elijah Newren (3):
 t3509: Add rename + D/F conflict testcases that recursive strategy
   fails
 merge-recursive: Small code cleanup
 merge-recursive: D/F conflicts where was_a_dir/file -> was_a_dir

 merge-recursive.c               |   50 ++++++++++++++++-------------
 t/t3509-cherry-pick-merge-df.sh |   66 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+), 22 deletions(-)

On Mon, Sep 6, 2010 at 2:47 PM, Elijah Newren [off-list ref] wrote:

process_renames() had a variable named "stage" and derived variables
src_other and dst_other whose purpose was not entirely clear to me.  Make
the name of stage slightly more descriptive and add a brief comment
explaining what is occurring.

Also, in d5af510 (RE: [PATCH] Avoid rename/add conflict when contents are
identical 2010-09-01), a separate if-block was added to provide a special
case for the rename/add conflict case that can be resolved (namely when
the contents on the destination side are identical).  However, as a
separate if block, it's not immediately obvious that its code is related to
the subsequent code checking for a rename/add conflict.  We can combine and
simplify the check slightly.

Signed-off-by: Elijah Newren <redacted>
---

Hmmm...should I have split this off from the rest of the series (its
only relation is that it cleans up code that made it harder for me to
find the real fix)?  If I did that, I could rebase the rest of the
series on maint...

Also, in d5af510 (RE: [PATCH] Avoid rename/add conflict when contents are
identical 2010-09-01), a separate if-block was added to provide a special
case for the rename/add conflict case that can be resolved (namely when
the contents on the destination side are identical).  However, as a
separate if block, it's not immediately obvious that its code is related to
the subsequent code checking for a rename/add conflict.  We can combine and
simplify the check slightly.

Originally I tried the fix the way you've re-structured it, just adding a test to the if around the rename/add conflict handling.  Unfortunately that didn't completely solve the problem in the case that originally motivated the fix (rename vs. rename+symlink, as in my initial post and my first attempt at adding a test to t/t3030-merge-recursive.sh).  That's why I changed it to a separate if block.

The problem comes down in the code inside the "if(try_merge)" block below.  It merges the source of the rename on the other side with the renamed file, rather than the destination.  In the case with the symlink on the other side, this code merged a symlink with a regular file which resulted in a conflict.  I was trying to eliminate both conflicts in this case by avoiding the final else that sets try_merge=1.

Your re-structuring will therefore only solve half the problem I was trying to solve.

I suppose an alternative solution would have been to change the "if(try_merge)" code to merge with the destination of the rename on the other side, if it exists and is the same type.  However that clearly would have had a much more significant impact on other merge cases, so it didn't seem like a good choice to me.

--Ken

On Tue, Sep 7, 2010 at 10:23 AM, Schalk, Ken [off-list ref] wrote:

quoted

Also, in d5af510 (RE: [PATCH] Avoid rename/add conflict when contents are
identical 2010-09-01), a separate if-block was added to provide a special
case for the rename/add conflict case that can be resolved (namely when
the contents on the destination side are identical).  However, as a
separate if block, it's not immediately obvious that its code is related to
the subsequent code checking for a rename/add conflict.  We can combine and
simplify the check slightly.

Originally I tried the fix the way you've re-structured it, just adding a test to the if around the rename/add conflict handling.  Unfortunately that didn't completely solve the problem in the case that originally motivated the fix (rename vs. rename+symlink, as in my initial post and my first attempt at adding a test to t/t3030-merge-recursive.sh).  That's why I changed it to a separate if block.

The problem comes down in the code inside the "if(try_merge)" block below.  It merges the source of the rename on the other side with the renamed file, rather than the destination.  In the case with the symlink on the other side, this code merged a symlink with a regular file which resulted in a conflict.  I was trying to eliminate both conflicts in this case by avoiding the final else that sets try_merge=1.

Your re-structuring will therefore only solve half the problem I was trying to solve.

I suppose an alternative solution would have been to change the "if(try_merge)" code to merge with the destination of the rename on the other side, if it exists and is the same type.  However that clearly would have had a much more significant impact on other merge cases, so it didn't seem like a good choice to me.

Interesting...that means we probably should have stuck with the
original testcase you suggested (though marking it with the SYMLINK
dependence), since the new one doesn't fail with my modifications but
the old one would.  The typechange is critical.  So I'll drop that
portion of my patch.

Perhaps you could submit another patch changing your testcase back to
using a symlink to make sure someone like me doesn't break your
original testcase in the future?


Thanks,
Elijah

Perhaps you could submit another patch changing your testcase back to
using a symlink to make sure someone like me doesn't break your
original testcase in the future?

Here's a patch relative to my last one.  Rather than restoring the previous test, I added it so that platforms with no symlink support can still test copy vs. rename and platforms with symlink support can also test rename vs. rename/symlink.

Signed-off-by: Ken Schalk <redacted>
---
 t/t3030-merge-recursive.sh |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index b23bd9f..9514ae2 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh

@@ -25,6 +25,10 @@ test_expect_success 'setup 1' '
        git branch submod &&
        git branch copy &&
        git branch rename &&
+       if test_have_prereq SYMLINKS
+       then
+               git branch rename-ln
+       fi &&

        echo hello >>a &&
        cp a d/e &&

@@ -256,7 +260,17 @@ test_expect_success 'setup 8' '
        git mv a e &&
        git add e &&
        test_tick &&
-       git commit -m "rename a->e"
+       git commit -m "rename a->e" &&
+       if test_have_prereq SYMLINKS
+       then
+               git checkout rename-ln &&
+               git mv a e &&
+               ln -s e a &&
+               git add a e &&
+               test_tick &&
+               git commit -m "rename a->e, symlink a->e"
+       fi
+
 '

 test_expect_success 'setup 9' '

@@ -618,5 +632,25 @@ test_expect_success 'merge-recursive copy vs. rename' '
        test_cmp expected actual
 '

+if test_have_prereq SYMLINKS
+then
+       test_expect_success 'merge-recursive rename vs. rename/symlink' '
+
+               git checkout -f rename &&
+               git merge rename-ln &&
+               ( git ls-tree -r HEAD ; git ls-files -s ) >actual &&
+               (
+                       echo "100644 blob $o0   b"
+                       echo "100644 blob $o0   c"
+                       echo "100644 blob $o0   d/e"
+                       echo "100644 blob $o0   e"
+                       echo "100644 $o0 0      b"
+                       echo "100644 $o0 0      c"
+                       echo "100644 $o0 0      d/e"
+                       echo "100644 $o0 0      e"
+               ) >expected &&
+               test_cmp expected actual
+       '
+fi

 test_done
--

1.7.0

On Thu, 9 Sep 2010 13:23:26 -0700
"Schalk, Ken" [off-list ref] wrote:

quoted

Perhaps you could submit another patch changing your testcase back to
using a symlink to make sure someone like me doesn't break your
original testcase in the future?

Here's a patch relative to my last one.  Rather than restoring the
previous test, I added it so that platforms with no symlink support
can still test copy vs. rename and platforms with symlink support can
also test rename vs. rename/symlink.

Hello, I think I have a test case that seems to be related to this
issue.

mkdir -p repo1
pushd repo1
git init .

mkdir dir1
echo file1 > dir1/file1
ln -s dir1 dir2
git add dir1 dir2
git commit -m "Initial status: dir2 -> dir1"

git checkout -b test1
git checkout -b test2

git co test1
git rm dir2
mkdir dir2
touch file2 > dir2/file1
git add dir2/file1
git commit -m "Removing link: dir1/ and dir2/"

message="New file in test1"
echo $message > new_file_test1
git add new_file_test1
git commit -m "$message"

git co test2
message="New file in test2"
echo $message > new_file_test2
git add new_file_test2
git commit -m "$message"

# Tries to get the last commit (which adds new_file_test1)
# into test2 fails.
git cherry-pick test1

# Would work with: git cherry-pick --strategy=resolve test1 
# (using 1.7.3.1)