threads / bug / 54819

Bug report: git rebase ignores different context, resulting in subtle breakage

Subject: Bug report: git rebase ignores different context, resulting in subtle breakage

## tl;dr

6 messages between Dec 14, 2020 and Jan 6, 2021.

replies: 5people: 4as markdown or json

Michel Dänzer· Dec 14, 2020, 14:37 UTC · lore

(Originally reported as a GitLab issue: https://gitlab.com/gitlab-org/gitlab/-/issues/292754)

All output below is from Debian git 2.29.2-1.

The following branches of https://gitlab.freedesktop.org/daenzer/mesa.git can be used to reproduce:

gitlab-issue-292754/pre-rebase (pre-rebase state) gitlab-issue-292754/base (new base) gitlab-issue-292754/bad-rebase (bad post-rebase state)

The pre-rebase commit b9f18d0ddb6b075819bc2c6b9fa36dee483ef443 contains this (truncated) hunk:

@@ -480,13 +491,9 @@ sanity:

    rules:

      - if: *is-pre-merge

        when: on_success

-    - if: *is-forked-branch

-      when: manual

      # Other cases default to never

    script:

      # ci-fairy check-commits --junit-xml=check-commits.xml



The new base commit f20153536087079f39f1ab9995ac3d36dd3c467f contains 
this hunk:

@@ -484,10 +484,8 @@ sanity:

      - .fdo.ci-fairy

    stage: sanity

    rules:

-    - if: *is-pre-merge

+    - if: *is-forked-branch-or-pre-merge

        when: on_success

-    - if: *is-forked-branch

-      when: manual

      # Other cases default to never

    variables:

      GIT_STRATEGY: none



Both remove the same 2 lines, but the context is different both before 
and after those lines.

My expectation for this case would be that

  git rebase gitlab-issue-292754/base gitlab-issue-292754/pre-rebase

fails with a conflict. However, it succeeds without any indication of 
trouble, resulting in these contents in commit 
4e549e1ac3354f465d8afe0174902e62143a6ff4:

    rules:


     - if: *is-forked-branch-or-pre-merge


        when: on_success


      # Other cases default to never


    variables:


      GIT_STRATEGY: none

    script:


      # ci-fairy check-commits --junit-xml=check-commits.xml


I.e. identical to the new base.

However, the 2 removed lines had a different effect in the original 
pre-rebase context, and the post-rebase state no longer matches the 
original intention. git rebase silently introduced a subtle bug.


git rebase --apply results in this output:

  First, rewinding head to replay your work on top of it...

  Applying: ci: Run sanity job only in pre-merge pipelines

  Using index info to reconstruct a base tree...

  M	.gitlab-ci.yml

  Falling back to patching base and 3-way merge...

  Auto-merging .gitlab-ci.yml


Looks like the applying strategy does detect the conflict, but there's 
an automatic fallback to the merging strategy, which again succeeds 
(with the same broken result).


The only way I've found to avoid the broken behaviour is git rebase -s 
octopus [...]:

  error: could not apply b9f18d0ddb6... ci: Run sanity job only in pre
  merge pipelines

  Resolve all conflicts manually, mark them as resolved with

  "git add/rm <conflicted_files>", then run "git rebase --continue".

  You can instead skip this commit: run "git rebase --skip".

  To abort and get back to the state before "git rebase", run "git rebase
  --abort".

  Could not apply b9f18d0ddb6... ci: Run sanity job only in pre-merge
  pipelines

-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
Michel Dänzer· Jan 5, 2021, 09:32 UTC · re: Michel Dänzer · lore

Re: Bug report: git rebase ignores different context, resulting in subtle breakage

On 2020-12-14 3:37 p.m., Michel Dänzer wrote:
Show 14 quoted lines
> 
> (Originally reported as a GitLab issue: 
> https://gitlab.com/gitlab-org/gitlab/-/issues/292754)
> 
> 
> All output below is from Debian git 2.29.2-1.
> 
> 
> The following branches of 
> https://gitlab.freedesktop.org/daenzer/mesa.git can be used to reproduce:
> 
> gitlab-issue-292754/pre-rebase (pre-rebase state)
> gitlab-issue-292754/base (new base)
> gitlab-issue-292754/bad-rebase (bad post-rebase state)
Does the lack of response mean this is considered not a bug?
-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer
Eric Sunshine· Jan 5, 2021, 18:12 UTC · re: Michel Dänzer · lore

Re: Bug report: git rebase ignores different context, resulting in subtle breakage

On Tue, Jan 5, 2021 at 4:34 AM Michel Dänzer <michel@daenzer.net> wrote:
Show 5 quoted lines
> On 2020-12-14 3:37 p.m., Michel Dänzer wrote:
> > (Originally reported as a GitLab issue:
> > https://gitlab.com/gitlab-org/gitlab/-/issues/292754)
>
> Does the lack of response mean this is considered not a bug?

Perhaps the original slipped under the radar of the area experts. Adding Elijah to the Cc: list...

Elijah Newren· Jan 6, 2021, 16:17 UTC · re: Eric Sunshine · lore

Re: Bug report: git rebase ignores different context, resulting in subtle breakage

On Tue, Jan 5, 2021 at 10:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 10 quoted lines
>
> On Tue, Jan 5, 2021 at 4:34 AM Michel Dänzer <michel@daenzer.net> wrote:
> > On 2020-12-14 3:37 p.m., Michel Dänzer wrote:
> > > (Originally reported as a GitLab issue:
> > > https://gitlab.com/gitlab-org/gitlab/-/issues/292754)
> >
> > Does the lack of response mean this is considered not a bug?
>
> Perhaps the original slipped under the radar of the area experts.
> Adding Elijah to the Cc: list...

I'm not actually an expert with three-way content merging. That's the stuff handled by the xdiff library; I work on the stuff above that layer (covering things such as directory/file conflicts, rename detection, recursive merge base consolidation, not enough or too many files for three-way content merging, filetypes that can't be three-way content merged such as symlinks or submodules or binary files, etc.)

It looks like Phillip already answered here too, but you had more questions. Maybe I could try to elaborate on the answer a tiny bit. I did try out the testcase and noted that while "git apply" fails (unless you pass the --3way flag), a "git rebase --apply" succeeds due to usage of the --3way flag which falls back to a three-way merge instead of doing a patch application. "git rebase --merge" succeeds since it jumps straight to the three-way merge.

git-apply/git-am need three context lines to succeed, yes, but xdiff has a full copy of all three files it is merging. It does NOT apply patches. And if you line up the relevant sections of the three files, to me it looks like the relevant change made by gitlab-issue-292754/pre-rebase is a subset of the change made by gitlab-issue-292754/base. Yes, there's just one common unchanged line separating the two hunks in the second commit, which seems to be your point of contention, but it doesn't seem surprising to me at least. I think it's kind of similar to clean-textual-merge-but-semantic-conflict cases -- for example, one side adds a new caller of some function, and the other side changes the signature of that function. That will merge cleanly, but fail to even compile. The "context" is different, but the only way to correctly merge that is have a human (perhaps with the aid of an automated testsuite) take a look at the result. Such is always the case with merges.

However, that's just my gut take and as I said, I'm by no means an expert with three-way content merges. Maybe an xdiff expert could comment more.

Elijah
Phillip Wood· Jan 6, 2021, 15:40 UTC · re: Michel Dänzer · lore

Re: Bug report: git rebase ignores different context, resulting in subtle breakage

Hi Michel
Sorry for the slow response, this fell through the Christmas cracks.
On 14/12/2020 14:37, Michel Dänzer wrote:
Show 103 quoted lines
> 
> (Originally reported as a GitLab issue: 
> https://gitlab.com/gitlab-org/gitlab/-/issues/292754)
> 
> 
> All output below is from Debian git 2.29.2-1.
> 
> 
> The following branches of 
> https://gitlab.freedesktop.org/daenzer/mesa.git can be used to reproduce:
> 
> gitlab-issue-292754/pre-rebase (pre-rebase state)
> gitlab-issue-292754/base (new base)
> gitlab-issue-292754/bad-rebase (bad post-rebase state)
> 
> 
> The pre-rebase commit b9f18d0ddb6b075819bc2c6b9fa36dee483ef443 contains 
> this (truncated) hunk:
> 
> @@ -480,13 +491,9 @@ sanity:
> 
>     rules:
> 
>       - if: *is-pre-merge
> 
>         when: on_success
> 
> -    - if: *is-forked-branch
> 
> -      when: manual
> 
>       # Other cases default to never
> 
>     script:
> 
>       # ci-fairy check-commits --junit-xml=check-commits.xml
> 
> 
> 
> The new base commit f20153536087079f39f1ab9995ac3d36dd3c467f contains 
> this hunk:
> 
> @@ -484,10 +484,8 @@ sanity:
> 
>       - .fdo.ci-fairy
> 
>     stage: sanity
> 
>     rules:
> 
> -    - if: *is-pre-merge
> 
> +    - if: *is-forked-branch-or-pre-merge
> 
>         when: on_success
> 
> -    - if: *is-forked-branch
> 
> -      when: manual
> 
>       # Other cases default to never
> 
>     variables:
> 
>       GIT_STRATEGY: none
> 
> 
> 
> Both remove the same 2 lines, but the context is different both before 
> and after those lines.
> 
> My expectation for this case would be that
> 
>   git rebase gitlab-issue-292754/base gitlab-issue-292754/pre-rebase
> 
> fails with a conflict. However, it succeeds without any indication of 
> trouble, resulting in these contents in commit 
> 4e549e1ac3354f465d8afe0174902e62143a6ff4:
> 
>     rules:
> 
> 
>      - if: *is-forked-branch-or-pre-merge
> 
> 
>         when: on_success
> 
> 
>       # Other cases default to never
> 
> 
>     variables:
> 
> 
>       GIT_STRATEGY: none
> 
>     script:
> 
> 
>       # ci-fairy check-commits --junit-xml=check-commits.xml
> 
> 
> I.e. identical to the new base.

This looks like the correct merge to me as the changes in the original commit are already in the new base. Rebase has detected that the context lines do not match and used a 3-way merge instead of a simple patch application. This matches the behavior of the merge based rebase backend which is the default in recent versions of git. Git tracks content not changes and so rebasing a branch means cherry-picking each commit onto the rebased version of it's parent, not applying each patch on top of the new base.

Best Wishes
Phillip
Show 44 quoted lines
> However, the 2 removed lines had a different effect in the original 
> pre-rebase context, and the post-rebase state no longer matches the 
> original intention. git rebase silently introduced a subtle bug.
> 
> 
> git rebase --apply results in this output:
> 
>   First, rewinding head to replay your work on top of it...
> 
>   Applying: ci: Run sanity job only in pre-merge pipelines
> 
>   Using index info to reconstruct a base tree...
> 
>   M    .gitlab-ci.yml
> 
>   Falling back to patching base and 3-way merge...
> 
>   Auto-merging .gitlab-ci.yml
> 
> 
> Looks like the applying strategy does detect the conflict, but there's 
> an automatic fallback to the merging strategy, which again succeeds 
> (with the same broken result).
> 
> 
> The only way I've found to avoid the broken behaviour is git rebase -s 
> octopus [...]:
> 
>   error: could not apply b9f18d0ddb6... ci: Run sanity job only in pre
>   merge pipelines
> 
>   Resolve all conflicts manually, mark them as resolved with
> 
>   "git add/rm <conflicted_files>", then run "git rebase --continue".
> 
>   You can instead skip this commit: run "git rebase --skip".
> 
>   To abort and get back to the state before "git rebase", run "git rebase
>   --abort".
> 
>   Could not apply b9f18d0ddb6... ci: Run sanity job only in pre-merge
>   pipelines
> 
>
Michel Dänzer· Jan 6, 2021, 16:04 UTC · re: Phillip Wood · lore

Re: Bug report: git rebase ignores different context, resulting in subtle breakage

On 2021-01-06 4:40 p.m., Phillip Wood wrote:
Show 111 quoted lines
> Hi Michel
> 
> Sorry for the slow response, this fell through the Christmas cracks.
> 
> On 14/12/2020 14:37, Michel Dänzer wrote:
>>
>> (Originally reported as a GitLab issue: 
>> https://gitlab.com/gitlab-org/gitlab/-/issues/292754)
>>
>>
>> All output below is from Debian git 2.29.2-1.
>>
>>
>> The following branches of 
>> https://gitlab.freedesktop.org/daenzer/mesa.git can be used to reproduce:
>>
>> gitlab-issue-292754/pre-rebase (pre-rebase state)
>> gitlab-issue-292754/base (new base)
>> gitlab-issue-292754/bad-rebase (bad post-rebase state)
>>
>>
>> The pre-rebase commit b9f18d0ddb6b075819bc2c6b9fa36dee483ef443 
>> contains this (truncated) hunk:
>>
>> @@ -480,13 +491,9 @@ sanity:
>>
>>     rules:
>>
>>       - if: *is-pre-merge
>>
>>         when: on_success
>>
>> -    - if: *is-forked-branch
>>
>> -      when: manual
>>
>>       # Other cases default to never
>>
>>     script:
>>
>>       # ci-fairy check-commits --junit-xml=check-commits.xml
>>
>>
>>
>> The new base commit f20153536087079f39f1ab9995ac3d36dd3c467f contains 
>> this hunk:
>>
>> @@ -484,10 +484,8 @@ sanity:
>>
>>       - .fdo.ci-fairy
>>
>>     stage: sanity
>>
>>     rules:
>>
>> -    - if: *is-pre-merge
>>
>> +    - if: *is-forked-branch-or-pre-merge
>>
>>         when: on_success
>>
>> -    - if: *is-forked-branch
>>
>> -      when: manual
>>
>>       # Other cases default to never
>>
>>     variables:
>>
>>       GIT_STRATEGY: none
>>
>>
>>
>> Both remove the same 2 lines, but the context is different both before 
>> and after those lines.
>>
>> My expectation for this case would be that
>>
>>   git rebase gitlab-issue-292754/base gitlab-issue-292754/pre-rebase
>>
>> fails with a conflict. However, it succeeds without any indication of 
>> trouble, resulting in these contents in commit 
>> 4e549e1ac3354f465d8afe0174902e62143a6ff4:
>>
>>     rules:
>>
>>
>>      - if: *is-forked-branch-or-pre-merge
>>
>>
>>         when: on_success
>>
>>
>>       # Other cases default to never
>>
>>
>>     variables:
>>
>>
>>       GIT_STRATEGY: none
>>
>>     script:
>>
>>
>>       # ci-fairy check-commits --junit-xml=check-commits.xml
>>
>>
>> I.e. identical to the new base.
> 
> This looks like the correct merge to me as the changes in the original 
> commit are already in the new base.

Are you referring to the same two lines which have been removed already? Since the context is different, removing those two lines has a different effect than intended.

Show 6 quoted lines
> Rebase has detected that the context lines do not match and used a
> 3-way merge instead of a simple patch application. This matches the
> behavior of the merge based rebase backend which is the default in
> recent versions of git. Git tracks content not changes and so rebasing
> a branch means cherry-picking each commit onto the rebased version of
> it's parent, not applying each patch on top of the new base.

There is a single commit that needs rebasing, and the context in the new base commit is different from what it's based on. To me this seems like an obvious conflict.

-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer

← back to recent threads