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

Fix an issue where jj prev could not create a new revision on top of @- #3445

Closed
wants to merge 1 commit into from

Conversation

emesterhazy
Copy link
Contributor

@emesterhazy emesterhazy commented Apr 3, 2024

Before this change, jj prev does this:

jj log -T 'separate(" ",change_id.short(), description)'
@  umspkqwnpqvx add file2
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz
jj prev
jj log -T 'separate(" ",change_id.short(),description)'
@  otzuvlqzsrxt
│ ◉  umspkqwnpqvx add file2
│ ◉  nprzlkrnkpuu add file1
├─╯
◉  zzzzzzzzzzzz

After, it does this:

jj log -T 'separate(" ",change_id.short(), description)'
@  okxltnqrlpsr
│ ◉  umspkqwnpqvx add file2
├─╯
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz

And jj prev 2 does this:

jj log -T 'separate(" ",change_id.short(),description)'
@  otzuvlqzsrxt
│ ◉  umspkqwnpqvx add file2
│ ◉  nprzlkrnkpuu add file1
├─╯
◉  zzzzzzzzzzzz

Without this change it is impossible to create a new commit on @- using jj prev. After this change it is still possible to create a new commit on top of
@-- (the previous default behavior) specifying an offset of 2.

There is more discussion about this change in
#3426.

This commit also allows jj prev to run when @ is a merge commit. Previously
it refused to do this.

// the new empty working commit is created on the parent of the current
// on (or further back depending on the value of args.amount). If we
// didn't do this, then we wouldn't move far enough.
args.amount + 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my mental mode, the current position is determined when I ran jj new/jj edit, not when I made some changes on the working-copy commit. In colocated repo, it's basically where HEAD@git points.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Yuya, it clearly shouldn't matter if a commit is discardable or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuja @martinvonz @PhilipMetzger
Of course commands which move @ determine the current position. That's self evident. I think what you're saying is that you're opposed to treating discardable commits differently. Let's table that discussion for now.

The problem with jj prev is that there's no way to go from this:

jj log -T 'separate(" ",change_id.short(), description)'
@  umspkqwnpqvx add file2
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz

To this:

jj log -T 'separate(" ",change_id.short(), description)'
@  okxltnqrlpsr
│ ◉  umspkqwnpqvx add file2
├─╯
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz

If I specify an offset of one, jj prev, using somewhat convoluted logic, actually moves back two revisions instead of one. It is impossible to move back by one without specifying --edit, which is a problem.

I changed this PR so that the user specified offset is never changed and discardable commits aren't treated any differently. This means that running jj prev on a discardable commits gets you a new discardable commit, and as a result @ appears not to have changed. This is consistent though, and you can move back by two if you specify an offset of 2.

I will move my ergonomics change to a separate PR. I'm fine if we don't submit that, but I think this PR should be submitted because it is clearly a bug that I cannot create a new empty commit on the parent of the current commit using jj prev.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know a lot of people (like @hooper) consider the changes in the working-copy commit as significantly different from changes in the working copy in other VCS. I see them as equivalent. If that's how we want others to think of them, we might need to document that better. Alternatively, if we seem them as significantly different, we should document how they're different.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course commands which move @ determine the current position. That's self evident. I think what you're saying is that you're opposed to treating discardable commits differently. Let's table that discussion for now.

But the whole point you've made everywhere is about that, so don't chicken out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly interested in making the CLI easy to use.

While treating jj as hg right? You still think of @ as not being attached to a parent and that the working copy is actually not a commit in the graph sense. While this seems consistent from that perspective, we never cared about that anyway, atleast not in the jj I know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot create a new empty commit on the parent of the current commit using jj prev.

If I want to do that with jj prev, I would call it jj prev 0.

Copy link
Contributor Author

@emesterhazy emesterhazy Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of jj prev as jj new @- where the minus is multiplied by the offset. In other words, move @ back by the offset and then run jj new. Or if --edit is supplied, don't run jj new.

So the offset is how far @ is moved. And --edit controls whether a new empty commit is created or not.

I'd expect that jj prev 0 would not move @, so it would create a new commit on top of the current one I suppose.

Edit: I see that the current implementation of jj prev and jj next when @ is on a tip commit is basically move @ backwards by 1 and then move backwards or forwards by the user specified offset. So I guess you're always moving relative to @- instead of @.

I guess I can do what I want using jj prev 0, and maybe that's good enough (I hadn't even thought to try this). But there's still a limitation about not being able to run jj prev on a merge commit. Is that actually necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of jj prev as jj new @- where the minus is multiplied by the offset. In other words, move @ back by the offset and then run jj new. Or if --edit is supplied, don't run jj new.

I'm not against adding such mode. I think one of the current problem is that the --edit option is somewhat overloaded to describe both "the current working-copy commit is editing" and "continue editing the destination commit".

Anyway, I insist that it's confusing to change the behavior depending on wc.is_discardable(). In the "currently-editing, but not edit the destination" mode, jj prev will be equivalent to jj new @- no matter if @ is empty or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if I had tried jj next sooner I would have been a lot less confused about how these functions work, so that's on me.

It seems like if --edit is not used or implied, then next and prev move @ back by 1 and then move it according to the user specified offset and create a new commit. That behavior is consistent and useful if you're usually editing a leaf commit. I think we can say the movement in this case is relative to @-, which I did not realize before.

