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

FR: jj split --horizontal/--siblings/--sideways #2274

Closed
martinvonz opened this issue Sep 19, 2023 · 13 comments
Closed

FR: jj split --horizontal/--siblings/--sideways #2274

martinvonz opened this issue Sep 19, 2023 · 13 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers polish🪒🐃 Make existing features more convenient and more consistent

Comments

@martinvonz
Copy link
Member

Is your feature request related to a problem? Please describe.

Sometimes it's useful to split a commit "sideways", producing two or more siblings instead of a chain of commits.

Describe the solution you'd like

We talked about adding a way of splitting commits sideways on Discord at some point. The same request just came up for Mercurial in a chat group at Google.

Describe alternatives you've considered

An alternative is to let the user finish splitting the commits and then let them flatten the result using something like #1079.

@PhilipMetzger PhilipMetzger added enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent labels Sep 21, 2023
@solson
Copy link
Member

solson commented Nov 11, 2023

I keep running into the lack of this. In my case, jj split --sibling -i seems like the most natural way to get something like git stash -p — I want to split off some part of my current changes (so they leave the working copy) and come back to them later. Naturally, they have to be based on the same parent as the working copy.

@martinvonz
Copy link
Member Author

This shouldn't be hard to implement, if you're interested. Let's say we have history like this:

D
|\
B C
|/
A

The user now runs jj split --siblings -r B and we call the diff editor as usual. The output from that is the tree contents of the first commit. We would create commit B1 with the same parents as B, i.e. A. We would then create commit B2 by merging the changes between the new B1 and B into A's parent tree. We would then rewrite the children of B (i.e. D) to have B1 and B2 as parents instead of B. So in this case, D would have B1, B2, and C as parents. I don't quite remember, but it looks like we might be able to just add both new commits where we currently have one commit here.

@solson
Copy link
Member

solson commented Nov 11, 2023

Thanks for the explanation! I may have time to work on some jj PRs some weekend soon.

@martinvonz martinvonz added the good first issue Good for newcomers label Nov 13, 2023
@emesterhazy
Copy link
Contributor

emesterhazy commented Feb 6, 2024

I'm going to look into implementing this, but I'd like a clarification. It sounds like Solson and Martin are describing different things.

I think that Solson is describing a command that does something like this:

jj split --siblings -r B

C      C
|      |
B  ->  B1  B2       
|      |  /
A      A--

I think Martin is describing a command that does this:

jj split --siblings -r B

C        --C--
|       /     \
B  ->  B1     B2       
|       \     /
A        --A--

Is this feature meant to do 1, 2, both, or something completely different?

@solson
Copy link
Member

solson commented Feb 6, 2024

jj split --siblings -r B

C        --C--
|       /     \
B  ->  B1     B2       
|       \     /
A        --A--

This is what I would expect, personally. In my earlier comment I was referring to a usecase where the revision I'm splitting has no children in the first place, i.e.

jj split --siblings

B  ->  B1     B2       
|       \     /
A        --A--

Where B is the working copy and A is the working copy parent, and B1 is the new working copy afterwards. But this is just a special case.

@emesterhazy
Copy link
Contributor

Ok got it. So then to get to the first state we could do this:

jj split --siblings -r B

C        --C--
|       /     \
B  ->  B1     B2
|       \     /
A        --A--

jj rebase -s C -d B1

  --C--      C
 /     \     |
B1     B2 -> B1  B2
 \     /     |  /
  --A--      A--

@emesterhazy emesterhazy self-assigned this Feb 8, 2024
emesterhazy added a commit that referenced this issue Feb 13, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any chilren of the original commit
will have both sibilings as their new parents.


ISSUE=#2274
emesterhazy added a commit that referenced this issue Feb 13, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original commit
will have both sibilings as their new parents.


ISSUE=#2274
@emesterhazy
Copy link
Contributor

I have this halfway working in #3037. By halfway, I mean that it is able to handle the simple case:

jj split --siblings

B  ->  B1     B2       
|       \     /
A        --A--

I'm still familiarizing myself with the internals to figure out the best way to handle the case where commit B has children that need to be rebased onto B1 and B2. If anyone has suggestions please let me know. Adding both siblings to the set_rewritten_commit call doesn't work and just ends up creating a divergent commit.

In that case, the log looks like this, where the commits with descriptions "Add file3" and "Add file1 and file2" are the originals and qtulxkyw is the commit being split.

◉  prnuroxt emesterhazy@ 2024-02-13 10:30:06.000 -05:00 83845508
│  Add file2
│ @  qtulxkyw?? emesterhazy@ 2024-02-13 10:30:03.000 -05:00 09f4d061
├─╯  Add file1
│ ◉  spltwkum emesterhazy@ 2024-02-13 10:00:45.000 -05:00 a0726d5f
│ │  Add file3
│ ◉  qtulxkyw?? emesterhazy@ 2024-02-11 10:58:42.000 -05:00 c98b9d59
├─╯  Add file1 and file2

