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

Simplify DescendantRebaser #3369

Merged
merged 10 commits into from
Mar 26, 2024
Merged

Simplify DescendantRebaser #3369

merged 10 commits into from
Mar 26, 2024

Conversation

martinvonz
Copy link
Member

This makes the rebase_one() step of DescendantRebaser simpler and safer. References are now updated after all commits have been rebased.

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
Contributor

@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.

Nice. I don't have much expertise, but looks good to me.

lib/src/repo.rs Outdated Show resolved Hide resolved
lib/src/rewrite.rs Show resolved Hide resolved
By adding the abandoned commit's parents to `parent_mapping`, we can
remove a bit more of the special handling of abandoned commitsin
`DescendantRebaser`.
When `rebase_commit_with_options()` decides to abandons a commit, it
records the new parents in the `MutableRepo`, but it's currently the
caller's responsibility to remember to mark it as abandoned. Let's
move that logic into the function to reduce the risk of future bugs.
The function only uses state from `MutableRepo`, so it should be
implemented on that type.
Now that we only call `update_references()` in one place, there's no
reason to have it also update `heads_to_add` and `heads_to_remove`. By
moving it out of the function, we can consolidate the logic in one
place.
We only use `new_commits` in `update_heads()`, so let's calculate it
there. It should also be more correct in case other commits were
created after we initialized `DescendantRebaser`.
We currently include the commits in `parent_mapping` and `abandoned`
in the set of commits to visit when rebasing descendants. The reason
was that we used to update branches and working copies when we visited
these commits. Since we started updating refs after rebasing all
commits, there's no need to even visit these commits.
@martinvonz martinvonz enabled auto-merge (rebase) March 26, 2024 16:42
@martinvonz martinvonz merged commit 9c382fd into main Mar 26, 2024
16 checks passed
@martinvonz martinvonz deleted the push-ywwlvvqvptvp branch March 26, 2024 16:50
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.

2 participants