From c63d4ff5cc38f6720a30a385f22534b41920404e Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 11 Feb 2024 21:49:28 -0800 Subject: [PATCH 1/2] next/prev: move `current_short` variable closer to first use --- cli/src/commands/next.rs | 2 +- cli/src/commands/prev.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index f32f25b941..25d623a196 100644 --- a/cli/src/commands/next.rs +++ b/cli/src/commands/next.rs @@ -106,7 +106,6 @@ pub(crate) fn cmd_next( .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; let current_wc = workspace_command.repo().store().get_commit(current_wc_id)?; - let current_short = short_commit_hash(current_wc.id()); // If we're editing, start at the working-copy commit. // Otherwise start from our direct parent. let start_id = if edit { @@ -140,6 +139,7 @@ pub(crate) fn cmd_next( } commits => choose_commit(ui, &workspace_command, "next", commits)?, }; + let current_short = short_commit_hash(current_wc.id()); let target_short = short_commit_hash(target.id()); // We're editing, just move to the target commit. if edit { diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index de3d0e6487..b39409bf49 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -69,7 +69,6 @@ pub(crate) fn cmd_prev( .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; let current_wc = workspace_command.repo().store().get_commit(current_wc_id)?; - let current_short = short_commit_hash(current_wc.id()); let start_id = if edit { current_wc_id } else { @@ -106,6 +105,7 @@ pub(crate) fn cmd_prev( commits => choose_commit(ui, &workspace_command, "prev", commits)?, }; // Generate a short commit hash, to make it readable in the op log. + let current_short = short_commit_hash(current_wc.id()); let target_short = short_commit_hash(target.id()); // If we're editing, just move to the revision directly. if edit { From 1236c394dd469721d237c8628b0a96e90dfa282a Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 11 Feb 2024 21:56:52 -0800 Subject: [PATCH 2/2] next/prev: make --edit implied when already on non-head commit Users who edit non-head commits usually expect `jj next/prev` to continue to edit the next/previous commit, so let's make that the default behavior. This should not confuse users who don't edit non-head commits since they will simply not be in this state. My main concern is that doing `jj next; jj prev` will now usually take you back to the previous commit, but not if you started on the parent of a head commit. --- CHANGELOG.md | 3 +++ cli/src/commands/next.rs | 10 +++++++++- cli/src/commands/prev.rs | 12 ++++++++++-- cli/tests/cli-reference@.md.snap | 8 +++++++- cli/tests/test_next_prev_commands.rs | 16 +++++++++++++++- 5 files changed, 44 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8edfc05e49..a6b4da742d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * In the templating language, Timestamps now have a `.local()` method for converting to the local timezone. +* `jj next/prev` now infer `--edit` when you're already editing a non-head + commit (a commit with children). + ### Fixed bugs * On Windows, symlinks in the repo are now materialized as regular files in the diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index 25d623a196..ead6ecbf94 100644 --- a/cli/src/commands/next.rs +++ b/cli/src/commands/next.rs @@ -48,6 +48,9 @@ use crate::ui::Ui; /// B => @ /// | | /// @ A +/// +/// If your working-copy commit already has visible children, then `--edit` is +/// implied. /// ``` #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] @@ -100,11 +103,16 @@ pub(crate) fn cmd_next( args: &NextArgs, ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; - let edit = args.edit; let amount = args.amount; let current_wc_id = workspace_command .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; + let edit = args.edit + || !workspace_command + .repo() + .view() + .heads() + .contains(current_wc_id); let current_wc = workspace_command.repo().store().get_commit(current_wc_id)?; // If we're editing, start at the working-copy commit. // Otherwise start from our direct parent. diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index b39409bf49..2da2fadf2e 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -29,7 +29,7 @@ use crate::ui::Ui; /// D @ D /// |/ | /// A => A @ -/// | | / +/// | |/ /// B B /// ``` /// @@ -44,6 +44,9 @@ use crate::ui::Ui; /// B B /// | | /// A A +/// +/// If your working-copy commit already has visible children, then `--edit` is +/// implied. /// ``` // TODO(#2126): Handle multiple parents, e.g merges. #[derive(clap::Args, Clone, Debug)] @@ -63,11 +66,16 @@ pub(crate) fn cmd_prev( args: &PrevArgs, ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; - let edit = args.edit; let amount = args.amount; let current_wc_id = workspace_command .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; + let edit = args.edit + || !workspace_command + .repo() + .view() + .heads() + .contains(current_wc_id); let current_wc = workspace_command.repo().store().get_commit(current_wc_id)?; let start_id = if edit { current_wc_id diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index c78cb8916e..1571595aea 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1145,6 +1145,9 @@ C C B => @ | | @ A + +If your working-copy commit already has visible children, then `--edit` is +implied. ``` **Usage:** `jj next [OPTIONS] [AMOUNT]` @@ -1330,7 +1333,7 @@ The command moves you to the parent in a linear fashion. D @ D |/ | A => A @ -| | / +| |/ B B ``` @@ -1345,6 +1348,9 @@ C => @ B B | | A A + +If your working-copy commit already has visible children, then `--edit` is +implied. ``` **Usage:** `jj prev [OPTIONS] [AMOUNT]` diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index dbf1ad7a8a..c42c882953 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -284,6 +284,13 @@ fn test_prev_editing() { Working copy now at: zsuskuln 009f88bf (empty) fourth Parent commit : kkmpptxz 3fa8931e (empty) third "###); + // --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: yqosqzyt d2edc95b (empty) (no description set) + Parent commit : rlvkpnrz 5c52832c (empty) second + "###); } #[test] @@ -296,10 +303,17 @@ fn test_next_editing() { test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "second"]); test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "third"]); test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "fourth"]); - test_env.jj_cmd_ok(&repo_path, &["edit", "@--"]); + test_env.jj_cmd_ok(&repo_path, &["edit", "@---"]); 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 3fa8931e (empty) third + Parent commit : rlvkpnrz 5c52832c (empty) second + "###); + // --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 009f88bf (empty) fourth Parent commit : kkmpptxz 3fa8931e (empty) third "###);