From aa2ce8854492e51baf709ed2f577729bd9c7cbb1 Mon Sep 17 00:00:00 2001 From: Glen Choo Date: Mon, 21 Nov 2022 20:46:50 -0800 Subject: [PATCH] cli: process args to "--help" and "help" Clap bails parsing when an "error" is encountered, e.g. a subcommand is missing, "--help" is passed, or the "help" subcommand is invoked. This means that the current approach of parsing args does not handle flags like `--no-pager` or `--color` when an error is encountered. Fix this by separating early args into their own struct and preprocessing them using `ignore_errors` (per https://github.com/clap-rs/clap/issues/1880). The early args are in a new `EarlyArgs` struct because of a known bug where `ignore_errors` causes default values not to be respected (https://github.com/clap-rs/clap/issues/4391 specifically calls out bool, but strings may also be ignored), so when `ignore_errors` is given, the default values will be missing and parsing will fail unless the right arg types are used (e.g`. Option`). By parsing only early args (using the new struct) we only need to adjust `no_pager`, instead of adjusting all args with a default value. --- src/cli_util.rs | 55 ++++++++++++++++++++++++++------------- tests/test_global_opts.rs | 25 +++++++++++++++++- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/src/cli_util.rs b/src/cli_util.rs index d70c2d1db..8230edd50 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -24,7 +24,7 @@ use std::sync::Arc; use clap; use clap::builder::{NonEmptyStringValueParser, TypedValueParser, ValueParserFactory}; -use clap::{Arg, ArgMatches, Command, Error, FromArgMatches}; +use clap::{Arg, ArgAction, ArgMatches, Command, Error, FromArgMatches}; use git2::{Oid, Repository}; use itertools::Itertools; use jujutsu_lib::backend::{BackendError, CommitId, TreeId}; @@ -1305,6 +1305,16 @@ pub struct GlobalArgs { default_value = "@" )] pub at_operation: String, + /// Enable verbose logging + #[arg(long, short = 'v', global = true, help_heading = "Global Options")] + pub verbose: bool, + + #[command(flatten)] + pub early_args: EarlyArgs, +} + +#[derive(clap::Args, Clone, Debug)] +pub struct EarlyArgs { /// When to colorize output (always, never, auto) #[arg( long, @@ -1318,9 +1328,12 @@ pub struct GlobalArgs { long, value_name = "WHEN", global = true, - help_heading = "Global Options" + help_heading = "Global Options", + action = ArgAction::SetTrue )] - pub no_pager: bool, + // Parsing with ignore_errors will crash if this is bool, so use + // Option. + pub no_pager: Option, /// Additional configuration options // TODO: Introduce a `--config` option with simpler syntax for simple // cases, designed so that `--config ui.color=auto` works @@ -1331,9 +1344,6 @@ pub struct GlobalArgs { help_heading = "Global Options" )] pub config_toml: Vec, - /// Enable verbose logging - #[arg(long, short = 'v', global = true, help_heading = "Global Options")] - pub verbose: bool, } #[derive(Clone, Debug)] @@ -1441,6 +1451,25 @@ fn resolve_aliases( } } +/// Parse args that must be interpreted early, e.g. before printing help. +fn handle_early_args(ui: &mut Ui, app: &clap::Command) -> Result<(), CommandError> { + // ignore_errors() bypasses errors like "--help" or missing subcommand + let early_matches = app.clone().ignore_errors(true).get_matches(); + let mut args: EarlyArgs = EarlyArgs::from_arg_matches(&early_matches).unwrap(); + + if let Some(choice) = args.color { + args.config_toml + .push(format!("ui.color=\"{}\"", choice.to_string())); + } + if args.no_pager.unwrap_or_default() { + ui.set_pagination(crate::ui::PaginationChoice::No); + } + if !args.config_toml.is_empty() { + ui.extra_toml_settings(&args.config_toml)?; + } + Ok(()) +} + pub fn parse_args( ui: &mut Ui, app: clap::Command, @@ -1454,22 +1483,12 @@ pub fn parse_args( return Err(CommandError::CliError("Non-utf8 argument".to_string())); } } + handle_early_args(ui, &app)?; let string_args = resolve_aliases(ui.settings(), &app, &string_args)?; let matches = app.clone().try_get_matches_from(&string_args)?; - let mut args: Args = Args::from_arg_matches(&matches).unwrap(); - if let Some(choice) = args.global_args.color { - args.global_args - .config_toml - .push(format!("ui.color=\"{}\"", choice.to_string())); - } - if args.global_args.no_pager { - ui.set_pagination(crate::ui::PaginationChoice::No); - } - if !args.global_args.config_toml.is_empty() { - ui.extra_toml_settings(&args.global_args.config_toml)?; - } + let args: Args = Args::from_arg_matches(&matches).unwrap(); let command_helper = CommandHelper::new(app, string_args, args.global_args); Ok((command_helper, matches)) } diff --git a/tests/test_global_opts.rs b/tests/test_global_opts.rs index a33bddd0e..49260d444 100644 --- a/tests/test_global_opts.rs +++ b/tests/test_global_opts.rs @@ -192,6 +192,29 @@ color="always""#, "###); } +#[test] +fn test_early_args() { + // Test that help output parses early args + let test_env = TestEnvironment::default(); + + // The default is no color. + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["help"]); + insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Jujutsu (An experimental VCS)"); + + // Check that output is colorized. + let stdout = test_env.jj_cmd_success(test_env.env_root(), &["--color=always", "help"]); + insta::assert_snapshot!(stdout.lines().next().unwrap(), @"Jujutsu (An experimental VCS)"); + + // Early args are parsed with clap's ignore_errors(), but there is a known + // bug that causes defaults to be unpopulated. Test that the early args are + // tolerant of this bug and don't cause a crash. + test_env.jj_cmd_success(test_env.env_root(), &["--no-pager", "help"]); + test_env.jj_cmd_success( + test_env.env_root(), + &["--config-toml", "ui.color = 'always'", "help"], + ); +} + #[test] fn test_invalid_config() { // Test that we get a reasonable error if the config is invalid (#55) @@ -252,10 +275,10 @@ fn test_help() { -R, --repository Path to repository to operate on --no-commit-working-copy Don't commit the working copy --at-operation Operation to load the repo at [default: @] [aliases: at-op] + -v, --verbose Enable verbose logging --color When to colorize output (always, never, auto) --no-pager Disable the pager --config-toml Additional configuration options - -v, --verbose Enable verbose logging "###); }