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