`jj prev` moves back by two revisions if `@` has no children but is not empty #3426

emesterhazy · 2024-04-02T16:11:23Z

Description

Here's an example:

jj log -T 'separate(" ",change_id.short(), description)'
@  umspkqwnpqvx add file2
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz
jj prev
Working copy now at: otzuvlqz 21a57e68 (empty) (no description set)
Parent commit      : zzzzzzzz 00000000 (empty) (no description set)
Added 0 files, modified 0 files, removed 2 files
jj log -T 'separate(" ",change_id.short(),description)'
@  otzuvlqzsrxt
│ ◉  umspkqwnpqvx add file2
│ ◉  nprzlkrnkpuu add file1
├─╯
◉  zzzzzzzzzzzz

Expected Behavior

I expect the state after jj prev to be this instead:

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

How to fix

I suppose it works this way because I do expect it to move by two if @ is empty:

jj log -T 'separate(" ",change_id.short(), description)'
@  umspkqwnpqvx add file2
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz
jj new
jj log -T 'separate(" ",change_id.short(), description)'
@  uoqxurltlovk
◉  umspkqwnpqvx add file2
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz
jj prev
Working copy now at: uvkovltz 265dc566 (empty) (no description set)
Parent commit      : nprzlkrn 6a17b2c5 add file1
Added 0 files, modified 0 files, removed 1 files
jj log -T 'separate(" ",change_id.short(), description)'
@  uvkovltzzxwm
│ ◉  umspkqwnpqvx add file2
├─╯
◉  nprzlkrnkpuu add file1
◉  zzzzzzzzzzzz

I think my expectation is that jj prev will move back by one if the @ revision will be abandoned once another revision is checked out (i.e. because it has no description changes). If the @ revision will not be abandoned, then jj prev should only move back by one.

The text was updated successfully, but these errors were encountered:

PhilipMetzger · 2024-04-02T17:02:44Z

I do not think this is a bug, it may be unexpected as the working copy is not treated as a "finished" commit but that is entirely accurate. It just does a jj new @- which is the root in your example.

martinvonz · 2024-04-02T17:09:41Z

I agree, I would not call it a bug; it's working as intended. But our intentions of course change if we think the current behavior is confusing. I know @hooper was also confused about the current behavior. Maybe --edit should become the default. I personally don't like that behavior, but maybe I should try to get used to it. I think I often don't actually need the new working-copy commit when I use jj prev, so it might work well for me to use the --edit form and then just remember to run jj new if i do want to make changes.

emesterhazy · 2024-04-02T17:50:25Z

~~You're right that it's simply doing jj new @-~~, so from that perspective it's not a bug. I can see that the current behavior is consistent in that it doesn't treat a completely empty @ commit differently from a non-empty @ commit. But I think that a completely empty commit is different than a non-empty one, and we acknowledge that elsewhere by, for example, abandoning completely empty commits when @ is moved off of them.

I think that the change I'm proposing would make the default behavior of jj prev more useful for the "normal" jj workflow where changes to an existing commit are made in an empty commit and squashed later (or given a name with jj describe).

The current behavior might be consistent, but I think that it reduces usability.

Edit: It's not doing jj new @- is it? It's doing jj new @--, which is only necessary because it's coded for the case where @ is an empty commit and you don't want to create a new empty commit on top of @- because that would do nothing.

PhilipMetzger · 2024-04-02T18:12:45Z

I can see that the current behavior is consistent in that it doesn't treat a completely empty @ commit differently from a non-empty @ commit. But I think that a completely empty commit is different than a non-empty one, and we acknowledge that elsewhere by, for example, abandoning completely empty commits when @ is moved off of them.

While a empty working copy is abandoned by other commands operating on the graph, I can't see next and prev working as that, it'll also break the current movement as we'd always need to adjust the working copy when moving backwards.

Here I am actually strongly in favor of keeping consistency.

I think that the change I'm proposing would make the default behavior of jj prev more useful for the "normal" jj workflow where changes to an existing commit are made in an empty commit and squashed later (or given a name with jj describe).

