This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7824

tests: add test of rebase with conflict in merge commit
ClosedPublic

Authored by martinvonz on Jan 11 2020, 1:47 AM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG7f7c8521e9bd: tests: add test of rebase with conflict in merge commit
Summary

It doesn't seem like we had any tests of this. I think it's pretty
weird that the two parents we're merging are not the working copy
parents during the conflict resolution.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Jan 11 2020, 1:47 AM
pulkit added a subscriber: pulkit.Jan 13 2020, 7:48 AM
pulkit added inline comments.
tests/test-rebase-conflicts.t
460

I think it will be weird to have current parents as 7 and 8. Rebase copies commit, so if there is a merge commit, it copies it instead of recreating one. Also if we have 7 and 8 as parents, we might not have correct conflicts.

martinvonz added inline comments.Jan 13 2020, 1:16 PM
tests/test-rebase-conflicts.t
460

I made more arguments for changing it in the next patch (D7827). See if those can convince you :)

I'm not sure what you mean about not having correct conflicts. Related to that, I think it's actually weird (with how it's currently done) how you would not see any changes made between the merge base and the rebase base. So if there had been commits between A and C here and we were rebasing from C, we would not see those changes in the working copy. That's of course correct, but the dirstate's two parents suggest, IMO, that we're merging the two commits, so we should see those changes.

I can see the value in having the thing you're grafting in as a working copy parent because it makes it appear in the hg log output as a reminder. However, I don't think that's what the purpose of the dirstate parent should be. We could perhaps add another mechanism for showing the commit that's being rebased, and maybe also the base (which is generally not the merge base).

martinvonz updated this revision to Diff 19859.Feb 3 2020, 1:08 PM
martinvonz marked an inline comment as done.Feb 3 2020, 1:09 PM
martinvonz added inline comments.
tests/test-rebase-conflicts.t
460

I've updated the comment to not be opinionated. I hope we can now queue this patch (and a few more in this series) regardless of what we think of D7827.

durin42 accepted this revision.Feb 10 2020, 9:15 PM
This revision is now accepted and ready to land.Feb 10 2020, 9:15 PM

Revision Contents
Changeset List

PathPackages
Mtests/test-rebase-conflicts.t (70 lines)

Diff 20132

tests/test-rebase-conflicts.t

@ 3:draft 'E' @ 3:draft 'E'
| |
| o 2:draft 'C' | o 2:draft 'C'
| | | |
| o 1:draft 'B' | o 1:draft 'B'
|/ |/
o 0:draft 'A' o 0:draft 'A'
Test where the conflict happens when rebasing a merge commit
$ cd $TESTTMP
$ hg init conflict-in-merge
$ cd conflict-in-merge
$ hg debugdrawdag <<'EOS'
> F # F/conflict = foo\n
> |\
> D E
> |/
> C B # B/conflict = bar\n
> |/
> A
> EOS
$ hg co F
5 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg rebase -d B
rebasing 2:dc0947a82db8 "C" (C)
rebasing 3:e7b3f00ed42e "D" (D)
rebasing 4:03ca77807e91 "E" (E)
rebasing 5:9a6b91dc2044 "F" (F tip)
merging conflict
warning: conflicts while merging conflict! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
[1]
The current parents are not 7 and 8 even though that's what we're merging
$ hg tglog
pulkitUnsubmitted
Done

I think it will be weird to have current parents as 7 and 8. Rebase copies commit, so if there is a merge commit, it copies it instead of recreating one. Also if we have 7 and 8 as parents, we might not have correct conflicts.

pulkit: I think it will be weird to have current parents as 7 and 8. Rebase copies commit, so if there…
martinvonzAuthorUnsubmitted
Done

I made more arguments for changing it in the next patch (D7827). See if those can convince you :)

I'm not sure what you mean about not having correct conflicts. Related to that, I think it's actually weird (with how it's currently done) how you would not see any changes made between the merge base and the rebase base. So if there had been commits between A and C here and we were rebasing from C, we would not see those changes in the working copy. That's of course correct, but the dirstate's two parents suggest, IMO, that we're merging the two commits, so we should see those changes.

I can see the value in having the thing you're grafting in as a working copy parent because it makes it appear in the hg log output as a reminder. However, I don't think that's what the purpose of the dirstate parent should be. We could perhaps add another mechanism for showing the commit that's being rebased, and maybe also the base (which is generally not the merge base).

martinvonz: I made more arguments for changing it in the next patch (D7827). See if those can convince you…
martinvonzAuthorUnsubmitted
Done

I've updated the comment to not be opinionated. I hope we can now queue this patch (and a few more in this series) regardless of what we think of D7827.

martinvonz: I've updated the comment to not be opinionated. I hope we can now queue this patch (and a few…
@ 8:draft 'E'
|
| o 7:draft 'D'
|/
o 6:draft 'C'
|
| @ 5:draft 'F'
| |\
| | o 4:draft 'E'
| | |
| o | 3:draft 'D'
| |/
| o 2:draft 'C'
| |
o | 1:draft 'B'
|/
o 0:draft 'A'
$ echo baz > conflict
$ hg resolve -m
(no more unresolved files)
continue: hg rebase --continue
$ hg rebase -c
already rebased 2:dc0947a82db8 "C" (C) as 0199610c343e
already rebased 3:e7b3f00ed42e "D" (D) as f0dd538aaa63
already rebased 4:03ca77807e91 "E" (E) as cbf25af8347d
rebasing 5:9a6b91dc2044 "F" (F)
saved backup bundle to $TESTTMP/conflict-in-merge/.hg/strip-backup/dc0947a82db8-ca7e7d5b-rebase.hg
$ hg tglog
@ 5:draft 'F'
|\
| o 4:draft 'E'
| |
o | 3:draft 'D'
|/
o 2:draft 'C'
|
o 1:draft 'B'
|
o 0:draft 'A'