rebase: add --insert-after and --insert-before options for --revisions

[bnjmnt4n]

This adds the --insert-after and --insert-before options to jj rebase, which can only be used with -r.
This allows for commits to be moved before or after any given commits.

Closes #1188.

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

[ilyagr]

Thank you for working on this!

I'm digesting the PR a bit slowly, here are a couple of first comments.

[bnjmnt4n]

Some notes about implementation and the DescendantRebaser API:

Considering just the first step ("extracting" the commit from its current location):

My initial approach was to mark the commit as abandoned, call MutRepo::rebase_descendants, then separately rebase the given revision on top of the destination. This didn't work as expected because abandoning a commit updates branches pointing to it to point to its parent instead, which isn't desired behavior. It can also cause the working copy commit to be abandoned, which also isn't desired in this case.

There doesn't seem to be an easy way to mark a commit as "moved" out of its location in the graph, i.e. treat it as abandoned so that its children are automatically rebased onto its parents, whilst preserving its branches. This is why I presume the current behavior is to rebase its children first onto its parents, then rebase the commit itself to its new destination, to allow the children to be auto-rebased, whilst preserving the branch of the commit when it is rewritten.

Considering the second step (shifting the commit to its new destination, and rebasing its new children on top of it):

I'm also not too sure if it's better to perform the moving in a single step: the current rebase -r implementation also does the same two steps.

[bnjmnt4n]

I created a POC of handling "moved commits" into MutRepo/DescendantRebaser in #3442, but not too sure if that's the way we want to approach this.

[ilyagr]

Just to set expectations: I haven't looked at this today, and it might take me a few days.

Also, somebody else might burst in to review this, which is totally fine.

[martinvonz]

I created a POC of handling "moved commits" into MutRepo/DescendantRebaser in #3442, but not too sure if that's the way we want to approach this.

I very recently finished unifying the different kinds of rewritten commits so they're all recorded in parent_mappings. It would be nice if we don't need another field again.

I'm hoping the interface I'm planning will be something like mut_repo.transform_descendants(roots, callback). The callback gets something like a CommitBuilder with the new parents already set. It can the update the parents, set a new root tree, or anything it needs. It's also free to update the repo if necessary, and descendants will be updated accordingly (but additional descendants will not be detected if you mark some unrelated commit rewritten).

[bnjmnt4n]

I've slightly modified that implementation to perform "extraction" instead, which rebases children onto its parents instead of trying to move them. I'm not tied very much to that implementation either, just a quick experiment to see how much it manages to simplify rebase.

I am interested in figuring out the new API would work. Would the new API replace all the existing MutRepo::set_rewritten_commit, MutRepo::set_abandoned_commit, etc.? Would it be called for all descendants of the roots? I guess that would be useful for more complex operations, but removing the existing methods also seems like it might the existing "common" operations more complicated.

What would the best way to implement "extraction" in the new API be (i.e. jj rebase -r A -d A-)? Would it be checking each commit's parents in the callback to see if it included A, then update the list of parents to include A's parents instead?

[martinvonz]

I am interested in figuring out the new API would work. Would the new API replace all the existing MutRepo::set_rewritten_commit, MutRepo::set_abandoned_commit, etc.?

No, those functions would still exist.

Would it be called for all descendants of the roots?

Yes.

I guess that would be useful for more complex operations, but removing the existing methods also seems like it might the existing "common" operations more complicated.

Yes, I plan to keep the existing functionality. This would just add new functionality on top (or underneath, if you like).

What would the best way to implement "extraction" in the new API be (i.e. jj rebase -r A -d A-)? Would it be checking each commit's parents in the callback to see if it included A, then update the list of parents to include A's parents instead?

Exactly.

[martinvonz]

Sorry it's taken me so long to get to this PR. It's partly that I'm wondering if there's a way we can generalize the feature so we don't have to add --insert-after/-before to other places. But it's mostly just being busy and/or prioritizing poorly. I'll try to review this later today.

[bnjmnt4n]

Sorry it's taken me so long to get to this PR. It's partly that I'm wondering if there's a way we can generalize the feature so we don't have to add --insert-after/-before to other places. But it's mostly just being busy and/or prioritizing poorly. I'll try to review this later today.

No worries, I would actually like to make --insert-after/-before support more complicated revsets instead of just a single revision only, but haven't had time to think thoroughly about how that would look. I'm happy to revisit after the rebasing API refactoring is done as well.

[bnjmnt4n]

I've updated this PR to stack the changes on top of those in #3564.

[bnjmnt4n]

Updated this; I think the biggest issue I'm not sure about is the message regarding commits displayed. It can be a bit ambiguous since the same commit might be moved multiple times during the whole rebase operation.

[martinvonz]

Thanks again! This was clearly a lot of work.