Define me "normal", it is just a unwritten expectation that the movement should work like hg or sl which we definitely don't need to conform to.

The current behavior might be consistent, but I think that it reduces usability.

This is purely a user perspective.

Please read this Discord thread for the last discussion and my opinion on it.

emesterhazy · 2024-04-02T18:24:20Z

While a empty working copy is abandoned by other commands operating on the graph, I can't see next and prev working as that...

jj prev already abandons the empty working copy. From another perspective you could say that the current behavior is inconsistent in that it doesn't special case the empty working copy the way that many other actions do.

it'll also break the current movement as we'd always need to adjust the working copy when moving backwards.

I don't understand this point. What do you mean by "adjust the working copy"? I want jj prev to create a new commit on @- if @ isn't an empty commit. jj prev --edit would move @ to @-. That seems fine to me? If you want to move to exactly @- then you can write that explicitly with jj new @- or jj edit @- instead of using prev.

This is purely a user perspective.

Can you expand on this? I usually view jj (the CLI program) through the lens of a human using the CLI. From that perspective I think this improves the usability of jj prev. Maybe you look at it through a different lens?

PhilipMetzger · 2024-04-02T20:31:35Z

jj prev already abandons the empty working copy. From another perspective you could say that the current behavior is inconsistent in that it doesn't special case the empty working copy the way that many other actions do.

Fair point.

I want jj prev to create a new commit on @- if @ isn't an empty commit. jj prev --edit would move @ to @-.

Why do you not use jj new then, as it achieves exactly what you want. jj prev says that it will move you to the grandparent as your working-copy is hanging on top of the parent. This also starts to treat commits differently which is not a thing I want.

This is purely a user perspective.

Can you expand on this? I usually view jj (the CLI program) through the lens of a human using the CLI. From that perspective I think this improves the usability of jj prev. Maybe you look at it through a different lens?

I'm just the implementor and a frequent user, which never thought of this use-case until now. I'm sure another user will have a differing perspective, so I'd rather wait until we have conclusive evidence that this won't work longterm.

emesterhazy · 2024-04-02T21:25:55Z

I realized something important. Earlier you said:

It just does a jj new @- which is the root in your example.

This is wrong, isn't it? It's actually doing jj new @--. Why does it use @-- instead of @-? I suppose it's because if @ is sitting on an empty commit then jj new @- is effectively a no-op. So like it or not, an empty @ commit is a special case that we need to handle for the command to be useful. The current implementation prioritizes the case where @ is empty at the expense of the case where @ is not. If it worked the other way around then someone would be complaining that jj prev doesn't work when @ is empty.

Why do you not use jj new then

I mean, why does jj prev exist at all if you can do the same thing with jj new? Personally I find it kind of hard to type jj new @-. I think it has something to to with @ and - being far apart on the keyboard and requiring the shift key to be held. It's certainly harder than typing jj prev, which I also have muscle memory for from mercurial.

This also starts to treat commits differently which is not a thing I want.

I really want to push back on the idea that this is a bad thing. I'm just another user like you said, but to me at least it really does seem like a completely empty @ commit created by jj new or jj commit is different than every other type of commit. If we didn't treat it differently it would be an ergonomics problem since we'd all probably need to manually abandon lots of empty and unneeded commits created by moving the @ pointer around as we work on things. Completely empty commits are not the same as commits which contain information, and I don't think treating the same way for the sake of consistency is a useful goal.

Edit:

The jj prev --help output even says that by default this moves backwards by one revision to the parent. That is not what happens in my example. In fact, unless you pass --edit, that's never what happens if you don't consider an empty @ commit to be a special case.

Usage: jj prev [OPTIONS] [AMOUNT]

Arguments:
  [AMOUNT]
          How many revisions to move backward. By default moves to the parent

          [default: 1]

emesterhazy · 2024-04-02T22:25:23Z

I am playing around with this and noticed that this text expectation is wrong:

