Skip to content

Commit

Permalink
cli: enrich the error about required template value with a hint
Browse files Browse the repository at this point in the history
Several `jj` commands accept `--template <TEMPLATE>` argument. When the argument
is empty, `jj` will show the list of defined symbol names.
  • Loading branch information
zummenix committed Mar 3, 2024
1 parent f135e09 commit 28e2d12
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 18 deletions.
22 changes: 11 additions & 11 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,21 +176,21 @@ impl TracingSubscription {
}
}

pub struct CommandHelper {
pub struct CommandHelper<'c> {
app: Command,
cwd: PathBuf,
string_args: Vec<String>,
matches: ArgMatches,
global_args: GlobalArgs,
settings: UserSettings,
layered_configs: LayeredConfigs,
layered_configs: &'c LayeredConfigs,
commit_template_extension: Option<Arc<dyn CommitTemplateLanguageExtension>>,
maybe_workspace_loader: Result<WorkspaceLoader, CommandError>,
store_factories: StoreFactories,
working_copy_factories: HashMap<String, Box<dyn WorkingCopyFactory>>,
}

impl CommandHelper {
impl<'c> CommandHelper<'c> {
pub fn app(&self) -> &Command {
&self.app
}
Expand Down Expand Up @@ -231,7 +231,7 @@ impl CommandHelper {
/// For most commands that depend on a loaded repo, you should use
/// `WorkspaceCommandHelper::template_aliases_map()` instead.
pub fn load_template_aliases(&self, ui: &Ui) -> Result<TemplateAliasesMap, CommandError> {
load_template_aliases(ui, &self.layered_configs)
load_template_aliases(ui, self.layered_configs)
}

pub fn workspace_loader(&self) -> Result<&WorkspaceLoader, CommandError> {
Expand Down Expand Up @@ -382,7 +382,7 @@ impl WorkspaceCommandHelper {
workspace: Workspace,
repo: Arc<ReadonlyRepo>,
) -> Result<Self, CommandError> {
let revset_aliases_map = load_revset_aliases(ui, &command.layered_configs)?;
let revset_aliases_map = load_revset_aliases(ui, command.layered_configs)?;
let template_aliases_map = command.load_template_aliases(ui)?;
// Parse commit_summary template early to report error before starting mutable
// operation.
Expand Down Expand Up @@ -1677,7 +1677,7 @@ pub fn update_working_copy(
Ok(stats)
}

fn load_template_aliases(
pub fn load_template_aliases(
ui: &Ui,
layered_configs: &LayeredConfigs,
) -> Result<TemplateAliasesMap, CommandError> {
Expand Down Expand Up @@ -2471,7 +2471,7 @@ impl CliRunner {
fn run_internal(
self,
ui: &mut Ui,
mut layered_configs: LayeredConfigs,
layered_configs: &mut LayeredConfigs,
) -> Result<(), CommandError> {
// `cwd` is canonicalized for consistency with `Workspace::workspace_root()` and
// to easily compute relative paths between them.
Expand Down Expand Up @@ -2501,7 +2501,7 @@ impl CliRunner {
&self.app,
&self.tracing_subscription,
&string_args,
&mut layered_configs,
layered_configs,
)?;
for process_global_args_fn in self.process_global_args_fns {
process_global_args_fn(ui, &matches)?;
Expand Down Expand Up @@ -2567,11 +2567,11 @@ impl CliRunner {
.build()
.unwrap();
}
let layered_configs = LayeredConfigs::from_environment(default_config);
let mut layered_configs = LayeredConfigs::from_environment(default_config);
let mut ui = Ui::with_config(&layered_configs.merge())
.expect("default config should be valid, env vars are stringly typed");
let result = self.run_internal(&mut ui, layered_configs);
let exit_code = handle_command_result(&mut ui, result);
let result = self.run_internal(&mut ui, &mut layered_configs);
let exit_code = handle_command_result(&mut ui, &layered_configs, result);
ui.finalize_pager();
exit_code
}
Expand Down
62 changes: 55 additions & 7 deletions cli/src/command_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::collections::HashMap;
use std::io::Write as _;
use std::process::ExitCode;
use std::sync::Arc;
use std::{error, io, iter, str};

use clap::error::{ContextKind, ContextValue};
use itertools::Itertools as _;
use jj_lib::backend::BackendError;
use jj_lib::git::{GitConfigParseError, GitExportError, GitImportError, GitRemoteManagementError};
Expand All @@ -35,10 +37,12 @@ use jj_lib::working_copy::{ResetError, SnapshotError, WorkingCopyStateError};
use jj_lib::workspace::WorkspaceInitError;
use thiserror::Error;

use crate::cli_util::load_template_aliases;
use crate::config::LayeredConfigs;
use crate::merge_tools::{
ConflictResolveError, DiffEditError, DiffGenerateError, MergeToolConfigError,
};
use crate::template_parser::{TemplateParseError, TemplateParseErrorKind};
use crate::template_parser::{TemplateAliasesMap, TemplateParseError, TemplateParseErrorKind};
use crate::ui::Ui;

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -438,12 +442,18 @@ impl From<GitIgnoreError> for CommandError {

const BROKEN_PIPE_EXIT_CODE: u8 = 3;

pub(crate) fn handle_command_result(ui: &mut Ui, result: Result<(), CommandError>) -> ExitCode {
try_handle_command_result(ui, result).unwrap_or_else(|_| ExitCode::from(BROKEN_PIPE_EXIT_CODE))
pub(crate) fn handle_command_result(
ui: &mut Ui,
layered_configs: &LayeredConfigs,
result: Result<(), CommandError>,
) -> ExitCode {
try_handle_command_result(ui, layered_configs, result)
.unwrap_or_else(|_| ExitCode::from(BROKEN_PIPE_EXIT_CODE))
}

fn try_handle_command_result(
ui: &mut Ui,
layered_configs: &LayeredConfigs,
result: Result<(), CommandError>,
) -> io::Result<ExitCode> {
match &result {
Expand Down Expand Up @@ -487,13 +497,22 @@ fn try_handle_command_result(
match inner.kind() {
clap::error::ErrorKind::DisplayHelp | clap::error::ErrorKind::DisplayVersion => {
write!(ui.stdout(), "{clap_str}")?;
Ok(ExitCode::SUCCESS)
return Ok(ExitCode::SUCCESS);
}
_ => {
write!(ui.stderr(), "{clap_str}")?;
Ok(ExitCode::from(2))
_ => {}
}

write!(ui.stderr(), "{clap_str}")?;
if let Some(CliArgInvalidValue::Template) = inspect_invalid_value(inner.context()) {
match load_template_aliases(ui, layered_configs) {
Ok(aliases_map) => {
let hint = format_symbol_names_hint(&aliases_map);
write!(ui.hint(), "\n{hint}")?;
}
Err(err) => return try_handle_command_result(ui, layered_configs, Err(err)),
}
}
Ok(ExitCode::from(2))
}
Err(CommandError::BrokenPipe) => {
// A broken pipe is not an error, but a signal to exit gracefully.
Expand Down Expand Up @@ -521,3 +540,32 @@ fn print_error_sources(ui: &Ui, source: Option<&dyn error::Error>) -> io::Result
}
Ok(())
}

fn format_symbol_names_hint(template_aliases: &TemplateAliasesMap) -> String {
let mut names = template_aliases.symbol_names().collect_vec();
names.sort_unstable();
let mut hint = String::from("The following symbol names are defined:\n");
for name in names {
hint.push_str(&format!("- {name}\n"));
}
hint
}

enum CliArgInvalidValue {
Template,
}

fn inspect_invalid_value<'a>(
iter: impl Iterator<Item = (ContextKind, &'a ContextValue)>,
) -> Option<CliArgInvalidValue> {
let map: HashMap<ContextKind, &ContextValue> = iter.collect();
if let (Some(&ContextValue::String(invalid_arg)), Some(&ContextValue::String(invalid_value))) = (
map.get(&ContextKind::InvalidArg),
map.get(&ContextKind::InvalidValue),
) {
if invalid_arg.as_str() == "--template <TEMPLATE>" && invalid_value.is_empty() {
return Some(CliArgInvalidValue::Template);
}
}
None
}
4 changes: 4 additions & 0 deletions cli/src/template_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 28e2d12

Please sign in to comment.