Skip to content
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

rewrite: don't resolve intermediate parent tree when rebasing #4675

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

martinvonz
Copy link
Owner

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Copy link
Collaborator

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, is this change for correctness, performance reason, or both?

@martinvonz
Copy link
Owner Author

Out of curiosity, is this change for correctness, performance reason, or both?

I'm not sure :) I wrote it many months ago (just before your similar improvements to avoid intermediate resolution in octopus merges and whatever else it was).

I think it was while I was thinking of a related problem, which is that conflicts can accrue many sides as they get rebased. I think that only happens if different files have conflicts in different rebases, so I think you can get at most N+1 sides if you have N files. I've been thinking that perhaps we should store the unmodified trees when a conflict can't be automatically resolved. That would lead to having to redo some work when you edit, diff, etc., but it would also lead to less work then you rebases repeatedly, thanks to earlier conflict simplification.

Anyway, I have not noticed in practice the performance problem this PR addresses (but it seems like it shouldn't be hard to produce work a somewhat complex merge).

Let's say we're updating one parent of a merge:


```
  E            E'
 /|\          /|\
B C D   ->   B C D'
 \|/          \|/
  A            A
```

When rebasing `E` to create `E'` there, we do that by merging the
changes compared to the auto-merged parents. The auto-merged parents
before is `B+(C-A)+(D-A)`, and after it's `B+(C-A)+(D'-A)`. Then we
rebase the diff, which gives us `E' = B+(C-A)+(D'-A) + (E -
(B+(C-A)+(D-A))) = D' + (E - D')`.

However, we currently don't do quite that simplification because we
first resolve conflicts when possible in the two auto-merged parent
trees (before and after). That rarely makes a difference to the
result, but it's wasteful to do it. It does make a difference in some
cases where our merge algorithm is lossy, which currently is only the
"A+(A-B)=A" case. I added a test case showing where it does make a
difference. It's a non-obvious cases but I think the new behavior is
more correct (the old behavior was a conflict).
@martinvonz martinvonz enabled auto-merge (rebase) October 21, 2024 17:49
@martinvonz martinvonz merged commit 9d4a973 into main Oct 21, 2024
31 checks passed
@martinvonz martinvonz deleted the push-ulyrlyrvntvp branch October 21, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants