{"thread":{"id":"54819","subject":"Bug report: git rebase ignores different context, resulting in subtle breakage","startedAt":"2020-12-14T14:45:33Z","lastAt":"2021-01-06T16:18:41Z","messageCount":6,"participants":["Michel Dänzer","Eric Sunshine","Phillip Wood","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"412168","messageId":"c22ba034-6d7d-866a-c6fb-d769d117eec4@daenzer.net","threadId":"54819","inReplyTo":null,"subject":"Bug report: git rebase ignores different context, resulting in subtle breakage","fromName":"Michel Dänzer","fromEmail":"michel@daenzer.net","sentAt":"2020-12-14T14:37:04Z","receivedAt":"2020-12-14T14:45:33Z","isPatch":false,"sender":{"key":"michel@daenzer.net","avatar":"https://gravatar.com/avatar/358817b59a47958c7db534f36c79395472809a74179d8a9040f256ea75d1be5e?d=mp&s=160"},"body":"\n(Originally reported as a GitLab issue: \nhttps://gitlab.com/gitlab-org/gitlab/-/issues/292754)\n\n\nAll output below is from Debian git 2.29.2-1.\n\n\nThe following branches of \nhttps://gitlab.freedesktop.org/daenzer/mesa.git can be used to reproduce:\n\ngitlab-issue-292754/pre-rebase (pre-rebase state)\ngitlab-issue-292754/base (new base)\ngitlab-issue-292754/bad-rebase (bad post-rebase state)\n\n\nThe pre-rebase commit b9f18d0ddb6b075819bc2c6b9fa36dee483ef443 contains \nthis (truncated) hunk:\n\n@@ -480,13 +491,9 @@ sanity:\n\n    rules:\n\n      - if: *is-pre-merge\n\n        when: on_success\n\n-    - if: *is-forked-branch\n\n-      when: manual\n\n      # Other cases default to never\n\n    script:\n\n      # ci-fairy check-commits --junit-xml=check-commits.xml\n\n\n\nThe new base commit f20153536087079f39f1ab9995ac3d36dd3c467f contains \nthis hunk:\n\n@@ -484,10 +484,8 @@ sanity:\n\n      - .fdo.ci-fairy\n\n    stage: sanity\n\n    rules:\n\n-    - if: *is-pre-merge\n\n+    - if: *is-forked-branch-or-pre-merge\n\n        when: on_success\n\n-    - if: *is-forked-branch\n\n-      when: manual\n\n      # Other cases default to never\n\n    variables:\n\n      GIT_STRATEGY: none\n\n\n\nBoth remove the same 2 lines, but the context is different both before \nand after those lines.\n\nMy expectation for this case would be that\n\n  git rebase gitlab-issue-292754/base gitlab-issue-292754/pre-rebase\n\nfails with a conflict. However, it succeeds without any indication of \ntrouble, resulting in these contents in commit \n4e549e1ac3354f465d8afe0174902e62143a6ff4:\n\n    rules:\n\n\n     - if: *is-forked-branch-or-pre-merge\n\n\n        when: on_success\n\n\n      # Other cases default to never\n\n\n    variables:\n\n\n      GIT_STRATEGY: none\n\n    script:\n\n\n      # ci-fairy check-commits --junit-xml=check-commits.xml\n\n\nI.e. identical to the new base.\n\nHowever, the 2 removed lines had a different effect in the original \npre-rebase context, and the post-rebase state no longer matches the \noriginal intention. git rebase silently introduced a subtle bug.\n\n\ngit rebase --apply results in this output:\n\n  First, rewinding head to replay your work on top of it...\n\n  Applying: ci: Run sanity job only in pre-merge pipelines\n\n  Using index info to reconstruct a base tree...\n\n  M\t.gitlab-ci.yml\n\n  Falling back to patching base and 3-way merge...\n\n  Auto-merging .gitlab-ci.yml\n\n\nLooks like the applying strategy does detect the conflict, but there's \nan automatic fallback to the merging strategy, which again succeeds \n(with the same broken result).\n\n\nThe only way I've found to avoid the broken behaviour is git rebase -s \noctopus [...]:\n\n  error: could not apply b9f18d0ddb6... ci: Run sanity job only in pre\n  merge pipelines\n\n  Resolve all conflicts manually, mark them as resolved with\n\n  \"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n\n  You can instead skip this commit: run \"git rebase --skip\".\n\n  To abort and get back to the state before \"git rebase\", run \"git rebase\n  --abort\".\n\n  Could not apply b9f18d0ddb6... ci: Run sanity job only in pre-merge\n  pipelines\n\n\n-- \nEarthling Michel Dänzer               |               https://redhat.com\nLibre software enthusiast             |             Mesa and X developer\n"},{"id":"413439","messageId":"34b425b5-0237-fb46-5613-e90346bd7114@daenzer.net","threadId":"54819","inReplyTo":"c22ba034-6d7d-866a-c6fb-d769d117eec4@daenzer.net","subject":"Re: Bug report: git rebase ignores different context, resulting in subtle breakage","fromName":"Michel Dänzer","fromEmail":"michel@daenzer.net","sentAt":"2021-01-05T09:32:50Z","receivedAt":"2021-01-05T09:33:36Z","isPatch":false,"sender":{"key":"michel@daenzer.net","avatar":"https://gravatar.com/avatar/358817b59a47958c7db534f36c79395472809a74179d8a9040f256ea75d1be5e?d=mp&s=160"},"body":"On 2020-12-14 3:37 p.m., Michel Dänzer wrote:\n> \n> (Originally reported as a GitLab issue: \n> https://gitlab.com/gitlab-org/gitlab/-/issues/292754)\n> \n> \n> All output below is from Debian git 2.29.2-1.\n> \n> \n> The following branches of \n> https://gitlab.freedesktop.org/daenzer/mesa.git can be used to reproduce:\n> \n> gitlab-issue-292754/pre-rebase (pre-rebase state)\n> gitlab-issue-292754/base (new base)\n> gitlab-issue-292754/bad-rebase (bad post-rebase state)\n\nDoes the lack of response mean this is considered not a bug?\n\n\n-- \nEarthling Michel Dänzer               |               https://redhat.com\nLibre software enthusiast             |             Mesa and X developer\n"},{"id":"413474","messageId":"CAPig+cSe267W7hO8a38ifCw_r4eabUA6WB9MDhS=GttTxELpVg@mail.gmail.com","threadId":"54819","inReplyTo":"34b425b5-0237-fb46-5613-e90346bd7114@daenzer.net","subject":"Re: Bug report: git rebase ignores different context, resulting in subtle breakage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-01-05T18:12:06Z","receivedAt":"2021-01-05T18:13:00Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 5, 2021 at 4:34 AM Michel Dänzer <michel@daenzer.net> wrote:\n> On 2020-12-14 3:37 p.m., Michel Dänzer wrote:\n> > (Originally reported as a GitLab issue:\n> > https://gitlab.com/gitlab-org/gitlab/-/issues/292754)\n>\n> Does the lack of response mean this is considered not a bug?\n\nPerhaps the original slipped under the radar of the area experts.\nAdding Elijah to the Cc: list...\n"},{"id":"413585","messageId":"502b710e-6347-5fa7-0461-21a84ae2250d@gmail.com","threadId":"54819","inReplyTo":"c22ba034-6d7d-866a-c6fb-d769d117eec4@daenzer.net","subject":"Re: Bug report: git rebase ignores different context, resulting in subtle breakage","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-01-06T15:40:07Z","receivedAt":"2021-01-06T15:40:53Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michel\n\nSorry for the slow response, this fell through the Christmas cracks.\n\nOn 14/12/2020 14:37, Michel Dänzer wrote:\n> \n> (Originally reported as a GitLab issue: \n> https://gitlab.com/gitlab-org/gitlab/-/issues/292754)\n> \n> \n> All output below is from Debian git 2.29.2-1.\n> \n> \n> The following branches of \n> https://gitlab.freedesktop.org/daenzer/mesa.git can be used to reproduce:\n> \n> gitlab-issue-292754/pre-rebase (pre-rebase state)\n> gitlab-issue-292754/base (new base)\n> gitlab-issue-292754/bad-rebase (bad post-rebase state)\n> \n> \n> The pre-rebase commit b9f18d0ddb6b075819bc2c6b9fa36dee483ef443 contains \n> this (truncated) hunk:\n> \n> @@ -480,13 +491,9 @@ sanity:\n> \n>     rules:\n> \n>       - if: *is-pre-merge\n> \n>         when: on_success\n> \n> -    - if: *is-forked-branch\n> \n> -      when: manual\n> \n>       # Other cases default to never\n> \n>     script:\n> \n>       # ci-fairy check-commits --junit-xml=check-commits.xml\n> \n> \n> \n> The new base commit f20153536087079f39f1ab9995ac3d36dd3c467f contains \n> this hunk:\n> \n> @@ -484,10 +484,8 @@ sanity:\n> \n>       - .fdo.ci-fairy\n> \n>     stage: sanity\n> \n>     rules:\n> \n> -    - if: *is-pre-merge\n> \n> +    - if: *is-forked-branch-or-pre-merge\n> \n>         when: on_success\n> \n> -    - if: *is-forked-branch\n> \n> -      when: manual\n> \n>       # Other cases default to never\n> \n>     variables:\n> \n>       GIT_STRATEGY: none\n> \n> \n> \n> Both remove the same 2 lines, but the context is different both before \n> and after those lines.\n> \n> My expectation for this case would be that\n> \n>   git rebase gitlab-issue-292754/base gitlab-issue-292754/pre-rebase\n> \n> fails with a conflict. However, it succeeds without any indication of \n> trouble, resulting in these contents in commit \n> 4e549e1ac3354f465d8afe0174902e62143a6ff4:\n> \n>     rules:\n> \n> \n>      - if: *is-forked-branch-or-pre-merge\n> \n> \n>         when: on_success\n> \n> \n>       # Other cases default to never\n> \n> \n>     variables:\n> \n> \n>       GIT_STRATEGY: none\n> \n>     script:\n> \n> \n>       # ci-fairy check-commits --junit-xml=check-commits.xml\n> \n> \n> I.e. identical to the new base.\n\nThis looks like the correct merge to me as the changes in the original \ncommit are already in the new base. Rebase has detected that the context \nlines do not match and used a 3-way merge instead of a simple patch \napplication. This matches the behavior of the merge based rebase backend \nwhich is the default in recent versions of git. Git tracks content not \nchanges and so rebasing a branch means cherry-picking each commit onto \nthe rebased version of it's parent, not applying each patch on top of \nthe new base.\n\nBest Wishes\n\nPhillip\n\n\n> However, the 2 removed lines had a different effect in the original \n> pre-rebase context, and the post-rebase state no longer matches the \n> original intention. git rebase silently introduced a subtle bug.\n> \n> \n> git rebase --apply results in this output:\n> \n>   First, rewinding head to replay your work on top of it...\n> \n>   Applying: ci: Run sanity job only in pre-merge pipelines\n> \n>   Using index info to reconstruct a base tree...\n> \n>   M    .gitlab-ci.yml\n> \n>   Falling back to patching base and 3-way merge...\n> \n>   Auto-merging .gitlab-ci.yml\n> \n> \n> Looks like the applying strategy does detect the conflict, but there's \n> an automatic fallback to the merging strategy, which again succeeds \n> (with the same broken result).\n> \n> \n> The only way I've found to avoid the broken behaviour is git rebase -s \n> octopus [...]:\n> \n>   error: could not apply b9f18d0ddb6... ci: Run sanity job only in pre\n>   merge pipelines\n> \n>   Resolve all conflicts manually, mark them as resolved with\n> \n>   \"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n> \n>   You can instead skip this commit: run \"git rebase --skip\".\n> \n>   To abort and get back to the state before \"git rebase\", run \"git rebase\n>   --abort\".\n> \n>   Could not apply b9f18d0ddb6... ci: Run sanity job only in pre-merge\n>   pipelines\n> \n> \n"},{"id":"413587","messageId":"8b4da09f-cc0f-8ea4-1b65-95a7fa9858d0@daenzer.net","threadId":"54819","inReplyTo":"502b710e-6347-5fa7-0461-21a84ae2250d@gmail.com","subject":"Re: Bug report: git rebase ignores different context, resulting in subtle breakage","fromName":"Michel Dänzer","fromEmail":"michel@daenzer.net","sentAt":"2021-01-06T16:04:05Z","receivedAt":"2021-01-06T16:05:08Z","isPatch":false,"sender":{"key":"michel@daenzer.net","avatar":"https://gravatar.com/avatar/358817b59a47958c7db534f36c79395472809a74179d8a9040f256ea75d1be5e?d=mp&s=160"},"body":"On 2021-01-06 4:40 p.m., Phillip Wood wrote:\n> Hi Michel\n> \n> Sorry for the slow response, this fell through the Christmas cracks.\n> \n> On 14/12/2020 14:37, Michel Dänzer wrote:\n>>\n>> (Originally reported as a GitLab issue: \n>> https://gitlab.com/gitlab-org/gitlab/-/issues/292754)\n>>\n>>\n>> All output below is from Debian git 2.29.2-1.\n>>\n>>\n>> The following branches of \n>> https://gitlab.freedesktop.org/daenzer/mesa.git can be used to reproduce:\n>>\n>> gitlab-issue-292754/pre-rebase (pre-rebase state)\n>> gitlab-issue-292754/base (new base)\n>> gitlab-issue-292754/bad-rebase (bad post-rebase state)\n>>\n>>\n>> The pre-rebase commit b9f18d0ddb6b075819bc2c6b9fa36dee483ef443 \n>> contains this (truncated) hunk:\n>>\n>> @@ -480,13 +491,9 @@ sanity:\n>>\n>>     rules:\n>>\n>>       - if: *is-pre-merge\n>>\n>>         when: on_success\n>>\n>> -    - if: *is-forked-branch\n>>\n>> -      when: manual\n>>\n>>       # Other cases default to never\n>>\n>>     script:\n>>\n>>       # ci-fairy check-commits --junit-xml=check-commits.xml\n>>\n>>\n>>\n>> The new base commit f20153536087079f39f1ab9995ac3d36dd3c467f contains \n>> this hunk:\n>>\n>> @@ -484,10 +484,8 @@ sanity:\n>>\n>>       - .fdo.ci-fairy\n>>\n>>     stage: sanity\n>>\n>>     rules:\n>>\n>> -    - if: *is-pre-merge\n>>\n>> +    - if: *is-forked-branch-or-pre-merge\n>>\n>>         when: on_success\n>>\n>> -    - if: *is-forked-branch\n>>\n>> -      when: manual\n>>\n>>       # Other cases default to never\n>>\n>>     variables:\n>>\n>>       GIT_STRATEGY: none\n>>\n>>\n>>\n>> Both remove the same 2 lines, but the context is different both before \n>> and after those lines.\n>>\n>> My expectation for this case would be that\n>>\n>>   git rebase gitlab-issue-292754/base gitlab-issue-292754/pre-rebase\n>>\n>> fails with a conflict. However, it succeeds without any indication of \n>> trouble, resulting in these contents in commit \n>> 4e549e1ac3354f465d8afe0174902e62143a6ff4:\n>>\n>>     rules:\n>>\n>>\n>>      - if: *is-forked-branch-or-pre-merge\n>>\n>>\n>>         when: on_success\n>>\n>>\n>>       # Other cases default to never\n>>\n>>\n>>     variables:\n>>\n>>\n>>       GIT_STRATEGY: none\n>>\n>>     script:\n>>\n>>\n>>       # ci-fairy check-commits --junit-xml=check-commits.xml\n>>\n>>\n>> I.e. identical to the new base.\n> \n> This looks like the correct merge to me as the changes in the original \n> commit are already in the new base.\n\nAre you referring to the same two lines which have been removed already? \nSince the context is different, removing those two lines has a different \neffect than intended.\n\n\n> Rebase has detected that the context lines do not match and used a\n> 3-way merge instead of a simple patch application. This matches the\n> behavior of the merge based rebase backend which is the default in\n> recent versions of git. Git tracks content not changes and so rebasing\n> a branch means cherry-picking each commit onto the rebased version of\n> it's parent, not applying each patch on top of the new base.\n\nThere is a single commit that needs rebasing, and the context in the new \nbase commit is different from what it's based on. To me this seems like \nan obvious conflict.\n\n\n-- \nEarthling Michel Dänzer               |               https://redhat.com\nLibre software enthusiast             |             Mesa and X developer\n"},{"id":"413589","messageId":"CABPp-BEOXgyq-JtjEkTkPe1sbW5e6hwwAgHc88n09PtAbZBBYQ@mail.gmail.com","threadId":"54819","inReplyTo":"CAPig+cSe267W7hO8a38ifCw_r4eabUA6WB9MDhS=GttTxELpVg@mail.gmail.com","subject":"Re: Bug report: git rebase ignores different context, resulting in subtle breakage","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-01-06T16:17:36Z","receivedAt":"2021-01-06T16:18:41Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jan 5, 2021 at 10:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Tue, Jan 5, 2021 at 4:34 AM Michel Dänzer <michel@daenzer.net> wrote:\n> > On 2020-12-14 3:37 p.m., Michel Dänzer wrote:\n> > > (Originally reported as a GitLab issue:\n> > > https://gitlab.com/gitlab-org/gitlab/-/issues/292754)\n> >\n> > Does the lack of response mean this is considered not a bug?\n>\n> Perhaps the original slipped under the radar of the area experts.\n> Adding Elijah to the Cc: list...\n\nI'm not actually an expert with three-way content merging.  That's the\nstuff handled by the xdiff library; I work on the stuff above that\nlayer (covering things such as directory/file conflicts, rename\ndetection, recursive merge base consolidation, not enough or too many\nfiles for three-way content merging, filetypes that can't be three-way\ncontent merged such as symlinks or submodules or binary files, etc.)\n\nIt looks like Phillip already answered here too, but you had more\nquestions.  Maybe I could try to elaborate on the answer a tiny bit.\nI did try out the testcase and noted that while \"git apply\" fails\n(unless you pass the --3way flag), a \"git rebase --apply\" succeeds due\nto usage of the --3way flag which falls back to a three-way merge\ninstead of doing a patch application.  \"git rebase --merge\" succeeds\nsince it jumps straight to the three-way merge.\n\ngit-apply/git-am need three context lines to succeed, yes, but xdiff\nhas a full copy of all three files it is merging.  It does NOT apply\npatches.  And if you line up the relevant sections of the three files,\nto me it looks like the relevant change made by\ngitlab-issue-292754/pre-rebase is a subset of the change made by\ngitlab-issue-292754/base.  Yes, there's just one common unchanged line\nseparating the two hunks in the second commit, which seems to be your\npoint of contention, but it doesn't seem surprising to me at least.  I\nthink it's kind of similar to\nclean-textual-merge-but-semantic-conflict cases -- for example, one\nside adds a new caller of some function, and the other side changes\nthe signature of that function.  That will merge cleanly, but fail to\neven compile.  The \"context\" is different, but the only way to\ncorrectly merge that is have a human (perhaps with the aid of an\nautomated testsuite) take a look at the result.  Such is always the\ncase with merges.\n\nHowever, that's just my gut take and as I said, I'm by no means an\nexpert with three-way content merges.  Maybe an xdiff expert could\ncomment more.\n\nElijah\n"}]}