From 37421583b259a73da9fc6e5c4439055c897b40d9 Mon Sep 17 00:00:00 2001 From: Essien Ita Essien <34972+essiene@users.noreply.github.com> Date: Tue, 13 Aug 2024 19:09:58 +0100 Subject: [PATCH] next/prev: Add config flag to control `prev/next` edit behaviour. * We started with 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. However, consensus the review thread is that `auto` mode where we try to infer when to switch to `edit mode`, should be removed. So `ui.movement.edit` is a boolean flag now. - true: edit mode - false: new+squash mode * Also add a `--no-edit` flag as the explicit inverse of `--edit` and ensure both flags take precedence over the config. * Update tests that assumed edit mode inference, to specify `--edit` explicitly. NOTE: #4302 was squashed into this commit, so see that closed PR for review history. Part of #3947 --- CHANGELOG.md | 8 + cli/src/commands/next.rs | 24 ++- cli/src/commands/prev.rs | 26 +-- cli/src/config/misc.toml | 3 + cli/src/movement_util.rs | 38 ++-- cli/tests/cli-reference@.md.snap | 12 +- cli/tests/test_next_prev_commands.rs | 261 +++++++++++++++++++++++---- 7 files changed, 299 insertions(+), 73 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 137c6efe7..4868bfa5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,18 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Breaking changes +* `next/prev` will no longer infer when to go into edit mode when moving from + commit to commit. It now either follows the flags `--edit|--no-edit` or it + gets the mode from `ui.movement.edit`. + ### Deprecations ### New features +* Add new boolean config knob, `ui.movement.edit` for controlling the behaviour + of `prev/next`. The flag turns `edit` mode `on` and `off` permanently when set + respectively to `true` or `false`. + * The following diff formats now include information about copies and moves: `--color-words`, `--git`, `--stat`, `--summary`, `--types`, and external diff tools in file-by-file mode. diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index 2708675f5..992884e65 100644 --- a/cli/src/commands/next.rs +++ b/cli/src/commands/next.rs @@ -51,15 +51,24 @@ use crate::ui::Ui; #[command(verbatim_doc_comment)] pub(crate) struct NextArgs { /// How many revisions to move forward. Advances to the next child by - /// default. + /// default #[arg(default_value = "1")] offset: u64, /// Instead of creating a new working-copy commit on top of the target /// commit (like `jj new`), edit the target commit directly (like `jj - /// edit`). + /// edit`) + /// + /// Takes precedence over config in `ui.movement.edit`; i.e. + /// will negate `ui.movement.edit = false` #[arg(long, short)] edit: bool, - /// Jump to the next conflicted descendant. + /// The inverse of `--edit` + /// + /// Takes precedence over config in `ui.movement.edit`; i.e. + /// will negate `ui.movement.edit = true` + #[arg(long, short, conflicts_with = "edit")] + no_edit: bool, + /// Jump to the next conflicted descendant #[arg(long, conflicts_with = "offset")] conflict: bool, } @@ -69,6 +78,7 @@ impl From<&NextArgs> for MovementArgs { MovementArgs { offset: val.offset, edit: val.edit, + no_edit: val.no_edit, conflict: val.conflict, } } @@ -79,11 +89,5 @@ pub(crate) fn cmd_next( command: &CommandHelper, args: &NextArgs, ) -> Result<(), CommandError> { - let mut workspace_command = command.workspace_helper(ui)?; - move_to_commit( - ui, - &mut workspace_command, - Direction::Next, - &MovementArgs::from(args), - ) + move_to_commit(ui, command, Direction::Next, &MovementArgs::from(args)) } diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index ed2cafc2f..0396115f5 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -45,17 +45,26 @@ use crate::ui::Ui; /// A A /// ``` /// If the working copy revision already has visible children, then `--edit` is -/// implied. +/// implied #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] pub(crate) struct PrevArgs { - /// How many revisions to move backward. Moves to the parent by default. + /// How many revisions to move backward. Moves to the parent by default #[arg(default_value = "1")] offset: u64, - /// Edit the parent directly, instead of moving the working-copy commit. + /// Edit the parent directly, instead of moving the working-copy commit + /// + /// Takes precedence over config in `ui.movement.edit`; i.e. + /// will negate `ui.movement.edit = false` #[arg(long, short)] edit: bool, - /// Jump to the previous conflicted ancestor. + /// The inverse of `--edit` + /// + /// Takes precedence over config in `ui.movement.edit`; i.e. + /// will negate `ui.movement.edit = true` + #[arg(long, short, conflicts_with = "edit")] + no_edit: bool, + /// Jump to the previous conflicted ancestor #[arg(long, conflicts_with = "offset")] conflict: bool, } @@ -65,6 +74,7 @@ impl From<&PrevArgs> for MovementArgs { MovementArgs { offset: val.offset, edit: val.edit, + no_edit: val.no_edit, conflict: val.conflict, } } @@ -75,11 +85,5 @@ pub(crate) fn cmd_prev( command: &CommandHelper, args: &PrevArgs, ) -> Result<(), CommandError> { - let mut workspace_command = command.workspace_helper(ui)?; - move_to_commit( - ui, - &mut workspace_command, - Direction::Prev, - &MovementArgs::from(args), - ) + move_to_commit(ui, command, Direction::Prev, &MovementArgs::from(args)) } diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index c96f31fcc..85a9f18ff 100644 --- a/cli/src/config/misc.toml +++ b/cli/src/config/misc.toml @@ -15,5 +15,8 @@ pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } } log-word-wrap = false log-synthetic-elided-nodes = true +[ui.movement] +edit = false + [snapshot] max-new-file-size = "1MiB" diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs index ff41d5c9e..be4bdae25 100644 --- a/cli/src/movement_util.rs +++ b/cli/src/movement_util.rs @@ -21,7 +21,7 @@ use jj_lib::commit::Commit; use jj_lib::repo::Repo; use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt}; -use crate::cli_util::{short_commit_hash, WorkspaceCommandHelper}; +use crate::cli_util::{short_commit_hash, CommandHelper, WorkspaceCommandHelper}; use crate::command_error::{user_error, CommandError}; use crate::ui::Ui; @@ -29,9 +29,17 @@ use crate::ui::Ui; pub(crate) struct MovementArgs { pub offset: u64, pub edit: bool, + pub no_edit: bool, pub conflict: bool, } +#[derive(Clone, Debug, Eq, PartialEq)] +struct MovementArgsInternal { + offset: u64, + should_edit: bool, + conflict: bool, +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) enum Direction { Next, @@ -56,12 +64,12 @@ impl Direction { fn get_target_revset( &self, working_commit_id: &CommitId, - args: &MovementArgs, + args: &MovementArgsInternal, ) -> Result, CommandError> { let wc_revset = RevsetExpression::commit(working_commit_id.clone()); // If we're editing, start at the working-copy commit. Otherwise, start from // its direct parent(s). - let start_revset = if args.edit { + let start_revset = if args.should_edit { wc_revset.clone() } else { wc_revset.parents() @@ -103,7 +111,7 @@ fn get_target_commit( workspace_command: &WorkspaceCommandHelper, direction: Direction, working_commit_id: &CommitId, - args: &MovementArgs, + args: &MovementArgsInternal, ) -> Result { let target_revset = direction.get_target_revset(working_commit_id, args)?; let targets: Vec = target_revset @@ -162,29 +170,29 @@ fn choose_commit<'a>( pub(crate) fn move_to_commit( ui: &mut Ui, - workspace_command: &mut WorkspaceCommandHelper, + command: &CommandHelper, direction: Direction, args: &MovementArgs, ) -> Result<(), CommandError> { + let mut workspace_command = command.workspace_helper(ui)?; + let current_wc_id = workspace_command .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; - let mut args = args.clone(); - args.edit = args.edit - || !&workspace_command - .repo() - .view() - .heads() - .contains(current_wc_id); + let config_edit_flag = command.settings().config().get_bool("ui.movement.edit")?; + let args = MovementArgsInternal { + should_edit: args.edit || (!args.no_edit && config_edit_flag), + offset: args.offset, + conflict: args.conflict, + }; - let target = get_target_commit(ui, workspace_command, direction, current_wc_id, &args)?; + let target = get_target_commit(ui, &workspace_command, direction, current_wc_id, &args)?; let current_short = short_commit_hash(current_wc_id); let target_short = short_commit_hash(target.id()); let cmd = direction.cmd(); - // We're editing, just move to the target commit. - if args.edit { + if args.should_edit { // We're editing, the target must be rewritable. workspace_command.check_rewritable([target.id()])?; let mut tx = workspace_command.start_transaction(); diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index fd81686e8..d749fead6 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1226,6 +1226,11 @@ implied. ###### **Options:** * `-e`, `--edit` — Instead of creating a new working-copy commit on top of the target commit (like `jj new`), edit the target commit directly (like `jj edit`) + + Takes precedence over config in `ui.movement.edit`; i.e. will negate `ui.movement.edit = false` +* `-n`, `--no-edit` — The inverse of `--edit` + + Takes precedence over config in `ui.movement.edit`; i.e. will negate `ui.movement.edit = true` * `--conflict` — Jump to the next conflicted descendant @@ -1517,7 +1522,7 @@ B B A A ``` If the working copy revision already has visible children, then `--edit` is -implied. +implied **Usage:** `jj prev [OPTIONS] [OFFSET]` @@ -1530,6 +1535,11 @@ implied. ###### **Options:** * `-e`, `--edit` — Edit the parent directly, instead of moving the working-copy commit + + Takes precedence over config in `ui.movement.edit`; i.e. will negate `ui.movement.edit = false` +* `-n`, `--no-edit` — The inverse of `--edit` + + Takes precedence over config in `ui.movement.edit`; i.e. will negate `ui.movement.edit = true` * `--conflict` — Jump to the previous conflicted ancestor diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index bea7c3911..91f539a55 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -227,8 +227,7 @@ fn test_next_parent_has_multiple_descendants() { ◆ zzzzzzzzzzzz "###); - // --edit is implied since the working copy isn't a leaf commit. - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--edit"]); insta::assert_snapshot!(stdout,@r###""###); insta::assert_snapshot!(stderr,@r###" Working copy now at: mzvwutvl 1b8531ce (empty) 4 @@ -312,8 +311,7 @@ fn test_next_on_merge_commit() { ◆ zzzzzzzzzzzz "###); - // --edit is implied since the working copy is not a leaf commit. - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--edit"]); insta::assert_snapshot!(stdout,@r###""###); insta::assert_snapshot!(stderr,@r###" Working copy now at: mzvwutvl b54bbdea (empty) 4 @@ -633,22 +631,6 @@ fn test_prev_editing() { ○ qpvuntsmwlqt first ◆ zzzzzzzzzzzz "###); - - // --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: rlvkpnrz 9ed53a4a (empty) second - Parent commit : qpvuntsm fa15625b (empty) first - "###); - - insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - ○ zsuskulnrvyr fourth - ○ kkmpptxzrspx third - @ rlvkpnrzqnoo second - ○ qpvuntsmwlqt first - ◆ zzzzzzzzzzzz - "###); } #[test] @@ -685,22 +667,6 @@ fn test_next_editing() { ○ qpvuntsmwlqt first ◆ zzzzzzzzzzzz "###); - - // --edit is implied when already editing a non-head commit - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); - insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(stderr, @r###" - Working copy now at: zsuskuln 9d7e5e99 (empty) fourth - Parent commit : kkmpptxz 30056b0c (empty) third - "###); - - insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ zsuskulnrvyr fourth - ○ kkmpptxzrspx third - ○ rlvkpnrzqnoo second - ○ qpvuntsmwlqt first - ◆ zzzzzzzzzzzz - "###); } #[test] @@ -869,6 +835,229 @@ fn test_next_conflict_head() { "###); } +#[test] +fn test_movement_edit_mode_true() { + let test_env = TestEnvironment::default(); + test_env.add_config(r#"ui.movement.edit = true"#); + + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "first"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "second"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "third"]); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ zsuskulnrvyr + ○ kkmpptxzrspx third + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + test_env.jj_cmd_ok(&repo_path, &["prev"]); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ kkmpptxzrspx third + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: rlvkpnrz 9ed53a4a (empty) second + Parent commit : qpvuntsm fa15625b (empty) first + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ○ kkmpptxzrspx third + @ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: qpvuntsm fa15625b (empty) first + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ○ kkmpptxzrspx third + ○ rlvkpnrzqnoo second + @ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: rlvkpnrz 9ed53a4a (empty) second + Parent commit : qpvuntsm fa15625b (empty) first + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ○ kkmpptxzrspx third + @ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: kkmpptxz 30056b0c (empty) third + Parent commit : rlvkpnrz 9ed53a4a (empty) second + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ kkmpptxzrspx third + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--no-edit"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: nkmrtpmo 67a001a6 (empty) (no description set) + Parent commit : qpvuntsm fa15625b (empty) first + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ nkmrtpmomlro + │ ○ kkmpptxzrspx third + │ ○ rlvkpnrzqnoo second + ├─╯ + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--no-edit"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: xznxytkn 22287813 (empty) (no description set) + Parent commit : rlvkpnrz 9ed53a4a (empty) second + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ xznxytknoqwo + │ ○ kkmpptxzrspx third + ├─╯ + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); +} + +#[test] +fn test_movement_edit_mode_false() { + let test_env = TestEnvironment::default(); + test_env.add_config(r#"ui.movement.edit = false"#); + + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "first"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "second"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "third"]); + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ zsuskulnrvyr + ○ kkmpptxzrspx third + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + test_env.jj_cmd_ok(&repo_path, &["prev"]); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ royxmykxtrkr + │ ○ kkmpptxzrspx third + ├─╯ + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--no-edit"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: vruxwmqv 087a65b1 (empty) (no description set) + Parent commit : qpvuntsm fa15625b (empty) first + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ vruxwmqvtpmx + │ ○ kkmpptxzrspx third + │ ○ rlvkpnrzqnoo second + ├─╯ + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: znkkpsqq a8419fd6 (empty) (no description set) + Parent commit : rlvkpnrz 9ed53a4a (empty) second + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ znkkpsqqskkl + │ ○ kkmpptxzrspx third + ├─╯ + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--no-edit"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: kmkuslsw 8c4d85ef (empty) (no description set) + Parent commit : kkmpptxz 30056b0c (empty) third + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ kmkuslswpqwq + ○ kkmpptxzrspx third + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--edit", "2"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: rlvkpnrz 9ed53a4a (empty) second + Parent commit : qpvuntsm fa15625b (empty) first + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + ○ kkmpptxzrspx third + @ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--edit"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Working copy now at: kkmpptxz 30056b0c (empty) third + Parent commit : rlvkpnrz 9ed53a4a (empty) second + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" + @ kkmpptxzrspx third + ○ rlvkpnrzqnoo second + ○ qpvuntsmwlqt first + ◆ zzzzzzzzzzzz + "###); +} + fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String { let template = r#"separate(" ", change_id.short(), local_branches, if(conflict, "conflict"), description)"#; test_env.jj_cmd_success(cwd, &["log", "-T", template])