@matheusbsbs / matheus-bagliano / commits / 2ec4150713

score_trees(): fix iteration over trees with missing entries

In score_trees(), we walk over two sorted trees to find which entries are missing or have different content between the two. So if we have two trees with these entries: one two --- --- a a b c c d we'd expect the loop to: - compare "a" to "a" - compare "b" to "c"; because these are sorted lists, we know that the second tree does not have "b" - compare "c" to "c" - compare "d" to end-of-list; we know that the first tree does not have "d" And prior to d8febde370 (match-trees: simplify score_trees() using tree_entry(), 2013-03-24) that worked. But after that commit, we mistakenly increment the tree pointers for every loop iteration, even when we've processed the entry for only one side. As a result, we end up doing this: - compare "a" to "a" - compare "b" to "c"; we know that we do not have "b", but we still increment both tree pointers; at this point we're out of sync and all further comparisons are wrong - compare "c" to "d" and mistakenly claim that the second tree does not have "c" - exit the loop, mistakenly not realizing that the first tree does not have "d" So contrary to the claim in d8febde370, we really do need to manually use update_tree_entry(), because advancing the tree pointer depends on the entry comparison. That means we must stop using tree_entry() to access each entry, since it auto-advances the pointer. Instead: - we'll use tree_desc.size directly to know if there's anything left to look at (which is what tree_entry() was doing under the hood) - rather than do an extra struct assignment to "e1" and "e2", we can just access the "entry" field of tree_desc directly That makes us a little more intimate with the tree_desc code, but that's not uncommon for its callers. The included test shows off the bug by adding a new entry "bar.t", which sorts early in the tree and de-syncs the comparison for "foo.t", which comes after. Reported-by: George Shammas <georgyo@gmail.com> Helped-by: René Scharfe <l.s.r@web.de> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Aug 2, 2018 at 14:58 UTC 2ec4150713eabc725ba1c43fbe032adb43d5d4cf
2 files changed +54 -17
match-trees.c
+26 -17
@@ -82,34 +82,43 @@ static int score_trees(const struct object_id *hash1, const struct object_id *ha
82 int score = 0;
83
84 for (;;) {
85 - struct name_entry e1, e2;
86 - int got_entry_from_one = tree_entry(&one, &e1);
87 - int got_entry_from_two = tree_entry(&two, &e2);
85 int cmp;
86
90 - if (got_entry_from_one && got_entry_from_two)
91 - cmp = base_name_entries_compare(&e1, &e2);
92 - else if (got_entry_from_one)
87 + if (one.size && two.size)
88 + cmp = base_name_entries_compare(&one.entry, &two.entry);
89 + else if (one.size)
90 /* two lacks this entry */
91 cmp = -1;
95 - else if (got_entry_from_two)
92 + else if (two.size)
93 /* two has more entries */
94 cmp = 1;
95 else
96 break;
97
101 - if (cmp < 0)
98 + if (cmp < 0) {
99 /* path1 does not appear in two */
103 - score += score_missing(e1.mode, e1.path);
104 - else if (cmp > 0)
100 + score += score_missing(one.entry.mode, one.entry.path);
101 + update_tree_entry(&one);
102 + } else if (cmp > 0) {
103 /* path2 does not appear in one */
106 - score += score_missing(e2.mode, e2.path);
107 - else if (oidcmp(e1.oid, e2.oid))
108 - /* they are different */
109 - score += score_differs(e1.mode, e2.mode, e1.path);
110 - else
111 - /* same subtree or blob */
112 - score += score_matches(e1.mode, e2.mode, e1.path);
104 + score += score_missing(two.entry.mode, two.entry.path);
105 + update_tree_entry(&two);
106 + } else {
107 + /* path appears in both */
108 + if (oidcmp(one.entry.oid, two.entry.oid)) {
109 + /* they are different */
110 + score += score_differs(one.entry.mode,
111 + two.entry.mode,
112 + one.entry.path);
113 + } else {
114 + /* same subtree or blob */
115 + score += score_matches(one.entry.mode,
116 + two.entry.mode,
117 + one.entry.path);
118 + }
119 + update_tree_entry(&one);
120 + update_tree_entry(&two);
121 + }
122 }
123 free(one_buf);
124 free(two_buf);
t/t6029-merge-subtree.sh
+28
@@ -29,6 +29,34 @@ test_expect_success 'subtree available and works like recursive' '
29
30 '
31
32 +test_expect_success 'setup branch sub' '
33 + git checkout --orphan sub &&
34 + git rm -rf . &&
35 + test_commit foo
36 +'
37 +
38 +test_expect_success 'setup branch main' '
39 + git checkout -b main master &&
40 + git merge -s ours --no-commit --allow-unrelated-histories sub &&
41 + git read-tree --prefix=dir/ -u sub &&
42 + git commit -m "initial merge of sub into main" &&
43 + test_path_is_file dir/foo.t &&
44 + test_path_is_file hello
45 +'
46 +
47 +test_expect_success 'update branch sub' '
48 + git checkout sub &&
49 + test_commit bar
50 +'
51 +
52 +test_expect_success 'update branch main' '
53 + git checkout main &&
54 + git merge -s subtree sub -m "second merge of sub into main" &&
55 + test_path_is_file dir/bar.t &&
56 + test_path_is_file dir/foo.t &&
57 + test_path_is_file hello
58 +'
59 +
60 test_expect_success 'setup' '
61 mkdir git-gui &&
62 cd git-gui &&