Skip to content

Commit

Permalink
Switch ui.movement.edit to a simple boolean.
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@essiene
essiene committed Aug 20, 2024
1 parent 8eb69a2 commit 7173dd9
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 191 deletions.
9 changes: 6 additions & 3 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion cli/src/config/misc.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
46 changes: 2 additions & 44 deletions cli/src/movement_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@
use std::io::Write;
use std::rc::Rc;

use config::Config;
use itertools::Itertools;
use jj_lib::backend::CommitId;
use jj_lib::commit::Commit;
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)]
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<Self, CommandError> {
config
.get::<Self>("ui.movement")
.map_err(|err| config_error_with_message("Invalid `ui.movement`", err))
}
}
151 changes: 8 additions & 143 deletions cli/tests/test_next_prev_commands.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down

0 comments on commit 7173dd9

Please sign in to comment.