Merge branch 'en/d-f-conflict-fix'

* en/d-f-conflict-fix:
  merge-recursive: Avoid excessive output for and reprocessing of renames
  merge-recursive: Fix multiple file rename across D/F conflict
  t6031: Add a testcase covering multiple renames across a D/F conflict
  merge-recursive: Fix typo
  Mark tests that use symlinks as needing SYMLINKS prerequisite
  t/t6035-merge-dir-to-symlink.sh: Remove TODO on passing test
  fast-import: Improve robustness when D->F changes provided in wrong order
  fast-export: Fix output order of D/F changes
  merge_recursive: Fix renames across paths below D/F conflicts
  merge-recursive: Fix D/F conflicts
  Add a rename + D/F conflict testcase
  Add additional testcases for D/F conflicts

Conflicts:
	merge-recursive.c

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 66cafe63301ab242b4e7994ba08d9c75d33eca62..a9bbf8653d1a8fa704a38c06102ec3ace4a59a28 100644 (file)
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -148,10 +148,39 @@ static void handle_object(const unsigned char *sha1)
        free(buf);
 }
 
+static int depth_first(const void *a_, const void *b_)
+{
+       const struct diff_filepair *a = *((const struct diff_filepair **)a_);
+       const struct diff_filepair *b = *((const struct diff_filepair **)b_);
+       const char *name_a, *name_b;
+       int len_a, len_b, len;
+       int cmp;
+
+       name_a = a->one ? a->one->path : a->two->path;
+       name_b = b->one ? b->one->path : b->two->path;
+
+       len_a = strlen(name_a);
+       len_b = strlen(name_b);
+       len = (len_a < len_b) ? len_a : len_b;
+
+       /* strcmp will sort 'd' before 'd/e', we want 'd/e' before 'd' */
+       cmp = memcmp(name_a, name_b, len);
+       if (cmp)
+               return cmp;
+       return (len_b - len_a);
+}
+
 static void show_filemodify(struct diff_queue_struct *q,
                            struct diff_options *options, void *data)
 {
        int i;
+
+       /*
+        * Handle files below a directory first, in case they are all deleted
+        * and the directory changes to a file or symlink.
+        */
+       qsort(q->queue, q->nr, sizeof(q->queue[0]), depth_first);
+
        for (i = 0; i < q->nr; i++) {
                struct diff_filespec *ospec = q->queue[i]->one;
                struct diff_filespec *spec = q->queue[i]->two;

diff --git a/fast-import.c b/fast-import.c
index dd51ac48b60d93031aa761d6731dd066c04e6989..2317b0fe7509b957577234890135509143c0872b 100644 (file)
--- a/fast-import.c
+++ b/fast-import.c
@@ -1528,6 +1528,14 @@ static int tree_content_remove(
        for (i = 0; i < t->entry_count; i++) {
                e = t->entries[i];
                if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
+                       if (slash1 && !S_ISDIR(e->versions[1].mode))
+                               /*
+                                * If p names a file in some subdirectory, and a
+                                * file or symlink matching the name of the
+                                * parent directory of p exists, then p cannot
+                                * exist and need not be deleted.
+                                */
+                               return 1;
                        if (!slash1 || !S_ISDIR(e->versions[1].mode))
                                goto del_entry;
                        if (!e->tree)

diff --git a/merge-recursive.c b/merge-recursive.c
index 638076ec6ecde537b51041d1bdf4ef5a00a4b3cc..df90be44a51f3af5ebf9eb65e95b7d4fc18cc2f9 100644 (file)
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1017,14 +1017,22 @@ static int process_renames(struct merge_options *o,
 
                                if (mfi.clean &&
                                    sha_eq(mfi.sha, ren1->pair->two->sha1) &&
-                                   mfi.mode == ren1->pair->two->mode)
+                                   mfi.mode == ren1->pair->two->mode) {
                                        /*
-                                        * This messaged is part of
+                                        * This message is part of
                                         * t6022 test. If you change
                                         * it update the test too.
                                         */
                                        output(o, 3, "Skipped %s (merged same as existing)", ren1_dst);
-                               else {
+
+                                       /* There may be higher stage entries left
+                                        * in the index (e.g. due to a D/F
+                                        * conflict) that need to be resolved.
+                                        */
+                                       if (!ren1->dst_entry->stages[2].mode !=
+                                           !ren1->dst_entry->stages[3].mode)
+                                               ren1->dst_entry->processed = 0;
+                               } else {
                                        if (mfi.merge || !mfi.clean)
                                                output(o, 1, "Renaming %s => %s", ren1_src, ren1_dst);
                                        if (mfi.merge)
@@ -1070,6 +1078,7 @@ static int process_entry(struct merge_options *o,
        unsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);
        unsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);
 
+       entry->processed = 1;
        if (o_sha && (!a_sha || !b_sha)) {
                /* Case A: Deleted in one */
                if ((!a_sha && !b_sha) ||
@@ -1102,33 +1111,28 @@ static int process_entry(struct merge_options *o,
        } else if ((!o_sha && a_sha && !b_sha) ||
                   (!o_sha && !a_sha && b_sha)) {
                /* Case B: Added in one. */
-               const char *add_branch;
-               const char *other_branch;
                unsigned mode;
                const unsigned char *sha;
-               const char *conf;
 
                if (a_sha) {
-                       add_branch = o->branch1;
-                       other_branch = o->branch2;
                        mode = a_mode;
                        sha = a_sha;
-                       conf = "file/directory";
                } else {
-                       add_branch = o->branch2;
-                       other_branch = o->branch1;
                        mode = b_mode;
                        sha = b_sha;
-                       conf = "directory/file";
                }
                if (string_list_has_string(&o->current_directory_set, path)) {
-                       const char *new_path = unique_path(o, path, add_branch);
-                       clean_merge = 0;
-                       output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
-                              "Adding %s as %s",
-                              conf, path, other_branch, path, new_path);
-                       remove_file(o, 0, path, 0);
-                       update_file(o, 0, sha, mode, new_path);
+                       /* Handle D->F conflicts after all subfiles */
+                       entry->processed = 0;
+                       /* But get any file out of the way now, so conflicted
+                        * entries below the directory of the same name can
+                        * be put in the working directory.
+                        */
+                       if (a_sha)
+                               output(o, 2, "Removing %s", path);
+                       /* do not touch working file if it did not exist */
+                       remove_file(o, 0, path, !a_sha);
+                       return 1; /* Assume clean till processed */
                } else {
                        output(o, 2, "Adding %s", path);
                        update_file(o, 1, sha, mode, path);
@@ -1176,6 +1180,64 @@ static int process_entry(struct merge_options *o,
        return clean_merge;
 }
 
+/*
+ * Per entry merge function for D/F conflicts, to be called only after
+ * all files below dir have been processed.  We do this because in the
+ * cases we can cleanly resolve D/F conflicts, process_entry() can clean
+ * out all the files below the directory for us.
+ */
+static int process_df_entry(struct merge_options *o,
+                        const char *path, struct stage_data *entry)
+{
+       int clean_merge = 1;
+       unsigned o_mode = entry->stages[1].mode;
+       unsigned a_mode = entry->stages[2].mode;
+       unsigned b_mode = entry->stages[3].mode;
+       unsigned char *o_sha = stage_sha(entry->stages[1].sha, o_mode);
+       unsigned char *a_sha = stage_sha(entry->stages[2].sha, a_mode);
+       unsigned char *b_sha = stage_sha(entry->stages[3].sha, b_mode);
+       const char *add_branch;
+       const char *other_branch;
+       unsigned mode;
+       const unsigned char *sha;
+       const char *conf;
+       struct stat st;
+
+       /* We currently only handle D->F cases */
+       assert((!o_sha && a_sha && !b_sha) ||
+              (!o_sha && !a_sha && b_sha));
+
+       entry->processed = 1;
+
+       if (a_sha) {
+               add_branch = o->branch1;
+               other_branch = o->branch2;
+               mode = a_mode;
+               sha = a_sha;
+               conf = "file/directory";
+       } else {
+               add_branch = o->branch2;
+               other_branch = o->branch1;
+               mode = b_mode;
+               sha = b_sha;
+               conf = "directory/file";
+       }
+       if (lstat(path, &st) == 0 && S_ISDIR(st.st_mode)) {
+               const char *new_path = unique_path(o, path, add_branch);
+               clean_merge = 0;
+               output(o, 1, "CONFLICT (%s): There is a directory with name %s in %s. "
+                      "Adding %s as %s",
+                      conf, path, other_branch, path, new_path);
+               remove_file(o, 0, path, 0);
+               update_file(o, 0, sha, mode, new_path);
+       } else {
+               output(o, 2, "Adding %s", path);
+               update_file(o, 1, sha, mode, path);
+       }
+
+       return clean_merge;
+}
+
 void set_porcelain_error_msgs(const char **msgs, const char *cmd)
 {
        const char *msg;
@@ -1269,6 +1331,13 @@ int merge_trees(struct merge_options *o,
                                && !process_entry(o, path, e))
                                clean = 0;
                }
+               for (i = 0; i < entries->nr; i++) {
+                       const char *path = entries->items[i].string;
+                       struct stage_data *e = entries->items[i].util;
+                       if (!e->processed
+                               && !process_df_entry(o, path, e))
+                               clean = 0;
+               }
 
                string_list_clear(re_merge, 0);
                string_list_clear(re_head, 0);

diff --git a/t/t3509-cherry-pick-merge-df.sh b/t/t3509-cherry-pick-merge-df.sh
new file mode 100755 (executable)
index 0000000..a5ccdbf
--- /dev/null
+++ b/t/t3509-cherry-pick-merge-df.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+test_description='Test cherry-pick with directory/file conflicts'
+. ./test-lib.sh
+
+test_expect_success SYMLINKS 'Setup rename across paths each below D/F conflicts' '
+       mkdir a &&
+       >a/f &&
+       git add a &&
+       git commit -m a &&
+
+       mkdir b &&
+       ln -s ../a b/a &&
+       git add b &&
+       git commit -m b &&
+
+       git checkout -b branch &&
+       rm b/a &&
+       mv a b/a &&
+       ln -s b/a a &&
+       git add . &&
+       git commit -m swap &&
+
+       >f1 &&
+       git add f1 &&
+       git commit -m f1
+'
+
+test_expect_success SYMLINKS 'Cherry-pick succeeds with rename across D/F conflicts' '
+       git reset --hard &&
+       git checkout master^0 &&
+       git cherry-pick branch
+'
+
+test_done

diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh
index e71c687f2b9473842684fae1a1b4b013c721497f..490d3971142a87e940f3d27e7e7f068b276ff602 100755 (executable)
--- a/t/t6020-merge-df.sh
+++ b/t/t6020-merge-df.sh
@@ -22,7 +22,7 @@ git commit -m "File: dir"'
 
 test_expect_code 1 'Merge with d/f conflicts' 'git merge "merge msg" B master'
 
-test_expect_failure 'F/D conflict' '
+test_expect_success 'F/D conflict' '
        git reset --hard &&
        git checkout master &&
        rm .git/index &&

diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh
index 8a3304fb0b5901fb02435d3b77c3d049404f4e25..bd75e0e6430ff6e536f0b10d28cc4793d0f7fe6c 100755 (executable)
--- a/t/t6031-merge-recursive.sh
+++ b/t/t6031-merge-recursive.sh
@@ -57,4 +57,35 @@ test_expect_success FILEMODE 'verify executable bit on file' '
        test -x file2
 '
 
+test_expect_success 'merging with triple rename across D/F conflict' '
+       git reset --hard HEAD &&
+       git checkout -b main &&
+       git rm -rf . &&
+
+       echo "just a file" >sub1 &&
+       mkdir -p sub2 &&
+       echo content1 >sub2/file1 &&
+       echo content2 >sub2/file2 &&
+       echo content3 >sub2/file3 &&
+       mkdir simple &&
+       echo base >simple/bar &&
+       git add -A &&
+       test_tick &&
+       git commit -m base &&
+
+       git checkout -b other &&
+       echo more >>simple/bar &&
+       test_tick &&
+       git commit -a -m changesimplefile &&
+
+       git checkout main &&
+       git rm sub1 &&
+       git mv sub2 sub1 &&
+       test_tick &&
+       git commit -m changefiletodir &&
+
+       test_tick &&
+       git merge other
+'
+
 test_done

diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
index cd3190c4a61f0404491b41a1b22f5143b63f4992..dc09513be5a78bf12139efd931ee0344b2710764 100755 (executable)
--- a/t/t6035-merge-dir-to-symlink.sh
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -48,7 +48,7 @@ test_expect_success 'setup for merge test' '
        git tag baseline
 '
 
-test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '
+test_expect_success 'Handle D/F conflict, do not lose a/b-2/c/d in merge (resolve)' '
        git reset --hard &&
        git checkout baseline^0 &&
        git merge -s resolve master &&
@@ -56,7 +56,7 @@ test_expect_success 'do not lose a/b-2/c/d in merge (resolve)' '
        test -f a/b-2/c/d
 '
 
-test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '
+test_expect_success 'Handle D/F conflict, do not lose a/b-2/c/d in merge (recursive)' '
        git reset --hard &&
        git checkout baseline^0 &&
        git merge -s recursive master &&
@@ -64,6 +64,54 @@ test_expect_failure 'do not lose a/b-2/c/d in merge (recursive)' '
        test -f a/b-2/c/d
 '
 
+test_expect_success 'Handle F/D conflict, do not lose a/b-2/c/d in merge (resolve)' '
+       git reset --hard &&
+       git checkout master^0 &&
+       git merge -s resolve baseline^0 &&
+       test -h a/b &&
+       test -f a/b-2/c/d
+'
+
+test_expect_success 'Handle F/D conflict, do not lose a/b-2/c/d in merge (recursive)' '
+       git reset --hard &&
+       git checkout master^0 &&
+       git merge -s recursive baseline^0 &&
+       test -h a/b &&
+       test -f a/b-2/c/d
+'
+
+test_expect_failure 'do not lose untracked in merge (resolve)' '
+       git reset --hard &&
+       git checkout baseline^0 &&
+       >a/b/c/e &&
+       test_must_fail git merge -s resolve master &&
+       test -f a/b/c/e &&
+       test -f a/b-2/c/d
+'
+
+test_expect_success 'do not lose untracked in merge (recursive)' '
+       git reset --hard &&
+       git checkout baseline^0 &&
+       >a/b/c/e &&
+       test_must_fail git merge -s recursive master &&
+       test -f a/b/c/e &&
+       test -f a/b-2/c/d
+'
+
+test_expect_success 'do not lose modifications in merge (resolve)' '
+       git reset --hard &&
+       git checkout baseline^0 &&
+       echo more content >>a/b/c/d &&
+       test_must_fail git merge -s resolve master
+'
+
+test_expect_success 'do not lose modifications in merge (recursive)' '
+       git reset --hard &&
+       git checkout baseline^0 &&
+       echo more content >>a/b/c/d &&
+       test_must_fail git merge -s recursive master
+'
+
 test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
        git reset --hard &&
        git checkout start^0 &&
@@ -74,7 +122,7 @@ test_expect_success 'setup a merge where dir a/b-2 changed to symlink' '
        git tag test2
 '
 
-test_expect_success 'merge should not have conflicts (resolve)' '
+test_expect_success 'merge should not have D/F conflicts (resolve)' '
        git reset --hard &&
        git checkout baseline^0 &&
        git merge -s resolve test2 &&
@@ -82,7 +130,7 @@ test_expect_success 'merge should not have conflicts (resolve)' '
        test -f a/b/c/d
 '
 
-test_expect_failure 'merge should not have conflicts (recursive)' '
+test_expect_success 'merge should not have D/F conflicts (recursive)' '
        git reset --hard &&
        git checkout baseline^0 &&
        git merge -s recursive test2 &&
@@ -90,4 +138,12 @@ test_expect_failure 'merge should not have conflicts (recursive)' '
        test -f a/b/c/d
 '
 
+test_expect_success 'merge should not have F/D conflicts (recursive)' '
+       git reset --hard &&
+       git checkout -b foo test2 &&
+       git merge -s recursive baseline^0 &&
+       test -h a/b-2 &&
+       test -f a/b/c/d
+'
+
 test_done

diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index d831404fba8d982f4600d6ed310c3acb3b893ac8..8c8e679468f4b191f93ca68a973d4d58fa1b72d2 100755 (executable)
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -390,4 +390,28 @@ test_expect_success 'tree_tag-obj'    'git fast-export tree_tag-obj'
 test_expect_success 'tag-obj_tag'     'git fast-export tag-obj_tag'
 test_expect_success 'tag-obj_tag-obj' 'git fast-export tag-obj_tag-obj'
 
+test_expect_success SYMLINKS 'directory becomes symlink'        '
+       git init dirtosymlink &&
+       git init result &&
+       (
+               cd dirtosymlink &&
+               mkdir foo &&
+               mkdir bar &&
+               echo hello > foo/world &&
+               echo hello > bar/world &&
+               git add foo/world bar/world &&
+               git commit -q -mone &&
+               git rm -r foo &&
+               ln -s bar foo &&
+               git add foo &&
+               git commit -q -mtwo
+       ) &&
+       (
+               cd dirtosymlink &&
+               git fast-export master -- foo |
+               (cd ../result && git fast-import --quiet)
+       ) &&
+       (cd result && git show master:foo)
+'
+
 test_done