ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

next/prev: Add config flag to control prev/next edit behaviour.

Browse source 
* 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
This commit is contained in:
Essien Ita Essien 2024-08-13 19:09:58 +01:00 committed by Essien Ita Essien
parent e3fcae2152
commit bb018a54c3
7 changed files with 299 additions and 73 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
CHANGELOG.md
View file

@ -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.

24
cli/src/commands/next.rs
View file

@ -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))
}

26
cli/src/commands/prev.rs
View file

@ -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))
}

3
cli/src/config/misc.toml
View file

@ -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"

38
cli/src/movement_util.rs
View file

@ -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<Rc<RevsetExpression>, 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<Commit, CommandError> {
let target_revset = direction.get_target_revset(working_commit_id, args)?;
let targets: Vec<Commit> = 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();

12
cli/tests/cli-reference@.md.snap
View file

@ -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

261
cli/tests/test_next_prev_commands.rs
View file

@ -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])