FR: Add a always `edit` mode to `prev/next`

[PhilipMetzger]

Some people don't use the new + squash workflow and always want to edit their commits which would match the behavior of sl next/prev.

[martinvonz]

Reminder to address https://github.com/martinvonz/jj/pull/3935#discussion_r1649520967 if we add a config for edit mode (and presumably stop making it implied in some cases then).

[essiene]

I don't really use the new+squash workflow myself.

What would be a good config flag name for this?

[ui]
prev-always-edit = true|false
next-always-edit = true|false

thoughts?

[bnjmnt4n]

Would this conflict with the decision of not allowing default configuration for commands to avoid inconsistent behavior in https://github.com/martinvonz/jj/issues/1509?

[essiene]

Would this conflict with the decision of not allowing default configuration for commands to avoid inconsistent behavior in #1509?

Hmmm... I'm not certain, though my own reading of the conclusion of #1509 is that allowing arbitrary defaults for any commands is the undesirable thing. This issue, IIUC, doesn't change the default for the --edit flag, it sets a default behaviour which should hold in the absence of the flag and will still respect the flag if set in either direction.

so if ui.prev-always-edit = true, but I run jj prev --edit=false I expect this to respect the commandline flag. Conversely ui.prev-always-edit = false (the intended default) with jj prev --edit should behave as expected too.

I'd like to get other opinions though, before proceeding in case I've interpreted that wrong.

[yuja]

Would this conflict with the decision of not allowing default configuration for commands

Suppose edit/squash workflow is user preference, config knob seems good. It might be also okay to remove or deprecate the --edit flag.

I personally want a config to never continue editing by jj new --insert-before && jj prev.

[joyously]

Since there is an --edit flag, wouldn't the solution be an alias, so that it follows 1509?

[PhilipMetzger]

Since this a FR based on this comment https://github.com/martinvonz/jj/issues/2126#issuecomment-1707527036, I always thought of having a ui.movement.always_edit flag, which responds to passing jj next/prev --edit.

Would this conflict with the decision of not allowing default configuration for commands

Suppose edit/squash workflow is user preference, config knob seems good.

Yes, that was always my goal here.

It might be also okay to remove or deprecate the --edit flag.

But I don't think we should deprecate it, as usually use edit to move along the stack and then use the new + squash version for dealing with conflicts.

[essiene]

To summarize the conversation:

• We agree that this should be done.
• Add a ui.movement.always_edit config flag that controls next/prev behaviour.
• The --edit flag should stay and have precedence over the config.

sg?

[yuja]

• Add a ui.movement.always_edit config flag that controls next/prev behaviour.

I don't have naming suggestion, but I think the flag should be tristate.

• always: equivalent to --edit
• auto: the current default (= deduce from wc)
• never: don't deduce

[bnjmnt4n]

Hmmm... I'm not certain, though my own reading of the conclusion of #1509 is that allowing arbitrary defaults for any commands is the undesirable thing. This issue, IIUC, doesn't change the default for the --edit flag, it sets a default behaviour which should hold in the absence of the flag and will still respect the flag if set in either direction.

My understanding of #1509 is that we should avoid configuration which can change the default behavior of commands (although I'm not sure if that's an ideal which can be met). To me this change looks like it will conflict with that description, in that it changes the default behavior of the command when no flags are specified, so different iterations of the command on different machines can have differing behavior.

[martinvonz]

I don't have naming suggestion, but I think the flag should be tristate.

• always: equivalent to --edit
• auto: the current default (= deduce from wc)
• never: don't deduce

I feel like the current behavior was just to make it more convenient for the "edit" folks without making it less convenient for the "squash" folks. If we have a config, I hope it's enough to make it a boolean. Do you think the auto mode will be useful?

[essiene]

I feel like the current behavior was just to make it more convenient for the "edit" folks without making it less convenient for the "squash" folks. If we have a config, I hope it's enough to make it a boolean. Do you think the auto mode will be useful?

fwiw, I think the auto mode and by extension, the current behaviour is rather confusing because it doesn't explicitly follow one workflow (new+squash vs inplace-edit). This personally obscured probably the most important thing about the new+squash work flow for me. I'll illustrate below.

# simple linear tree.
$ jj log -T builtin_log_oneline                                                                                 
○  ntqwupxt 34972+essiene 2024-08-15 14:31:12 5ea02047 (empty) third                                                                              
○  ntryxzss 34972+essiene 2024-08-15 14:31:08 affcd6ce (empty) second                                                                             
@  wwyttuku 34972+essiene 2024-08-15 14:31:04 b406e1b4 (empty) first                                                                              
◆  zzzzzzzz root() 00000000

