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.

Sign up for GitHub

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

Jump to bottom

rewrite.rs: fix working copy position after jj rebase --abandon-empty #2901

Merged
merged 2 commits into from
Jan 31, 2024
Merged

rewrite.rs: fix working copy position after jj rebase --abandon-empty #2901

merged 2 commits into from
Jan 31, 2024

Conversation

ilyagr
Copy link
Contributor

@ilyagr ilyagr commented Jan 31, 2024 •
edited
Loading

Fixes #2869.

I did this somewhat mechanically. Let me know if I missed some situation where this could do the wrong thing. This could happen especially if the rename in the first commit doesn't match the actual meaning of the argument.

@ilyagr
rewrite.rs DescendantRebaser: rename variable for clarity
2145fcf 
The `edit` argument seems to be true if and only if the
old commit was *not* abandoned. So, I flipped its value
and renamed it to `abandoned_old_commit`.
@martinvonz martinvonz mentioned this pull request Jan 31, 2024
Merged
4 tasks
@martinvonz
Copy link
Member

This could happen especially if the rename in the first commit doesn't match the actual meaning of the argument.

I think I'll end up changing that code for the fix for #2600 anyway. I think your commit is fine as is.

@ilyagr ilyagr marked this pull request as ready for review January 31, 2024 06:47
@ilyagr ilyagr enabled auto-merge (rebase) January 31, 2024 06:50
@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 31, 2024

Thanks!

@ilyagr ilyagr merged commit becbc88 into jj-vcs:main Jan 31, 2024
15 checks passed
@ilyagr ilyagr deleted the wcpos branch January 31, 2024 07:01
@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 31, 2024 •
edited
Loading

I think this is actually buggy if the working copy is empty and you do jj rebase -s @ --abandon-empty. That's because the return value is not checked in

https://github.com/martinvonz/jj/blob/7c87fe243c3b14f0321c2bd101af3879770cb90f/cli/src/commands/rebase.rs#L294-L300

so the references (and the working copy) will not be updated properly when that call abandons a commit. Ideally, DescendantRebaser should keep track of this information internally, e.g. record_abandoned_commit could keep track of the new parent and rebase_commit_with_options could call it.

I will try to write a test and then fix it that way, but I'm not 100% sure it will work. It's confusing to me that rebase_next calls rebase_commit_with_option, which currently calls record_rewritten_commit (and would, in my plan, call record_abandoned_commit instead) which creates a TODO list for DescendantRebasser and then also calls update_references.

Another alternative would be to backout this PR for now. The old behavior is wrong, but not dangerous.

This is the kind of thing that bothers me about the API: details that should be governed by the rewrite crate leak out into the CLI crate, and we pretty much have to think about every use of the rewrite crate when changing anything in it.

@ilyagr ilyagr mentioned this pull request Feb 1, 2024
Closed
ilyagr added a commit to ilyagr/jj that referenced this pull request Feb 3, 2024
@ilyagr
jj rebase --abandon-empty: revert commits cfcc7c5 and becbc88
9290475 
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
@ilyagr ilyagr mentioned this pull request Feb 3, 2024
Merged
ilyagr added a commit to ilyagr/jj that referenced this pull request Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
82b635a 
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
ilyagr added a commit to ilyagr/jj that referenced this pull request Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
f1b9b48 
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
ilyagr added a commit to ilyagr/jj that referenced this pull request Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
7aed104 
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
ilyagr added a commit to ilyagr/jj that referenced this pull request Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
13884fc 
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
ilyagr added a commit that referenced this pull request Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
d439de0 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinvonz martinvonz martinvonz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

jj rebase --abandon-empty leaves the working copy in the wrong place if it is empty and has children
2 participants
@ilyagr @martinvonz