I think I'm going to need to change how rebase_descendants works or manually rebase the children of the split commit onto the new siblings.

@martinvonz
Copy link
Member Author

set_rewritten_commit call

I actually thought that would work. Anyway, the way regular (serial) split is a bit weird and it's probably a good idea to do the rebasing of children manually for that case, so maybe it's good to do it for sibling split as well. The drawback is that we would need to replicate some of the logic from rebase_descendants(), in particular the logic for updating branches (should presumably point to both commits) and the working copy (we'll have to pick one of the commits).

emesterhazy added a commit that referenced this issue Mar 31, 2024
This refactor will allow us to reuse new `rebase_descendants` function for the
`jj split --siblings` feature (#2274) and later possibly for `jj parallelize`
(#1079).
emesterhazy added a commit that referenced this issue Mar 31, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both sibilings as their new parents.


ISSUE=#2274
@emesterhazy
Copy link
Contributor

I think I have this done and ready for review and polish in #3037.

@emesterhazy
Copy link
Contributor

I'm not sure all of the considerations in #2274 (comment) are addressed though. I'm still setting the rewritten commit to the second commit, so any branches are moved to the second commit and it is checked out as the working copy.

I don't think that checking out the second commit is an issue (presumably we have to pick one), but perhaps we should leave any branches in a conflicted state. Thoughts?

emesterhazy added a commit that referenced this issue Mar 31, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both sibilings as their new parents.


ISSUE=#2274
@martinvonz
Copy link
Member Author

I don't think that checking out the second commit is an issue (presumably we have to pick one), but perhaps we should leave any branches in a conflicted state. Thoughts?

Good question. I think it would make more sense to leave the branch in a conflicted state, but it seems like it's practically quite useful to leave it on one of the commits instead. That way it requires action from the user in some of the cases instead of all of the cases. I'd say we should leave it that way for now and change it later if we hear that it's confusing - or worse, that it leads people to push the wrong commit to a remote.

By the way, we have had a discussion internally at Google about where regular (parent/child) jj split should put the branch. It currently leaves it on the child, but it lets the parent keep the change id. We use branches for tracking code reviews at Google (I know you're at Google, @emesterhazy; I'm just providing context for others). For that use case, it's confusing that the branch doesn't follow the change id. It makes sense for the branch to move to the child in more Git-like workflows, since branches typically point to the tip of a chain of commits in Git. However, I don't want us to design a behavior that makes less sense just because it better matches Git. What do people think?

emesterhazy added a commit that referenced this issue Apr 1, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both sibilings as their new parents.


ISSUE=#2274
emesterhazy added a commit that referenced this issue Apr 1, 2024
This refactor will allow us to reuse new `rebase_descendants` function for the
`jj split --siblings` feature (#2274) and later possibly for `jj parallelize`
(#1079).
emesterhazy added a commit that referenced this issue Apr 1, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both sibilings as their new parents.


ISSUE=#2274
emesterhazy added a commit that referenced this issue Apr 1, 2024
This refactor will allow us to reuse new `rebase_descendants` function for the
`jj split --siblings` feature (#2274) and later possibly for `jj parallelize`
(#1079).
emesterhazy added a commit that referenced this issue Apr 1, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both sibilings as their new parents.


#2274
emesterhazy added a commit that referenced this issue Apr 1, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both sibilings as their new parents.


#2274
emesterhazy added a commit that referenced this issue Apr 1, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both siblings as their new parents.


#2274
emesterhazy added a commit that referenced this issue Apr 1, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both siblings as their new parents.


#2274
@emesterhazy
Copy link
Contributor

I created #3419 to discuss what happens to branches when a commit is split. Currently both jj split and jj split --siblings move the branch to the second commit, so at least they are consistent. If we want to change the behavior for either of these then let's pick up the discussion over there.

emesterhazy added a commit that referenced this issue Apr 1, 2024
This refactor will allow us to reuse new `rebase_descendants` function for the
`jj split --siblings` feature (#2274) and later possibly for `jj parallelize`
(#1079).
emesterhazy added a commit that referenced this issue Apr 1, 2024
If the --siblings option is used, the target commit is split into two sibling
commits instead of parent and child commits. Any children of the original
commit will have both siblings as their new parents.


#2274
@emesterhazy
Copy link
Contributor

Closed by #3037. The new option is --siblings, although in hindsight maybe we should have named the flag --parallelize or --parallel to match jj parallelize. The short name -p would also be less ambiguous than -s, which means source for jj rebase.

If anyone has opinions on the name we can still easily change it until the next release. Feel free to open a new issue :)

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

No branches or pull requests

4 participants