# Current behaviour via config
# next and prev will happily navigate this tree.                                                                                                                     
$ jj config set --repo ui.movement.edit auto                                                                            
$ jj next                                                                                                               
Working copy now at: ntryxzss affcd6ce (empty) second                                                                                             
Parent commit      : wwyttuku b406e1b4 (empty) first
$ jj log -T builtin_log_oneline                 
○  ntqwupxt 34972+essiene 2024-08-15 14:31:12 5ea02047 (empty) third
@  ntryxzss 34972+essiene 2024-08-15 14:31:08 affcd6ce (empty) second
○  wwyttuku 34972+essiene 2024-08-15 14:31:04 b406e1b4 (empty) first
◆  zzzzzzzz root() 00000000
$ jj prev  
Working copy now at: wwyttuku b406e1b4 (empty) first
Parent commit      : zzzzzzzz 00000000 (empty) (no description set)
$ jj log -T builtin_log_oneline
○  ntqwupxt 34972+essiene 2024-08-15 14:31:12 5ea02047 (empty) third
○  ntryxzss 34972+essiene 2024-08-15 14:31:08 affcd6ce (empty) second
@  wwyttuku 34972+essiene 2024-08-15 14:31:04 b406e1b4 (empty) first
◆  zzzzzzzz root() 00000000

# Switch to `never` mode, so force the new+squash workflow.
$ jj config set --repo ui.movement.edit never
$ jj log -T builtin_log_oneline                 
○  ntqwupxt 34972+essiene 2024-08-15 14:31:12 5ea02047 (empty) third
○  ntryxzss 34972+essiene 2024-08-15 14:31:08 affcd6ce (empty) second
@  wwyttuku 34972+essiene 2024-08-15 14:31:04 b406e1b4 (empty) first
◆  zzzzzzzz root() 00000000
$ jj next
Error: No other descendant found 1 commit forward from zzzzzzzzzzzz

My first reaction was wth??!!! Actually, the original error message was more confusing: Error: No descendant found 1 commit forward which made no sense since I could see that the WC had a descendant which the auto mode happily navigated to.

Folks that have internalized the new+squash workflow intuitively know that the problem is that when navigating in the new+squash workflow, you actually navigate using the parent of the WC, not the WC itself. The UI could help with a visual hint to draw the eye more to the WC parent when in new+squash mode. So:

# Once we add a new child of the current node...
$ jj new
Working copy now at: lsuwqswy 2c7181c8 (empty) (no description set)
Parent commit      : wwyttuku b406e1b4 (empty) first
$ jj log -T builtin_log_oneline
@  lsuwqswy 34972+essiene 2024-08-16 16:13:03 2c7181c8 (empty) (no description set)
│ ○  ntqwupxt 34972+essiene 2024-08-15 14:31:12 5ea02047 (empty) third
│ ○  ntryxzss 34972+essiene 2024-08-15 14:31:08 affcd6ce (empty) second
├─╯
○  wwyttuku 34972+essiene 2024-08-15 14:31:04 b406e1b4 (empty) first
◆  zzzzzzzz root() 00000000

# Everything works now, since the new+squash workflow uses the parent as the movement anchor.
$ jj next
Working copy now at: lpttowwx aa7966fb (empty) (no description set)
Parent commit      : ntryxzss affcd6ce (empty) second
$ jj log -T builtin_log_oneline
@  lpttowwx 34972+essiene 2024-08-16 16:13:07 aa7966fb (empty) (no description set)
│ ○  ntqwupxt 34972+essiene 2024-08-15 14:31:12 5ea02047 (empty) third
├─╯
○  ntryxzss 34972+essiene 2024-08-15 14:31:08 affcd6ce (empty) second
○  wwyttuku 34972+essiene 2024-08-15 14:31:04 b406e1b4 (empty) first
◆  zzzzzzzz root() 00000000
$ 

All that to say that the current behaviour obscures this and though it is a convenience, it not consistent behaviour.

Sorry for the wall of text! 😄

[yuja]

I feel like the current behavior was just to make it more convenient for the "edit" folks without making it less convenient for the "squash" folks. If we have a config, I hope it's enough to make it a boolean. Do you think the auto mode will be useful?

No, and it would be nice if we can remove the auto.

In any case, we'll need --edit/--no-edit or --edit=<value> if we want to control the behavior by both config and command-line flag. That's unfortunate. Other than that, I don't think adding ui.movement config will introduce problems described in #1509. jj next/prev are kinda aliases of jj new.