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

PhilipMetzger · 2024-06-22T22:25:16Z

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.

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

martinvonz · 2024-06-23T21:29:09Z

Reminder to address #3935 (comment) if we add a config for edit mode (and presumably stop making it implied in some cases then).

essiene · 2024-08-11T20:21:47Z

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 · 2024-08-12T09:21:16Z

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

essiene · 2024-08-12T09:58:22Z

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 · 2024-08-12T12:09:21Z

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 · 2024-08-12T13:55:29Z

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

PhilipMetzger · 2024-08-12T15:16:25Z

Since this a FR based on this comment #2126 (comment), 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 · 2024-08-12T19:16:29Z

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 · 2024-08-13T04:01:48Z

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

The code in both cli/src/commands/{next,prev}.rs is identical except for the direction of movement. This no-op PR pull the parts that make sense out into cli/src/movement_util.rs so it's easier to see the differences. Part of #3947

The flag is a tristate flag where: - Auto - Maintain current behaviour. This edits if the wc parent is not a head commit. Else, it will create a new commit on the parent of the wc in the direction of movement. - Always - Always edit - Never - Never edit, prefer the new+squash workflow. Part of #3947

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Part of #3947

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Also, specifying 'No **other** descendant|ancestor...' helps make it clear what `jj` is really looking for. Part of #3947

bnjmnt4n · 2024-08-15T22:00:43Z

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.

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Also, specifying 'No **other** descendant|ancestor...' helps make it clear what `jj` is really looking for. Part of #3947

martinvonz · 2024-08-16T13:14:30Z

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 · 2024-08-16T15:23:16Z

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! 😄

Add gratuitous `jj log` output to more points in the tests. This makes it easier to understand the intended changes by literally visualizing the commit tree we are after each movement. This is at least useful for me since I find the new+squash workflow confusing. Test behaviour is not changed. Part of #3947

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Also, specifying 'No **other** descendant...' helps make it clear what `jj` is really looking for. Part of #3947

See context in [this discussion](#3935 (comment)) Fixes #3947

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Also, specifying 'No **other** descendant...' helps make it clear what `jj` is really looking for. Part of #3947

See context in [this discussion](#3935 (comment)) Fixes #3947

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Also, specifying 'No **other** descendant...' helps make it clear what `jj` is really looking for. Part of #3947

See context in [this discussion](#3935 (comment)) Fixes #3947

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Also, specifying 'No **other** descendant...' helps make it clear what `jj` is really looking for. Part of #3947

See context in [this discussion](#3935 (comment)) Fixes #3947

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Also, specifying 'No **other** descendant...' helps make it clear what `jj` is really looking for. Part of #3947

See context in [this discussion](#3935 (comment)) Fixes #3947

If movement commands don't find a target commit, they fail. However, it's usually not intuitive why they fail because in non-edit mode the start commit is the parent of the working commit. Adding the start commit change hash to the error message makes it easier for the user to figure out what is going on. Also, specifying 'No **other** descendant...' helps make it clear what `jj` is really looking for. Part of #3947

See context in [this discussion](#3935 (comment)) Fixes #3947

PhilipMetzger added good first issue Good for newcomers polish🪒🐃 Make existing features more convenient and more consistent labels Jun 22, 2024

essiene self-assigned this Aug 11, 2024

This was referenced Aug 15, 2024

no-op: Add jj log output to more prev_next tests. #4281

Merged

next/prev: refactor movement utilities into cli/src/movement_utils.rs #4282

Merged

essiene mentioned this issue Aug 15, 2024

Add config flag to control prev/next edit behaviour. #4283

Merged

This was referenced Aug 16, 2024

Update error message when no movement target is found. #4284

Merged

next/prev: Fetch descendants with more correctness #4286

Merged

joyously mentioned this issue Aug 23, 2024

FR: Better support the edit workflow #4238

Open

essiene added a commit that referenced this issue Aug 23, 2024

Fetch descendants more correctly.

acc6802

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Aug 23, 2024

Fetch descendants more correctly.

cdf6a22

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Aug 23, 2024

Fetch descendants more correctly.

3edf228

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Aug 24, 2024

Fetch descendants more correctly.

d09e424

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Aug 24, 2024

Fetch descendants more correctly.

661c1e3

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Aug 24, 2024

Fetch descendants more correctly.

b019e86

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Aug 25, 2024

Fetch descendants more correctly.

e49f750

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Aug 25, 2024

Fetch descendants more correctly.

63d5615

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Aug 27, 2024

Fetch descendants more correctly.

f695719

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Sep 12, 2024

Fetch descendants more correctly.

f12ae43

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Sep 15, 2024

Fetch descendants more correctly.

6121166

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Sep 16, 2024

Fetch descendants more correctly.

95263c1

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Sep 16, 2024

Fetch descendants more correctly.

36d637b

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Sep 16, 2024

Fetch descendants more correctly.

4231b87

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Sep 16, 2024

Fetch descendants more correctly.

a58e9f4

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Sep 17, 2024

next/prev: Fetch descendants with more correctness.

17f22af

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene added a commit that referenced this issue Sep 17, 2024

next/prev: Fetch descendants with more correctness.

5195954

See context in [this discussion](#3935 (comment)) Fixes #3947

essiene closed this as completed in #4286 Sep 17, 2024

essiene closed this as completed in e25ec53 Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PhilipMetzger commented Jun 22, 2024

martinvonz commented Jun 23, 2024

essiene commented Aug 11, 2024

bnjmnt4n commented Aug 12, 2024

essiene commented Aug 12, 2024

yuja commented Aug 12, 2024

joyously commented Aug 12, 2024

PhilipMetzger commented Aug 12, 2024

essiene commented Aug 12, 2024

yuja commented Aug 13, 2024

bnjmnt4n commented Aug 15, 2024

martinvonz commented Aug 16, 2024

essiene commented Aug 16, 2024

FR: Add a always edit mode to prev/next #3947

FR: Add a always edit mode to prev/next #3947

Comments

PhilipMetzger commented Jun 22, 2024

martinvonz commented Jun 23, 2024

essiene commented Aug 11, 2024

bnjmnt4n commented Aug 12, 2024

essiene commented Aug 12, 2024

yuja commented Aug 12, 2024

joyously commented Aug 12, 2024

PhilipMetzger commented Aug 12, 2024

essiene commented Aug 12, 2024

yuja commented Aug 13, 2024

bnjmnt4n commented Aug 15, 2024

martinvonz commented Aug 16, 2024

essiene commented Aug 16, 2024

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

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