{"thread":{"id":"52370","subject":"git-rebase produces incorrect output","startedAt":"2019-11-29T08:21:54Z","lastAt":"2019-11-30T17:58:34Z","messageCount":6,"participants":["Pavel Roskin","Philip Oakley","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"387298","messageId":"CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com","threadId":"52370","inReplyTo":null,"subject":"git-rebase produces incorrect output","fromName":"Pavel Roskin","fromEmail":"plroskin@gmail.com","sentAt":"2019-11-29T08:21:40Z","receivedAt":"2019-11-29T08:21:54Z","isPatch":false,"sender":{"key":"plroskin@gmail.com","avatar":null},"body":"Hi!\n\nI've discovered an issue with \"git rebase\" producing a subtly\nincorrect file. In fact, that files even compiled but failed in unit\ntests! That's so scary that I'm going to stop using \"git rebase\" for\nnow. Fortunately, \"git rebase --merge\" is working correctly, so I'll\nuse it. Too bad there is no option to use \"--merge\" by default.\n\nThe issue was observed in git 2.23 and reproduced in today's next\nbranch (2.24.0.449.g4c06f74957) on up-to-date Fedora 31 x86_64.\n\nI've created a repository that demonstrates the issue:\nhttps://github.com/proski/git-rebase-demo\n\nThe branch names should be self-explanatory. \"master\" is the base,\n\"branch1\" and \"branch2\" contain one change each. If \"branch1\" is\nrebased on top of \"branch2\", the result is incorrect, saved in the\n\"rebase-bad\" branch. If \"git rebase -m\" is used, the result is\ncorrect, saved in the \"merge-good\" branch.\n\nThe files in \"rebase-bad\" and \"merge-good\" have exactly the same lines\nbut in a different order. Yet the changes on branch1 and branch2\naffect non-overlapping parts of the file. There should be no doubt how\nthe merged code should look like.\n\nI believe the change on branch2 shifts the lines, so that the first\nchange from branch1 is applies to a place below the intended location,\nand then git goes back to an earlier line to apply the next hunk. I\ncan imagine that it would do the right thing in case of swapped blocks\nof code. Yet I have a real life example where it does a very wrong\nthing.\n\nIndeed, \"git diff origin/branch2 origin/rebase-bad\" and \"git diff\norigin/branch2 origin/merge-good\" both produce diffs of 9957 bytes\nlong, different only in the order of the hunks.\n\nAnother interesting data point - \"git rebase --interactive\" is working\ncorrectly.\n\n-- \nRegards,\nPavel Roskin\n"},{"id":"387305","messageId":"eadb7c7e-5c8c-7e0b-2be0-ddc31d6ff7d4@iee.email","threadId":"52370","inReplyTo":"CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com","subject":"Re: git-rebase produces incorrect output","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2019-11-29T13:31:23Z","receivedAt":"2019-11-29T13:31:28Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 29/11/2019 08:21, Pavel Roskin wrote:\n> Hi!\n>\n> I've discovered an issue with \"git rebase\" producing a subtly\n> incorrect file. In fact, that files even compiled but failed in unit\n> tests! That's so scary that I'm going to stop using \"git rebase\" for\n> now. Fortunately, \"git rebase --merge\" is working correctly, so I'll\n> use it. Too bad there is no option to use \"--merge\" by default.\n>\n> The issue was observed in git 2.23 and reproduced in today's next\n> branch (2.24.0.449.g4c06f74957) on up-to-date Fedora 31 x86_64.\n>\n> I've created a repository that demonstrates the issue:\n> https://github.com/proski/git-rebase-demo\n>\n> The branch names should be self-explanatory. \"master\" is the base,\n> \"branch1\" and \"branch2\" contain one change each. If \"branch1\" is\n> rebased on top of \"branch2\", the result is incorrect, saved in the\n> \"rebase-bad\" branch. If \"git rebase -m\" is used, the result is\n> correct, saved in the \"merge-good\" branch.\n>\n> The files in \"rebase-bad\" and \"merge-good\" have exactly the same lines\n> but in a different order. Yet the changes on branch1 and branch2\n> affect non-overlapping parts of the file. There should be no doubt how\n> the merged code should look like.\n>\n> I believe the change on branch2 shifts the lines, so that the first\n> change from branch1 is applies to a place below the intended location,\n> and then git goes back to an earlier line to apply the next hunk. I\n> can imagine that it would do the right thing in case of swapped blocks\n> of code. Yet I have a real life example where it does a very wrong\n> thing.\n>\n> Indeed, \"git diff origin/branch2 origin/rebase-bad\" and \"git diff\n> origin/branch2 origin/merge-good\" both produce diffs of 9957 bytes\n> long, different only in the order of the hunks.\n>\n> Another interesting data point - \"git rebase --interactive\" is working\n> correctly.\n>\nWhich specific lines is this on?\n\nUsing the Github compare facility [1], I see multiple changes, some of \nwhich are probably just noise from the example.\n  https://github.com/proski/git-rebase-demo/compare/merge-good...rebase-bad\n\nPhilip\n\n[1] \nhttps://help.github.com/en/github/committing-changes-to-your-project/comparing-commits-across-time\n"},{"id":"387318","messageId":"CAN_72e3PLSyc_qXVztrSwKn+3tFA_x1TDkmA5Pi21KZvbb0ENQ@mail.gmail.com","threadId":"52370","inReplyTo":"eadb7c7e-5c8c-7e0b-2be0-ddc31d6ff7d4@iee.email","subject":"Re: git-rebase produces incorrect output","fromName":"Pavel Roskin","fromEmail":"plroskin@gmail.com","sentAt":"2019-11-29T21:28:52Z","receivedAt":"2019-11-29T21:29:07Z","isPatch":false,"sender":{"key":"plroskin@gmail.com","avatar":null},"body":"Hello Philip,\n\nI just found out that the true diff on Github should use two dots, not three:\nhttps://github.com/proski/git-rebase-demo/compare/merge-good..rebase-bad\n\nThe first difference is on line 187 in\ngetPullRequests_throws_on_not_found(). The correct change should have\n\"nullValue\" (see the diff between \"master\" and \"branch1\"), but\n\"rebase-bad\" has \"SocketException\" there instead.\n\n[Sorry for the private email that I sent by mistake]\n\nPavel\n"},{"id":"387326","messageId":"CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+h+t4Mn4A@mail.gmail.com","threadId":"52370","inReplyTo":"CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com","subject":"Re: git-rebase produces incorrect output","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-11-30T04:22:37Z","receivedAt":"2019-11-30T04:22:52Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Nov 29, 2019 at 12:24 AM Pavel Roskin <plroskin@gmail.com> wrote:\n>\n> Hi!\n>\n> I've discovered an issue with \"git rebase\" producing a subtly\n> incorrect file. In fact, that files even compiled but failed in unit\n> tests! That's so scary that I'm going to stop using \"git rebase\" for\n> now. Fortunately, \"git rebase --merge\" is working correctly, so I'll\n> use it. Too bad there is no option to use \"--merge\" by default.\n\nIndeed.  We really should fix that, if not just make it the default\nfor everyone.\n\n> The issue was observed in git 2.23 and reproduced in today's next\n> branch (2.24.0.449.g4c06f74957) on up-to-date Fedora 31 x86_64.\n>\n> I've created a repository that demonstrates the issue:\n> https://github.com/proski/git-rebase-demo\n>\n> The branch names should be self-explanatory. \"master\" is the base,\n> \"branch1\" and \"branch2\" contain one change each. If \"branch1\" is\n> rebased on top of \"branch2\", the result is incorrect, saved in the\n> \"rebase-bad\" branch. If \"git rebase -m\" is used, the result is\n> correct, saved in the \"merge-good\" branch.\n>\n> The files in \"rebase-bad\" and \"merge-good\" have exactly the same lines\n> but in a different order. Yet the changes on branch1 and branch2\n> affect non-overlapping parts of the file. There should be no doubt how\n> the merged code should look like.\n>\n> I believe the change on branch2 shifts the lines, so that the first\n> change from branch1 is applies to a place below the intended location,\n> and then git goes back to an earlier line to apply the next hunk. I\n> can imagine that it would do the right thing in case of swapped blocks\n> of code. Yet I have a real life example where it does a very wrong\n> thing.\n>\n> Indeed, \"git diff origin/branch2 origin/rebase-bad\" and \"git diff\n> origin/branch2 origin/merge-good\" both produce diffs of 9957 bytes\n> long, different only in the order of the hunks.\n>\n> Another interesting data point - \"git rebase --interactive\" is working\n> correctly.\n\nThanks for the detailed report and simple testcase.  Turns out the\n--interactive isn't so interesting, because a few cycles back we\nre-implemented the --merge behavior on top of the interactive\nmachinery so the two use the exact same engine.  Anyway, I can\nduplicate the problem and noticed a few interesting things.  Since the\nam-backend for rebase (the default) basically just uses diff and\napply, I tried duplicating with just those after looking at things and\nnoticing that it appeared to be applying patch hunks on the wrong\nlines:\n\n$ git switch branch2\n\n$ git reset --hard origin/branch2\nHEAD is now at 1331204 Change on branch 2\n$ git diff -U3 origin/master origin/branch1 >diff.patch\n$ git apply diff.patch\n$ git diff --shortstat origin/merge-good\n 1 file changed, 43 insertions(+), 43 deletions(-)\n$ git diff --shortstat origin/rebase-bad\n\nSo, this reproduces your bad results.  Let's repeat with -U4:\n\n$ git reset --hard origin/branch2\nHEAD is now at 1331204 Change on branch 2\n$ git diff -U4 origin/master origin/branch1 >diff.patch\n$ git apply diff.patch\n$ git diff --shortstat origin/merge-good\n 1 file changed, 10 insertions(+), 10 deletions(-)\n$ git diff --shortstat origin/rebase-bad\n 1 file changed, 37 insertions(+), 37 deletions(-)\n\nThat gives us a result that matches neither merge-good nor rebase-bad,\nbut is closer to the good side.  Let's try again with -U5:\n\n$ git reset --hard origin/branch2\nHEAD is now at 1331204 Change on branch 2\n$ git diff -U5 origin/master origin/branch1 >diff.patch\n$ git apply diff.patch\n$ git diff --shortstat origin/merge-good\n$ git diff --shortstat origin/rebase-bad\n 1 file changed, 43 insertions(+), 43 deletions(-)\n\nAhah!  With five lines of context, git diff & git apply can produce\nthe correct result.\n\nSadly, I tried to force this with git rebase, but -C5 only affected\nthe apply side and there's no option to pass to rebase to pass through\n-U5 to the diff logic.  Also, although there is a diff.context config\noption, git-am ignores it (Note that git_am_config() does not directly\ncheck that value and it calls git_default_config(), not\ngit_diff_ui_config() or even git_diff_basic_config()).  So, it's not\npossible to force the am-based rebase to get the right answer\ncurrently even if you figure out what the problem is.\n\nThe merge-based rebase, by contrast, essentially benfits from having\nthe entire files of each version accessible so it automatically gets\nit right.\n\n\nSo, to summarize here:\n  * you have a case where the default 3 lines of context mess stuff\nup; but rebase --merge works great\n  * am doesn't have a -U option, and ignores the diff.context setting,\nmaking it impossible to force the am backend to work on your case\nand also:\n  * rebase doesn't have an option to use the merge/interactive backend\nby default (nor an --am option to override it)\nAlso,\n  * The performance of the merge/interactive backend is slightly\nbetter than the am-backend\n(https://public-inbox.org/git/CABPp-BF=ev03WgODk6TMQmuNoatg2kiEe5DR__gJ0OTVqHSnfQ@mail.gmail.com/)\nand will be getting better\n  * The merge/interactive backend supports many more options than the\nam-backend, though the am one still has a few the merge backend\ndoesn't.  Once the ra/rebase-i-more-options topic merges, --whitespace\nwill be the only consequential option that the am-backend supports\nthat the merge/interactive-backend doesn't.  (There's also -C, but as\nnoted above, the merge/interactive backend already have access to the\nfull file).\n\nMaybe we should just switch the default, for everyone?  (And provide\nan --am option to override it and a config setting to get the old\ndefault?)\n"},{"id":"387335","messageId":"CABPp-BHPkwnbLR0xNaY3SphCpmR6QRA1j953c2hU7ZFpL38Nig@mail.gmail.com","threadId":"52370","inReplyTo":"CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+h+t4Mn4A@mail.gmail.com","subject":"Re: git-rebase produces incorrect output","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-11-30T16:37:54Z","receivedAt":"2019-11-30T16:38:08Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Nov 29, 2019 at 8:22 PM Elijah Newren <newren@gmail.com> wrote:\n> On Fri, Nov 29, 2019 at 12:24 AM Pavel Roskin <plroskin@gmail.com> wrote:\n\n> So, to summarize here:\n>   * you have a case where the default 3 lines of context mess stuff\n> up; but rebase --merge works great\n>   * am doesn't have a -U option, and ignores the diff.context setting,\n> making it impossible to force the am backend to work on your case\n>   * rebase doesn't have an option to use the merge/interactive backend\n> by default (nor an --am option to override it)\n>\n> Also:\n>   * The performance of the merge/interactive backend is slightly\n> better than the am-backend\n> (https://public-inbox.org/git/CABPp-BF=ev03WgODk6TMQmuNoatg2kiEe5DR__gJ0OTVqHSnfQ@mail.gmail.com/)\n> and will continue to get better\n>   * The merge/interactive backend supports many more options than the\n> am-backend, though the am one still has a few the merge backend\n> doesn't.  Once the ra/rebase-i-more-options topic merges, --whitespace\n> will be the only consequential option that the am-backend supports\n> that the merge/interactive-backend doesn't.  (There's also -C, but as\n> noted above, the merge/interactive backend already have access to the\n> full file).\n\nIn case it wasn't clear above, the merge/interactive backend could\njust accept the -C option and ignore it and do nothing, since it\nalready has access to the full file (thus why I consider the -C option\nto not be consequential).\n\nAlso, I remembered and dug out a few more items about the default\nrebase backend that might be worth including in this summary:\n  * The am backend operates with incomplete tree information as well,\nlimiting what the merge/resolve/whatever can do and what information\ncan be provided to the user (see\nhttps://public-inbox.org/git/xmqqh8jeh1id.fsf@gitster-ct.c.googlers.com/)\n  * The interactive backend, although slightly faster than the\nam-backend (on p3400 at least), is slightly slower with split-index\nwhich hasn't yet been investigated (see\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1901312310280.41@tvgsbejvaqbjf.bet/)\n\n> Maybe we should just switch the default, for everyone?  (And provide\n> an --am option to override it and a config setting to get the old\n> default?)\n\nCC'ing a few folks for opinions on switching the default backend of\nrebase from --am to --merge.  Johannes already agreed it was the right\npath eventually[1], and Junio suggested the am backend should be\ndeprecated[2] and eventually replaced, so I was going to push on this\nafter some merge performance work but perhaps it's a good time to\nquery if it's time to switch the default sooner.\n\n[1] See the end of\nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1808311158540.71@tvgsbejvaqbjf.bet/,\nalso linked above.\n[2] https://public-inbox.org/git/xmqqh8jeh1id.fsf@gitster-ct.c.googlers.com/,\nalso linked above.\n"},{"id":"387342","messageId":"xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com","threadId":"52370","inReplyTo":"CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+h+t4Mn4A@mail.gmail.com","subject":"Re: git-rebase produces incorrect output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-30T17:58:11Z","receivedAt":"2019-11-30T17:58:34Z","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> Sadly, I tried to force this with git rebase, but -C5 only affected\n> the apply side and there's no option to pass to rebase to pass through\n> -U5 to the diff logic.  Also, although there is a diff.context config\n> option, git-am ignores it (Note that git_am_config() does not directly\n> check that value and it calls git_default_config(), not\n> git_diff_ui_config() or even git_diff_basic_config()).\n\nNot essential but puzzled.  The context applies to the generation\nside, not acceptance side, no?  IOW, I suspect that you are talking\nabout \"git format-patch\" that sits on the upstream side of the pipe\nthat feeds \"git am\".\n\n> So, to summarize here:\n>   * you have a case where the default 3 lines of context mess stuff\n> up; but rebase --merge works great\n>   * am doesn't have a -U option, and ignores the diff.context setting,\n> making it impossible to force the am backend to work on your case\n> and also:\n\nI do not think it is super hard to teach \"git rebase\" to pass\nbackend specific options so that \"git rebase--am\" can be told to\nwork with wider context (which will reduce the risk of ambiguous\npatch like this example, trading the increased risk of unnecessary\nconflicts; it is a good trade-off most of the time for added safety,\nas nobody wants a system that produces a wrong result silently and\nquickly).\n\nHaving said that,\n\n>   * rebase doesn't have an option to use the merge/interactive backend\n> by default (nor an --am option to override it)\n\nI think addition of rebase.backend would be a good first step for\neventually flipping the default, which by the way I have no trouble\nwith.\n\n> Maybe we should just switch the default, for everyone?  (And provide\n> an --am option to override it and a config setting to get the old\n> default?)\n\nYes, that would be a sensible second step.  I actually think a\nlonger term goal is to deprecate the am backend.  It was invented\nfirst and then kept to be the default backend for a long time\nbecause the merge based backend historically has been noticeably\nslow (it was expected to be---it was essentially a shell script that\nrun cherry-pick repeatedly in a loop).  In some future, it would\noutlive its usefulness, and that I think that that future is just\naround the corner.\n\n\n"}]}