From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 17 Aug 2018 20:09:55 +0000 (-0700)
Subject: Merge branch 'jk/merge-subtree-heuristics'
X-Git-Tag: v2.19.0-rc0~53
X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/60858f343aa909305c28790c7684bf88905695a2?hp=-c

Merge branch 'jk/merge-subtree-heuristics'

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
---

60858f343aa909305c28790c7684bf88905695a2
diff --combined match-trees.c
index 4cdeff53e1,ad9e5e9744..37653308d3
--- a/match-trees.c
+++ b/match-trees.c
@@@ -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;
  
@@@ -181,9 -191,9 +190,9 @@@
  	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;
@@@ -196,26 -206,26 +205,26 @@@
  		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;
@@@ -279,7 -289,7 +288,7 @@@
  	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;
  
@@@ -333,7 -343,7 +342,7 @@@
  		 * 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 7d5bc78472,474a850de6..793f0c8bf3
--- a/t/t6029-merge-subtree.sh
+++ b/t/t6029-merge-subtree.sh
@@@ -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