From 15bbfe3eb935c95e48a415d0a6b3ecb55aa2d62b Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Mon, 28 Sep 2026 13:56:41 -0700 Subject: [PATCH 1/4] Let clap split arguments at -- Behavior change: a trailing `last = true` positional takes everything after `--`, replacing the argv surgery before parsing. - `diffr config -- foo` passes `foo` as the settings search; it was dropped. - `diffr config show -- x` is rejected; `x` was dropped. - Usage lines gain `[-- ...]`. A scan for `--` remains: git reads an argument before a bare trailing `--` as a revision even when a file shares its name, and clap does not report an empty trailing group. AI assistance: Claude Code. Agent-Session: b28d94c6-2325-41ab-9fab-e9027acc5b09 Agent-Session: 01a0e9c2-2c41-7062-b484-92e030c4ab9c --- src/cli.rs | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index cb7f110ec..9e7a59e8b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -84,6 +84,8 @@ struct Cli { parse_error_limit: Option, #[arg(value_name = "items", num_args = 0..)] items: Vec, + #[arg(value_name = "paths", last = true)] + paths: Vec, } impl Cli { @@ -134,20 +136,11 @@ enum ConfigCommand { } pub(crate) fn run() -> Result { - let mut argv: Vec = 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 = 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 args = Cli::parse(); match &args.command { Some(Command::Config(config)) => return run_config(config), None => {} @@ -167,7 +160,7 @@ pub(crate) fn run() -> Result { 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 +170,7 @@ pub(crate) fn run() -> Result { 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. @@ -231,7 +224,6 @@ fn select( repo: &Repository, location: &Path, args: &Cli, - explicit_paths: Vec, has_separator: bool, ) -> Result<(Comparison, Vec)> { let mut revisions = Vec::new(); @@ -259,7 +251,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)?; From 13caa98b7bc174496ab1dcbb68184143b75a63f1 Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Mon, 28 Sep 2026 13:56:41 -0700 Subject: [PATCH 2/4] Make debug a hidden clap subcommand Behavior change: main no longer checks argv[1] == "debug"; clap routes `debug` like `config`. So `diffr --stat debug ...` enters debug and ignores --stat, where it was "unknown revision or path". The next commit rejects that for both subcommands. DebugArgs derives Args and hangs off Command::Debug, hidden from --help, with the same version string and usage line as before. AI assistance: Claude Code. Agent-Session: b28d94c6-2325-41ab-9fab-e9027acc5b09 Agent-Session: 01a0e9c2-2c41-7062-b484-92e030c4ab9c --- src/cli.rs | 16 +++++++++++++++- src/main.rs | 14 ++++---------- src/options.rs | 32 +++----------------------------- 3 files changed, 22 insertions(+), 40 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 9e7a59e8b..ddac0bbbb 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,7 +1,7 @@ //! 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 git2::{DiffStatsFormat, Repository}; @@ -103,6 +103,16 @@ 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)] @@ -143,6 +153,10 @@ pub(crate) fn run() -> Result { let args = Cli::parse(); 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(); diff --git a/src/main.rs b/src/main.rs index 6dd75d718..c995372ae 100644 --- a/src/main.rs +++ b/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, diff --git a/src/options.rs b/src/options.rs index aa09873dc..fa8cf2c20 100644 --- a/src/options.rs +++ b/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. From fbe401997fb64635b0f6f8184abf531b75d46b77 Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Mon, 28 Sep 2026 13:56:42 -0700 Subject: [PATCH 3/4] Reject diff flags before a subcommand Behavior change: `diffr --cached config show` now fails with "the argument '--cached' cannot be used with the 'config' subcommand"; the flag was silently ignored. Likewise for `debug`. clap's args_conflicts_with_subcommands does not fit: once any top-level flag is given it reads the subcommand's name as a positional. Instead, check after parsing whether any top-level argument came from the command line, and raise clap's ArgumentConflict. AI assistance: Claude Code. Agent-Session: b28d94c6-2325-41ab-9fab-e9027acc5b09 Agent-Session: 01a0e9c2-2c41-7062-b484-92e030c4ab9c --- src/cli.rs | 32 ++++++++++++++++++++++++++++++-- tests/cli.rs | 12 ++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index ddac0bbbb..68b913f57 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -3,7 +3,10 @@ use crate::config::{self, Config}; use crate::git::{Comparison, DiffSession, FileParams, Operand, Result}; 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, @@ -150,7 +153,9 @@ pub(crate) fn run() -> Result { // 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 args = Cli::parse(); + 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)) => { @@ -234,6 +239,29 @@ pub(crate) fn run() -> Result { }) } +/// 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, diff --git a/tests/cli.rs b/tests/cli.rs index 4946a56de..3f89e9607 100644 --- a/tests/cli.rs +++ b/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", + )); +} From e0f8e68914cf4e92c2e6ddce9fc500095b86b9f8 Mon Sep 17 00:00:00 2001 From: Sidharth Menon Date: Mon, 28 Sep 2026 13:56:42 -0700 Subject: [PATCH 4/4] Use clap's default value names and a PathBuf --repo Behavior change, help and errors only for the names: `` becomes ``, `[items]...` becomes `[ITEMS]...` (or `...` in conflict usage lines), and so on. `--repo` is a PathBuf, so a non-UTF-8 path is accepted instead of rejected. AI assistance: Claude Code. Agent-Session: b28d94c6-2325-41ab-9fab-e9027acc5b09 Agent-Session: 01a0e9c2-2c41-7062-b484-92e030c4ab9c --- src/cli.rs | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 68b913f57..57e440203 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -26,13 +26,13 @@ use std::{ struct Cli { #[command(subcommand)] command: Option, - #[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, #[arg(long, visible_alias = "staged")] cached: bool, @@ -63,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, /// Write the event stream to stdout instead of opening the terminal UI - #[arg(long, value_name = "format")] + #[arg(long)] format: Option, /// Emit initial files followed by deferred annotations (NDJSON v4) #[arg(long, requires = "format")] @@ -75,19 +75,18 @@ struct Cli { #[arg(long)] syntax: bool, /// Columns for --stat; defaults to the terminal's width - #[arg(long, value_name = "width")] + #[arg(long)] width: Option, #[arg(long)] ignore_comments: bool, - #[arg(long, value_name = "byte-limit")] + #[arg(long)] byte_limit: Option, - #[arg(long, value_name = "graph-limit")] + #[arg(long)] graph_limit: Option, - #[arg(long, value_name = "parse-error-limit")] + #[arg(long)] parse_error_limit: Option, - #[arg(value_name = "items", num_args = 0..)] items: Vec, - #[arg(value_name = "paths", last = true)] + #[arg(last = true)] paths: Vec, } @@ -121,7 +120,6 @@ enum Command { #[derive(Args)] struct ConfigArgs { /// Initial search in the settings screen - #[arg(value_name = "query")] query: Option, #[command(subcommand)] command: Option, @@ -140,12 +138,7 @@ 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 {