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

jj rebase -r should be able to rebase a commit onto its descendant, insert commits before or after another commit (now AKA better tools for reordering commits part 1) #1188

Closed
3 tasks done
ilyagr opened this issue Feb 4, 2023 · 7 comments · Fixed by #3396
Labels
enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent

Comments

@ilyagr
Copy link
Collaborator

ilyagr commented Feb 4, 2023

  • jj rebase -r B -d D should work for A->B->C->D
  • jj rebase -r B --insert-after C
  • jj rebase -r B --insert-before D

The latter two were suggested below in #1188 (comment) and on Discord.

Older description

Conceptually, this shouldn't be a problem.

In practice, this is less trivial than it might sound and may require careful changes to some rebase primitives. Simply removing the relevant check causes a panic because of a cycle in the graph in the test_rebase_invalid test in test_rebase_command.rs.

@martinvonz also noted the following difficulty on Discord (likely related, and certainly will need to be tested):

martinvonz: it feels a little weird since it temporarily creates divergence if you rebase the commit first and then rebase it again
martinvonz: you start with this:

C
|
B
|
A

then run jj rebase -r B -d C, which temporarily creates

B'
|
C
|
B
|
A

and the the auto-rebase step converts that to

B''
|
C'
|
A
@martinvonz
Copy link
Owner

Since the usecase for this is to reorder commits, we probably also want to add a --insert option, so you can reorder the other way: jj rebase -r C -d A --insert. That has the same effect in this case since we only reordered two commits, but it would be useful in longer chains of commits. That option will also be useful if you're moving a commit forward but not all the way to the top, such as jj rebase -r A -d B --insert to get B'-A'-C'.

I just thought I'd mention that here, but I don't think we need that option in order to consider this feature implemented.

@ilyagr ilyagr added enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent labels Feb 4, 2023
@ilyagr ilyagr changed the title jj rebase -r should be able to rebase a commit onto its descendant jj rebase -r should be able to rebase a commit onto its descendant (now AKA better tools for reordering commits part 1) Jun 14, 2023
@ilyagr
Copy link
Collaborator Author

ilyagr commented Jun 14, 2023

#1531 depends on this.

@ilyagr ilyagr self-assigned this Oct 28, 2023
@ilyagr ilyagr changed the title jj rebase -r should be able to rebase a commit onto its descendant (now AKA better tools for reordering commits part 1) jj rebase -r should be able to rebase a commit onto its descendant, insert commits before or after another commit (now AKA better tools for reordering commits part 1) Oct 28, 2023
ilyagr added a commit that referenced this issue Oct 30, 2023
#1188

There are some additional test changes because children and descendants are now
rebased before the commit itself.
@gasche
Copy link
Collaborator

gasche commented Dec 15, 2023

I ended up here after being frustrated at how hard it is to reorder commits. I had a feature proposal (see below), and I thought to look for other issues where this is discussed.

My feature proposal: jj move --from foo --insert-{before,after} bar.

Instead of extending jj rebase, the idea is to add to jj move some of the capabilities of jj new. In particular,

jj move --from foo --insert-{before,after} bar other-args

can currently be done as

jj new --insert-{before,after} bar
jj move --from foo other-args
# ... and then go back to whichever chage you were editing

Personally I think that extending jj move makes slightly more sense than existing jj rebase for the purpose of reordering a single change, for the following reason:

  • The other options/arguments of jj move are related to selecting only a part of the --from commit to move, and they still make perfect sense here and would work well and be useful here.
  • On the other hand, the other options/arguments of jj rebase are related to selecting not just one commit to move, but also its descendants. This makes much less sense for commit reordering in general: if C has two children C1, C2, and we want to jj rebase -s C --insert-before B, would we expect that B will be added as a parent of both C1 or C2? This is probably not the expected/right behavior in most cases. We would want to be able to specify a linear path of commits (for example C..C1 or C..C2) to rebase before B, and this is not currently a rebase option.

@martinvonz
Copy link
Owner

@gasche: I think the most important reason not to use jj move for it is that that command moves diffs while preserving change IDs and branches. We'll want to make sure that change IDs and branches are moved when we reorder commits. Hmm, or at least change IDs should move. Branches are less obvious. If you have commits A through C, with a branch pointing to C, what should happen with that branch if you reorder B and C? Both Git and Mercurial move the branch to the new tip commit (i.e. the new B), so that's probably what we should do.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Dec 16, 2023

I think that both jj rebase --after (or --before) and jj move --after are reasonable to want. The major difference, in my mind, is where the change id ends up, as Martin described.

jj move --after is already almost possible, but it takes two commands: jj new --after followed by jj move. So, I think rebase --after is higher priority. (Even though one could say that jj rebase --after is also possible as two rebases, the user then has to simulate the behavior of --after, which involves looking up a bunch of change ids all over the commit graph.)

Branches are less obvious. If you have commits A through C, with a branch pointing to C, what should happen with that branch if you reorder B and C? Both Git and Mercurial move the branch to the new tip commit (i.e. the new B), so that's probably what we should do.

@martinvonz, I'm not sure about this and I'm worried this might create a bunch of weird edge cases where branches move unexpectedly. I was going to punt on this, at least at first.

Vague thoughts: This could be considered independently from --after, actually: if you have root -> A -> B (branch) and do jj rebase -r A -d B, where should the branch end up? Currently, it stays on B. OTOH, we could go back to considering a jj reorder command that might be different from jj rebase in where it moves branches.

On the other hand, the other options/arguments of jj rebase are related to selecting not just one commit to move, but also its descendants. This makes much less sense for commit reordering in general: if C has two children C1, C2, and we want to jj rebase -s C --insert-before B, would we expect that B will be added as a parent of both C1 or C2?

@gasche, this is a good question. Currently, for this reason, I was going to focus on jj rebase -r --after and to postpone (or even skip entirely) jj rebase -s --after (or --before).

A behavior I was thinking of would be to make jj rebase -s C --before B to rebase C onto B and the children of B onto C, ignoring is children. This could also work for jj rebase -s revset --before B. I like this because it's easy to understand, and I thought I needed this occasionally, but I'm not sure if it's useful enough to implement.

Separate, loosely related, thought: after the basic jj rebase -r --after works, I would like jj rebase -r A::B --after C to work as well. My current plan is to implement jj rebase -r revset --after C to rebase roots(revset) onto C and to rebase the children of C onto heads(revset). I'm not sure how useful this is in cases where either roots(revset) or heads(revset) have more than one commit, but at least it's consistent, and seems slightly better than artificially refusing to function if that condition is not satisfied.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Feb 7, 2024

I'm not actively working on this ATM, it seems best to wait until the DescendantRebaser is refactored somewhat.

@bnjmnt4n
Copy link
Collaborator

I created an initial PR for this in #3396.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants