From 343d00522c6721deea89da1ece95316f175f1982 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 29 Sep 2024 14:25:04 -0700 Subject: [PATCH] cli `new`: Allow `--before/--after` without a value to default to `@` Without this, I find it a bit jarring that `jj new` works but `jj new --before` does not. By contrast, since `jj rebase` does not currently work, I don't think `jj rebase --before` should either. Note that `jj new --before @ another_revision` is invalid, so `jj new --before another_revision` can only be parsed correctly in one way. I am slightly concerned that `clap` might forbids this in the future even in the cases where a human can tell there is no ambiguity, but I'm hoping for the best. --- CHANGELOG.md | 3 + cli/src/commands/new.rs | 14 ++-- cli/tests/cli-reference@.md.snap | 4 +- cli/tests/test_new_command.rs | 110 +++++++++++++++++++++++++++++-- 4 files changed, 118 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a4f890a3e..11bf875493 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * Color author and committer names yellow +* `jj new --insert-before` (without an argument) is now equivalent to +`jj new --insert-before @`; same for `--insert-after`. + ### Fixed bugs * Update working copy before reporting changes. This prevents errors during reporting diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index 70a30cab72..b36aca03f4 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -63,20 +63,26 @@ pub(crate) struct NewArgs { /// No-op flag to pair with --no-edit #[arg(long, hide = true)] _edit: bool, - /// Insert the new change after the given commit(s) + /// Insert the new change after the given commit(s), defaults to `@` #[arg( long, short = 'A', visible_alias = "after", - conflicts_with = "revisions" + conflicts_with = "revisions", + default_missing_value = "@", + // At most one argument per --after, does not restrict repeating --after + num_args=0..=1, )] insert_after: Vec, - /// Insert the new change before the given commit(s) + /// Insert the new change before the given commit(s), defaults to `@` #[arg( long, short = 'B', visible_alias = "before", - conflicts_with = "revisions" + conflicts_with = "revisions", + default_missing_value = "@", + // At most one argument per --before, does not restrict repeating --before + num_args=0..=1, )] insert_before: Vec, } diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 07d888757c..4e003bbd27 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1268,8 +1268,8 @@ For more information, see https://martinvonz.github.io/jj/latest/working-copy/. * `-m`, `--message ` — The change description to use * `--no-edit` — Do not edit the newly created change -* `-A`, `--insert-after ` — Insert the new change after the given commit(s) -* `-B`, `--insert-before ` — Insert the new change before the given commit(s) +* `-A`, `--insert-after ` — Insert the new change after the given commit(s), defaults to `@` +* `-B`, `--insert-before ` — Insert the new change before the given commit(s), defaults to `@` diff --git a/cli/tests/test_new_command.rs b/cli/tests/test_new_command.rs index 35f9ec87ee..b00f8b171b 100644 --- a/cli/tests/test_new_command.rs +++ b/cli/tests/test_new_command.rs @@ -224,13 +224,109 @@ fn test_new_insert_after() { // --after cannot be used with revisions let stderr = test_env.jj_cmd_cli_error(&repo_path, &["new", "--after", "B", "D"]); - insta::assert_snapshot!(stderr, @r###" - error: the argument '--insert-after ' cannot be used with '[REVISIONS]...' + insta::assert_snapshot!(stderr, @r#" + error: the argument '--insert-after []' cannot be used with '[REVISIONS]...' - Usage: jj new --insert-after [REVISIONS]... + Usage: jj new --insert-after [] [REVISIONS]... For more information, try '--help'. + "#); +} + +#[test] +fn test_new_insert_before_after_no_arg() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + setup_before_insertion(&test_env, &repo_path); + test_env.jj_cmd_ok(&repo_path, &["edit", "-r", "B"]); + insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" + ○ F + ├─╮ + │ ○ E + ○ │ D + ├─╯ + │ ○ C + │ @ B + │ ○ A + ├─╯ + ◆ root "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["new", "-m", "G", "--after"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r#" + Rebased 1 descendant commits + Working copy now at: nkmrtpmo cf1ca757 (empty) G + Parent commit : kkmpptxz bfd4157e B | (empty) B + "#); + insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r#" + ○ C + @ G + ○ B + ○ A + │ ○ F + │ ├─╮ + │ │ ○ E + ├───╯ + │ ○ D + ├─╯ + ◆ root + "#); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["new", "-m", "H", "--before"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r#" + Rebased 2 descendant commits + Working copy now at: xznxytkn f9f74f27 (empty) H + Parent commit : kkmpptxz bfd4157e B | (empty) B + "#); + insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r#" + ○ C + ○ G + @ H + ○ B + ○ A + │ ○ F + │ ├─╮ + │ │ ○ E + ├───╯ + │ ○ D + ├─╯ + ◆ root + "#); + + // Not necessarily recommended, but allowed + let (stdout, stderr) = + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "I", "--after", "--after", "D"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r#" + Rebased 3 descendant commits + Working copy now at: nmzmmopx 56056dac (empty) I + Parent commit : xznxytkn f9f74f27 (empty) H + Parent commit : vruxwmqv c9257eff D | (empty) D + "#); + insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r#" + ○ C + ○ G + │ ○ F + ╭─┤ + @ │ I + ├───╮ + │ │ ○ D + ○ │ │ H + ○ │ │ B + ○ │ │ A + ├───╯ + │ ○ E + ├─╯ + ◆ root + "#); + + let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "--before", "--after"]); + insta::assert_snapshot!(stderr, @r#" + Error: Refusing to create a loop: commit 56056dac5136 would be both an ancestor and a descendant of the new commit + "#); } #[test] @@ -346,13 +442,13 @@ fn test_new_insert_before() { // --before cannot be used with revisions let stderr = test_env.jj_cmd_cli_error(&repo_path, &["new", "--before", "B", "D"]); - insta::assert_snapshot!(stderr, @r###" - error: the argument '--insert-before ' cannot be used with '[REVISIONS]...' + insta::assert_snapshot!(stderr, @r#" + error: the argument '--insert-before []' cannot be used with '[REVISIONS]...' - Usage: jj new --insert-before [REVISIONS]... + Usage: jj new --insert-before [] [REVISIONS]... For more information, try '--help'. - "###); + "#); } #[test]