Re: git merge -s subtree seems to be broken.
From: Jeff King <hidden>
Date: 2018-07-31 16:16:03
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2018-08-02 · Re: git merge -s subtree seems to be broken. · Jeff King <hidden>
- 2018-08-02 · Re: git merge -s subtree seems to be broken. · Jeff King <hidden>
- 2018-08-01 · Re: git merge -s subtree seems to be broken. · René Scharfe <hidden>
- 2018-08-01 · Re: git merge -s subtree seems to be broken. · René Scharfe <hidden>
- 2018-07-31 · Re: git merge -s subtree seems to be broken. · Junio C Hamano <hidden>
On Tue, Jul 31, 2018 at 08:53:23AM -0700, Junio C Hamano wrote:
George Shammas [off-list ref] writes:quoted
Bisecting around, this might be the commit that introduced the breakage. https://github.com/git/git/commit/d8febdeInteresting. 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.
Yeah, this is definitely not "well, the heuristic changed a bit". It's just broken. This fixes it, but we should probably add a test.
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);