https://github.com/martinvonz/jj/blob/e5b9e8ff58c23a451c4201bbfae80f7cfe5c5a6f/cli/tests/test_next_prev_commands.rs#L291-L297

@ is not sitting on an interior commit in this scenario. It's sitting at the head, which is the "fourth" commit. It's not an interior commit because the empty commit created by jj commit was discarded when we ran jj prev --edit.

Edit: I opened #3430 to fix this and review the other tests. I'll mail it out when I finish going through them.

PhilipMetzger · 2024-04-03T19:13:42Z

This is wrong, isn't it? It's actually doing jj new @--. Why does it use @-- instead of @-? I suppose it's because if @ is sitting on an empty commit then jj new @- is effectively a no-op.

The revset I posted initially was simplified, it actually uses this revset jj new @-- & ~@-, but as your working-copy is attached to the second commit, you land on the root. It only uses @-- when not editing which is the default.

See for yourself here:
https://github.com/martinvonz/jj/blob/6826be4af91e88b03c882c8f923d51f72180e48f/cli/src/commands/prev.rs#L81-L99

This also starts to treat commits differently which is not a thing I want.

I really want to push back on the idea that this is a bad thing. I'm just another user like you said, but to me at least it really does seem like a completely empty @ commit created by jj new or jj commit is different than every other type of commit.

But it really isn't, we just cleanup our garbage after moving off empty commits (we'd just have to many empty heads lying around, if it weren't done) and while prev and next operate on them, they do it out of consistency.

Completely empty commits are not the same as commits which contain information, and I don't think treating the same way for the sake of consistency is a useful goal.

I still disagree, they only get garbage collected automatically, which is a UI distinction not one in code or in a backend.

emesterhazy · 2024-04-03T20:15:07Z

How they get garbage collected doesn't really matter. My point is that jj prev is only using @-- as the target instead of @- because empty working copy commits exist and need to be handled differently. Otherwise jj prev where @ is empty would just create another empty commit on @- and do nothing. Maybe I'm wrong?

If we set aside philosophical objections about treating discardable commits differently, I think these changes would make the command more useful and consistent:

jj prev is basically an alias for jj new @-. However, if @ is discardable, it does jj new @-- instead. It never edits an existing commit.
jj prev --edit is basically an alias for jj edit @-. It doesn't care if @ is empty or not.
Of course they are not actually aliases and allow the user to select a commit when the choice is ambiguous.

PhilipMetzger · 2024-04-03T21:05:12Z

My point is that jj prev is only using @-- as the target instead of @- because empty working copy commits exist and need to be handled differently. Otherwise jj prev where @ is empty would just create another empty commit on @- and do nothing. Maybe I'm wrong?

I said it doesn't and pointed you to the code, what more can I do to convince you?

I think these changes would make the command more useful and consistent:

jj prev is basically an alias for jj new @-. However, if @ is discardable, it does jj new @-- instead. It never edits an existing commit.

jj prev always moves the working copy commit to a new empty one, it doesn't matter if @ is discardable or not. This behavior is consistent and matches the graph I've drawn in the help output.

As I've said, it takes time to get used to but is actually quite nice.

emesterhazy · 2024-04-03T21:07:10Z

I said it doesn't and pointed you to the code, what more can I do to convince you?

Sorry, I am clearly misunderstanding something. I will take another look.

emesterhazy · 2024-04-03T21:20:44Z

The revset I posted initially was simplified, it actually uses this revset jj new @-- & ~@-

I am squinting hard, but doesn't @-- & ~@- simplify to @--? This is the intersection of @-- and all revisions that are not @-, which is just @--.

I'm sorry I just don't understand what you mean. Can you give an example or point out exactly what I said that's incorrect?

Before this change, `jj prev` does this: ``` jj log -T 'separate(" ",change_id.short(), description)' @ umspkqwnpqvx add file2 ◉ nprzlkrnkpuu add file1 ◉ zzzzzzzzzzzz jj prev Working copy now at: otzuvlqz 21a57e68 (empty) (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 0 files, removed 2 files 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 ``` 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 · 2024-04-03T22:34:59Z

