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

[emesterhazy]

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.

[PhilipMetzger]

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]

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]

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]

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]

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]

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]

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]

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

jj/cli/tests/test_next_prev_commands.rs

Lines 291 to 297 in e5b9e8f

	// --edit is implied when already editing a non-head commit
	let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev"]);
	insta::assert_snapshot!(stdout, @"");
	insta::assert_snapshot!(stderr, @r###"
	Working copy now at: yqosqzyt d2edc95b (empty) (no description set)
	Parent commit : rlvkpnrz 5c52832c (empty) second
	"###);

@ 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]

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:

jj/cli/src/commands/prev.rs

Lines 81 to 99 in 6826be4

	let start_id = if edit {
	current_wc_id
	} else {
	match current_wc.parent_ids() {
	[parent_id] => parent_id,
	_ => return Err(user_error("Cannot run `jj prev` on a merge commit")),
	}
	};
	let ancestor_expression = RevsetExpression::commit(start_id.clone()).ancestors_at(amount);
	let target_revset = if edit {
	ancestor_expression
	} else {
	// Jujutsu will always create a new commit for prev, even where Mercurial cannot
	// and fails. The decision and all discussion around it are available
	// here: https://github.com/martinvonz/jj/pull/1200#discussion_r1298623933
	//
	// If users ever request erroring out, add `.ancestors()` to the revset below.
	ancestor_expression.minus(&RevsetExpression::commit(current_wc_id.clone()))
	};

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]

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]

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]

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]

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?

[emesterhazy]

@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

[emesterhazy]

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