I will say that this behavior is a little bit surprising to me for jj prev, but I see that it's necessary for jj next to be useful and for jj prev to be its inverse. I'd like to retract this PR and my suggestion to change the movement behavior.

If you don't mind I will probably send a PR to tweak the documentation a little bit based on this discussion so that others with the same misunderstanding might be less confused.

Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-2, blocking until you've fully explained your model in the issue, which clearly disagrees with the one we've previously decided on.

cli/src/commands/prev.rs Outdated Show resolved Hide resolved
cli/src/commands/prev.rs Outdated Show resolved Hide resolved
"No ancestor found {offset} commit{} back",
if offset > 1 { "s" } else { "" }
)))
return Err(user_error("No ancestor found at requested offset."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous message was definitely clearer.

Copy link
Contributor Author

@emesterhazy emesterhazy Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And worse for i18n, which came up on another PR. IMO the user doesn't need to be reminded of the offset they specified because they just typed it into the CLI and it is probably still visible.

But I don't really care about the message. We can use the old one if people prefer.

…f `@-`

Before this change, `jj prev` does this:
```
jj log -T 'separate(" ",change_id.short(), description)'
@  umspkqwnpqvx add file2
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz
jj prev
jj log -T 'separate(" ",change_id.short(),description)'
@  otzuvlqzsrxt
│ ◉  umspkqwnpqvx add file2
│ ◉  nprzlkrnkpuu add file1
├─╯
◉  zzzzzzzzzzzz
```

After, it does this:
```
jj log -T 'separate(" ",change_id.short(), description)'
@  okxltnqrlpsr
│ ◉  umspkqwnpqvx add file2
├─╯
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz
```

And `jj prev 2` does this:
```
jj log -T 'separate(" ",change_id.short(),description)'
@  otzuvlqzsrxt
│ ◉  umspkqwnpqvx add file2
│ ◉  nprzlkrnkpuu add file1
├─╯
◉  zzzzzzzzzzzz
```

Without this change it is impossible to create a new commit on `@-` using `jj
prev`. After this change it is still possible to create a new commit on top of
`@--` (the previous default behavior) specifying an offset of 2.

There is more discussion about this change in
#3426.

This commit also allows `jj prev` to run when `@` is a merge commit. Previously
it refused to do this.
@emesterhazy emesterhazy changed the title Tweak the behavior of jj prev when @ is a non-discardable tip Fix an issue where jj prev could not create a new revision on top of @- Apr 4, 2024
emesterhazy added a commit that referenced this pull request Apr 4, 2024
This will hopefully make it clear that `jj prev` does not
move by [OFFSET] relative to `@`, which is a misconception
that I had and I think others may also have.

I am suggesting this change as a result of the vigorous discussion in
these two issues:

- #3426
- #3445

We should make similar changes to `jj next` as well since
it follows similar rules.
@emesterhazy
Copy link
Contributor Author

Per my last comment, I want to thank everyone for the time you've invested in discussing what ultimately seems to be a misunderstanding on my part.

I'm going to close this PR and have opened a new one to adjust the documentation to hopefully make things clearer for people like me. Looking forward to your feedback there. Thank you!

@emesterhazy emesterhazy closed this Apr 4, 2024
@emesterhazy emesterhazy deleted the push-zozyzovprmkp branch April 4, 2024 17:20
emesterhazy added a commit that referenced this pull request Apr 5, 2024
This will hopefully make it clear that `jj prev` does not
move by [OFFSET] relative to `@`, which is a misconception
that I had and I think others may also have.

I am suggesting this change as a result of the vigorous discussion in
these two issues:

- #3426
- #3445

We should make similar changes to `jj next` as well since
it follows similar rules.
emesterhazy added a commit that referenced this pull request Apr 5, 2024
This will hopefully make it clear that `jj prev` does not
move by [OFFSET] relative to `@`, which is a misconception
that I had and I think others may also have.

I am suggesting this change as a result of the vigorous discussion in
these two issues:

- #3426
- #3445

We should make similar changes to `jj next` as well since
it follows similar rules.
emesterhazy added a commit that referenced this pull request Apr 5, 2024
This will hopefully make it clear that `jj prev` does not
move by [OFFSET] relative to `@`, which is a misconception
that I had and I think others may also have.

I am suggesting this change as a result of the vigorous discussion in
these two issues:

- #3426
- #3445

We should make similar changes to `jj next` as well since
it follows similar rules.
emesterhazy added a commit that referenced this pull request Apr 5, 2024
This will hopefully make it clear that `jj prev` does not
move by [OFFSET] relative to `@`, which is a misconception
that I had and I think others may also have.

I am suggesting this change as a result of the vigorous discussion in
these two issues:

- #3426
- #3445

We should make similar changes to `jj next` as well since
it follows similar rules.
emesterhazy added a commit that referenced this pull request Apr 7, 2024
This will hopefully make it clear that `jj prev` does not
move by [OFFSET] relative to `@`, which is a misconception
that I had and I think others may also have.

I am suggesting this change as a result of the vigorous discussion in
these two issues:

- #3426
- #3445

We should make similar changes to `jj next` as well since
it follows similar rules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants