Merge branch 'jk/merge-subtree-heuristics'
authorJunio C Hamano <gitster@pobox.com>
Fri, 17 Aug 2018 20:09:55 +0000 (13:09 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 17 Aug 2018 20:09:55 +0000 (13:09 -0700)
The automatic tree-matching in "git merge -s subtree" was broken 5
years ago and nobody has noticed since then, which is now fixed.

* jk/merge-subtree-heuristics:
score_trees(): fix iteration over trees with missing entries

1  2 
match-trees.c
t/t6029-merge-subtree.sh
diff --combined match-trees.c
index 4cdeff53e1e8533b0dba168c19187353901253ea,ad9e5e97444aeb7b2371a171a2133070b3e2e630..37653308d3601e56191d2f83f967cc77ad1517af
@@@ -1,7 -1,6 +1,7 @@@
  #include "cache.h"
  #include "tree.h"
  #include "tree-walk.h"
 +#include "object-store.h"
  
  static int score_missing(unsigned mode, const char *path)
  {
@@@ -55,7 -54,7 +55,7 @@@ static void *fill_tree_desc_strict(stru
        enum object_type type;
        unsigned long size;
  
 -      buffer = read_sha1_file(hash->hash, &type, &size);
 +      buffer = read_object_file(hash, &type, &size);
        if (!buffer)
                die("unable to read tree (%s)", oid_to_hex(hash));
        if (type != OBJ_TREE)
@@@ -83,34 -82,43 +83,43 @@@ static int score_trees(const struct obj
        int score = 0;
  
        for (;;) {
-               struct name_entry e1, e2;
-               int got_entry_from_one = tree_entry(&one, &e1);
-               int got_entry_from_two = tree_entry(&two, &e2);
                int cmp;
  
-               if (got_entry_from_one && got_entry_from_two)
-                       cmp = base_name_entries_compare(&e1, &e2);
-               else if (got_entry_from_one)
+               if (one.size && two.size)
+                       cmp = base_name_entries_compare(&one.entry, &two.entry);
+               else if (one.size)
                        /* two lacks this entry */
                        cmp = -1;
-               else if (got_entry_from_two)
+               else if (two.size)
                        /* two has more entries */
                        cmp = 1;
                else
                        break;
  
-               if (cmp < 0)
+               if (cmp < 0) {
                        /* path1 does not appear in two */
-                       score += score_missing(e1.mode, e1.path);
-               else if (cmp > 0)
+                       score += score_missing(one.entry.mode, one.entry.path);
+                       update_tree_entry(&one);
+               } else if (cmp > 0) {
                        /* path2 does not appear in one */
-                       score += score_missing(e2.mode, e2.path);
-               else if (oidcmp(e1.oid, e2.oid))
-                       /* they are different */
-                       score += score_differs(e1.mode, e2.mode, e1.path);
-               else
-                       /* same subtree or blob */
-                       score += score_matches(e1.mode, e2.mode, e1.path);
+                       score += score_missing(two.entry.mode, two.entry.path);
+                       update_tree_entry(&two);
+               } else {
+                       /* path appears in both */
+                       if (oidcmp(one.entry.oid, two.entry.oid)) {
+                               /* they are different */
+                               score += score_differs(one.entry.mode,
+                                                      two.entry.mode,
+                                                      one.entry.path);
+                       } else {
+                               /* same subtree or blob */
+                               score += score_matches(one.entry.mode,
+                                                      two.entry.mode,
+                                                      one.entry.path);
+                       }
+                       update_tree_entry(&one);
+                       update_tree_entry(&two);
+               }
        }
        free(one_buf);
        free(two_buf);
@@@ -159,20 -167,22 +168,20 @@@ static void match_trees(const struct ob
  }
  
  /*
 - * A tree "hash1" has a subdirectory at "prefix".  Come up with a
 - * tree object by replacing it with another tree "hash2".
 + * A tree "oid1" has a subdirectory at "prefix".  Come up with a tree object by
 + * replacing it with another tree "oid2".
   */
 -static int splice_tree(const unsigned char *hash1,
 -                     const char *prefix,
 -                     const unsigned char *hash2,
 -                     unsigned char *result)
 +static int splice_tree(const struct object_id *oid1, const char *prefix,
 +                     const struct object_id *oid2, struct object_id *result)
  {
        char *subpath;
        int toplen;
        char *buf;
        unsigned long sz;
        struct tree_desc desc;
 -      unsigned char *rewrite_here;
 -      const unsigned char *rewrite_with;
 -      unsigned char subtree[20];
 +      struct object_id *rewrite_here;
 +      const struct object_id *rewrite_with;
 +      struct object_id subtree;
        enum object_type type;
        int status;
  
        if (*subpath)
                subpath++;
  
 -      buf = read_sha1_file(hash1, &type, &sz);
 +      buf = read_object_file(oid1, &type, &sz);
        if (!buf)
 -              die("cannot read tree %s", sha1_to_hex(hash1));
 +              die("cannot read tree %s", oid_to_hex(oid1));
        init_tree_desc(&desc, buf, sz);
  
        rewrite_here = NULL;
                if (strlen(name) == toplen &&
                    !memcmp(name, prefix, toplen)) {
                        if (!S_ISDIR(mode))
 -                              die("entry %s in tree %s is not a tree",
 -                                  name, sha1_to_hex(hash1));
 -                      rewrite_here = (unsigned char *) oid->hash;
 +                              die("entry %s in tree %s is not a tree", name,
 +                                  oid_to_hex(oid1));
 +                      rewrite_here = (struct object_id *)oid;
                        break;
                }
                update_tree_entry(&desc);
        }
        if (!rewrite_here)
 -              die("entry %.*s not found in tree %s",
 -                  toplen, prefix, sha1_to_hex(hash1));
 +              die("entry %.*s not found in tree %s", toplen, prefix,
 +                  oid_to_hex(oid1));
        if (*subpath) {
 -              status = splice_tree(rewrite_here, subpath, hash2, subtree);
 +              status = splice_tree(rewrite_here, subpath, oid2, &subtree);
                if (status)
                        return status;
 -              rewrite_with = subtree;
 +              rewrite_with = &subtree;
 +      } else {
 +              rewrite_with = oid2;
        }
 -      else
 -              rewrite_with = hash2;
 -      hashcpy(rewrite_here, rewrite_with);
 -      status = write_sha1_file(buf, sz, tree_type, result);
 +      oidcpy(rewrite_here, rewrite_with);
 +      status = write_object_file(buf, sz, tree_type, result);
        free(buf);
        return status;
  }
@@@ -270,7 -280,7 +279,7 @@@ void shift_tree(const struct object_id 
                if (!*del_prefix)
                        return;
  
 -              if (get_tree_entry(hash2->hash, del_prefix, shifted->hash, &mode))
 +              if (get_tree_entry(hash2, del_prefix, shifted, &mode))
                        die("cannot find path %s in tree %s",
                            del_prefix, oid_to_hex(hash2));
                return;
        if (!*add_prefix)
                return;
  
 -      splice_tree(hash1->hash, add_prefix, hash2->hash, shifted->hash);
 +      splice_tree(hash1, add_prefix, hash2, shifted);
  }
  
  /*
@@@ -297,12 -307,12 +306,12 @@@ void shift_tree_by(const struct object_
        unsigned candidate = 0;
  
        /* Can hash2 be a tree at shift_prefix in tree hash1? */
 -      if (!get_tree_entry(hash1->hash, shift_prefix, sub1.hash, &mode1) &&
 +      if (!get_tree_entry(hash1, shift_prefix, &sub1, &mode1) &&
            S_ISDIR(mode1))
                candidate |= 1;
  
        /* Can hash1 be a tree at shift_prefix in tree hash2? */
 -      if (!get_tree_entry(hash2->hash, shift_prefix, sub2.hash, &mode2) &&
 +      if (!get_tree_entry(hash2, shift_prefix, &sub2, &mode2) &&
            S_ISDIR(mode2))
                candidate |= 2;
  
                 * shift tree2 down by adding shift_prefix above it
                 * to match tree1.
                 */
 -              splice_tree(hash1->hash, shift_prefix, hash2->hash, shifted->hash);
 +              splice_tree(hash1, shift_prefix, hash2, shifted);
        else
                /*
                 * shift tree2 up by removing shift_prefix from it
diff --combined t/t6029-merge-subtree.sh
index 7d5bc784721411a3abe475a8ed9f291204072b7d,474a850de6084966f5137cb4fa4d8c5cf036e8e7..793f0c8bf38aba350542c0710f509a1c69b33a9b
@@@ -29,6 -29,34 +29,34 @@@ test_expect_success 'subtree available 
  
  '
  
+ test_expect_success 'setup branch sub' '
+       git checkout --orphan sub &&
+       git rm -rf . &&
+       test_commit foo
+ '
+ test_expect_success 'setup branch main' '
+       git checkout -b main master &&
+       git merge -s ours --no-commit --allow-unrelated-histories sub &&
+       git read-tree --prefix=dir/ -u sub &&
+       git commit -m "initial merge of sub into main" &&
+       test_path_is_file dir/foo.t &&
+       test_path_is_file hello
+ '
+ test_expect_success 'update branch sub' '
+       git checkout sub &&
+       test_commit bar
+ '
+ test_expect_success 'update branch main' '
+       git checkout main &&
+       git merge -s subtree sub -m "second merge of sub into main" &&
+       test_path_is_file dir/bar.t &&
+       test_path_is_file dir/foo.t &&
+       test_path_is_file hello
+ '
  test_expect_success 'setup' '
        mkdir git-gui &&
        cd git-gui &&
@@@ -55,7 -83,7 +83,7 @@@ test_expect_success 'initial merge' 
        git checkout -b work &&
        git ls-files -s >actual &&
        (
 -              echo "100644 $o1 0      git-gui/git-gui.sh"
 +              echo "100644 $o1 0      git-gui/git-gui.sh" &&
                echo "100644 $o2 0      git.c"
        ) >expected &&
        test_cmp expected actual
@@@ -72,7 -100,7 +100,7 @@@ test_expect_success 'merge update' 
        git pull -s subtree gui master2 &&
        git ls-files -s >actual &&
        (
 -              echo "100644 $o3 0      git-gui/git-gui.sh"
 +              echo "100644 $o3 0      git-gui/git-gui.sh" &&
                echo "100644 $o2 0      git.c"
        ) >expected &&
        test_cmp expected actual
@@@ -88,8 -116,8 +116,8 @@@ test_expect_success 'initial ambiguous 
        git checkout -b work2 &&
        git ls-files -s >actual &&
        (
 -              echo "100644 $o1 0      git-gui/git-gui.sh"
 -              echo "100644 $o1 0      git-gui2/git-gui.sh"
 +              echo "100644 $o1 0      git-gui/git-gui.sh" &&
 +              echo "100644 $o1 0      git-gui2/git-gui.sh" &&
                echo "100644 $o2 0      git.c"
        ) >expected &&
        test_cmp expected actual
@@@ -101,8 -129,8 +129,8 @@@ test_expect_success 'merge using explic
        git pull -Xsubtree=git-gui gui master2 &&
        git ls-files -s >actual &&
        (
 -              echo "100644 $o3 0      git-gui/git-gui.sh"
 -              echo "100644 $o1 0      git-gui2/git-gui.sh"
 +              echo "100644 $o3 0      git-gui/git-gui.sh" &&
 +              echo "100644 $o1 0      git-gui2/git-gui.sh" &&
                echo "100644 $o2 0      git.c"
        ) >expected &&
        test_cmp expected actual
@@@ -114,8 -142,8 +142,8 @@@ test_expect_success 'merge2 using expli
        git pull -Xsubtree=git-gui2 gui master2 &&
        git ls-files -s >actual &&
        (
 -              echo "100644 $o1 0      git-gui/git-gui.sh"
 -              echo "100644 $o3 0      git-gui2/git-gui.sh"
 +              echo "100644 $o1 0      git-gui/git-gui.sh" &&
 +              echo "100644 $o3 0      git-gui2/git-gui.sh" &&
                echo "100644 $o2 0      git.c"
        ) >expected &&
        test_cmp expected actual