@PhilipMetzger and @martinvonz, when you have time, please take a look at the two pull requests below. I think they will make things clearer and I'm sorry about any confusion I might have caused in the discussion so far.

The first one addresses a few issues and inconsistencies in the tests for jj prev. There are a couple of instances where the tests are wrong or testing a scenario that doesn't match the comments in the test.

Fix a few minor issues with the jj prev tests #3430

The second pull request is my suggestion for how we should change this command to make it more useful when @ is a non-discardable tip. It does not change the implicit --edit behavior but I think that is also worth considering. This is a very simple change, and I think the code for jj prev is actually simpler now than before. Please take a look; I really think this will make the command more useful.

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

Before this change, `jj prev` does this: ``` jj log -T 'separate(" ",change_id.short(), description)' @ umspkqwnpqvx add file2 ◉ nprzlkrnkpuu add file1 ◉ zzzzzzzzzzzz jj prev Working copy now at: otzuvlqz 21a57e68 (empty) (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 0 files, removed 2 files 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 ``` 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.

…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.

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 · 2024-04-04T17:19:00Z

Hi everyone, thank you for all the time you've invested in discussing what ultimately seems to be a misunderstanding on my part.

I'm going to close this issue 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!

Improve the documentation of jj prev #3453

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 the 🐛bug Something isn't working label Apr 2, 2024

emesterhazy added polish🪒🐃 Make existing features more convenient and more consistent and removed 🐛bug Something isn't working labels Apr 2, 2024

emesterhazy mentioned this issue Apr 3, 2024

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

Closed

emesterhazy mentioned this issue Apr 4, 2024

Improve the documentation of jj prev #3453

Merged

emesterhazy closed this as completed Apr 4, 2024

emesterhazy closed this as not planned Won't fix, can't repro, duplicate, stale Apr 4, 2024

PhilipMetzger mentioned this issue May 10, 2024

FR: prev/next --rebase/--keep to carry along your current changes when moving #3665

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`jj prev` moves back by two revisions if `@` has no children but is not empty #3426

`jj prev` moves back by two revisions if `@` has no children but is not empty #3426

emesterhazy commented Apr 2, 2024

PhilipMetzger commented Apr 2, 2024

martinvonz commented Apr 2, 2024

emesterhazy commented Apr 2, 2024 •

edited

Loading

PhilipMetzger commented Apr 2, 2024

emesterhazy commented Apr 2, 2024

PhilipMetzger commented Apr 2, 2024

emesterhazy commented Apr 2, 2024 •

edited

Loading

emesterhazy commented Apr 2, 2024 •

edited

Loading

PhilipMetzger commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

PhilipMetzger commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

emesterhazy commented Apr 4, 2024 •

edited

Loading

jj prev moves back by two revisions if @ has no children but is not empty #3426

jj prev moves back by two revisions if @ has no children but is not empty #3426

Comments

emesterhazy commented Apr 2, 2024

Description

Expected Behavior

How to fix

PhilipMetzger commented Apr 2, 2024

martinvonz commented Apr 2, 2024

emesterhazy commented Apr 2, 2024 • edited Loading

PhilipMetzger commented Apr 2, 2024

emesterhazy commented Apr 2, 2024

PhilipMetzger commented Apr 2, 2024

emesterhazy commented Apr 2, 2024 • edited Loading

emesterhazy commented Apr 2, 2024 • edited Loading

PhilipMetzger commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

PhilipMetzger commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

emesterhazy commented Apr 3, 2024

emesterhazy commented Apr 4, 2024 • edited Loading

`jj prev` moves back by two revisions if `@` has no children but is not empty #3426

`jj prev` moves back by two revisions if `@` has no children but is not empty #3426

emesterhazy commented Apr 2, 2024 •

edited

Loading

emesterhazy commented Apr 2, 2024 •

edited

Loading

emesterhazy commented Apr 2, 2024 •

edited

Loading

emesterhazy commented Apr 4, 2024 •

edited

Loading