jj
jj copied to clipboard
rebase: skip duplicate divergent commits by default
This is a partial fix for #5381 and #2979. It doesn't handle the general case (it only detects duplicate changes between divergent commits with the same change ID), but I think it should handle many useful cases, especially for users with the change-id header enabled. I'm looking for feedback about whether this is a good general approach and about the implementation.
This serves a similar purpose to Git's patch ID mechanism, however it is slightly different in that it only compares commits which have the same change ID as each other (divergent changes), and it does a full comparison of the commits to see if they would have identical trees if rebased onto the same parents. Since most changes aren't divergent, I believe this should have a negligible performance cost in most cases.
I think skipping these commits by default makes sense for jj rebase, since usually this will be a helpful behavior for the user. With this behavior, a safe first step when encountering divergent changes would be to rebase one branch on top of the other, since that will abandon any divergent changes that have identical contents to existing commits, leaving behind any non-trivial divergent changes for the user to resolve manually.
Checklist
If applicable:
- [X] 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)
- [X] I have added tests to cover my changes
I think this is ready for review now; I updated it to abandon the commits before starting the rebase and added tests.
@yuja Your other PR made the code a lot simpler, thanks! I haven't had much time recently, but I'm hoping to address the comments soon.
Hmm I just tested it after merging, and I found a bug immediately (I think I forgot to test this case previously because I was too focused on testing the scenario of having an abandoned commit in the middle of a rebase, which I thought would cover everything).
Apparently rewrite::move_commits doesn't work when the root of the set of target commits has already been abandoned. In this case, it just leaves the commits in place (although interestingly, it does still rebase these commits and they are recorded in the rebase stats). It does work correctly when the abandoned commit isn't the root of the set of target commits. @yuja and @martinvonz do you have any ideas for how to fix this? I can make another PR to revert this if it's not a simple fix.
Edit: I came up with a workaround, but there's probably a better solution: #6581