From aba64c366de8baf285a568c37d370f4f1d9ff3b1 Mon Sep 17 00:00:00 2001 From: Daniel Ploch Date: Fri, 19 Jan 2024 12:49:13 -0500 Subject: [PATCH] next: support prompting for ambiguous commit targets Resolves part of issue #2126 --- cli/src/commands/next.rs | 46 +++++++++++++--- cli/src/commands/prev.rs | 3 +- cli/tests/test_next_prev_commands.rs | 81 +++++++++++++++++++++++++--- 3 files changed, 116 insertions(+), 14 deletions(-) diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index b05ebeceb5..1fc57063ba 100644 --- a/cli/src/commands/next.rs +++ b/cli/src/commands/next.rs @@ -12,6 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::io::Write; +use std::ops::Deref; + use itertools::Itertools; use jj_lib::commit::Commit; use jj_lib::repo::Repo; @@ -45,7 +48,6 @@ use crate::ui::Ui; /// B => @ /// | | /// @ A -// TODO(#2126): Handle multiple child revisions properly. #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] pub(crate) struct NextArgs { @@ -60,6 +62,42 @@ pub(crate) struct NextArgs { edit: bool, } +pub fn choose_commit<'a>( + ui: &mut Ui, + cmd: &str, + commits: &'a [Commit], +) -> Result<&'a Commit, CommandError> { + writeln!(ui.stdout(), "ambiguous {cmd} commit, choose one to target:")?; + for (i, commit) in commits.iter().enumerate() { + writeln!( + ui.stdout(), + "{}: [{}] {}", + i + 1, + short_commit_hash(commit.id()), + commit.description().lines().next().unwrap_or_default() + )?; + } + writeln!(ui.stdout(), "q: quit the prompt")?; + + let mut choices: Vec = Default::default(); + for i in 1..(commits.len() + 1) { + choices.push(format!("{i}")); + } + choices.push("q".to_string()); + let refs: Vec<&str> = choices.iter().map(|s| s.deref()).collect(); + + let choice = ui.prompt_choice( + "enter the index of the commit you want to target", + &refs, + None, + )?; + if choice == "q" { + return Err(user_error("ambiguous target commit")); + } + + Ok(&commits[choice.parse::().unwrap() - 1]) +} + pub(crate) fn cmd_next( ui: &mut Ui, command: &CommandHelper, @@ -104,11 +142,7 @@ pub(crate) fn cmd_next( if amount > 1 { "s" } else { "" } ))); } - _ => { - // TODO(#2126) We currently cannot deal with multiple children, which result - // from branches. Prompt the user for resolution. - return Err(user_error("Ambiguous target commit")); - } + commits => choose_commit(ui, "next", commits)?, }; let target_short = short_commit_hash(target.id()); // We're editing, just move to the target commit. diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index 8056edefeb..a8f2224eb6 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -17,6 +17,7 @@ use jj_lib::repo::Repo; use jj_lib::revset::{RevsetExpression, RevsetIteratorExt}; use crate::cli_util::{short_commit_hash, user_error, CommandError, CommandHelper}; +use crate::commands::next::choose_commit; use crate::ui::Ui; /// Move the working copy commit to the parent of the current revision. @@ -101,7 +102,7 @@ pub(crate) fn cmd_prev( if amount > 1 { "s" } else { "" } ))) } - _ => return Err(user_error("Ambiguous target commit")), + commits => choose_commit(ui, "prev", commits)?, }; // Generate a short commit hash, to make it readable in the op log. let target_short = short_commit_hash(target.id()); diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index 28b4b483a5..03b4e4179b 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -13,7 +13,7 @@ // limitations under the License. // -use crate::common::TestEnvironment; +use crate::common::{get_stderr_string, get_stdout_string, TestEnvironment}; pub mod common; @@ -135,7 +135,7 @@ fn test_next_fails_on_merge_commit() { } #[test] -fn test_next_fails_on_branching_children() { +fn test_next_fails_on_branching_children_no_stdin() { // TODO(#2126): Fix this behavior let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); @@ -145,10 +145,69 @@ fn test_next_fails_on_branching_children() { test_env.jj_cmd_ok(&repo_path, &["co", "@--"]); test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "third"]); test_env.jj_cmd_ok(&repo_path, &["co", "@--"]); + // Try to advance the working copy commit. - let stderr = test_env.jj_cmd_failure(&repo_path, &["next"]); + let assert = test_env.jj_cmd(&repo_path, &["next"]).assert().code(255); + let stderr = test_env.normalize_output(&get_stderr_string(&assert)); + insta::assert_snapshot!(stderr,@r###" + Internal error: I/O error: Cannot prompt for input since the output is not connected to a terminal + "###); +} + +#[test] +fn test_next_fails_on_branching_children_quit_prompt() { + // TODO(#2126): Fix this behavior + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + 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, &["co", "@--"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "third"]); + test_env.jj_cmd_ok(&repo_path, &["co", "@--"]); + + // Try to advance the working copy commit. + let assert = test_env + .jj_cmd_stdin(&repo_path, &["next"], "q\n") + .assert() + .code(1); + let stdout = test_env.normalize_output(&get_stdout_string(&assert)); + let stderr = test_env.normalize_output(&get_stderr_string(&assert)); + insta::assert_snapshot!(stdout,@r###" + ambiguous next commit, choose one to target: + 1: [40a959a0f311] third + 2: [5c52832c3483] second + q: quit the prompt + enter the index of the commit you want to target: + "###); + insta::assert_snapshot!(stderr,@r###" + Error: ambiguous target commit + "###); +} + +#[test] +fn test_next_choose_branching_child() { + // TODO(#2126): Fix this behavior + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + 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, &["co", "@--"]); + test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "third"]); + test_env.jj_cmd_ok(&repo_path, &["co", "@--"]); + // Advance the working copy commit. + let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["next"], "1\n"); + insta::assert_snapshot!(stdout,@r###" + ambiguous next commit, choose one to target: + 1: [40a959a0f311] third + 2: [5c52832c3483] second + q: quit the prompt + enter the index of the commit you want to target: + "###); insta::assert_snapshot!(stderr,@r###" - Error: Ambiguous target commit + Working copy now at: yqosqzyt f9f7101a (empty) (no description set) + Parent commit : zsuskuln 40a959a0 (empty) third "###); } @@ -184,10 +243,18 @@ fn test_prev_fails_on_multiple_parents() { // Create a merge commit, which has two parents. test_env.jj_cmd_ok(&repo_path, &["new", "left", "right"]); test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "merge"]); - // We have more than one parent, prev fails. - let stderr = test_env.jj_cmd_failure(&repo_path, &["prev"]); + // Advance the working copy commit. + let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev"], "2\n"); + insta::assert_snapshot!(stdout,@r###" + ambiguous prev commit, choose one to target: + 1: [edad76e9bd8c] second + 2: [5ae1a6a5250a] first + q: quit the prompt + enter the index of the commit you want to target: + "###); insta::assert_snapshot!(stderr,@r###" - Error: Ambiguous target commit + Working copy now at: yostqsxw a9de0711 (empty) (no description set) + Parent commit : qpvuntsm 5ae1a6a5 left | (empty) first "###); }