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

jj rebase --abandon-empty leaves the working copy in the wrong place if it is empty and has children #2869

Closed
ilyagr opened this issue Jan 23, 2024 · 1 comment · Fixed by #2901 or #2921
Closed

jj rebase --abandon-empty leaves the working copy in the wrong place if it is empty and has children #2869

ilyagr opened this issue Jan 23, 2024 · 1 comment · Fixed by #2901 or #2921
Labels
🐛bug Something isn't working polish🪒🐃 Make existing features more convenient and more consistent

Comments

@ilyagr
Copy link
Collaborator

ilyagr commented Jan 23, 2024 •
edited
Loading

See the TODO in the diff in https://github.com/martinvonz/jj/pull/2870/files for a detailed description of this bug.

It was originally discovered by Martin and discussed in the discussion #2766 (comment).
ilyagr added a commit to ilyagr/jj that referenced this issue Jan 23, 2024
@ilyagr
test_rebase_abandoning_empty: Demonstrate effects on children of an…
859d037 
… empty `@`

This demonstrates the minor bug discussed in
martinvonz#2766 (comment)
AKA martinvonz#2869.

It's also interesting whether changing the definition of "discardable" commit
would affect this test, see
martinvonz#2859 (comment)

(I think it won't, but still)
ilyagr added a commit to ilyagr/jj that referenced this issue Jan 23, 2024
@ilyagr
test_rebase_abandoning_empty: Demonstrate effects on children of an…
5922068 
… empty `@`

This demonstrates the minor bug discussed in
martinvonz#2766 (comment)
AKA martinvonz#2869.

It's also interesting whether changing the definition of "discardable" commit
would affect this test, see
martinvonz#2859 (comment)

(I think it won't, but still)
@ilyagr ilyagr mentioned this issue Jan 23, 2024
Merged
ilyagr added a commit to ilyagr/jj that referenced this issue Jan 23, 2024
@ilyagr
test_rebase_abandoning_empty: Demonstrate effects on children of an…
464b0e9 
… empty `@`

This demonstrates the minor bug discussed in
martinvonz#2766 (comment)
AKA martinvonz#2869.

It's also interesting whether changing the definition of "discardable" commit
would affect this test, see
martinvonz#2859 (comment)

(I think it won't, but still)
ilyagr added a commit to ilyagr/jj that referenced this issue Jan 23, 2024
@ilyagr
test_rebase_abandoning_empty: add children of an empty @ to the t…
f0846ac 
…est case

This demonstrates the minor bug discussed in
martinvonz#2766 (comment)
AKA martinvonz#2869.

It's also interesting whether changing the definition of "discardable" commit
would affect this test, see
martinvonz#2859 (comment)

(I think it won't, but still)
@ilyagr ilyagr mentioned this issue Jan 23, 2024
Merged
ilyagr added a commit that referenced this issue Jan 23, 2024
@ilyagr
test_rebase_abandoning_empty: add children of an empty @ to the t…
d168fd2 
…est case

This demonstrates the minor bug discussed in
#2766 (comment)
AKA #2869.

It's also interesting whether changing the definition of "discardable" commit
would affect this test, see
#2859 (comment)

(I think it won't, but still)
@ilyagr ilyagr added 🐛bug Something isn't working polish🪒🐃 Make existing features more convenient and more consistent labels Jan 23, 2024
ilyagr added a commit to ilyagr/jj that referenced this issue Jan 31, 2024
@ilyagr
rewrite.rs: fix working copy position after jj rebase --abandon-empty
b690ba7 
Fixes martinvonz#2869
@ilyagr ilyagr mentioned this issue Jan 31, 2024
Merged
ilyagr added a commit to ilyagr/jj that referenced this issue Jan 31, 2024
@ilyagr
rewrite.rs: fix working copy position after jj rebase --abandon-empty
952834c 
Fixes martinvonz#2869
ilyagr added a commit to ilyagr/jj that referenced this issue Jan 31, 2024
@ilyagr
rewrite.rs: fix working copy position after jj rebase --abandon-empty
195f4c0 
Fixes martinvonz#2869
ilyagr added a commit that referenced this issue Jan 31, 2024
@ilyagr
rewrite.rs: fix working copy position after jj rebase --abandon-empty
becbc88 
Fixes #2869
@ilyagr
Copy link
Collaborator Author

ilyagr commented Feb 1, 2024

I think my fix is buggy (see #2901 (comment)), and I'm not sure there's an easy way to fix that without reworking DescendantRebaser (which Martin is working on). I'm thinking of reverting that fix soon unless I have a better idea, since this bug is more of an annoyance than a real problem.

@ilyagr ilyagr reopened this Feb 1, 2024
ilyagr added a commit to ilyagr/jj that referenced this issue Feb 3, 2024
@ilyagr
jj rebase --abandon-empty: revert commits cfcc7c5 and becbc88
9290475 
This mostly reverts martinvonz#2901 as well as its
fixup martinvonz#2903. The related bug is reopened,
see martinvonz#2869 (comment).

The problem is that while the fix did fix martinvonz#2896 in most cases, it did
reintroduce the more severe bug martinvonz#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 martinvonz#2760:

```
root -> A @ -> C
         \-- B
```

with the working copy at `A`. The reason for this is explained in
martinvonz#2901 (comment).

### After this commit

After this commit, both case 1 and case 2 will be wrong in the sense of martinvonz#2896,
it will not exhibit the worse bug martinvonz#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 issue Feb 3, 2024
Merged
ilyagr added a commit to ilyagr/jj that referenced this issue Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
82b635a 
This mostly reverts martinvonz#2901 as well as its
fixup martinvonz#2903. The related bug is reopened,
see martinvonz#2869 (comment).

The problem is that while the fix did fix martinvonz#2896 in most cases, it did
reintroduce the more severe bug martinvonz#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 martinvonz#2760:

```
root -> A @ -> C
         \-- B
```

with the working copy at `A`. The reason for this is explained in
martinvonz#2901 (comment).

### After this commit

After this commit, both case 1 and case 2 will be wrong in the sense of martinvonz#2896,
but it will no longer exhibit the worse bug martinvonz#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 issue Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
f1b9b48 
This mostly reverts martinvonz#2901 as well as its
fixup martinvonz#2903. The related bug is reopened,
see martinvonz#2869 (comment).

The problem is that while the fix did fix martinvonz#2896 in most cases, it did
reintroduce the more severe bug martinvonz#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 martinvonz#2760:

```
root -> A @ -> C
         \-- B
```

with the working copy at `A`. The reason for this is explained in
martinvonz#2901 (comment).

### After this commit

After this commit, both case 1 and case 2 will be wrong in the sense of martinvonz#2896,
but it will no longer exhibit the worse bug martinvonz#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 issue Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
7aed104 
This mostly reverts martinvonz#2901 as well as its
fixup martinvonz#2903. The related bug is reopened,
see martinvonz#2869 (comment).

The problem is that while the fix did fix martinvonz#2869 in most cases, it did
reintroduce the more severe bug martinvonz#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 martinvonz#2760:

```
root -> A @ -> C
         \-- B
```

with the working copy at `A`. The reason for this is explained in
martinvonz#2901 (comment).

### After this commit

After this commit, both case 1 and case 2 will be wrong in the sense of martinvonz#2869,
but it will no longer exhibit the worse bug martinvonz#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 issue Feb 3, 2024
@ilyagr
rewrite.rs: revert commits cfcc7c5 and becbc88
13884fc 
This mostly reverts martinvonz#2901 as well as its
fixup martinvonz#2903. The related bug is reopened,
see martinvonz#2869 (comment).

The problem is that while the fix did fix martinvonz#2869 in most cases, it did
reintroduce the more severe bug martinvonz#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 martinvonz#2760:

```
root -> A @ -> C
         \-- B
```

with the working copy at `A`. The reason for this is explained in
martinvonz#2901 (comment).

### After this commit

After this commit, both case 1 and case 2 will be wrong in the sense of martinvonz#2869,
but it will no longer exhibit the worse bug martinvonz#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 ilyagr closed this as completed in d439de0 Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🐛bug Something isn't working polish🪒🐃 Make existing features more convenient and more consistent
Projects
None yet
Milestone
No milestone
1 participant
@ilyagr