Re: git merge -s subtree seems to be broken.

From

Jeff King <peff@peff.net>

Date

Jul 31, 2018, 16:15 UTC

Message-ID

<20180731161559.GB16910@sigill.intra.peff.net>

In-Reply-To

<xmqqtvofcsgc.fsf@gitster-ct.c.googlers.com>

On Tue, Jul 31, 2018 at 08:53:23AM -0700, Junio C Hamano wrote:

> George Shammas <georgyo@gmail.com> writes:
> 
> > Bisecting around, this might be the commit that introduced the breakage.
> >
> > https://github.com/git/git/commit/d8febde
> 
> Interesting.  I've never used the "-s subtree" strategy without
> "-Xsubtree=..." to explicitly tell where the thing should go for a
> long time, so I am not surprised if I did not notice if an update to
> the heuristics made long time ago had affected tree matching.
> 
> d8febde3 ("match-trees: simplify score_trees() using tree_entry()",
> 2013-03-24) does touch the area that may affect the subtree matching
> behaviour.
> 
> Because it is an update to heuristics, and as such, we need to be
> careful when saying it is or is not "broken".  Some heuristics may
> work better with your particular case, and may do worse with other
> cases.
> 
> But from the log message description, it looks like it was meant to
> be a no-op simplification rewrite that should not affect the outcome,
> so it is a bit surprising.

diff --git a/match-trees.c b/match-trees.c
index 4cdeff53e1..730fff4cfb 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -83,34 +83,40 @@ static int score_trees(const struct object_id *hash1, const struct object_id *ha
 	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);
+			continue;
+		} else if (cmp > 0) {
 			/* path2 does not appear in one */
-			score += score_missing(e2.mode, e2.path);
-		else if (oidcmp(e1.oid, e2.oid))
+			score += score_missing(two.entry.mode, two.entry.path);
+			update_tree_entry(&two);
+			continue;
+		} if (oidcmp(one.entry.oid, two.entry.oid)) {
 			/* they are different */
-			score += score_differs(e1.mode, e2.mode, e1.path);
-		else
+			score += score_differs(one.entry.mode, two.entry.mode,
+					       one.entry.path);
+		} else {
 			/* same subtree or blob */
-			score += score_matches(e1.mode, e2.mode, e1.path);
+			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);