From 38d14eafe24b9d3ae4bc8b34e96b6b054928873f Mon Sep 17 00:00:00 2001
From: Aleksey Kuznetsov <zummenix@gmail.com>
Date: Sun, 3 Mar 2024 11:04:59 +0500
Subject: [PATCH] cli: enrich the error about required template value with a
 hint

Several `jj` commands accept `--template <TEMPLATE>` argument. When the argument
is empty, `jj` will show the list of defined template aliases.
---
 CHANGELOG.md                     |  4 ++++
 cli/src/cli_util.rs              | 41 +++++++++++++++++++++++++++++++-
 cli/src/command_error.rs         | 32 +++++++++++++++----------
 cli/src/template_parser.rs       |  4 ++++
 cli/tests/test_log_command.rs    | 26 ++++++++++++++++++++
 cli/tests/test_obslog_command.rs | 26 ++++++++++++++++++++
 cli/tests/test_operations.rs     | 26 ++++++++++++++++++++
 cli/tests/test_show_command.rs   | 26 ++++++++++++++++++++
 8 files changed, 172 insertions(+), 13 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index fe6699841..f1bd4baf7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -90,6 +90,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * Commands producing diffs now accept a `--context` flag for the number of
   lines of context to show.
 
+* `jj` commands with the `-T`/`--template` option now provide a hint containing
+  defined template names when no argument is given, assisting the user in making
+  a selection.
+
 ### Fixed bugs
 
 * On Windows, symlinks in the repo are now supported when Developer Mode is enabled.
diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
index ea6de9183..6938a2882 100644
--- a/cli/src/cli_util.rs
+++ b/cli/src/cli_util.rs
@@ -28,6 +28,7 @@ use std::time::SystemTime;
 use std::{fs, str};
 
 use clap::builder::{NonEmptyStringValueParser, TypedValueParser, ValueParserFactory};
+use clap::error::{ContextKind, ContextValue};
 use clap::{Arg, ArgAction, ArgMatches, Command, FromArgMatches};
 use indexmap::{IndexMap, IndexSet};
 use itertools::Itertools;
@@ -2525,7 +2526,8 @@ impl CliRunner {
             &self.tracing_subscription,
             &string_args,
             &mut layered_configs,
-        )?;
+        )
+        .map_err(|err| map_clap_cli_error(err, ui, &layered_configs))?;
         for process_global_args_fn in self.process_global_args_fns {
             process_global_args_fn(ui, &matches)?;
         }
@@ -2599,3 +2601,40 @@ impl CliRunner {
         exit_code
     }
 }
+
+fn map_clap_cli_error(
+    cmd_err: CommandError,
+    ui: &Ui,
+    layered_configs: &LayeredConfigs,
+) -> CommandError {
+    let CommandError::ClapCliError { err, hint: None } = &cmd_err else {
+        return cmd_err;
+    };
+    if let (Some(ContextValue::String(arg)), Some(ContextValue::String(value))) = (
+        err.get(ContextKind::InvalidArg),
+        err.get(ContextKind::InvalidValue),
+    ) {
+        if arg.as_str() == "--template <TEMPLATE>" && value.is_empty() {
+            // Suppress the error, it's less important than the original error.
+            if let Ok(template_aliases) = load_template_aliases(ui, layered_configs) {
+                return CommandError::ClapCliError {
+                    err: err.clone(),
+                    hint: Some(format_template_aliases_hint(&template_aliases)),
+                };
+            }
+        }
+    }
+    cmd_err
+}
+
+fn format_template_aliases_hint(template_aliases: &TemplateAliasesMap) -> String {
+    let mut hint = String::from("The following template aliases are defined:\n");
+    hint.push_str(
+        &template_aliases
+            .symbol_names()
+            .sorted_unstable()
+            .map(|name| format!("- {name}"))
+            .join("\n"),
+    );
+    hint
+}
diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs
index 2621fb0ad..f5a34447f 100644
--- a/cli/src/command_error.rs
+++ b/cli/src/command_error.rs
@@ -51,7 +51,10 @@ pub enum CommandError {
     /// Invalid command line
     CliError(String),
     /// Invalid command line detected by clap
-    ClapCliError(Arc<clap::Error>),
+    ClapCliError {
+        err: Arc<clap::Error>,
+        hint: Option<String>,
+    },
     BrokenPipe,
     InternalError(Arc<dyn error::Error + Send + Sync>),
 }
