On Tue, Jul 31, 2018 at 08:53:23AM -0700, Junio C Hamano wrote: > George Shammas <georgyo@xxxxxxxxx> 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. 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);
