-
Notifications
You must be signed in to change notification settings - Fork 746
conflicts: add labels to conflicts #7692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
68c536e
to
df41bd5
Compare
Note that this PR seems to implement it a bit differently than what I suggested on #1176 . My suggestion there was to record a label associated with each diff in the conflict, not with each term. I don't know which is better, and I haven't reviewed the code yet (only at a high enough level to notice that). One advantage of attaching a label to a diff is that it can also make sense when the involved trees don't come directly from commits. |
df41bd5
to
861033d
Compare
Oh yeah, I may have misunderstood. I think I was implementing it based on a Discord conversation we had a few weeks ago. I think it may actually be better to record labels per term rather than per diff because it seems to me like it would work better with the current simplification logic. Sometimes simplifying conflicts causes two diffs to be combined into a single diff, and it might be difficult to combine the labels when this happens. For instance, if you rebase a commit with parent A onto commit B, it might get a conflict label indicating "diff from A to B". If you then rebase it from commit B to commit C, I think ideally the label should indicate "diff from A to C". With term labels, this happens automatically, since the two terms with label "B" are discarded when simplifying. With diff labels, I think we'd either need some non-trivial logic to merge labels, or we'd need to settle for something like "diff from A to B, then diff from B to C".
I think we can still get this to work with term labels. For instance, I'm thinking we could do something like this for conflicts resulting from interactive squash:
It might still be tricky though for cases where |
6efdc57
to
1d695b8
Compare
529abe8
to
1ae55b4
Compare
c09ae21
to
f41d655
Compare
7133792
to
c06fac3
Compare
f41d655
to
6408973
Compare
088a27e
to
ec5da3d
Compare
This will help maintain the invariant that resolved merges cannot have labels. I added `Arc` since the labels will often need to be cloned, such as in `MergedTree::sub_tree`, `MergedTree::id`, and when reading and writing root trees.
Since two merged trees can now have the same contents but different conflict labels, many places that previously compared `MergedTreeId` for equality now need to ignore the conflict labels. To solve this, I added a `MergedTreeId::has_changes` method to check whether the underlying `Merge<TreeId>` has any changes.
To implement simplification of conflict labels, I decided to add more functions such as `zip` and `unzip` to `Merge`. I think these functions could be useful in other situations so I thought this was a nice solution, but an alternative solution could be to make `get_simplified_mapping` and `apply_simplified_mapping` public and manually apply the same mapping to both merges.
The old method is renamed to `MergedTree::merge_unlabeled` to make it easy to find unmigrated callers. The goal is that almost all callers will eventually use `MergedTree::merge` to add labels, unless the resulting tree is never visible to the user.
Conflict labels are stored in a separate header for backwards compatibility.
If a conflicted file didn't change, but the conflict labels changed, we will still need to re-materialize the file in the working copy to ensure that the conflict labels are displayed properly.
I swapped the order of added and removed term in the label for diff sections, because I think having the added term always come first makes conflicts easier to read, especially when there are conflict labels. For instance, if I see this conflict: ``` <<<<<<< Conflict 1 of 1 %%%%%%% Changes from parents of ysrnknol 7a20f389 to rebase destination (rtsqusxu 2768b0b9) -base +left +++++++ Contents of rebased commit (ysrnknol 7a20f389) right >>>>>>> Conflict 1 of 1 ends ``` It's not obvious that the first side is the changes from the rebase destination, whereas this is more clear to me: ``` <<<<<<< conflict 1 of 1 %%%%%%% rebase destination (rtsqusxu 2768b0b9) compared with parents of ysrnknol 7a20f389 -base +left +++++++ rebased commit (ysrnknol 7a20f389) right >>>>>>> conflict 1 of 1 ends ```
An example with parents `rtsqusxu` and `ysrnknol`: ``` <<<<<<< conflict 1 of 1 %%%%%%% rtsqusxu 2768b0b9 compared with vpxusssl 38d49363 -base +left +++++++ ysrnknol 7a20f389 right >>>>>>> conflict 1 of 1 ends ```
An example of rebasing `rtsqusxu` onto `ysrnknol`: ``` <<<<<<< conflict 1 of 1 %%%%%%% rebase destination (rtsqusxu 2768b0b9) compared with parents of ysrnknol 7a20f389 -base +left +++++++ rebased commit (ysrnknol 7a20f389) right >>>>>>> conflict 1 of 1 ends ```
An example of squashing `ysrnknol` into `rtsqusxu`: ``` <<<<<<< conflict 1 of 1 +++++++ squash destination (rtsqusxu 2768b0b9) updated in destination %%%%%%% squashed commit (ysrnknol 7a20f389) compared with parents of ysrnknol 7a20f389 -base +squashed >>>>>>> conflict 1 of 1 ends ```
ec5da3d
to
51c1184
Compare
Resolves #1176. I'm not sure if this is the best solution, but I wanted to try it out and see how it feels. This could probably be split into smaller PRs if we decide we want to follow this approach. These are the steps I took:
MergedTreeId::Legacy
variant, since it complicated implementation and I believe it is unnecessary now.MergedTree
andMergedTreeId
(as suggested by @martinvonz)Many other places still do not add labels (e.g.
jj split
,jj absorb
), so they would have to be added later.Example conflict markers for merge with
rtsqusxu
andysrnknol
as parents:Example conflict markers for rebase of
ysrnknol
ontortsqusxu
:Example conflict markers for squash of
ysrnknol
intortsqusxu
:Checklist
If applicable:
CHANGELOG.md
README.md
,docs/
,demos/
)cli/src/config-schema.json
)