{"thread":{"id":"57967","subject":"Bug in merge-ort (rename detection can have collisions?)","startedAt":"2022-06-08T02:16:00Z","lastAt":"2022-06-22T16:59:17Z","messageCount":7,"participants":["Glen Choo","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"456844","messageId":"kl6lee006mle.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57967","inReplyTo":null,"subject":"Bug in merge-ort (rename detection can have collisions?)","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-08T00:11:09Z","receivedAt":"2022-06-08T02:16:00Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\n(I'm not 100% what the bug _is_, only that there is one.)\n\n= Report\n\nAt $DAYJOB, there was a report that \"git merge\" was failing on certain\nbranches. Fortunately, the repo is publicly accessible, so I can share\nthe full reproduction recipe:\n\n  git clone https://android.googlesource.com/platform/external/tensorflow tensorflow &&\n  cd tensorflow &&\n  git merge origin/upstream-master # HEAD is at origin/master\n\nThis gives:\n\n  Performing inexact rename detection: 100% (4371280/4371280), done.\n  Performing inexact rename detection: 100% (12529218/12529218), done.\n  Assertion failed: (ci->filemask == 2 || ci->filemask == 4), function apply_directory_rename_modifications, file merge-ort.c, line 2410. \n\nThis bug seems specific to merge-ort; \"git merge -s recursive\norigin/upstream-master\" seems to work as expected.\n\nIn case the branches have changed since then, here are the commit ids:\n\n  $ git rev-parse origin/master\n  68e55281824e8a79fa67e1a3061f39bd4c4b2e57\n  $ git rev-parse origin/upstream-master\n  0be5bb09aeeff3a6825842326fadc8159a5553ab\n  $ git merge-base 68e55281824e8a79fa67e1a3061f39bd4c4b2e57 0be5bb09aeeff3a6825842326fadc8159a5553ab\n  8e819019081f39d83df42baba4acfced3abf3f90\n\n= Interesting info\n\nI don't understand the merge-ort code enough to understand what's going\non, but I was able to find some (hopefully helpful) details. I added\nthis log line just above the offending assert() call:\n\n\ttrace2_printf(\"0 %s, 1 %s, 2 %s, fm %d, dm %d\", ci->pathnames[0],\n    ci->pathnames[1], ci->pathnames[2], ci->filemask, ci->dirmask);\n\nHere are the lines I thought were suspicious:\n\n  0 <path1>, 1 <path1>, 2 <path1>, fm 2, dm 0\n  [...]\n  0 <path2>, 1 <path1>, 2 <path2>, fm 6, dm 0 # this is the last line\n\nNotice that the last line detected a rename from <path2> to <path1>, but\nwe already saw <path1> earlier.\n\nIIUC \"(ci->filemask == 2 || ci->filemask == 4)\" can be read as \"the path\neither exists on only the left side or only the right side of the\nmerge\", so ci->filemask == 6 should mean \"the path exists on both sides\nof the merge\"?\n\n\"-s recursive\" seems to handle the rename just fine (it picks <path2>\nIIRC).\n\nI also dug into each commit to see which paths were present:\n\n  head=\"origin/master\"\n  other=\"origin/upstream-master\"\n  merge_base=\"$(git merge-base origin/master origin/upstream-master)\"\n  path1=\"tensorflow/lite/g3doc/convert/metadata_writer_tutorial.ipynb\"\n  path2=\"tensorflow/lite/g3doc/models/convert/metadata_writer_tutorial.ipynb\"\n\n  git rev-parse \"$head:$path1\" # (exists)\n  git rev-parse \"$head:$path2\" # (doesn't exist)\n\n  git rev-parse \"$other:$path1\" # (doesn't exist)\n  git rev-parse \"$other:$path2\" # (exists)\n\n  git rev-parse \"$merge_base:$path1\" # (doesn't exist)\n  git rev-parse \"$merge_base:$path2\" # (doesn't exist)\n\ni.e. both files are new and are renames of each other. I haven't tried\nusing this property to create a minimally-reproducing recipe though.\n"},{"id":"457011","messageId":"CABPp-BEW2WF5AJeKqCiL2zhcPwPH-u6p=myoX_GkU6tbV=+TZA@mail.gmail.com","threadId":"57967","inReplyTo":"kl6lee006mle.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bug in merge-ort (rename detection can have collisions?)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-06-10T06:41:06Z","receivedAt":"2022-06-10T06:41:25Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jun 7, 2022 at 5:11 PM Glen Choo <chooglen@google.com> wrote:\n>\n> (I'm not 100% what the bug _is_, only that there is one.)\n>\n> = Report\n>\n> At $DAYJOB, there was a report that \"git merge\" was failing on certain\n> branches. Fortunately, the repo is publicly accessible, so I can share\n> the full reproduction recipe:\n>\n>   git clone https://android.googlesource.com/platform/external/tensorflow tensorflow &&\n>   cd tensorflow &&\n>   git merge origin/upstream-master # HEAD is at origin/master\n>\n> This gives:\n>\n>   Performing inexact rename detection: 100% (4371280/4371280), done.\n>   Performing inexact rename detection: 100% (12529218/12529218), done.\n>   Assertion failed: (ci->filemask == 2 || ci->filemask == 4), function apply_directory_rename_modifications, file merge-ort.c, line 2410.\n>\n> This bug seems specific to merge-ort; \"git merge -s recursive\n> origin/upstream-master\" seems to work as expected.\n>\n> In case the branches have changed since then, here are the commit ids:\n>\n>   $ git rev-parse origin/master\n>   68e55281824e8a79fa67e1a3061f39bd4c4b2e57\n>   $ git rev-parse origin/upstream-master\n>   0be5bb09aeeff3a6825842326fadc8159a5553ab\n>   $ git merge-base 68e55281824e8a79fa67e1a3061f39bd4c4b2e57 0be5bb09aeeff3a6825842326fadc8159a5553ab\n>   8e819019081f39d83df42baba4acfced3abf3f90\n>\n> = Interesting info\n>\n> I don't understand the merge-ort code enough to understand what's going\n> on, but I was able to find some (hopefully helpful) details. I added\n> this log line just above the offending assert() call:\n>\n>         trace2_printf(\"0 %s, 1 %s, 2 %s, fm %d, dm %d\", ci->pathnames[0],\n>     ci->pathnames[1], ci->pathnames[2], ci->filemask, ci->dirmask);\n>\n> Here are the lines I thought were suspicious:\n>\n>   0 <path1>, 1 <path1>, 2 <path1>, fm 2, dm 0\n>   [...]\n>   0 <path2>, 1 <path1>, 2 <path2>, fm 6, dm 0 # this is the last line\n>\n> Notice that the last line detected a rename from <path2> to <path1>, but\n> we already saw <path1> earlier.\n>\n> IIUC \"(ci->filemask == 2 || ci->filemask == 4)\" can be read as \"the path\n> either exists on only the left side or only the right side of the\n> merge\", so ci->filemask == 6 should mean \"the path exists on both sides\n> of the merge\"?\n>\n> \"-s recursive\" seems to handle the rename just fine (it picks <path2>\n> IIRC).\n>\n> I also dug into each commit to see which paths were present:\n>\n>   head=\"origin/master\"\n>   other=\"origin/upstream-master\"\n>   merge_base=\"$(git merge-base origin/master origin/upstream-master)\"\n>   path1=\"tensorflow/lite/g3doc/convert/metadata_writer_tutorial.ipynb\"\n>   path2=\"tensorflow/lite/g3doc/models/convert/metadata_writer_tutorial.ipynb\"\n>\n>   git rev-parse \"$head:$path1\" # (exists)\n>   git rev-parse \"$head:$path2\" # (doesn't exist)\n>\n>   git rev-parse \"$other:$path1\" # (doesn't exist)\n>   git rev-parse \"$other:$path2\" # (exists)\n>\n>   git rev-parse \"$merge_base:$path1\" # (doesn't exist)\n>   git rev-parse \"$merge_base:$path2\" # (doesn't exist)\n>\n> i.e. both files are new and are renames of each other. I haven't tried\n> using this property to create a minimally-reproducing recipe though.\n\nThanks for the detailed report; very cool.  Interestingly, if you\nreverse the direction of the merge (checkout origin/upstream-master\nand merge origin/master) then you get a different error:\n\n    error: cache entry has null sha1:\ntensorflow/lite/g3doc/examples/convert/metadata_writer_tutorial.ipynb\n    fatal: unable to write .git/index\n\nThis merge has a lot of stuff going on, including some big directory\nrenames, new files, loads of conflicts, etc.  I think the relevant\nbits are the following:\n\nmerge base -> side1:\n    Rename tensorflow/lite/g3doc/models/ -> tensorflow/lite/g3doc/examples\n    Add tensorflow/lite/g3doc/convert/metadata_writer_tutorial.ipynb\n\nmerge base -> side2:\n    Rename tensorflow/lite/g3doc/convert/ ->\ntensorflow/lite/g3doc/models/convert/\n    Add tensorflow/lite/g3doc/models/convert/metadata_writer_tutorial.ipynb\n\nSo the combination of the above means:\n    side1:tensorflow/lite/g3doc/convert/metadata_writer_tutorial.ipynb\nwould be affected by the side2 directory rename to become\n    tensorflow/lite/g3doc/models/convert/metadata_writer_tutorial.ipynb\nwhich would match the name of the file on side2, BUT the side2 version of\nthat file, i.e.:\n    side2:tensorflow/lite/g3doc/models/convert/metadata_writer_tutorial.ipynb\nwould be affected by the side1 directory rename to become\n    tensorflow/lite/g3doc/examples/convert/metadata_writer_tutorial.ipynb\nand what becomes of the side1 version of that file?  It gets messy...\n\n\nI have a small reproduction recipe (using the style from t6423 to explain it):\n\n#   Commit O: sub1/file,                 sub2/other\n#   Commit A: sub3/file,                 sub2/{other, new_add_add_file_1}\n#   Commit B: sub1/{file, newfile}, sub1/sub2/{other, new_add_add_file_2}\n#\n#   In words:\n#     A: sub1/ -> sub3/, add sub2/new_add_add_file_1\n#     B: sub2/ -> sub1/sub2, add sub1/newfile, add sub1/sub2/new_add_add_file_2\n\nThis small reproduction recipe triggers the same assertion-failure you\nreported; further, when the merge direction is reversed this testcase\nshows the same alternative error I showed above for your bigger\ntestcase.  However, interestingly, this simple testcase also triggers\nthose same null-sha1 and unable-to-write-.git/index errors in\nmerge-recursive, regardless of the direction of the merge.  I don't\nknow why my testcase triggers bugs in merge-recursive and your bigger\ntestcase doesn't, but I wasn't too motivated to find out either; I was\nmostly focused on trying to understand the merge-ort side of things.\n\nNow, there's code in both merge-recursive and merge-ort for avoiding\ndoubly transitive renames (i.e. one side renames directory A/ -> B/,\nand the other side renames directory B/ -> C/, and even worse if the\noriginal side also renames C/ -> D/, because this combination would\notherwise make a mess for new files added to A/ on the first side and\nwondering which directory they end up in).  However, this is a\ntestcase where a _leading directory_ of B/ is renamed to C/, which\nsidesteps the normal doubly transitive rename check, and then the code\nheads down what looks like the wrong path until it is caught by the\nassertion check you reported.  In addition to the\nfunny-doubly-transitive-rename (involving the leading directory), your\ntestcase also has both sides add a file, one side to A/ and the other\nside to B/ (with the same basename but different file contents), which\nadds to the \"fun\".\n\nAnyway, long story short...I don't have a fix yet, but just thought\nI'd mention I saw the email and spent some hours digging in.\n"},{"id":"457033","messageId":"xmqqczfgfojb.fsf@gitster.g","threadId":"57967","inReplyTo":"CABPp-BEW2WF5AJeKqCiL2zhcPwPH-u6p=myoX_GkU6tbV=+TZA@mail.gmail.com","subject":"Re: Bug in merge-ort (rename detection can have collisions?)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-10T16:53:28Z","receivedAt":"2022-06-10T16:53:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Tue, Jun 7, 2022 at 5:11 PM Glen Choo <chooglen@google.com> wrote:\n>>\n>> (I'm not 100% what the bug _is_, only that there is one.)\n>>\n>> = Report\n>>\n>> At $DAYJOB, there was a report that \"git merge\" was failing on certain\n>> branches. Fortunately, the repo is publicly accessible, so I can share\n>> the full reproduction recipe:\n>> ...\n> Thanks for the detailed report; very cool.  Interestingly, if you\n> reverse the direction of the merge (checkout origin/upstream-master\n> and merge origin/master) then you get a different error:\n> ...\n> Anyway, long story short...I don't have a fix yet, but just thought\n> I'd mention I saw the email and spent some hours digging in.\n\nThanks for continued support for the ort strategy.  From the very\nbeginning, I was hesitant to make our tools try to be too clever\nwith excessive heuristics, but at least we are not making a silent\nmismerge in this case, so it is probably OK, especially with \"-s\nrecursive\" still left as an escape hatch.\n"},{"id":"457061","messageId":"CABPp-BEXdfEw5jYn-WM_pyEyS5AHmYEJhVNS8GtHAd2BXCaB_A@mail.gmail.com","threadId":"57967","inReplyTo":"xmqqczfgfojb.fsf@gitster.g","subject":"Re: Bug in merge-ort (rename detection can have collisions?)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-06-11T08:56:19Z","receivedAt":"2022-06-11T08:56:35Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 10, 2022 at 9:53 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > On Tue, Jun 7, 2022 at 5:11 PM Glen Choo <chooglen@google.com> wrote:\n> >>\n> >> (I'm not 100% what the bug _is_, only that there is one.)\n> >>\n> >> = Report\n> >>\n> >> At $DAYJOB, there was a report that \"git merge\" was failing on certain\n> >> branches. Fortunately, the repo is publicly accessible, so I can share\n> >> the full reproduction recipe:\n> >> ...\n> > Thanks for the detailed report; very cool.  Interestingly, if you\n> > reverse the direction of the merge (checkout origin/upstream-master\n> > and merge origin/master) then you get a different error:\n> > ...\n> > Anyway, long story short...I don't have a fix yet, but just thought\n> > I'd mention I saw the email and spent some hours digging in.\n>\n> Thanks for continued support for the ort strategy.  From the very\n> beginning, I was hesitant to make our tools try to be too clever\n> with excessive heuristics, but at least we are not making a silent\n> mismerge in this case, so it is probably OK, especially with \"-s\n> recursive\" still left as an escape hatch.\n\nI'm pretty sure the bug would still trigger even if we removed all the\nheuristic differences that the ort strategy has relative to the\nrecursive one; I don't think those are related to this problem at all.\n\nIn fact, the more general problem area here appears to affect the\nrecursive strategy as well.  I'm glad the specific testcase reported\nworks under recursive and gave Glen (or his user) a workaround, but\nthat feels like luck rather than design because my minimal\nreproduction testcase not only triggers the same issue he saw with the\nort strategy, but also triggers a previously unknown fatal bug in the\nrecursive strategy too.\n"},{"id":"457107","messageId":"kl6l35g87bga.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57967","inReplyTo":"CABPp-BEXdfEw5jYn-WM_pyEyS5AHmYEJhVNS8GtHAd2BXCaB_A@mail.gmail.com","subject":"Re: Bug in merge-ort (rename detection can have collisions?)","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-13T16:52:21Z","receivedAt":"2022-06-13T19:04:25Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nThanks Elijah, your explanation is really great :)\nThis looks like a pretty nasty bug. Ugh..\n\nElijah Newren <newren@gmail.com> writes:\n\n> On Fri, Jun 10, 2022 at 9:53 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Elijah Newren <newren@gmail.com> writes:\n>>\n>> > On Tue, Jun 7, 2022 at 5:11 PM Glen Choo <chooglen@google.com> wrote:\n>> >>\n>> >> (I'm not 100% what the bug _is_, only that there is one.)\n>> >>\n>> >> = Report\n>> >>\n>> >> At $DAYJOB, there was a report that \"git merge\" was failing on certain\n>> >> branches. Fortunately, the repo is publicly accessible, so I can share\n>> >> the full reproduction recipe:\n>> >> ...\n>> > Thanks for the detailed report; very cool.  Interestingly, if you\n>> > reverse the direction of the merge (checkout origin/upstream-master\n>> > and merge origin/master) then you get a different error:\n>> > ...\n>> > Anyway, long story short...I don't have a fix yet, but just thought\n>> > I'd mention I saw the email and spent some hours digging in.\n>>\n>> Thanks for continued support for the ort strategy.  From the very\n>> beginning, I was hesitant to make our tools try to be too clever\n>> with excessive heuristics, but at least we are not making a silent\n>> mismerge in this case, so it is probably OK, especially with \"-s\n>> recursive\" still left as an escape hatch.\n>\n> In fact, the more general problem area here appears to affect the\n> recursive strategy as well.  I'm glad the specific testcase reported\n> works under recursive and gave Glen (or his user) a workaround, but\n> that feels like luck rather than design because my minimal\n> reproduction testcase not only triggers the same issue he saw with the\n> ort strategy, but also triggers a previously unknown fatal bug in the\n> recursive strategy too.\n\nYeah, hm. I was surprised that we encountered this bug at all, but it's\nnot so surprising after seeing how many edge conditions this bug\ncontains.\n\nI wonder if there other rename detection bugs lurking beyond the\nhorizon, whether we already assume that these bugs exist, and if so,\nwhether we should officially document \"merge without rename detection\"\nas a workaround [1].\n\n[1] Assuming that the workaround works of course. I tried to disable\nrename detection several times, but I couldn't really figure out whether\nI did it correctly or whether it fixed the bug (which is why I didn't\ninclude it in the initial report.)\n"},{"id":"457677","messageId":"CABPp-BGN0DoSr3bcjTmGZkcoj_dSVzOgFUQ++R=_z8v=nAJsTg@mail.gmail.com","threadId":"57967","inReplyTo":"kl6l35g87bga.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: Bug in merge-ort (rename detection can have collisions?)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-06-22T04:30:31Z","receivedAt":"2022-06-22T04:30:47Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Sorry for the long delay.  I haven't gotten much Git time lately...\n\nOn Mon, Jun 13, 2022 at 9:52 AM Glen Choo <chooglen@google.com> wrote:\n>\n[...]\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > On Fri, Jun 10, 2022 at 9:53 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >>\n> >> Elijah Newren <newren@gmail.com> writes:\n> >>\n> >> > On Tue, Jun 7, 2022 at 5:11 PM Glen Choo <chooglen@google.com> wrote:\n> >> >>\n> >> >> (I'm not 100% what the bug _is_, only that there is one.)\n> >> >>\n> >> >> = Report\n> >> >>\n> >> >> At $DAYJOB, there was a report that \"git merge\" was failing on certain\n> >> >> branches. Fortunately, the repo is publicly accessible, so I can share\n> >> >> the full reproduction recipe:\n> >> >> ...\n> >> > Thanks for the detailed report; very cool.  Interestingly, if you\n> >> > reverse the direction of the merge (checkout origin/upstream-master\n> >> > and merge origin/master) then you get a different error:\n> >> > ...\n> >> > Anyway, long story short...I don't have a fix yet, but just thought\n> >> > I'd mention I saw the email and spent some hours digging in.\n> >>\n> >> Thanks for continued support for the ort strategy.  From the very\n> >> beginning, I was hesitant to make our tools try to be too clever\n> >> with excessive heuristics, but at least we are not making a silent\n> >> mismerge in this case, so it is probably OK, especially with \"-s\n> >> recursive\" still left as an escape hatch.\n> >\n> > In fact, the more general problem area here appears to affect the\n> > recursive strategy as well.  I'm glad the specific testcase reported\n> > works under recursive and gave Glen (or his user) a workaround, but\n> > that feels like luck rather than design because my minimal\n> > reproduction testcase not only triggers the same issue he saw with the\n> > ort strategy, but also triggers a previously unknown fatal bug in the\n> > recursive strategy too.\n>\n> Yeah, hm. I was surprised that we encountered this bug at all, but it's\n> not so surprising after seeing how many edge conditions this bug\n> contains.\n\nTo be fair, I've dug into cases with more.  :-)\n\n> I wonder if there other rename detection bugs lurking beyond the\n> horizon,\n\nI was trying to dig around for related issues so I can fix the class\nof problems rather than just the instance.  Reversing the direction of\nthe merge was just one component of that (and I reported that\nparticular tweak since it triggered something a little different).\n\nThe original motivation for writing merge-ort was to address bugs I\ncouldn't otherwise fix within merge-recursive's implementation.  I've\nput a lot of time into corner cases, many of which (perhaps even the\nmajority) were not actually motivated by real-life testcases but me\njust having an obsession with making Git's merge machinery handle\nweird inputs.  Junio even commented on some of my testcases with 'I am\nnot sure if there is a single \"correct\" answer everybody can agree on\nfor each of these \"insane\" cases, though.'.  Now, obviously I can miss\nsome inputs, as evidenced by the issue you reported, so there is\nalways a chance there are more.  However...\n\n> whether we already assume that these bugs exist, and if so,\n> whether we should officially document \"merge without rename detection\"\n> as a workaround [1].\n>\n> [1] Assuming that the workaround works of course. I tried to disable\n> rename detection several times, but I couldn't really figure out whether\n> I did it correctly or whether it fixed the bug (which is why I didn't\n> include it in the initial report.)\n\nTurning off renames and relying on users to correct merge issues may\nbe reasonable when there are only a few.  When there are more than a\nfew, my experience in the past with turning off rename detection (or\nthere being too many renames that rename detection turns itself off)\nis that users often:\n\n  * don't match up renamed files and do a three-way merge, but just\npick one of the two conflicting sides, unknowingly discarding changes\nmade on the other side\n  * sometimes notice the files that should have been renames, and\nmanually hand apply the subset of changes they remember from one file\nto the other, and unknowingly discarding the remaining subset of\nchanges (which were often changes made by people other than the one\ndoing the merges).\n\nIn the particular repository in question, you've got 600+ renames on\none side, and 200+ on the other -- including multiple different entire\ndirectories.  (Also, since lack of rename detection makes renames get\nreported as modify/delete conflicts, and you've got 400+ actual\nmodify/delete conflicts on top of all the renames, users would have\nlots of \"fun\" attempting to sort things all out without tool support.)\n So, I'm worried the \"fallback\"/\"workaround\" is likely to put users in\na worse situation rather than a better one.\n\nBut, even if your goal really is to have a fallback, why not just use\nthe `resolve` strategy?  Your testcase doesn't have multiple merge\nbases, and the resolve strategy is roughly the recursive strategy\nminus the renames and the multiple merge base handling.\n\n(Also, I'm not just avoiding work.  I have already written patches to\nturn off rename detection in the ort strategy.  I pointed these out to\nStolee and Dscho for a special internal usecase of theirs, and at\nleast one of those emails cc'ed the mailing list. so you should be\nable to find those patches with a search.  I'm just not convinced of\nthe need to merge those patches.)\n\n\n\nAnyway, that all said, I posted a fix for this issue over at\nhttps://lore.kernel.org/git/pull.1268.git.1655871651.gitgitgadget@gmail.com/.\nWith it, I can repeat the tensorflow merge you highlighted, in either\ndirection, without issue.\n"},{"id":"457726","messageId":"kl6l7d58k535.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57967","inReplyTo":"CABPp-BGN0DoSr3bcjTmGZkcoj_dSVzOgFUQ++R=_z8v=nAJsTg@mail.gmail.com","subject":"Re: Bug in merge-ort (rename detection can have collisions?)","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-22T16:58:38Z","receivedAt":"2022-06-22T16:59:17Z","isPatch":false,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Sorry for the long delay.  I haven't gotten much Git time lately...\n>\n> On Mon, Jun 13, 2022 at 9:52 AM Glen Choo <chooglen@google.com> wrote:\n>>\n> [...]\n>> Elijah Newren <newren@gmail.com> writes:\n>>\n>> > On Fri, Jun 10, 2022 at 9:53 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> >>\n>> >> Elijah Newren <newren@gmail.com> writes:\n>> >>\n>> >> > On Tue, Jun 7, 2022 at 5:11 PM Glen Choo <chooglen@google.com> wrote:\n>> >> >>\n>> >> >> (I'm not 100% what the bug _is_, only that there is one.)\n>> >> >>\n>> >> >> = Report\n>> >> >>\n>> >> >> At $DAYJOB, there was a report that \"git merge\" was failing on certain\n>> >> >> branches. Fortunately, the repo is publicly accessible, so I can share\n>> >> >> the full reproduction recipe:\n>> >> >> ...\n>> >> > Thanks for the detailed report; very cool.  Interestingly, if you\n>> >> > reverse the direction of the merge (checkout origin/upstream-master\n>> >> > and merge origin/master) then you get a different error:\n>> >> > ...\n>> >> > Anyway, long story short...I don't have a fix yet, but just thought\n>> >> > I'd mention I saw the email and spent some hours digging in.\n>> >>\n>> >> Thanks for continued support for the ort strategy.  From the very\n>> >> beginning, I was hesitant to make our tools try to be too clever\n>> >> with excessive heuristics, but at least we are not making a silent\n>> >> mismerge in this case, so it is probably OK, especially with \"-s\n>> >> recursive\" still left as an escape hatch.\n>> >\n>> > In fact, the more general problem area here appears to affect the\n>> > recursive strategy as well.  I'm glad the specific testcase reported\n>> > works under recursive and gave Glen (or his user) a workaround, but\n>> > that feels like luck rather than design because my minimal\n>> > reproduction testcase not only triggers the same issue he saw with the\n>> > ort strategy, but also triggers a previously unknown fatal bug in the\n>> > recursive strategy too.\n>>\n>> Yeah, hm. I was surprised that we encountered this bug at all, but it's\n>> not so surprising after seeing how many edge conditions this bug\n>> contains.\n>\n> To be fair, I've dug into cases with more.  :-)\n\n:)\n\n>> I wonder if there other rename detection bugs lurking beyond the\n>> horizon,\n>\n> I was trying to dig around for related issues so I can fix the class\n> of problems rather than just the instance.  Reversing the direction of\n> the merge was just one component of that (and I reported that\n> particular tweak since it triggered something a little different).\n>\n> The original motivation for writing merge-ort was to address bugs I\n> couldn't otherwise fix within merge-recursive's implementation.  I've\n> put a lot of time into corner cases, many of which (perhaps even the\n> majority) were not actually motivated by real-life testcases but me\n> just having an obsession with making Git's merge machinery handle\n> weird inputs.  Junio even commented on some of my testcases with 'I am\n> not sure if there is a single \"correct\" answer everybody can agree on\n> for each of these \"insane\" cases, though.'.  Now, obviously I can miss\n> some inputs, as evidenced by the issue you reported, so there is\n> always a chance there are more.  However...\n>\n>> whether we already assume that these bugs exist, and if so,\n>> whether we should officially document \"merge without rename detection\"\n>> as a workaround [1].\n>>\n>> [1] Assuming that the workaround works of course. I tried to disable\n>> rename detection several times, but I couldn't really figure out whether\n>> I did it correctly or whether it fixed the bug (which is why I didn't\n>> include it in the initial report.)\n>\n> Turning off renames and relying on users to correct merge issues may\n> be reasonable when there are only a few.  When there are more than a\n> few, my experience in the past with turning off rename detection (or\n> there being too many renames that rename detection turns itself off)\n> is that users often:\n>\n>   * don't match up renamed files and do a three-way merge, but just\n> pick one of the two conflicting sides, unknowingly discarding changes\n> made on the other side\n>   * sometimes notice the files that should have been renames, and\n> manually hand apply the subset of changes they remember from one file\n> to the other, and unknowingly discarding the remaining subset of\n> changes (which were often changes made by people other than the one\n> doing the merges).\n>\n> In the particular repository in question, you've got 600+ renames on\n> one side, and 200+ on the other -- including multiple different entire\n> directories.  (Also, since lack of rename detection makes renames get\n> reported as modify/delete conflicts, and you've got 400+ actual\n> modify/delete conflicts on top of all the renames, users would have\n> lots of \"fun\" attempting to sort things all out without tool support.)\n>  So, I'm worried the \"fallback\"/\"workaround\" is likely to put users in\n> a worse situation rather than a better one.\n\nAh, that's a good point. I hadn't given too much thought to the\ncomplexity of manually resolving a merge without rename detection, but,\nlike you said, it's probably not a useful fallback anyway.\n\n\n> But, even if your goal really is to have a fallback, why not just use\n> the `resolve` strategy?  Your testcase doesn't have multiple merge\n> bases, and the resolve strategy is roughly the recursive strategy\n> minus the renames and the multiple merge base handling.\n\nOh, today I learned about the `resolve` strategy..\n\n\n> (Also, I'm not just avoiding work.  I have already written patches to\n> turn off rename detection in the ort strategy.  I pointed these out to\n> Stolee and Dscho for a special internal usecase of theirs, and at\n> least one of those emails cc'ed the mailing list. so you should be\n> able to find those patches with a search.  I'm just not convinced of\n> the need to merge those patches.)\n>\n>\n>\n> Anyway, that all said, I posted a fix for this issue over at\n> https://lore.kernel.org/git/pull.1268.git.1655871651.gitgitgadget@gmail.com/.\n> With it, I can repeat the tensorflow merge you highlighted, in either\n> direction, without issue.\n\nThat's fantastic. Thanks for keeping me in the loop :)\n"}]}