@@ -414,7 +417,10 @@ impl From<FsPathParseError> for CommandError {
 
 impl From<clap::Error> for CommandError {
     fn from(err: clap::Error) -> Self {
-        CommandError::ClapCliError(Arc::new(err))
+        CommandError::ClapCliError {
+            err: Arc::new(err),
+            hint: None,
+        }
     }
 }
 
@@ -468,14 +474,14 @@ fn try_handle_command_result(
             writeln!(ui.error(), "Error: {message}")?;
             Ok(ExitCode::from(2))
         }
-        Err(CommandError::ClapCliError(inner)) => {
+        Err(CommandError::ClapCliError { err, hint }) => {
             let clap_str = if ui.color() {
-                inner.render().ansi().to_string()
+                err.render().ansi().to_string()
             } else {
-                inner.render().to_string()
+                err.render().to_string()
             };
 
-            match inner.kind() {
+            match err.kind() {
                 clap::error::ErrorKind::DisplayHelp
                 | clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand => {
                     ui.request_pager()
@@ -484,16 +490,18 @@ fn try_handle_command_result(
             };
             // Definitions for exit codes and streams come from
             // https://github.com/clap-rs/clap/blob/master/src/error/mod.rs
-            match inner.kind() {
+            match err.kind() {
                 clap::error::ErrorKind::DisplayHelp | clap::error::ErrorKind::DisplayVersion => {
                     write!(ui.stdout(), "{clap_str}")?;
-                    Ok(ExitCode::SUCCESS)
-                }
-                _ => {
-                    write!(ui.stderr(), "{clap_str}")?;
-                    Ok(ExitCode::from(2))
+                    return Ok(ExitCode::SUCCESS);
                 }
+                _ => {}
             }
+            write!(ui.stderr(), "{clap_str}")?;
+            if let Some(hint) = hint {
+                writeln!(ui.hint(), "Hint: {hint}")?;
+            }
+            Ok(ExitCode::from(2))
         }
         Err(CommandError::BrokenPipe) => {
             // A broken pipe is not an error, but a signal to exit gracefully.
diff --git a/cli/src/template_parser.rs b/cli/src/template_parser.rs
index 8bad4af9a..89a545993 100644
--- a/cli/src/template_parser.rs
+++ b/cli/src/template_parser.rs
@@ -561,6 +561,10 @@ impl TemplateAliasesMap {
         Self::default()
     }
 
+    pub fn symbol_names(&self) -> impl Iterator<Item = &str> {
+        self.symbol_aliases.keys().map(|s| s.as_str())
+    }
+
     /// Adds new substitution rule `decl = defn`.
     ///
     /// Returns error if `decl` is invalid. The `defn` part isn't checked. A bad
diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs
index b0997b68d..18561523c 100644
--- a/cli/tests/test_log_command.rs
+++ b/cli/tests/test_log_command.rs
@@ -28,6 +28,32 @@ fn test_log_with_empty_revision() {
     "###);
 }
 
+#[test]
+fn test_log_with_no_template() {
+    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");
+
+    let stderr = test_env.jj_cmd_cli_error(&repo_path, &["log", "-T"]);
+    insta::assert_snapshot!(stderr, @r###"
+    error: a value is required for '--template <TEMPLATE>' but none was supplied
+
+    For more information, try '--help'.
+    Hint: The following template aliases are defined:
+    - builtin_change_id_with_hidden_and_divergent_info
+    - builtin_log_comfortable
+    - builtin_log_compact
+    - builtin_log_detailed
+    - builtin_log_oneline
+    - builtin_op_log_comfortable
+    - builtin_op_log_compact
+    - commit_summary_separator
+    - description_placeholder
+    - email_placeholder
+    - name_placeholder
+    "###);
+}
+
 #[test]
 fn test_log_with_or_without_diff() {
     let test_env = TestEnvironment::default();
diff --git a/cli/tests/test_obslog_command.rs b/cli/tests/test_obslog_command.rs
index 296918b05..5341637de 100644
--- a/cli/tests/test_obslog_command.rs
+++ b/cli/tests/test_obslog_command.rs
@@ -235,3 +235,29 @@ fn test_obslog_squash() {
          (empty) second
     "###);
 }
+
+#[test]
+fn test_obslog_with_no_template() {
+    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");
+
+    let stderr = test_env.jj_cmd_cli_error(&repo_path, &["obslog", "-T"]);
+    insta::assert_snapshot!(stderr, @r###"
+    error: a value is required for '--template <TEMPLATE>' but none was supplied
+
+    For more information, try '--help'.
+    Hint: The following template aliases are defined:
+    - builtin_change_id_with_hidden_and_divergent_info
+    - builtin_log_comfortable
+    - builtin_log_compact
+    - builtin_log_detailed
+    - builtin_log_oneline
+    - builtin_op_log_comfortable
+    - builtin_op_log_compact
+    - commit_summary_separator
+    - description_placeholder
+    - email_placeholder
+    - name_placeholder
+    "###);
+}
diff --git a/cli/tests/test_operations.rs b/cli/tests/test_operations.rs
index d60694a8c..0444da960 100644
--- a/cli/tests/test_operations.rs
+++ b/cli/tests/test_operations.rs
@@ -93,6 +93,32 @@ fn test_op_log() {
     "###);
 }
 
+#[test]
+fn test_op_log_with_no_template() {
+    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");
+
+    let stderr = test_env.jj_cmd_cli_error(&repo_path, &["op", "log", "-T"]);
+    insta::assert_snapshot!(stderr, @r###"
+    error: a value is required for '--template <TEMPLATE>' but none was supplied
+
+    For more information, try '--help'.
+    Hint: The following template aliases are defined:
+    - builtin_change_id_with_hidden_and_divergent_info
+    - builtin_log_comfortable
+    - builtin_log_compact
+    - builtin_log_detailed
+    - builtin_log_oneline
+    - builtin_op_log_comfortable
+    - builtin_op_log_compact
+    - commit_summary_separator
+    - description_placeholder
+    - email_placeholder
+    - name_placeholder
+    "###);
+}
+
 #[test]
 fn test_op_log_limit() {
     let test_env = TestEnvironment::default();
diff --git a/cli/tests/test_show_command.rs b/cli/tests/test_show_command.rs
index 8cee1a4be..baa681ffc 100644
--- a/cli/tests/test_show_command.rs
+++ b/cli/tests/test_show_command.rs
@@ -48,6 +48,32 @@ fn test_show_with_template() {
     "###);
 }
 
+#[test]
+fn test_show_with_no_template() {
+    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");
+
+    let stderr = test_env.jj_cmd_cli_error(&repo_path, &["show", "-T"]);
+    insta::assert_snapshot!(stderr, @r###"
+    error: a value is required for '--template <TEMPLATE>' but none was supplied
+
+    For more information, try '--help'.
+    Hint: The following template aliases are defined:
+    - builtin_change_id_with_hidden_and_divergent_info
+    - builtin_log_comfortable
+    - builtin_log_compact
+    - builtin_log_detailed
+    - builtin_log_oneline
+    - builtin_op_log_comfortable
+    - builtin_op_log_compact
+    - commit_summary_separator
+    - description_placeholder
+    - email_placeholder
+    - name_placeholder
+    "###);
+}
+
 #[test]
 fn test_show_relative_timestamps() {
     let test_env = TestEnvironment::default();