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 clap-rs/clap#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
(clap-rs/clap#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

@@ -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<bool>.
+    pub no_pager: Option<bool>,
     /// 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<String>,
-    /// 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))
 }

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 <REPOSITORY>      Path to repository to operate on
           --no-commit-working-copy       Don't commit the working copy
           --at-operation <AT_OPERATION>  Operation to load the repo at [default: @] [aliases: at-op]
+      -v, --verbose                      Enable verbose logging
           --color <WHEN>                 When to colorize output (always, never, auto)
           --no-pager                     Disable the pager
           --config-toml <TOML>           Additional configuration options
-      -v, --verbose                      Enable verbose logging
     "###);
 }