jj rebase --skip-empty unexpectedly changes the commit graph
#2760
Comments
Yeah, that's probably it (though I don't know why The |
|
Hmm, that's a weird bug. @matts1, do you have time to look into it? Also, @ilyagr, since you're working on refactoring this code, do you want us to not touch this until you're done refactoring? I'm not sure exactly which parts of it you're going to touch.
I think |
|
Interesting bug. Thanks for finding the cause, sounds very plausible. I think the bug could also occur with I think that if we identified the cause correctly, the fix would be to add another condition to Update: It's unclear to me what the best behavior is if the working copy has children. We could either rebase the children into the parent or not. At first, we could do the latter, since it's simpler.
This wouldn't interfere with what I'm doing at all, unless I'm missing something and it's actually a much more complicated fix. |
|
Was able to repro, thanks for filing the bug. |
|
After rereading the comments, I should probably clarify some things.
This was intentional. I'm not a big fan of rebase changing how it works depending on your working commit, but I can definitely see the use for it here. I'm far more concerned about the fact that the history remains branched. There's open questions here (eg. "It's unclear to me what the best behavior is if the working copy has children"). For now, I won't touch this aspect of the bug. Someone else is welcome to make a change if they want though. I care far more about the rebase creating seperate branches, which is definitely not WAI. I'll be working on fixing that. |
Fixes jj-vcs#2760 Given the tree: ``` A-B-C \ B2 ``` And the command `jj rebase -s B -d B2` We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to `rewrite(rewrites={}, abandoned={B})` instead of `rewrite(rewrites={B=>B2}, abandoned={})`, which then made the new parent of `C` into `A` instead of `B2`
Fixes jj-vcs#2760 Given the tree: ``` A-B-C \ B2 ``` And the command `jj rebase -s B -d B2` We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to `rewrite(rewrites={}, abandoned={B})` instead of `rewrite(rewrites={B=>B2}, abandoned={})`, which then made the new parent of `C` into `A` instead of `B2`
Fixes jj-vcs#2760 Given the tree: ``` A-B-C \ B2 ``` And the command `jj rebase -s B -d B2` We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to `rewrite(rewrites={}, abandoned={B})` instead of `rewrite(rewrites={B=>B2}, abandoned={})`, which then made the new parent of `C` into `A` instead of `B2`
|
Tangentially related, @matts1, do you remember why we never implemented |
Fixes jj-vcs#2760 Given the tree: ``` A-B-C \ B2 ``` And the command `jj rebase -s B -d B2` We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to `rewrite(rewrites={}, abandoned={B})` instead of `rewrite(rewrites={B=>B2}, abandoned={})`, which then made the new parent of `C` into `A` instead of `B2`
|
I felt that there was a few open questions still regarding API. Here are three options I was considering:
I implemented the third option, but in retrospect, I'd probably be happy changing it to the fourth option. I personally think option 1 adds too much complexity to the API. |
|
I like option 1 and option 4. Option 2 is annoying since the user will have to specify something each time; I don't think clap provides a good way to allow both Update: If we go with option 4, I mildly prefer still calling it --skip-emptied as @afgomez suggested below. It's probably not worth the trouble, but even if our goal is option 4, we could have both options for a transition period and then deprecate the old one. |
Fixes jj-vcs#2760 Given the tree: ``` A-B-C \ B2 ``` And the command `jj rebase -s B -d B2` We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to `rewrite(rewrites={}, abandoned={B})` instead of `rewrite(rewrites={B=>B2}, abandoned={})`, which then made the new parent of `C` into `A` instead of `B2`
To me this sounds like what I would want the tool to do. Only skip the commits if they become empty. If for whatever reason I have created empty commits (Like... I don't know... I want to outline how the history might look like before implementing anything) I think they should be preserved. It's worth considering renaming the flag to
I think in practice I would only use |
Fixes #2760 Given the tree: ``` A-B-C \ B2 ``` And the command `jj rebase -s B -d B2` We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to `rewrite(rewrites={}, abandoned={B})` instead of `rewrite(rewrites={B=>B2}, abandoned={})`, which then made the new parent of `C` into `A` instead of `B2`
This mostly reverts jj-vcs#2901 as well as its fixup jj-vcs#2903. The related bug is reopened, see jj-vcs#2869 (comment). The problem is that while the fix did fix jj-vcs#2896 in most cases, it did reintroduce the more severe bug jj-vcs#2760 in one case, if the working copy is the commit being rebased. For example, suppose you have the tree ``` root -> A -> B -> @ (empty) -> C ``` ### Before this commit #### Case 1 `jj rebase -s B -d root` would work perfectly before this commit, resulting in ``` root -> A \-------B -> C \- @ (new, empty) ``` #### Case 2 Unfortunately, if you run `jj rebase -s @ -d A`, you'd have the following result (before this commit), which shows the reintroduction of jj-vcs#2760: ``` root -> A @ -> C \-- B ``` with the working copy at `A`. The reason for this is explained in jj-vcs#2901 (comment). ### After this commit After this commit, both case 1 and case 2 will be wrong in the sense of jj-vcs#2896, it will not exhibit the worse bug jj-vcs#2760. Case 1 would result in: ``` root -> A \-------B -> @ (empty) -> C ``` Case 2 would result in: ``` root -> A -> @ -> C \-- B ``` with the working copy remaining a descendant of A
This mostly reverts jj-vcs#2901 as well as its fixup jj-vcs#2903. The related bug is reopened, see jj-vcs#2869 (comment). The problem is that while the fix did fix jj-vcs#2896 in most cases, it did reintroduce the more severe bug jj-vcs#2760 in one case, if the working copy is the commit being rebased. For example, suppose you have the tree ``` root -> A -> B -> @ (empty) -> C ``` ### Before this commit #### Case 1 `jj rebase -s B -d root --abandon-empty` would work perfectly before this commit, resulting in ``` root -> A \-------B -> C \- @ (new, empty) ``` #### Case 2 Unfortunately, if you run `jj rebase -s @ -d A --abandon-empty`, you'd have the following result (before this commit), which shows the reintroduction of jj-vcs#2760: ``` root -> A @ -> C \-- B ``` with the working copy at `A`. The reason for this is explained in jj-vcs#2901 (comment). ### After this commit After this commit, both case 1 and case 2 will be wrong in the sense of jj-vcs#2896, but it will no longer exhibit the worse bug jj-vcs#2760 in the second case. Case 1 would result in: ``` root -> A \-------B -> @ (empty) -> C ``` Case 2 would result in: ``` root -> A -> @ -> C \-- B ``` with the working copy remaining a descendant of A
This mostly reverts jj-vcs#2901 as well as its fixup jj-vcs#2903. The related bug is reopened, see jj-vcs#2869 (comment). The problem is that while the fix did fix jj-vcs#2896 in most cases, it did reintroduce the more severe bug jj-vcs#2760 in one case, if the working copy is the commit being rebased. For example, suppose you have the tree ``` root -> A -> B -> @ (empty) -> C ``` ### Before this commit #### Case 1 `jj rebase -s B -d root --abandon-empty` would work perfectly before this commit, resulting in ``` root -> A \-------B -> C \- @ (new, empty) ``` #### Case 2 Unfortunately, if you run `jj rebase -s @ -d A --abandon-empty`, you'd have the following result (before this commit), which shows the reintroduction of jj-vcs#2760: ``` root -> A @ -> C \-- B ``` with the working copy at `A`. The reason for this is explained in jj-vcs#2901 (comment). ### After this commit After this commit, both case 1 and case 2 will be wrong in the sense of jj-vcs#2896, but it will no longer exhibit the worse bug jj-vcs#2760 in the second case. Case 1 would result in: ``` root -> A \-------B -> @ (empty) -> C ``` Case 2 would result in: ``` root -> A -> @ -> C \-- B ``` with the working copy remaining a descendant of A
This mostly reverts jj-vcs#2901 as well as its fixup jj-vcs#2903. The related bug is reopened, see jj-vcs#2869 (comment). The problem is that while the fix did fix jj-vcs#2869 in most cases, it did reintroduce the more severe bug jj-vcs#2760 in one case, if the working copy is the commit being rebased. For example, suppose you have the tree ``` root -> A -> B -> @ (empty) -> C ``` ### Before this commit #### Case 1 `jj rebase -s B -d root --abandon-empty` would work perfectly before this commit, resulting in ``` root -> A \-------B -> C \- @ (new, empty) ``` #### Case 2 Unfortunately, if you run `jj rebase -s @ -d A --abandon-empty`, you'd have the following result (before this commit), which shows the reintroduction of jj-vcs#2760: ``` root -> A @ -> C \-- B ``` with the working copy at `A`. The reason for this is explained in jj-vcs#2901 (comment). ### After this commit After this commit, both case 1 and case 2 will be wrong in the sense of jj-vcs#2869, but it will no longer exhibit the worse bug jj-vcs#2760 in the second case. Case 1 would result in: ``` root -> A \-------B -> @ (empty) -> C ``` Case 2 would result in: ``` root -> A -> @ -> C \-- B ``` with the working copy remaining a descendant of A
This mostly reverts jj-vcs#2901 as well as its fixup jj-vcs#2903. The related bug is reopened, see jj-vcs#2869 (comment). The problem is that while the fix did fix jj-vcs#2869 in most cases, it did reintroduce the more severe bug jj-vcs#2760 in one case, if the working copy is the commit being rebased. For example, suppose you have the tree ``` root -> A -> B -> @ (empty) -> C ``` ### Before this commit #### Case 1 `jj rebase -s B -d root --skip-empty` would work perfectly before this commit, resulting in ``` root -> A \-------B -> C \- @ (new, empty) ``` #### Case 2 Unfortunately, if you run `jj rebase -s @ -d A --skip-empty`, you'd have the following result (before this commit), which shows the reintroduction of jj-vcs#2760: ``` root -> A @ -> C \-- B ``` with the working copy at `A`. The reason for this is explained in jj-vcs#2901 (comment). ### After this commit After this commit, both case 1 and case 2 will be wrong in the sense of jj-vcs#2869, but it will no longer exhibit the worse bug jj-vcs#2760 in the second case. Case 1 would result in: ``` root -> A \-------B -> @ (empty) -> C ``` Case 2 would result in: ``` root -> A -> @ -> C \-- B ``` with the working copy remaining a descendant of A
This mostly reverts #2901 as well as its fixup #2903. The related bug is reopened, see #2869 (comment). The problem is that while the fix did fix #2869 in most cases, it did reintroduce the more severe bug #2760 in one case, if the working copy is the commit being rebased. For example, suppose you have the tree ``` root -> A -> B -> @ (empty) -> C ``` ### Before this commit #### Case 1 `jj rebase -s B -d root --skip-empty` would work perfectly before this commit, resulting in ``` root -> A \-------B -> C \- @ (new, empty) ``` #### Case 2 Unfortunately, if you run `jj rebase -s @ -d A --skip-empty`, you'd have the following result (before this commit), which shows the reintroduction of #2760: ``` root -> A @ -> C \-- B ``` with the working copy at `A`. The reason for this is explained in #2901 (comment). ### After this commit After this commit, both case 1 and case 2 will be wrong in the sense of #2869, but it will no longer exhibit the worse bug #2760 in the second case. Case 1 would result in: ``` root -> A \-------B -> @ (empty) -> C ``` Case 2 would result in: ``` root -> A -> @ -> C \-- B ``` with the working copy remaining a descendant of A
Description
When running
jj rebase --skip-emptythe commit graph gets mangled if there are empty commits created as a result of the operation.Steps to Reproduce the Problem
lrk, with the same changes asyvv--skip-emptyon top oflxk(the commit described as 'Another B').In comparison, this is what happens with a regular rebase:
Expected Behavior
jjcreates a new empty commit at the tip of the branchActual Behavior
jjkeeps the current commit at the last commit of the branch.Specifications
The text was updated successfully, but these errors were encountered: