Let clap handle --, the debug subcommand and value names

src/cli.rs

@@ -1,9 +1,12 @@
 //! Git-style CLI input: the terminal UI, the NDJSON stream, and Git metadata.
 use crate::config::{self, Config};
 use crate::git::{Comparison, DiffSession, FileParams, Operand, Result};
-use crate::options::DiffOptions;
+use crate::options::{DebugArgs, DiffOptions};
 use crate::plugin::Pipeline;
-use clap::{ArgGroup, Args, Parser, Subcommand, ValueEnum};
+use clap::{
+    error::ErrorKind, parser::ValueSource, ArgGroup, ArgMatches, Args, CommandFactory,
+    FromArgMatches, Parser, Subcommand, ValueEnum,
+};
 use git2::{DiffStatsFormat, Repository};
 use std::{
     ffi::OsString,
@@ -23,13 +26,13 @@ use std::{
 struct Cli {
     #[command(subcommand)]
     command: Option<Command>,
-    #[arg(long, value_name = "repo", default_value = ".")]
-    repo: String,
+    #[arg(long, default_value = ".")]
+    repo: PathBuf,
     /// Concurrent file diffs for --format ndjson; results are emitted as each finishes
-    #[arg(short, long, value_name = "jobs", default_value_t = 16)]
+    #[arg(short, long, default_value_t = 16)]
     jobs: usize,
     /// File tag priority: files carrying an earlier listed tag come first
-    #[arg(long, value_name = "order", value_delimiter = ',')]
+    #[arg(long, value_delimiter = ',')]
     order: Vec<String>,
     #[arg(long, visible_alias = "staged")]
     cached: bool,
@@ -60,10 +63,10 @@ struct Cli {
     #[arg(short = 'M', long, conflicts_with = "no_renames")]
     find_renames: bool,
     /// Unchanged lines kept around each change; defaults to plugins.bundled.context.lines
-    #[arg(short = 'U', long, value_name = "unified")]
+    #[arg(short = 'U', long)]
     unified: Option<u32>,
     /// Write the event stream to stdout instead of opening the terminal UI
-    #[arg(long, value_name = "format")]
+    #[arg(long)]
     format: Option<Format>,
     /// Emit initial files followed by deferred annotations (NDJSON v4)
     #[arg(long, requires = "format")]
@@ -72,18 +75,19 @@ struct Cli {
     #[arg(long)]
     syntax: bool,
     /// Columns for --stat; defaults to the terminal's width
-    #[arg(long, value_name = "width")]
+    #[arg(long)]
     width: Option<usize>,
     #[arg(long)]
     ignore_comments: bool,
-    #[arg(long, value_name = "byte-limit")]
+    #[arg(long)]
     byte_limit: Option<usize>,
-    #[arg(long, value_name = "graph-limit")]
+    #[arg(long)]
     graph_limit: Option<usize>,
-    #[arg(long, value_name = "parse-error-limit")]
+    #[arg(long)]
     parse_error_limit: Option<usize>,
-    #[arg(value_name = "items", num_args = 0..)]
     items: Vec<OsString>,
+    #[arg(last = true)]
+    paths: Vec<OsString>,
 }
 
 impl Cli {
@@ -101,12 +105,21 @@ enum Format {
 enum Command {
     /// Show, edit, or open the settings screen for diffr's configuration
     Config(ConfigArgs),
+    #[command(
+        hide = true,
+        display_name = env!("CARGO_BIN_NAME"),
+        about,
+        version,
+        long_version = crate::version::VERSION.as_str(),
+        next_display_order = None,
+        arg_required_else_help = true
+    )]
+    Debug(DebugArgs),
 }
 
 #[derive(Args)]
 struct ConfigArgs {
     /// Initial search in the settings screen
-    #[arg(value_name = "query")]
     query: Option<String>,
     #[command(subcommand)]
     command: Option<ConfigCommand>,
@@ -125,31 +138,23 @@ enum ConfigCommand {
         reveal: bool,
     },
     /// Write one key to the global configuration file
-    Set {
-        #[arg(value_name = "key")]
-        key: String,
-        #[arg(value_name = "value")]
-        value: String,
-    },
+    Set { key: String, value: String },
 }
 
 pub(crate) fn run() -> Result<i32> {
-    let mut argv: Vec<OsString> = std::env::args_os().collect();
-    let frontend_args = argv[1..].to_vec();
-    // Preserve the distinction between revisions and paths explicitly following --.
-    let has_separator = argv.iter().any(|arg| arg == "--");
-    let explicit_paths = argv
-        .iter()
-        .position(|arg| arg == "--")
-        .map(|at| {
-            let paths = argv.split_off(at + 1);
-            argv.pop();
-            paths
-        })
-        .unwrap_or_default();
-    let args = Cli::parse_from(argv);
+    let frontend_args: Vec<OsString> = std::env::args_os().skip(1).collect();
+    // Git treats an argument before `--` as a revision even when a file shares
+    // its name, so a bare trailing `--` still matters.
+    let has_separator = frontend_args.iter().any(|arg| arg == "--");
+    let matches = Cli::command().get_matches();
+    reject_diff_arguments_before_subcommand(&matches);
+    let args = Cli::from_arg_matches(&matches).unwrap_or_else(|error| error.exit());
     match &args.command {
         Some(Command::Config(config)) => return run_config(config),
+        Some(Command::Debug(debug)) => {
+            crate::run_debug(debug.mode());
+            return Ok(0);
+        }
         None => {}
     }
     let streaming = args.format.is_some();
@@ -167,7 +172,7 @@ pub(crate) fn run() -> Result<i32> {
     if args.no_index {
         return no_index(
             &args,
-            args.items.iter().cloned().chain(explicit_paths).collect(),
+            args.items.iter().chain(&args.paths).cloned().collect(),
             stream_options,
         );
     }
@@ -177,7 +182,7 @@ pub(crate) fn run() -> Result<i32> {
     let location = std::fs::canonicalize(&args.repo)?;
     let repo = Repository::discover(&location)?;
     let workspace = repo.workdir().unwrap_or(repo.path());
-    let (comparison, paths) = select(&repo, &location, &args, explicit_paths, has_separator)?;
+    let (comparison, paths) = select(&repo, &location, &args, has_separator)?;
     let files = FileParams {
         paths,
         // `-M` conflicts with `--no-renames`; renames are on by default.
@@ -227,11 +232,33 @@ pub(crate) fn run() -> Result<i32> {
     })
 }
 
+/// The top-level flags configure a diff; a subcommand given after one would
+/// silently drop it.
+fn reject_diff_arguments_before_subcommand(matches: &ArgMatches) {
+    let Some((name, _)) = matches.subcommand() else {
+        return;
+    };
+    let mut command = Cli::command();
+    // Formatting an `Arg` reads settings that only `build` fills in.
+    command.build();
+    let given = command
+        .get_arguments()
+        .find(|arg| matches.value_source(arg.get_id().as_str()) == Some(ValueSource::CommandLine))
+        .map(ToString::to_string);
+    if let Some(arg) = given {
+        command
+            .error(
+                ErrorKind::ArgumentConflict,
+                format!("the argument '{arg}' cannot be used with the '{name}' subcommand"),
+            )
+            .exit();
+    }
+}
+
 fn select(
     repo: &Repository,
     location: &Path,
     args: &Cli,
-    explicit_paths: Vec<OsString>,
     has_separator: bool,
 ) -> Result<(Comparison, Vec<String>)> {
     let mut revisions = Vec::new();
@@ -259,7 +286,7 @@ fn select(
             );
         }
     }
-    paths.extend(explicit_paths);
+    paths.extend(args.paths.iter().cloned());
     // Match canonical path forms, including Windows verbatim path prefixes.
     let root = std::fs::canonicalize(repo.workdir().unwrap_or(repo.path()))?;
     let prefix = location.strip_prefix(&root)?;

src/main.rs

@@ -134,14 +134,7 @@ fn main() {
         .expect("The logger has not been previously initialized");
     reset_sigpipe();
 
-    let result = match std::env::args_os().nth(1).as_deref() {
-        Some(arg) if arg == "debug" => {
-            run_debug();
-            return;
-        }
-        _ => cli::run(),
-    };
-    match result {
+    match cli::run() {
         Ok(code) => std::process::exit(code),
         Err(error) => {
             eprintln!("{error}");
@@ -150,10 +143,11 @@ fn main() {
     }
 }
 
-fn run_debug() {
+/// `diffr debug`: syntax dumps and the language list.
+fn run_debug(mode: Mode) {
     let params = &Params::default();
 
-    match options::parse_args() {
+    match mode {
         Mode::DumpTreeSitter {
             path,
             language_overrides,

src/options.rs

@@ -5,11 +5,10 @@ use std::ffi::OsStr;
 use std::fmt::Display;
 use std::path::{Path, PathBuf};
 
-use clap::{crate_description, Parser};
+use clap::Args;
 
 use crate::exit_codes::EXIT_BAD_ARGUMENTS;
 use crate::parse::guess_language::{language_override_from_name, LanguageOverride};
-use crate::version::VERSION;
 
 pub(crate) const DEFAULT_BYTE_LIMIT: usize = 1_000_000;
 // Chosen experimentally: this is sufficiently many for all the sample
@@ -18,8 +17,6 @@ pub(crate) const DEFAULT_BYTE_LIMIT: usize = 1_000_000;
 pub(crate) const DEFAULT_GRAPH_LIMIT: usize = 3_000_000;
 pub(crate) const DEFAULT_PARSE_ERROR_LIMIT: usize = 0;
 
-pub(crate) const USAGE: &str = concat!(env!("CARGO_BIN_NAME"), " debug [OPTIONS]");
-
 pub(crate) const DEFAULT_TERMINAL_WIDTH: usize = 80;
 
 #[derive(Debug, Clone)]
@@ -45,18 +42,7 @@ impl Default for DiffOptions {
 }
 
 /// `diffr debug`: syntax dumps and the language list.
-#[derive(Parser)]
-#[command(
-    name = "diffr",
-    // Show options in alphabetical order, rather than in
-    // declaration order.
-    next_display_order = None,
-    override_usage = USAGE,
-    version = env!("CARGO_PKG_VERSION"),
-    long_version = VERSION.as_str(),
-    about = crate_description!(),
-    arg_required_else_help = true
-)]
+#[derive(Args)]
 pub(crate) struct DebugArgs {
     #[arg(
         long,
@@ -231,13 +217,8 @@ fn parse_overrides_or_die(raw_overrides: &[String]) -> Vec<(LanguageOverride, Ve
     combined_overrides
 }
 
-/// Parse CLI arguments passed to the binary.
-pub(crate) fn parse_args() -> Mode {
-    DebugArgs::parse_from(std::env::args_os().skip(1)).mode()
-}
-
 impl DebugArgs {
-    fn mode(&self) -> Mode {
+    pub(crate) fn mode(&self) -> Mode {
         let mut raw_overrides = self.overrides.clone();
         for i in 1..=9 {
             if let Ok(value) = env::var(format!("DFT_OVERRIDE_{}", i)) {
@@ -306,15 +287,8 @@ pub(crate) fn detect_terminal_width() -> usize {
 
 #[cfg(test)]
 mod tests {
-    use clap::CommandFactory;
-
     use super::*;
 
-    #[test]
-    fn test_app() {
-        DebugArgs::command().debug_assert();
-    }
-
     #[test]
     fn test_detect_display_width() {
         // Basic smoke test.

tests/cli.rs

@@ -172,3 +172,15 @@ fn config_ui_requires_a_terminal() {
         .code(2)
         .stderr(predicate::str::contains("terminal UI needs a terminal"));
 }
+
+#[test]
+fn diff_flags_before_a_subcommand_are_rejected() {
+    get_base_command()
+        .args(["--cached", "config", "show"])
+        .assert()
+        .failure()
+        .code(2)
+        .stderr(predicate::str::contains(
+            "the argument '--cached' cannot be used with the 'config' subcommand",
+        ));
+}