From 666be542be8eef0617306f59923c0113b978d8e7 Mon Sep 17 00:00:00 2001 From: Essien Ita Essien <34972+essiene@users.noreply.github.com> Date: Mon, 19 Aug 2024 10:45:31 +0100 Subject: [PATCH] Switch `ui.movement.edit` to a simple boolean. * Consensus on #4283 is that `auto` mode where we try to infer when to switch to `edit mode`, should be removed. * Change `ui.movement.edit` to a simple boolean flag. true - means edit mode false- new+squash mode * Remove `MovementSettings` and related config elements as things are simpler now. * Simplify code that used the previous enough. * Update tests that assumed edit mode inference, to specify `--edit` explicitly. * Update tests to update how we specify `ui.movement.edit` Part of #3947 --- CHANGELOG.md | 9 +- cli/src/config/misc.toml | 2 +- cli/src/movement_util.rs | 46 +------- cli/tests/test_next_prev_commands.rs | 151 ++------------------------- 4 files changed, 17 insertions(+), 191 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 034bc3d4ef..380f7bf972 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,14 +9,17 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] ### 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 config knob, `ui.movement.edit` for controlling the behaviour of `prev/next`. - `auto` maintains existing behaviour while `always` and `never` turn `edit` mode - permanently `on` and `off` respectively. +* 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 diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index 00f4cc11a2..85a9f18ff9 100644 --- a/cli/src/config/misc.toml +++ b/cli/src/config/misc.toml @@ -16,7 +16,7 @@ log-word-wrap = false log-synthetic-elided-nodes = true [ui.movement] -edit = "auto" +edit = false [snapshot] max-new-file-size = "1MiB" diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs index 6cf6d8939b..30aa9cc7c1 100644 --- a/cli/src/movement_util.rs +++ b/cli/src/movement_util.rs @@ -15,7 +15,6 @@ use std::io::Write; use std::rc::Rc; -use config::Config; use itertools::Itertools; use jj_lib::backend::CommitId; use jj_lib::commit::Commit; @@ -23,7 +22,7 @@ use jj_lib::repo::Repo; use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt}; use crate::cli_util::{short_commit_hash, CommandHelper, WorkspaceCommandHelper}; -use crate::command_error::{config_error_with_message, user_error, CommandError}; +use crate::command_error::{user_error, CommandError}; use crate::ui::Ui; #[derive(Clone, Debug, Eq, PartialEq)] @@ -181,17 +180,7 @@ pub(crate) fn move_to_commit( .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; - let config_edit_flag = - match MovementSettings::try_from(command.settings().config())?.edit_mode() { - MovementEditMode::Always => true, - MovementEditMode::Never => false, - MovementEditMode::Auto => !&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, @@ -220,34 +209,3 @@ pub(crate) fn move_to_commit( tx.finish(ui, format!("{cmd}: {current_short} -> {target_short}"))?; Ok(()) } - -#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, serde::Deserialize)] -#[serde(rename_all(deserialize = "kebab-case"))] -pub enum MovementEditMode { - #[default] - Auto, - Always, - Never, -} - -#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, serde::Deserialize)] -#[serde(rename_all(deserialize = "kebab-case"))] -pub struct MovementSettings { - edit: MovementEditMode, -} - -impl MovementSettings { - fn edit_mode(&self) -> MovementEditMode { - self.edit - } -} - -impl TryFrom<&Config> for MovementSettings { - type Error = CommandError; - - fn try_from(config: &Config) -> Result { - config - .get::("ui.movement") - .map_err(|err| config_error_with_message("Invalid `ui.movement`", err)) - } -} diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 0e63eaa545..1dba660613 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 @@ -634,8 +632,7 @@ fn test_prev_editing() { ◆ zzzzzzzzzzzz "###); - // --edit is implied when already editing a non-head commit - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--edit"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Working copy now at: rlvkpnrz 9ed53a4a (empty) second @@ -686,8 +683,7 @@ fn test_next_editing() { ◆ zzzzzzzzzzzz "###); - // --edit is implied when already editing a non-head 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, @""); insta::assert_snapshot!(stderr, @r###" Working copy now at: zsuskuln 9d7e5e99 (empty) fourth @@ -870,140 +866,9 @@ fn test_next_conflict_head() { } #[test] -fn test_movement_edit_mode_auto() { - let test_env = TestEnvironment::default(); - test_env.add_config(r#"ui.movement.edit = 'auto'"#); - - 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"]); - 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 - "###); - - // next --no-edit will fail on a linear tree. - let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--no-edit"]); - insta::assert_snapshot!(stderr, @r###" - Error: No descendant found 1 commit(s) forward - "###); - - // to make it pass, first create a new commit - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["new"]); - insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(stderr, @r###" - Working copy now at: uyznsvlq db8f04ea (empty) (no description set) - Parent commit : rlvkpnrz 9ed53a4a (empty) second - "###); - - insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ uyznsvlquzzm - │ ○ kkmpptxzrspx third - ├─╯ - ○ rlvkpnrzqnoo second - ○ qpvuntsmwlqt first - ◆ zzzzzzzzzzzz - "###); - - // next --no-edit will pass now - 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: xtnwkqum e9fdcaec (empty) (no description set) - Parent commit : kkmpptxz 30056b0c (empty) third - "###); - - insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ xtnwkqumpolk - ○ kkmpptxzrspx third - ○ rlvkpnrzqnoo second - ○ qpvuntsmwlqt first - ◆ zzzzzzzzzzzz - "###); -} - -#[test] -fn test_movement_edit_mode_always() { +fn test_movement_edit_mode_true() { let test_env = TestEnvironment::default(); - test_env.add_config(r#"ui.movement.edit = 'always'"#); + 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"); @@ -1118,9 +983,9 @@ fn test_movement_edit_mode_always() { } #[test] -fn test_movement_edit_mode_never() { +fn test_movement_edit_mode_false() { let test_env = TestEnvironment::default(); - test_env.add_config(r#"ui.movement.edit = 'never'"#); + 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");