* Bug report: git rebase ignores different context, resulting in subtle breakage
@ 2020-12-14 14:37 Michel Dänzer
2021-01-05 9:32 ` Michel Dänzer
2021-01-06 15:40 ` Phillip Wood
0 siblings, 2 replies; 6+ messages in thread
From: Michel Dänzer @ 2020-12-14 14:37 UTC (permalink / raw)
To: git; +Cc: Daniel Stone
(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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Bug report: git rebase ignores different context, resulting in subtle breakage
2020-12-14 14:37 Bug report: git rebase ignores different context, resulting in subtle breakage Michel Dänzer
@ 2021-01-05 9:32 ` Michel Dänzer
2021-01-05 18:12 ` Eric Sunshine
2021-01-06 15:40 ` Phillip Wood
1 sibling, 1 reply; 6+ messages in thread
From: Michel Dänzer @ 2021-01-05 9:32 UTC (permalink / raw)
To: git; +Cc: Daniel Stone
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)
>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug report: git rebase ignores different context, resulting in subtle breakage
2021-01-05 9:32 ` Michel Dänzer
@ 2021-01-05 18:12 ` Eric Sunshine
2021-01-06 16:17 ` Elijah Newren
0 siblings, 1 reply; 6+ messages in thread
From: Eric Sunshine @ 2021-01-05 18:12 UTC (permalink / raw)
To: Michel Dänzer; +Cc: Git List, Daniel Stone, Elijah Newren
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...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug report: git rebase ignores different context, resulting in subtle breakage
2021-01-05 18:12 ` Eric Sunshine
@ 2021-01-06 16:17 ` Elijah Newren
0 siblings, 0 replies; 6+ messages in thread
From: Elijah Newren @ 2021-01-06 16:17 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Michel Dänzer, Git List, Daniel Stone
On Tue, Jan 5, 2021 at 10:12 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug report: git rebase ignores different context, resulting in subtle breakage
2020-12-14 14:37 Bug report: git rebase ignores different context, resulting in subtle breakage Michel Dänzer
2021-01-05 9:32 ` Michel Dänzer
@ 2021-01-06 15:40 ` Phillip Wood
2021-01-06 16:04 ` Michel Dänzer
1 sibling, 1 reply; 6+ messages in thread
From: Phillip Wood @ 2021-01-06 15:40 UTC (permalink / raw)
To: Michel Dänzer, git; +Cc: Daniel Stone
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. 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
> 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Bug report: git rebase ignores different context, resulting in subtle breakage
2021-01-06 15:40 ` Phillip Wood
@ 2021-01-06 16:04 ` Michel Dänzer
0 siblings, 0 replies; 6+ messages in thread
From: Michel Dänzer @ 2021-01-06 16:04 UTC (permalink / raw)
To: phillip.wood, git; +Cc: Daniel Stone
On 2021-01-06 4:40 p.m., Phillip Wood wrote:
> 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.
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-06 16:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-14 14:37 Bug report: git rebase ignores different context, resulting in subtle breakage Michel Dänzer
2021-01-05 9:32 ` Michel Dänzer
2021-01-05 18:12 ` Eric Sunshine
2021-01-06 16:17 ` Elijah Newren
2021-01-06 15:40 ` Phillip Wood
2021-01-06 16:04 ` Michel Dänzer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).