{"thread":{"id":"48990","subject":"git merge -s subtree seems to be broken.","startedAt":"2018-07-31T14:09:27Z","lastAt":"2018-08-02T18:58:25Z","messageCount":17,"participants":["George Shammas","Jeff King","Junio C Hamano","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"354035","messageId":"CAF1Ko+FBP5UmETmh071dvn9iv8-N-3YgaP61q-4jQvxFdN0GTA@mail.gmail.com","threadId":"48990","inReplyTo":null,"subject":"git merge -s subtree seems to be broken.","fromName":"George Shammas","fromEmail":"georgyo@gmail.com","sentAt":"2018-07-31T14:09:12Z","receivedAt":"2018-07-31T14:09:27Z","isPatch":false,"sender":{"key":"georgyo@gmail.com","avatar":"https://gravatar.com/avatar/bd053d9417028b3a61467f80a6d9573899f9a505ee3d776bd7dad0ef9352fe0d?d=mp&s=160"},"body":"At work, we recently updated from a massively old version of git (1.7.10)\nto 2.18. There are a few code bases that use subtrees, and they seem to\nhave completely broke when trying to merge in updates.\n\nI have confirmed that it works correctly in 1.7.10. The 2.18 behavior is\nclearly incorrect.\n\ngit init\necho init > test\ngit add test\ngit commit -m init\n\ngit remote add tig https://github.com/jonas/tig.git\ngit fetch tig\ngit merge -s ours --no-commit --allow-unrelated-histories tig-2.3.0\ngit read-tree --prefix=src/ -u tig-2.3.0\ngit commit -m \"Get upstream tig-2.3.0\"\n# Notice how the history are merged, and that the source from the upstream\nrepo is in src\n\necho update > test\ngit commit -a -m \"test\"\n\ngit merge -s subtree tig-2.4.0\n# Boom, in 2.18 instead of merging into the subtree, it just deletes\neverything in the repository, which is clearly the wrong behavior.\n"},{"id":"354040","messageId":"CAF1Ko+FNfjWMteccfKDBjPEW76rGBLQkGb1icUHmzEZ0fKQJBA@mail.gmail.com","threadId":"48990","inReplyTo":"CAF1Ko+FBP5UmETmh071dvn9iv8-N-3YgaP61q-4jQvxFdN0GTA@mail.gmail.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"George Shammas","fromEmail":"georgyo@gmail.com","sentAt":"2018-07-31T15:03:17Z","receivedAt":"2018-07-31T15:03:32Z","isPatch":false,"sender":{"key":"georgyo@gmail.com","avatar":"https://gravatar.com/avatar/bd053d9417028b3a61467f80a6d9573899f9a505ee3d776bd7dad0ef9352fe0d?d=mp&s=160"},"body":"Bisecting around, this might be the commit that introduced the breakage.\n\nhttps://github.com/git/git/commit/d8febde\n\nI really hope that it hasn't been broken for 5 years and I am just doing\nsomething wrong.\n\nOn Tue, Jul 31, 2018 at 10:09 AM George Shammas wrote:\n\n> At work, we recently updated from a massively old version of git (1.7.10)\n> to 2.18. There are a few code bases that use subtrees, and they seem to\n> have completely broke when trying to merge in updates.\n>\n> I have confirmed that it works correctly in 1.7.10. The 2.18 behavior is\n> clearly incorrect.\n>\n> git init\n> echo init > test\n> git add test\n> git commit -m init\n>\n> git remote add tig https://github.com/jonas/tig.git\n> git fetch tig\n> git merge -s ours --no-commit --allow-unrelated-histories tig-2.3.0\n> git read-tree --prefix=src/ -u tig-2.3.0\n> git commit -m \"Get upstream tig-2.3.0\"\n> # Notice how the history are merged, and that the source from the upstream\n> repo is in src\n>\n> echo update > test\n> git commit -a -m \"test\"\n>\n> git merge -s subtree tig-2.4.0\n> # Boom, in 2.18 instead of merging into the subtree, it just deletes\n> everything in the repository, which is clearly the wrong behavior.\n>\n"},{"id":"354046","messageId":"20180731155027.GA16910@sigill.intra.peff.net","threadId":"48990","inReplyTo":"CAF1Ko+FNfjWMteccfKDBjPEW76rGBLQkGb1icUHmzEZ0fKQJBA@mail.gmail.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-31T15:50:28Z","receivedAt":"2018-07-31T15:50:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 11:03:17AM -0400, George Shammas wrote:\n\n> Bisecting around, this might be the commit that introduced the breakage.\n> \n> https://github.com/git/git/commit/d8febde\n> \n> I really hope that it hasn't been broken for 5 years and I am just doing\n> something wrong.\n\nUnfortunately, I think it has been broken for five years.\n\nThe problem introduced in that commit is that each iteration through the\nloop advances the tree pointers. But when we're walking two lists and\nsee that one omits an entry the other has, we have to advance _one_ list\nand keep the other where it is. So if we instrument the score_*\nfunctions to see which ones trigger, your reproduction gives this with\nthe original code:\n\n warning: scoring trees:\n 8e12d9b6bc57fe6308315914628dd4fd7665ca59\n aed1d7c5809e53d49b52c43a6103827046d60286\n warning: score_matches: .bookignore\n warning: score_matches: .gitignore\n warning: score_matches: .mailmap\n warning: score_differs: .travis.yml\n warning: score_matches: COPYING\n warning: score_differs: INSTALL.adoc\n warning: score_differs: Makefile\n warning: score_differs: NEWS.adoc\n warning: score_differs: README.adoc\n warning: score_missing: appveyor.yml\n warning: score_matches: autogen.sh\n warning: score_matches: book.json\n [...]\n\nand the new one does:\n\n warning: scoring trees:\n 8e12d9b6bc57fe6308315914628dd4fd7665ca59\n aed1d7c5809e53d49b52c43a6103827046d60286\n warning: score_matches: .bookignore\n warning: score_matches: .gitignore\n warning: score_matches: .mailmap\n warning: score_differs: .travis.yml\n warning: score_matches: COPYING\n warning: score_differs: INSTALL.adoc\n warning: score_differs: Makefile\n warning: score_differs: NEWS.adoc\n warning: score_differs: README.adoc\n warning: score_missing: appveyor.yml\n warning: score_missing: autogen.sh\n warning: score_missing: book.json\n\nWe're fine at first, but as soon as one tree has appveyor.yml and the\nother doesn't, we get out of sync. We compare \"autogen\" and \"appveyor\",\nand realize that they do not match. But then we need to increment\npointer for the tree with \"appveyor\" only, and leave the other in place,\nat which point we'd realize that they both have \"autogen\". Instead, we\nincrement both, and after that we compare \"autogen.sh\" to \"book.json\",\nand so on.\n\nSo the assertion in that commit message that \"the calls to\nupdate_tree_entry() are not needed any more\" is just wrong. We have\ndecide whether to call it based on the \"cmp\" value.\n\nI quoted your original reproduction below for the benefit of René\n(cc'd).\n\n-Peff\n\n> On Tue, Jul 31, 2018 at 10:09 AM George Shammas wrote:\n> \n> > At work, we recently updated from a massively old version of git (1.7.10)\n> > to 2.18. There are a few code bases that use subtrees, and they seem to\n> > have completely broke when trying to merge in updates.\n> >\n> > I have confirmed that it works correctly in 1.7.10. The 2.18 behavior is\n> > clearly incorrect.\n> >\n> > git init\n> > echo init > test\n> > git add test\n> > git commit -m init\n> >\n> > git remote add tig https://github.com/jonas/tig.git\n> > git fetch tig\n> > git merge -s ours --no-commit --allow-unrelated-histories tig-2.3.0\n> > git read-tree --prefix=src/ -u tig-2.3.0\n> > git commit -m \"Get upstream tig-2.3.0\"\n> > # Notice how the history are merged, and that the source from the upstream\n> > repo is in src\n> >\n> > echo update > test\n> > git commit -a -m \"test\"\n> >\n> > git merge -s subtree tig-2.4.0\n> > # Boom, in 2.18 instead of merging into the subtree, it just deletes\n> > everything in the repository, which is clearly the wrong behavior.\n> >\n"},{"id":"354047","messageId":"xmqqtvofcsgc.fsf@gitster-ct.c.googlers.com","threadId":"48990","inReplyTo":"CAF1Ko+FNfjWMteccfKDBjPEW76rGBLQkGb1icUHmzEZ0fKQJBA@mail.gmail.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T15:53:23Z","receivedAt":"2018-07-31T15:53:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"George Shammas writes:\n\n> Bisecting around, this might be the commit that introduced the breakage.\n>\n> https://github.com/git/git/commit/d8febde\n\nInteresting. I've never used the \"-s subtree\" strategy without\n\"-Xsubtree=...\" to explicitly tell where the thing should go for a\nlong time, so I am not surprised if I did not notice if an update to\nthe heuristics made long time ago had affected tree matching.\n\nd8febde3 (\"match-trees: simplify score_trees() using tree_entry()\",\n2013-03-24) does touch the area that may affect the subtree matching\nbehaviour.\n\nBecause it is an update to heuristics, and as such, we need to be\ncareful when saying it is or is not \"broken\". Some heuristics may\nwork better with your particular case, and may do worse with other\ncases.\n\nBut from the log message description, it looks like it was meant to\nbe a no-op simplification rewrite that should not affect the outcome,\nso it is a bit surprising.\n"},{"id":"354048","messageId":"CAF1Ko+HAusyCOaTWT-cT0DW0MoJdDv3RdoW+U25QRqaacCAh5w@mail.gmail.com","threadId":"48990","inReplyTo":"xmqqtvofcsgc.fsf@gitster-ct.c.googlers.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"George Shammas","fromEmail":"georgyo@gmail.com","sentAt":"2018-07-31T15:56:11Z","receivedAt":"2018-07-31T15:56:26Z","isPatch":false,"sender":{"key":"georgyo@gmail.com","avatar":"https://gravatar.com/avatar/bd053d9417028b3a61467f80a6d9573899f9a505ee3d776bd7dad0ef9352fe0d?d=mp&s=160"},"body":"While debugging this, I did try -X subtree=src/ however the effect was the\nsame.\n\nOn Tue, Jul 31, 2018 at 11:53 AM Junio C Hamano wrote:\n\n> George Shammas writes:\n>\n> > Bisecting around, this might be the commit that introduced the breakage.\n> >\n> > https://github.com/git/git/commit/d8febde\n>\n> Interesting. I've never used the \"-s subtree\" strategy without\n> \"-Xsubtree=...\" to explicitly tell where the thing should go for a\n> long time, so I am not surprised if I did not notice if an update to\n> the heuristics made long time ago had affected tree matching.\n>\n> d8febde3 (\"match-trees: simplify score_trees() using tree_entry()\",\n> 2013-03-24) does touch the area that may affect the subtree matching\n> behaviour.\n>\n> Because it is an update to heuristics, and as such, we need to be\n> careful when saying it is or is not \"broken\". Some heuristics may\n> work better with your particular case, and may do worse with other\n> cases.\n>\n> But from the log message description, it looks like it was meant to\n> be a no-op simplification rewrite that should not affect the outcome,\n> so it is a bit surprising.\n>\n"},{"id":"354052","messageId":"xmqqlg9rcrqs.fsf@gitster-ct.c.googlers.com","threadId":"48990","inReplyTo":"20180731155027.GA16910@sigill.intra.peff.net","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T16:08:43Z","receivedAt":"2018-07-31T16:08:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The problem introduced in that commit is that each iteration through the\n> loop advances the tree pointers.\n\nAh, indeed. \n\nThe original used tree_entry_extract() and update_tree_entry()\nseparately, but the update does tree_entry() on both sides.\n\n> So the assertion in that commit message that \"the calls to\n> update_tree_entry() are not needed any more\" is just wrong. We have\n> decide whether to call it based on the \"cmp\" value.\n\nYup.\n"},{"id":"354055","messageId":"20180731161559.GB16910@sigill.intra.peff.net","threadId":"48990","inReplyTo":"xmqqtvofcsgc.fsf@gitster-ct.c.googlers.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-31T16:15:59Z","receivedAt":"2018-07-31T16:16:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 08:53:23AM -0700, Junio C Hamano wrote:\n\n> George Shammas writes:\n> \n> > Bisecting around, this might be the commit that introduced the breakage.\n> >\n> > https://github.com/git/git/commit/d8febde\n> \n> Interesting. I've never used the \"-s subtree\" strategy without\n> \"-Xsubtree=...\" to explicitly tell where the thing should go for a\n> long time, so I am not surprised if I did not notice if an update to\n> the heuristics made long time ago had affected tree matching.\n> \n> d8febde3 (\"match-trees: simplify score_trees() using tree_entry()\",\n> 2013-03-24) does touch the area that may affect the subtree matching\n> behaviour.\n> \n> Because it is an update to heuristics, and as such, we need to be\n> careful when saying it is or is not \"broken\". Some heuristics may\n> work better with your particular case, and may do worse with other\n> cases.\n> \n> But from the log message description, it looks like it was meant to\n> be a no-op simplification rewrite that should not affect the outcome,\n> so it is a bit surprising.\n\nYeah, this is definitely not \"well, the heuristic changed a bit\". It's\njust broken. This fixes it, but we should probably add a test.\n\ndiff --git a/match-trees.c b/match-trees.c\nindex 4cdeff53e1..730fff4cfb 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -83,34 +83,40 @@ static int score_trees(const struct object_id *hash1, const struct object_id *ha\n \tint score = 0;\n \n \tfor (;;) {\n-\t\tstruct name_entry e1, e2;\n-\t\tint got_entry_from_one = tree_entry(&one, &e1);\n-\t\tint got_entry_from_two = tree_entry(&two, &e2);\n \t\tint cmp;\n \n-\t\tif (got_entry_from_one && got_entry_from_two)\n-\t\t\tcmp = base_name_entries_compare(&e1, &e2);\n-\t\telse if (got_entry_from_one)\n+\t\tif (one.size && two.size)\n+\t\t\tcmp = base_name_entries_compare(&one.entry, &two.entry);\n+\t\telse if (one.size)\n \t\t\t/* two lacks this entry */\n \t\t\tcmp = -1;\n-\t\telse if (got_entry_from_two)\n+\t\telse if (two.size)\n \t\t\t/* two has more entries */\n \t\t\tcmp = 1;\n \t\telse\n \t\t\tbreak;\n \n-\t\tif (cmp < 0)\n+\t\tif (cmp < 0) {\n \t\t\t/* path1 does not appear in two */\n-\t\t\tscore += score_missing(e1.mode, e1.path);\n-\t\telse if (cmp > 0)\n+\t\t\tscore += score_missing(one.entry.mode, one.entry.path);\n+\t\t\tupdate_tree_entry(&one);\n+\t\t\tcontinue;\n+\t\t} else if (cmp > 0) {\n \t\t\t/* path2 does not appear in one */\n-\t\t\tscore += score_missing(e2.mode, e2.path);\n-\t\telse if (oidcmp(e1.oid, e2.oid))\n+\t\t\tscore += score_missing(two.entry.mode, two.entry.path);\n+\t\t\tupdate_tree_entry(&two);\n+\t\t\tcontinue;\n+\t\t} if (oidcmp(one.entry.oid, two.entry.oid)) {\n \t\t\t/* they are different */\n-\t\t\tscore += score_differs(e1.mode, e2.mode, e1.path);\n-\t\telse\n+\t\t\tscore += score_differs(one.entry.mode, two.entry.mode,\n+\t\t\t\t\t one.entry.path);\n+\t\t} else {\n \t\t\t/* same subtree or blob */\n-\t\t\tscore += score_matches(e1.mode, e2.mode, e1.path);\n+\t\t\tscore += score_matches(one.entry.mode, two.entry.mode,\n+\t\t\t\t\t one.entry.path);\n+\t\t}\n+\t\tupdate_tree_entry(&one);\n+\t\tupdate_tree_entry(&two);\n \t}\n \tfree(one_buf);\n \tfree(two_buf);\n"},{"id":"354064","messageId":"xmqqh8kfcokk.fsf@gitster-ct.c.googlers.com","threadId":"48990","inReplyTo":"20180731161559.GB16910@sigill.intra.peff.net","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T17:17:15Z","receivedAt":"2018-07-31T17:17:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +...\n> +\t\t} else if (cmp > 0) {\n> \t\t\t/* path2 does not appear in one */\n> +\t\t\tscore += score_missing(two.entry.mode, two.entry.path);\n> +\t\t\tupdate_tree_entry(&two);\n> +\t\t\tcontinue;\n> +\t\t} if (oidcmp(one.entry.oid, two.entry.oid)) {\n\nAs the earlier ones do the \"continue at the end of the block\", this\ndoes not affect the correctness, but I think you either meant \"else if\"\nor a fresh \"if/else\" that is disconnected from the previous if/else if/...\nchain.\n\n\n\n> \t\t\t/* they are different */\n> ...\n> +\t\t\tscore += score_differs(one.entry.mode, two.entry.mode,\n> +\t\t\t\t\t one.entry.path);\n> +\t\t} else {\n\n"},{"id":"354066","messageId":"20180731172304.GA16977@sigill.intra.peff.net","threadId":"48990","inReplyTo":"xmqqh8kfcokk.fsf@gitster-ct.c.googlers.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-31T17:23:04Z","receivedAt":"2018-07-31T17:23:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 10:17:15AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > +...\n> > +\t\t} else if (cmp > 0) {\n> > \t\t\t/* path2 does not appear in one */\n> > +\t\t\tscore += score_missing(two.entry.mode, two.entry.path);\n> > +\t\t\tupdate_tree_entry(&two);\n> > +\t\t\tcontinue;\n> > +\t\t} if (oidcmp(one.entry.oid, two.entry.oid)) {\n> \n> As the earlier ones do the \"continue at the end of the block\", this\n> does not affect the correctness, but I think you either meant \"else if\"\n> or a fresh \"if/else\" that is disconnected from the previous if/else if/...\n> chain.\n\nYes, thanks. I actually started to write it without the \"continue\" at\nall, and a big \"else\" that checked the \"we have both\" case. But I backed\nthat out (in favor of a smaller diff), and forgot to add back in the\n\"else if\".\n\n-Peff\n"},{"id":"354102","messageId":"20180731190459.GA3372@sigill.intra.peff.net","threadId":"48990","inReplyTo":"20180731172304.GA16977@sigill.intra.peff.net","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-31T19:04:59Z","receivedAt":"2018-07-31T19:05:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:\n\n> On Tue, Jul 31, 2018 at 10:17:15AM -0700, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > +...\n> > > +\t\t} else if (cmp > 0) {\n> > > \t\t\t/* path2 does not appear in one */\n> > > +\t\t\tscore += score_missing(two.entry.mode, two.entry.path);\n> > > +\t\t\tupdate_tree_entry(&two);\n> > > +\t\t\tcontinue;\n> > > +\t\t} if (oidcmp(one.entry.oid, two.entry.oid)) {\n> > \n> > As the earlier ones do the \"continue at the end of the block\", this\n> > does not affect the correctness, but I think you either meant \"else if\"\n> > or a fresh \"if/else\" that is disconnected from the previous if/else if/...\n> > chain.\n> \n> Yes, thanks. I actually started to write it without the \"continue\" at\n> all, and a big \"else\" that checked the \"we have both\" case. But I backed\n> that out (in favor of a smaller diff), and forgot to add back in the\n> \"else if\".\n\nSo here it is fixed, and with a commit message. I'm not happy to omit a\nregression test, but I actually couldn't come up with a minimal one that\ntickled the problem, because we're playing around with heuristics. So I\ncompensated by probably over-explaining in the commit message. But\nclearly this is not a well-tested code path given the length of time\nbetween introducing and detecting the bug.\n\n-- >8 --\nSubject: [PATCH] score_trees(): fix iteration over trees with missing entries\n\nIn score_trees(), we walk over two sorted trees to find\nwhich entries are missing or have different content between\nthe two. So if we have two trees with these entries:\n\n one two\n --- ---\n a a\n b c\n c d\n\nwe'd expect the loop to:\n\n - compare \"a\" to \"a\"\n\n - compare \"b\" to \"c\"; because these are sorted lists, we\n know that the second tree does not have \"b\"\n\n - compare \"c\" to \"c\"\n\n - compare \"d\" to end-of-list; we know that the first tree\n does not have \"d\"\n\nAnd prior to d8febde370 (match-trees: simplify score_trees()\nusing tree_entry(), 2013-03-24) that worked. But after that\ncommit, we mistakenly increment the tree pointers for every\nloop iteration, even when we've processed the entry for only\none side. As a result, we end up doing this:\n\n - compare \"a\" to \"a\"\n\n - compare \"b\" to \"c\"; we know that we do not have \"b\", but\n we still increment both tree pointers; at this point\n we're out of sync and all further comparisons are wrong\n\n - compare \"c\" to \"d\" and mistakenly claim that the second\n tree does not have \"c\"\n\n - exit the loop, mistakenly not realizing that the first\n tree does not have \"d\"\n\nSo contrary to the claim in d8febde370, we really do need to\nmanually use update_tree_entry(), because advancing the tree\npointer depends on the entry comparison.\n\nThat means we must stop using tree_entry() to access each\nentry, since it auto-advances the pointer. Instead:\n\n - we'll use tree_desc.size directly to know if there's\n anything left to look at (which is what tree_entry() was\n doing under the hood)\n\n - rather than do an extra struct assignment to \"e1\" and\n \"e2\", we can just access the \"entry\" field of tree_desc\n directly\n\nThat makes us a little more intimate with the tree_desc\ncode, but that's not uncommon for its callers.\n\nThere's no regression test here, as it's a little tricky to\ntrigger this with a minimal example. The user-visible effect\nis that the heuristics fail to correlate two trees that\nshould be. But in a minimal example, there aren't a lot of\nother trees to match, so we often end up doing the right\nthing anyway.\n\nA real-world example (from the original bug report) is:\n\n-- >8 --\ngit init repo\ncd repo\n\necho init >file\ngit add file\ngit commit -m init\n\ngit remote add tig https://github.com/jonas/tig.git\ngit fetch tig\ngit merge -s ours --no-commit --allow-unrelated-histories tig-2.3.0\ngit read-tree --prefix=src/ -u tig-2.3.0\ngit commit -m 'get upstream tig-2.3.0'\n\necho update >file\ngit commit -a -m update\n\ngit merge -s subtree tig-2.4.0\n-- 8< --\n\nBefore this patch, we fail to realize that the tig-2.4.0\ncontent should go into the \"src\" directory.\n\nSigned-off-by: Jeff King \n---\n match-trees.c | 43 ++++++++++++++++++++++++++-----------------\n 1 file changed, 26 insertions(+), 17 deletions(-)\n\ndiff --git a/match-trees.c b/match-trees.c\nindex 4cdeff53e1..37653308d3 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -83,34 +83,43 @@ static int score_trees(const struct object_id *hash1, const struct object_id *ha\n \tint score = 0;\n \n \tfor (;;) {\n-\t\tstruct name_entry e1, e2;\n-\t\tint got_entry_from_one = tree_entry(&one, &e1);\n-\t\tint got_entry_from_two = tree_entry(&two, &e2);\n \t\tint cmp;\n \n-\t\tif (got_entry_from_one && got_entry_from_two)\n-\t\t\tcmp = base_name_entries_compare(&e1, &e2);\n-\t\telse if (got_entry_from_one)\n+\t\tif (one.size && two.size)\n+\t\t\tcmp = base_name_entries_compare(&one.entry, &two.entry);\n+\t\telse if (one.size)\n \t\t\t/* two lacks this entry */\n \t\t\tcmp = -1;\n-\t\telse if (got_entry_from_two)\n+\t\telse if (two.size)\n \t\t\t/* two has more entries */\n \t\t\tcmp = 1;\n \t\telse\n \t\t\tbreak;\n \n-\t\tif (cmp < 0)\n+\t\tif (cmp < 0) {\n \t\t\t/* path1 does not appear in two */\n-\t\t\tscore += score_missing(e1.mode, e1.path);\n-\t\telse if (cmp > 0)\n+\t\t\tscore += score_missing(one.entry.mode, one.entry.path);\n+\t\t\tupdate_tree_entry(&one);\n+\t\t} else if (cmp > 0) {\n \t\t\t/* path2 does not appear in one */\n-\t\t\tscore += score_missing(e2.mode, e2.path);\n-\t\telse if (oidcmp(e1.oid, e2.oid))\n-\t\t\t/* they are different */\n-\t\t\tscore += score_differs(e1.mode, e2.mode, e1.path);\n-\t\telse\n-\t\t\t/* same subtree or blob */\n-\t\t\tscore += score_matches(e1.mode, e2.mode, e1.path);\n+\t\t\tscore += score_missing(two.entry.mode, two.entry.path);\n+\t\t\tupdate_tree_entry(&two);\n+\t\t} else {\n+\t\t\t/* path appears in both */\n+\t\t\tif (oidcmp(one.entry.oid, two.entry.oid)) {\n+\t\t\t\t/* they are different */\n+\t\t\t\tscore += score_differs(one.entry.mode,\n+\t\t\t\t\t\t two.entry.mode,\n+\t\t\t\t\t\t one.entry.path);\n+\t\t\t} else {\n+\t\t\t\t/* same subtree or blob */\n+\t\t\t\tscore += score_matches(one.entry.mode,\n+\t\t\t\t\t\t two.entry.mode,\n+\t\t\t\t\t\t one.entry.path);\n+\t\t\t}\n+\t\t\tupdate_tree_entry(&one);\n+\t\t\tupdate_tree_entry(&two);\n+\t\t}\n \t}\n \tfree(one_buf);\n \tfree(two_buf);\n-- \n2.18.0.796.g4bfd63b683\n\n"},{"id":"354111","messageId":"CAF1Ko+FHsvmqzwVHh+fEnk=UGUftNW8VkFwaWTSKu3xYprb+wg@mail.gmail.com","threadId":"48990","inReplyTo":"20180731190459.GA3372@sigill.intra.peff.net","subject":"Re: git merge -s subtree seems to be broken.","fromName":"George Shammas","fromEmail":"georgyo@gmail.com","sentAt":"2018-07-31T19:52:26Z","receivedAt":"2018-07-31T19:52:40Z","isPatch":false,"sender":{"key":"georgyo@gmail.com","avatar":"https://gravatar.com/avatar/bd053d9417028b3a61467f80a6d9573899f9a505ee3d776bd7dad0ef9352fe0d?d=mp&s=160"},"body":"This is the fastest I ever seen an open source project respond to an issue\nI reported. Thanks for being awesome!\n\nOn Tue, Jul 31, 2018 at 3:05 PM Jeff King wrote:\n\n> On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:\n>\n> > On Tue, Jul 31, 2018 at 10:17:15AM -0700, Junio C Hamano wrote:\n> >\n> > > Jeff King writes:\n> > >\n> > > > +...\n> > > > + } else if (cmp > 0) {\n> > > > /* path2 does not appear in one */\n> > > > + score += score_missing(two.entry.mode,\n> two.entry.path);\n> > > > + update_tree_entry(&two);\n> > > > + continue;\n> > > > + } if (oidcmp(one.entry.oid, two.entry.oid)) {\n> > >\n> > > As the earlier ones do the \"continue at the end of the block\", this\n> > > does not affect the correctness, but I think you either meant \"else if\"\n> > > or a fresh \"if/else\" that is disconnected from the previous if/else\n> if/...\n> > > chain.\n> >\n> > Yes, thanks. I actually started to write it without the \"continue\" at\n> > all, and a big \"else\" that checked the \"we have both\" case. But I backed\n> > that out (in favor of a smaller diff), and forgot to add back in the\n> > \"else if\".\n>\n> So here it is fixed, and with a commit message. I'm not happy to omit a\n> regression test, but I actually couldn't come up with a minimal one that\n> tickled the problem, because we're playing around with heuristics. So I\n> compensated by probably over-explaining in the commit message. But\n> clearly this is not a well-tested code path given the length of time\n> between introducing and detecting the bug.\n>\n> -- >8 --\n> Subject: [PATCH] score_trees(): fix iteration over trees with missing\n> entries\n>\n> In score_trees(), we walk over two sorted trees to find\n> which entries are missing or have different content between\n> the two. So if we have two trees with these entries:\n>\n> one two\n> --- ---\n> a a\n> b c\n> c d\n>\n> we'd expect the loop to:\n>\n> - compare \"a\" to \"a\"\n>\n> - compare \"b\" to \"c\"; because these are sorted lists, we\n> know that the second tree does not have \"b\"\n>\n> - compare \"c\" to \"c\"\n>\n> - compare \"d\" to end-of-list; we know that the first tree\n> does not have \"d\"\n>\n> And prior to d8febde370 (match-trees: simplify score_trees()\n> using tree_entry(), 2013-03-24) that worked. But after that\n> commit, we mistakenly increment the tree pointers for every\n> loop iteration, even when we've processed the entry for only\n> one side. As a result, we end up doing this:\n>\n> - compare \"a\" to \"a\"\n>\n> - compare \"b\" to \"c\"; we know that we do not have \"b\", but\n> we still increment both tree pointers; at this point\n> we're out of sync and all further comparisons are wrong\n>\n> - compare \"c\" to \"d\" and mistakenly claim that the second\n> tree does not have \"c\"\n>\n> - exit the loop, mistakenly not realizing that the first\n> tree does not have \"d\"\n>\n> So contrary to the claim in d8febde370, we really do need to\n> manually use update_tree_entry(), because advancing the tree\n> pointer depends on the entry comparison.\n>\n> That means we must stop using tree_entry() to access each\n> entry, since it auto-advances the pointer. Instead:\n>\n> - we'll use tree_desc.size directly to know if there's\n> anything left to look at (which is what tree_entry() was\n> doing under the hood)\n>\n> - rather than do an extra struct assignment to \"e1\" and\n> \"e2\", we can just access the \"entry\" field of tree_desc\n> directly\n>\n> That makes us a little more intimate with the tree_desc\n> code, but that's not uncommon for its callers.\n>\n> There's no regression test here, as it's a little tricky to\n> trigger this with a minimal example. The user-visible effect\n> is that the heuristics fail to correlate two trees that\n> should be. But in a minimal example, there aren't a lot of\n> other trees to match, so we often end up doing the right\n> thing anyway.\n>\n> A real-world example (from the original bug report) is:\n>\n> -- >8 --\n> git init repo\n> cd repo\n>\n> echo init >file\n> git add file\n> git commit -m init\n>\n> git remote add tig https://github.com/jonas/tig.git\n> git fetch tig\n> git merge -s ours --no-commit --allow-unrelated-histories tig-2.3.0\n> git read-tree --prefix=src/ -u tig-2.3.0\n> git commit -m 'get upstream tig-2.3.0'\n>\n> echo update >file\n> git commit -a -m update\n>\n> git merge -s subtree tig-2.4.0\n> -- 8< --\n>\n> Before this patch, we fail to realize that the tig-2.4.0\n> content should go into the \"src\" directory.\n>\n> Signed-off-by: Jeff King \n> ---\n> match-trees.c | 43 ++++++++++++++++++++++++++-----------------\n> 1 file changed, 26 insertions(+), 17 deletions(-)\n>\n> diff --git a/match-trees.c b/match-trees.c\n> index 4cdeff53e1..37653308d3 100644\n> --- a/match-trees.c\n> +++ b/match-trees.c\n> @@ -83,34 +83,43 @@ static int score_trees(const struct object_id *hash1,\n> const struct object_id *ha\n> int score = 0;\n>\n> for (;;) {\n> - struct name_entry e1, e2;\n> - int got_entry_from_one = tree_entry(&one, &e1);\n> - int got_entry_from_two = tree_entry(&two, &e2);\n> int cmp;\n>\n> - if (got_entry_from_one && got_entry_from_two)\n> - cmp = base_name_entries_compare(&e1, &e2);\n> - else if (got_entry_from_one)\n> + if (one.size && two.size)\n> + cmp = base_name_entries_compare(&one.entry,\n> &two.entry);\n> + else if (one.size)\n> /* two lacks this entry */\n> cmp = -1;\n> - else if (got_entry_from_two)\n> + else if (two.size)\n> /* two has more entries */\n> cmp = 1;\n> else\n> break;\n>\n> - if (cmp < 0)\n> + if (cmp < 0) {\n> /* path1 does not appear in two */\n> - score += score_missing(e1.mode, e1.path);\n> - else if (cmp > 0)\n> + score += score_missing(one.entry.mode,\n> one.entry.path);\n> + update_tree_entry(&one);\n> + } else if (cmp > 0) {\n> /* path2 does not appear in one */\n> - score += score_missing(e2.mode, e2.path);\n> - else if (oidcmp(e1.oid, e2.oid))\n> - /* they are different */\n> - score += score_differs(e1.mode, e2.mode, e1.path);\n> - else\n> - /* same subtree or blob */\n> - score += score_matches(e1.mode, e2.mode, e1.path);\n> + score += score_missing(two.entry.mode,\n> two.entry.path);\n> + update_tree_entry(&two);\n> + } else {\n> + /* path appears in both */\n> + if (oidcmp(one.entry.oid, two.entry.oid)) {\n> + /* they are different */\n> + score += score_differs(one.entry.mode,\n> + two.entry.mode,\n> + one.entry.path);\n> + } else {\n> + /* same subtree or blob */\n> + score += score_matches(one.entry.mode,\n> + two.entry.mode,\n> + one.entry.path);\n> + }\n> + update_tree_entry(&one);\n> + update_tree_entry(&two);\n> + }\n> }\n> free(one_buf);\n> free(two_buf);\n> --\n> 2.18.0.796.g4bfd63b683\n>\n>\n"},{"id":"354121","messageId":"20180731204012.GB9442@sigill.intra.peff.net","threadId":"48990","inReplyTo":"CAF1Ko+FHsvmqzwVHh+fEnk=UGUftNW8VkFwaWTSKu3xYprb+wg@mail.gmail.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-31T20:40:13Z","receivedAt":"2018-07-31T20:40:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 03:52:26PM -0400, George Shammas wrote:\n\n> This is the fastest I ever seen an open source project respond to an issue\n> I reported. Thanks for being awesome!\n\nYou're welcome. My speed is an inverse to how embarrassingly long we\ncarried the bug for. ;)\n\n> > Signed-off-by: Jeff King \n> > ---\n> > match-trees.c | 43 ++++++++++++++++++++++++++-----------------\n> > 1 file changed, 26 insertions(+), 17 deletions(-)\n\nSorry, I meant to actually add a:\n\n Reported-by: George Shammas \n\nhere.\n\n-Peff\n"},{"id":"354125","messageId":"xmqqeffj9ku3.fsf@gitster-ct.c.googlers.com","threadId":"48990","inReplyTo":"20180731190459.GA3372@sigill.intra.peff.net","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T21:06:12Z","receivedAt":"2018-07-31T21:06:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:\n> ...\n> So here it is fixed, and with a commit message. I'm not happy to omit a\n> regression test, but I actually couldn't come up with a minimal one that\n> tickled the problem, because we're playing around with heuristics. So I\n> compensated by probably over-explaining in the commit message. But\n\nHave you tried to apply the message yourself? I'll fix it up but\nthe hint to answer that question is in two extra pair of scissors.\n\n> clearly this is not a well-tested code path given the length of time\n> between introducing and detecting the bug.\n\nThanks for writing it up. The patch itself still looks correct, too.\n\n"},{"id":"354141","messageId":"bef616af-6b1a-d96a-8c24-841137ecf506@web.de","threadId":"48990","inReplyTo":"20180731155027.GA16910@sigill.intra.peff.net","subject":"Re: git merge -s subtree seems to be broken.","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-08-01T00:58:45Z","receivedAt":"2018-08-01T00:58:56Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.07.2018 um 17:50 schrieb Jeff King:\n> On Tue, Jul 31, 2018 at 11:03:17AM -0400, George Shammas wrote:\n> \n>> Bisecting around, this might be the commit that introduced the breakage.\n>>\n>> https://github.com/git/git/commit/d8febde\n>>\n>> I really hope that it hasn't been broken for 5 years and I am just doing\n>> something wrong.\n> \n> Unfortunately, I think it has been broken for five years.\n\nI don't remember this change at all. :-( Sorry for the trouble, everyone.\nI should feel ashamed, but I'm only staring in bewilderment.\n\nRené\n"},{"id":"354142","messageId":"d60fc243-7271-bc49-b687-ade2b6e315ea@web.de","threadId":"48990","inReplyTo":"xmqqeffj9ku3.fsf@gitster-ct.c.googlers.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-08-01T00:58:50Z","receivedAt":"2018-08-01T00:59:01Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 31.07.2018 um 23:06 schrieb Junio C Hamano:\n> Jeff King writes:\n> \n>> On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:\n>> ...\n>> So here it is fixed, and with a commit message. I'm not happy to omit a\n>> regression test, but I actually couldn't come up with a minimal one that\n>> tickled the problem, because we're playing around with heuristics.\nHow about something like this? (squashable)\n\n---\n t/t6029-merge-subtree.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t6029-merge-subtree.sh b/t/t6029-merge-subtree.sh\nindex 3e692454a7..474a850de6 100755\n--- a/t/t6029-merge-subtree.sh\n+++ b/t/t6029-merge-subtree.sh\n@@ -29,6 +29,34 @@ test_expect_success 'subtree available and works like recursive' '\n \n '\n \n+test_expect_success 'setup branch sub' '\n+\tgit checkout --orphan sub &&\n+\tgit rm -rf . &&\n+\ttest_commit foo\n+'\n+\n+test_expect_success 'setup branch main' '\n+\tgit checkout -b main master &&\n+\tgit merge -s ours --no-commit --allow-unrelated-histories sub &&\n+\tgit read-tree --prefix=dir/ -u sub &&\n+\tgit commit -m \"initial merge of sub into main\" &&\n+\ttest_path_is_file dir/foo.t &&\n+\ttest_path_is_file hello\n+'\n+\n+test_expect_success 'update branch sub' '\n+\tgit checkout sub &&\n+\ttest_commit bar\n+'\n+\n+test_expect_success 'update branch main' '\n+\tgit checkout main &&\n+\tgit merge -s subtree sub -m \"second merge of sub into main\" &&\n+\ttest_path_is_file dir/bar.t &&\n+\ttest_path_is_file dir/foo.t &&\n+\ttest_path_is_file hello\n+'\n+\n test_expect_success 'setup' '\n \tmkdir git-gui &&\n \tcd git-gui &&\n-- \n2.18.0\n"},{"id":"354294","messageId":"20180802184515.GC23690@sigill.intra.peff.net","threadId":"48990","inReplyTo":"xmqqeffj9ku3.fsf@gitster-ct.c.googlers.com","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T18:45:15Z","receivedAt":"2018-08-02T18:45:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 02:06:12PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:\n> > ...\n> > So here it is fixed, and with a commit message. I'm not happy to omit a\n> > regression test, but I actually couldn't come up with a minimal one that\n> > tickled the problem, because we're playing around with heuristics. So I\n> > compensated by probably over-explaining in the commit message. But\n> \n> Have you tried to apply the message yourself? I'll fix it up but\n> the hint to answer that question is in two extra pair of scissors.\n\nHeh, thank you for noticing. I actually wondered about that while\nwriting it and meant to test, but then got distracted.\n\nI wonder \"am --scissors\" should actually look for the _first_ scissors.\nI guess that has the opposite problem, which is that we might include\ntoo much cruft in an email that uses scissors. Perhaps \"too much\" is a\nbetter failure mode than \"too little\", though.\n\n-Peff\n"},{"id":"354298","messageId":"20180802185821.GD23690@sigill.intra.peff.net","threadId":"48990","inReplyTo":"d60fc243-7271-bc49-b687-ade2b6e315ea@web.de","subject":"Re: git merge -s subtree seems to be broken.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T18:58:21Z","receivedAt":"2018-08-02T18:58:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 01, 2018 at 02:58:50AM +0200, René Scharfe wrote:\n\n> Am 31.07.2018 um 23:06 schrieb Junio C Hamano:\n> > Jeff King writes:\n> > \n> >> On Tue, Jul 31, 2018 at 01:23:04PM -0400, Jeff King wrote:\n> >> ...\n> >> So here it is fixed, and with a commit message. I'm not happy to omit a\n> >> regression test, but I actually couldn't come up with a minimal one that\n> >> tickled the problem, because we're playing around with heuristics.\n> How about something like this? (squashable)\n\nThanks. This is quite similar to what I tried, but I had started a new\nscript, and I suspect that what is in t6029's master branch tickles the\nheuristic in a more interesting way. Or possibly I just botched my\nattempt, though I did spend quite a bit of time fiddling with it. The\nmaster branch seems to only contain one file, \"hello\". Hmph.\n\nAt any rate, it does trigger the bug and demonstrate the fix, so let's\ngo with it. I see Junio already squashed the tests into\njk/merge-subtree-heuristics, but I think we can cut down the commit\nmessage a bit (and stop claiming that we don't have a test ;) ). Like\nso (and yes, this version omits the extra scissors):\n\n-- >8 --\nSubject: score_trees(): fix iteration over trees with missing entries\n\nIn score_trees(), we walk over two sorted trees to find\nwhich entries are missing or have different content between\nthe two. So if we have two trees with these entries:\n\n one two\n --- ---\n a a\n b c\n c d\n\nwe'd expect the loop to:\n\n - compare \"a\" to \"a\"\n\n - compare \"b\" to \"c\"; because these are sorted lists, we\n know that the second tree does not have \"b\"\n\n - compare \"c\" to \"c\"\n\n - compare \"d\" to end-of-list; we know that the first tree\n does not have \"d\"\n\nAnd prior to d8febde370 (match-trees: simplify score_trees()\nusing tree_entry(), 2013-03-24) that worked. But after that\ncommit, we mistakenly increment the tree pointers for every\nloop iteration, even when we've processed the entry for only\none side. As a result, we end up doing this:\n\n - compare \"a\" to \"a\"\n\n - compare \"b\" to \"c\"; we know that we do not have \"b\", but\n we still increment both tree pointers; at this point\n we're out of sync and all further comparisons are wrong\n\n - compare \"c\" to \"d\" and mistakenly claim that the second\n tree does not have \"c\"\n\n - exit the loop, mistakenly not realizing that the first\n tree does not have \"d\"\n\nSo contrary to the claim in d8febde370, we really do need to\nmanually use update_tree_entry(), because advancing the tree\npointer depends on the entry comparison.\n\nThat means we must stop using tree_entry() to access each\nentry, since it auto-advances the pointer. Instead:\n\n - we'll use tree_desc.size directly to know if there's\n anything left to look at (which is what tree_entry() was\n doing under the hood)\n\n - rather than do an extra struct assignment to \"e1\" and\n \"e2\", we can just access the \"entry\" field of tree_desc\n directly\n\nThat makes us a little more intimate with the tree_desc\ncode, but that's not uncommon for its callers.\n\nThe included test shows off the bug by adding a new entry\n\"bar.t\", which sorts early in the tree and de-syncs the\ncomparison for \"foo.t\", which comes after.\n\nReported-by: George Shammas \nHelped-by: René Scharfe \nSigned-off-by: Jeff King \n---\n match-trees.c | 43 ++++++++++++++++++++++++----------------\n t/t6029-merge-subtree.sh | 28 ++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 17 deletions(-)\n\ndiff --git a/match-trees.c b/match-trees.c\nindex 4cdeff53e1..37653308d3 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -83,34 +83,43 @@ static int score_trees(const struct object_id *hash1, const struct object_id *ha\n \tint score = 0;\n \n \tfor (;;) {\n-\t\tstruct name_entry e1, e2;\n-\t\tint got_entry_from_one = tree_entry(&one, &e1);\n-\t\tint got_entry_from_two = tree_entry(&two, &e2);\n \t\tint cmp;\n \n-\t\tif (got_entry_from_one && got_entry_from_two)\n-\t\t\tcmp = base_name_entries_compare(&e1, &e2);\n-\t\telse if (got_entry_from_one)\n+\t\tif (one.size && two.size)\n+\t\t\tcmp = base_name_entries_compare(&one.entry, &two.entry);\n+\t\telse if (one.size)\n \t\t\t/* two lacks this entry */\n \t\t\tcmp = -1;\n-\t\telse if (got_entry_from_two)\n+\t\telse if (two.size)\n \t\t\t/* two has more entries */\n \t\t\tcmp = 1;\n \t\telse\n \t\t\tbreak;\n \n-\t\tif (cmp < 0)\n+\t\tif (cmp < 0) {\n \t\t\t/* path1 does not appear in two */\n-\t\t\tscore += score_missing(e1.mode, e1.path);\n-\t\telse if (cmp > 0)\n+\t\t\tscore += score_missing(one.entry.mode, one.entry.path);\n+\t\t\tupdate_tree_entry(&one);\n+\t\t} else if (cmp > 0) {\n \t\t\t/* path2 does not appear in one */\n-\t\t\tscore += score_missing(e2.mode, e2.path);\n-\t\telse if (oidcmp(e1.oid, e2.oid))\n-\t\t\t/* they are different */\n-\t\t\tscore += score_differs(e1.mode, e2.mode, e1.path);\n-\t\telse\n-\t\t\t/* same subtree or blob */\n-\t\t\tscore += score_matches(e1.mode, e2.mode, e1.path);\n+\t\t\tscore += score_missing(two.entry.mode, two.entry.path);\n+\t\t\tupdate_tree_entry(&two);\n+\t\t} else {\n+\t\t\t/* path appears in both */\n+\t\t\tif (oidcmp(one.entry.oid, two.entry.oid)) {\n+\t\t\t\t/* they are different */\n+\t\t\t\tscore += score_differs(one.entry.mode,\n+\t\t\t\t\t\t two.entry.mode,\n+\t\t\t\t\t\t one.entry.path);\n+\t\t\t} else {\n+\t\t\t\t/* same subtree or blob */\n+\t\t\t\tscore += score_matches(one.entry.mode,\n+\t\t\t\t\t\t two.entry.mode,\n+\t\t\t\t\t\t one.entry.path);\n+\t\t\t}\n+\t\t\tupdate_tree_entry(&one);\n+\t\t\tupdate_tree_entry(&two);\n+\t\t}\n \t}\n \tfree(one_buf);\n \tfree(two_buf);\ndiff --git a/t/t6029-merge-subtree.sh b/t/t6029-merge-subtree.sh\nindex 3e692454a7..474a850de6 100755\n--- a/t/t6029-merge-subtree.sh\n+++ b/t/t6029-merge-subtree.sh\n@@ -29,6 +29,34 @@ test_expect_success 'subtree available and works like recursive' '\n \n '\n \n+test_expect_success 'setup branch sub' '\n+\tgit checkout --orphan sub &&\n+\tgit rm -rf . &&\n+\ttest_commit foo\n+'\n+\n+test_expect_success 'setup branch main' '\n+\tgit checkout -b main master &&\n+\tgit merge -s ours --no-commit --allow-unrelated-histories sub &&\n+\tgit read-tree --prefix=dir/ -u sub &&\n+\tgit commit -m \"initial merge of sub into main\" &&\n+\ttest_path_is_file dir/foo.t &&\n+\ttest_path_is_file hello\n+'\n+\n+test_expect_success 'update branch sub' '\n+\tgit checkout sub &&\n+\ttest_commit bar\n+'\n+\n+test_expect_success 'update branch main' '\n+\tgit checkout main &&\n+\tgit merge -s subtree sub -m \"second merge of sub into main\" &&\n+\ttest_path_is_file dir/bar.t &&\n+\ttest_path_is_file dir/foo.t &&\n+\ttest_path_is_file hello\n+'\n+\n test_expect_success 'setup' '\n \tmkdir git-gui &&\n \tcd git-gui &&\n-- \n2.18.0.800.g770d9f3396\n\n\n\n"}]}