-
Notifications
You must be signed in to change notification settings - Fork 359
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
Conversation
9b7c720
to
0afb725
Compare
cli/src/commands/prev.rs
Outdated
// 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
asjj new @-
where the minus is multiplied by the offset. In other words, move@
back by the offset and then runjj new
. Or if--edit
is supplied, don't runjj 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.
There was a problem hiding this comment.
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.
2e6ca9e
to
005f803
Compare
0afb725
to
4b64c39
Compare
There was a problem hiding this 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.
"No ancestor found {offset} commit{} back", | ||
if offset > 1 { "s" } else { "" } | ||
))) | ||
return Err(user_error("No ancestor found at requested offset.")); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
4b64c39
to
5f1c2a2
Compare
jj prev
when @
is a non-discardable tipjj prev
could not create a new revision on top of @-
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.
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! |
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.
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.
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.
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.
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.
Before this change,
jj prev
does this:After, it does this:
And
jj prev 2
does this:Without this change it is impossible to create a new commit on
@-
usingjj 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. Previouslyit refused to do this.