From 9f9af78c452126985edfac0fdc8c3e66cb10e139 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Sun, 24 Apr 2022 11:52:04 -0700
Subject: [PATCH] cli: provide better error message for no-arg `jj move`

---
 src/commands.rs            | 14 ++++++++------
 tests/test_move_command.rs | 22 +++++++++-------------
 2 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/commands.rs b/src/commands.rs
index 6bda20f72..73051c94d 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -1263,13 +1263,14 @@ struct NewArgs {
 /// changes will be removed from the source. If that means that the source is
 /// now empty compared to its parent, it will be abandoned.
 #[derive(clap::Args, Clone, Debug)]
+#[clap(group(ArgGroup::new("to_move").args(&["from", "to"]).multiple(true).required(true)))]
 struct MoveArgs {
     /// Move part of this change into the destination
-    #[clap(long, default_value = "@")]
-    from: String,
+    #[clap(long)]
+    from: Option<String>,
     /// Move part of the source into this change
-    #[clap(long, default_value = "@")]
-    to: String,
+    #[clap(long)]
+    to: Option<String>,
     /// Interactively choose which parts to move
     #[clap(long, short)]
     interactive: bool,
@@ -3193,8 +3194,9 @@ fn cmd_new(ui: &mut Ui, command: &CommandHelper, args: &NewArgs) -> Result<(), C
 
 fn cmd_move(ui: &mut Ui, command: &CommandHelper, args: &MoveArgs) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
-    let source = workspace_command.resolve_single_rev(ui, &args.from)?;
-    let mut destination = workspace_command.resolve_single_rev(ui, &args.to)?;
+    let source = workspace_command.resolve_single_rev(ui, args.from.as_deref().unwrap_or("@"))?;
+    let mut destination =
+        workspace_command.resolve_single_rev(ui, args.to.as_deref().unwrap_or("@"))?;
     if source.id() == destination.id() {
         return Err(CommandError::UserError(String::from(
             "Source and destination cannot be the same.",
diff --git a/tests/test_move_command.rs b/tests/test_move_command.rs
index 0ead7af10..fc4a9e6cd 100644
--- a/tests/test_move_command.rs
+++ b/tests/test_move_command.rs
@@ -14,6 +14,8 @@
 
 use std::path::Path;
 
+use itertools::Itertools;
+
 use crate::common::TestEnvironment;
 
 pub mod common;
@@ -67,22 +69,16 @@ fn test_move() {
     o 000000000000 
     "###);
 
-    // Doesn't do anything without arguments
-    // TODO: We should make this error more helpful (saying that --from and/or --to
-    // are required)
+    // Errors out without arguments
     let stderr = test_env.jj_cmd_failure(&repo_path, &["move"]);
+    insta::assert_snapshot!(stderr.lines().take(2).join("\n"), @r###"
+    error: The following required arguments were not provided:
+        <--from <FROM>|--to <TO>>
+    "###);
+    // Errors out if source and destination are the same
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["move", "--to", "@"]);
     insta::assert_snapshot!(stderr, @"Error: Source and destination cannot be the same.
 ");
-    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    @ 0d7353584003 f
-    o e9515f21068c e
-    o bdd835cae844 d
-    | o caa4d0b23201 c
-    | o 55171e33db26 b
-    |/  
-    o 3db0a2f5b535 a
-    o 000000000000 
-    "###);
 
     // Can move from sibling, which results in the source being abandoned
     let stdout = test_env.jj_cmd_success(&repo_path, &["move", "--from", "c"]);