From 6ce9cb2b17f16c16c1adb99256794e8047d2c876 Mon Sep 17 00:00:00 2001 From: Stephen Waits Date: Fri, 1 May 2026 23:57:17 -0600 Subject: [PATCH] feat(dispatch): walk subcommands and detect revset flag from help MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real failures motivated this: - `jjf bm main` (where `bm` is a user alias for `bookmark move`) ran `jj bm main -r ` — but bookmark move uses `--to`, not `-r`. - `jjf tag set v0.2.0` errored with "doesn't take revisions" because jjf only inspected `jj tag --help` (parent) instead of walking into the `set` leaf, which does take `-r`. Replace the boolean `supports_revisions()` (a substring match on "REVSET") with `RevsetTarget { leaf, passthrough, flag }`: - Walks multi-level subcommands by parsing each parent's `Commands:` block — including `[aliases: …]` annotations, so `jjf tag s` walks to `tag set`. Aliases like `bm` work transparently because `jj bm --help` already emits the resolved leaf's help. - Parses the leaf's `Options:` block and picks the revset flag in priority order: `-r`/`--revision[s]` first, then `-t, --to`. `--from` is intentionally not a fallback (different semantics from "the revision the picker chose"). - Bypasses the picker entirely when the user already passed a revset flag in their args, so `jjf describe -r @-` runs verbatim instead of double-injecting `-r`. `exec` and `command_line` now take `&RevsetTarget` and emit the resolved flag; signatures thread through `tui.rs` and `main.rs`. New unit tests cover the help parsers and command-line construction against captured fixtures of `jj tag set --help`, `jj bookmark move --help`, and `jj tag --help`. --- src/jj.rs | 438 +++++++++++++++++++++++++++++++++++++++++++++++++--- src/main.rs | 68 +++++--- src/tui.rs | 15 +- 3 files changed, 471 insertions(+), 50 deletions(-) diff --git a/src/jj.rs b/src/jj.rs index 8bfc339..c46978b 100644 --- a/src/jj.rs +++ b/src/jj.rs @@ -265,42 +265,294 @@ fn parse_show_summary(bytes: &[u8]) -> PreviewParts { PreviewParts { description, files } } -pub fn supports_revisions(subcommand: &str) -> Result { +/// Which flag a leaf jj subcommand uses to accept a revset, picked in +/// priority order: `-r`/`--revision[s]` first, then `-t, --to`. `--from` is +/// intentionally not a fallback — its semantics ("filter source revisions") +/// differ enough from "the revision the picker chose" that silently injecting +/// it would surprise users. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RevsetFlag { + Revision, + To, +} + +impl RevsetFlag { + pub fn as_str(self) -> &'static str { + match self { + RevsetFlag::Revision => "-r", + RevsetFlag::To => "--to", + } + } +} + +/// A jj invocation jjf has analyzed and is ready to dispatch on. `leaf` is +/// the resolved subcommand path (preserving the user's tokens, including +/// aliases like `bm` — jj resolves them again at exec time). `passthrough` +/// is everything after the leaf. `flag` is `None` when the leaf accepts no +/// revset; in that case jjf refuses to run the picker. +#[derive(Debug, Clone)] +pub struct RevsetTarget { + pub leaf: Vec, + pub passthrough: Vec, + pub flag: Option, +} + +pub fn resolve_target(args: &[String]) -> Result { + debug_assert!(!args.is_empty(), "resolve_target requires at least one arg"); + + let first_help = match run_help(std::slice::from_ref(&args[0]))? { + Some(h) => h, + None => { + // Unknown subcommand or jj help failed for some reason. Fall + // through optimistically so jj surfaces its own error message at + // exec time. + return Ok(RevsetTarget { + leaf: vec![args[0].clone()], + passthrough: args[1..].to_vec(), + flag: Some(RevsetFlag::Revision), + }); + } + }; + + let (leaf, leaf_help) = resolve_leaf(args, first_help)?; + let passthrough = args[leaf.len()..].to_vec(); + let flag = parse_revset_flag(&leaf_help); + + Ok(RevsetTarget { + leaf, + passthrough, + flag, + }) +} + +fn resolve_leaf(args: &[String], first_help: String) -> Result<(Vec, String)> { + let mut leaf = vec![args[0].clone()]; + let mut help = first_help; + for arg in &args[1..] { + if arg.starts_with('-') { + break; + } + let children = parse_commands_section(&help); + if !children.contains(arg.as_str()) { + break; + } + let mut candidate = leaf.clone(); + candidate.push(arg.clone()); + match run_help(&candidate)? { + Some(next_help) => { + leaf = candidate; + help = next_help; + } + None => break, + } + } + Ok((leaf, help)) +} + +fn run_help(path: &[String]) -> Result> { let output = Command::new("jj") - .arg(subcommand) + .args(path) .arg("--help") + .stderr(Stdio::null()) .output() .context("failed to spawn jj for preflight")?; if !output.status.success() { - // unknown subcommand or other error — let the main flow surface jj's error - return Ok(true); + return Ok(None); } - let help = String::from_utf8_lossy(&output.stdout); - Ok(help.contains("REVSET")) + Ok(Some(String::from_utf8_lossy(&output.stdout).into_owned())) } -pub fn exec(subcommand: &str, passthrough: &[String], ids: &[String]) -> Result { - let revset = ids.join("|"); - let status = Command::new("jj") - .arg(subcommand) - .args(passthrough) - .arg("-r") - .arg(&revset) - .status() - .context("failed to spawn jj")?; +/// Parse the `Commands:` block of a jj help output. Returns the set of +/// subcommand names AND their `[aliases: …]` annotations, so a user typing +/// `jjf tag s` (where `s` is the alias for `set`) walks correctly. +fn parse_commands_section(help: &str) -> std::collections::HashSet { + let mut set = std::collections::HashSet::new(); + let mut in_commands = false; + for line in help.lines() { + if line == "Commands:" { + in_commands = true; + continue; + } + if !in_commands { + continue; + } + if line.is_empty() { + break; + } + if !line.starts_with(' ') { + break; + } + let trimmed = line.trim_start(); + let indent = line.len() - trimmed.len(); + // Primary entries have exactly 2-space indent. Anything deeper is + // a description continuation we don't care about. + if indent != 2 { + continue; + } + if let Some(name) = trimmed.split_whitespace().next() { + set.insert(name.to_string()); + } + scan_aliases(line, &mut set); + } + set +} + +fn scan_aliases(line: &str, names: &mut std::collections::HashSet) { + let Some(start) = line.find("[aliases:") else { + return; + }; + let after = &line[start + "[aliases:".len()..]; + let Some(end) = after.find(']') else { + return; + }; + for a in after[..end].split(',') { + let n = a.trim(); + if !n.is_empty() { + names.insert(n.to_string()); + } + } +} + +/// Pick the right revset-accepting flag from a leaf's help, preferring +/// `-r`/`--revision[s]` over `-t, --to`. Returns `None` if the leaf has +/// neither. +pub fn parse_revset_flag(help: &str) -> Option { + let opts = parse_options(help); + for opt in &opts { + if opt.is_revset + && opt + .longs + .iter() + .any(|n| n == "--revision" || n == "--revisions") + { + return Some(RevsetFlag::Revision); + } + } + for opt in &opts { + if opt.is_revset && opt.longs.iter().any(|n| n == "--to") { + return Some(RevsetFlag::To); + } + } + None +} + +#[derive(Debug)] +struct OptionEntry { + longs: Vec, + /// True when the placeholder is `` or ``. + is_revset: bool, +} + +fn parse_options(help: &str) -> Vec { + let mut opts = Vec::new(); + let mut in_options = false; + let mut current: Option = None; + + for line in help.lines() { + if line == "Options:" { + in_options = true; + continue; + } + if !in_options { + continue; + } + // A non-indented non-empty line ends `Options:` (e.g. `Global Options:`). + if !line.is_empty() && !line.starts_with(' ') { + if let Some(o) = current.take() { + opts.push(o); + } + break; + } + if line.is_empty() { + // Blank lines separate options inside the section; don't end it. + continue; + } + + let trimmed = line.trim_start(); + let indent = line.len() - trimmed.len(); + + // New entry: 2-space indent with `-X,` (short + long) OR 6-space + // indent with `--` (long-only flag, e.g. `--allow-move`). + let is_new_entry = (indent == 2 && trimmed.starts_with('-')) + || (indent == 6 && trimmed.starts_with("--")); + + if is_new_entry { + if let Some(o) = current.take() { + opts.push(o); + } + current = Some(parse_option_header(trimmed)); + } else if let Some(ref mut o) = current { + // Continuation line. Pull any `[aliases: --foo, --bar]` into the + // current option's long-name set so the priority match sees them. + let mut alias_set = std::collections::HashSet::new(); + scan_aliases(line, &mut alias_set); + for n in alias_set { + if n.starts_with("--") { + o.longs.push(n); + } + } + } + } + if let Some(o) = current { + opts.push(o); + } + opts +} + +fn parse_option_header(s: &str) -> OptionEntry { + let mut longs = Vec::new(); + let mut placeholder: Option = None; + for token in s.split(|c: char| c == ',' || c.is_whitespace()) { + let t = token.trim(); + if t.is_empty() { + continue; + } + if let Some(rest) = t.strip_prefix("--") { + // Strip a trailing `=...` if clap ever emits one (it doesn't + // today, but cheap insurance). + let name = rest.split('=').next().unwrap_or(rest); + longs.push(format!("--{name}")); + } else if let Some(stripped) = t.strip_prefix('<') { + // Placeholder: ``, ``, `...`, etc. + let inner = stripped.trim_end_matches("..."); + if let Some(end) = inner.find('>') { + placeholder = Some(inner[..end].to_string()); + } + } + } + let is_revset = matches!(placeholder.as_deref(), Some("REVSET") | Some("REVSETS")); + OptionEntry { longs, is_revset } +} + +pub fn exec(target: &RevsetTarget, ids: &[String]) -> Result { + let mut cmd = Command::new("jj"); + cmd.args(&target.leaf); + cmd.args(&target.passthrough); + if !ids.is_empty() { + if let Some(flag) = target.flag { + cmd.arg(flag.as_str()); + cmd.arg(ids.join("|")); + } + } + let status = cmd.status().context("failed to spawn jj")?; Ok(status) } -pub fn command_line(subcommand: &str, passthrough: &[String], ids: &[String]) -> String { - let mut parts: Vec = Vec::with_capacity(4 + passthrough.len()); +pub fn command_line(target: &RevsetTarget, ids: &[String]) -> String { + let mut parts: Vec = + Vec::with_capacity(2 + target.leaf.len() + target.passthrough.len() + 2); parts.push("jj".into()); - parts.push(subcommand.into()); - for a in passthrough { + for w in &target.leaf { + parts.push(w.clone()); + } + for a in &target.passthrough { parts.push(shell_quote(a)); } if !ids.is_empty() { - parts.push("-r".into()); - parts.push(shell_quote(&ids.join("|"))); + if let Some(flag) = target.flag { + parts.push(flag.as_str().into()); + parts.push(shell_quote(&ids.join("|"))); + } } parts.join(" ") } @@ -326,3 +578,147 @@ pub fn shell_quote(s: &str) -> String { out.push('\''); out } + +#[cfg(test)] +mod tests { + use super::*; + + const TAG_SET_HELP: &str = "Create or update tags + +Usage: jj tag set [OPTIONS] ... + +Arguments: + ... + Tag names to create or update + +Options: + -r, --revision + Target revision to point to + + [default: @] + [aliases: --to] + + --allow-move + Allow moving existing tags + + -h, --help + Print help (see a summary with '-h') + +Global Options: + -R, --repository + Path to repository +"; + + const BOOKMARK_MOVE_HELP: &str = "Move existing bookmarks to target revision + +Usage: jj bookmark move [OPTIONS] > + +Arguments: + [NAMES]... + Move bookmarks matching the given name patterns + +Options: + -f, --from + Move bookmarks from the given revisions + + -t, --to + Move bookmarks to this revision + + [default: @] + + -B, --allow-backwards + Allow moving bookmarks backwards or sideways + + -h, --help + Print help (see a summary with '-h') + +Global Options: + -R, --repository + Path to repository +"; + + const TAG_PARENT_HELP: &str = "Manage tags + +Usage: jj tag [OPTIONS] + +Commands: + delete Delete existing tags [aliases: d] + list List tags and their targets [aliases: l] + set Create or update tags [aliases: s] + +Options: + -h, --help + Print help (see a summary with '-h') + +Global Options: + -R, --repository + Path to repository +"; + + #[test] + fn revset_flag_prefers_revision_over_to_alias() { + // tag set has `-r, --revision ` with `[aliases: --to]`. + // The Revision-class match must win over the To-class match. + assert_eq!(parse_revset_flag(TAG_SET_HELP), Some(RevsetFlag::Revision)); + } + + #[test] + fn revset_flag_falls_back_to_to_when_no_revision() { + // bookmark move has only `-f, --from ` and `-t, --to `. + assert_eq!(parse_revset_flag(BOOKMARK_MOVE_HELP), Some(RevsetFlag::To)); + } + + #[test] + fn revset_flag_none_for_parent_without_options() { + assert_eq!(parse_revset_flag(TAG_PARENT_HELP), None); + } + + #[test] + fn commands_section_includes_names_and_aliases() { + let cmds = parse_commands_section(TAG_PARENT_HELP); + for name in ["delete", "list", "set", "d", "l", "s"] { + assert!(cmds.contains(name), "missing {name} in {cmds:?}"); + } + } + + #[test] + fn commands_section_empty_for_leaf() { + let cmds = parse_commands_section(BOOKMARK_MOVE_HELP); + assert!(cmds.is_empty(), "expected no commands, got {cmds:?}"); + } + + #[test] + fn command_line_emits_revision_flag() { + let target = RevsetTarget { + leaf: vec!["tag".into(), "set".into()], + passthrough: vec!["v0.2.0".into()], + flag: Some(RevsetFlag::Revision), + }; + let s = command_line(&target, &["abcd1234".into()]); + assert_eq!(s, "jj tag set v0.2.0 -r abcd1234"); + } + + #[test] + fn command_line_emits_to_flag() { + let target = RevsetTarget { + leaf: vec!["bm".into()], + passthrough: vec!["main".into()], + flag: Some(RevsetFlag::To), + }; + let s = command_line(&target, &["abcd1234".into()]); + assert_eq!(s, "jj bm main --to abcd1234"); + } + + #[test] + fn command_line_omits_flag_when_no_ids() { + let target = RevsetTarget { + leaf: vec!["describe".into()], + passthrough: vec!["-r".into(), "@".into()], + flag: Some(RevsetFlag::Revision), + }; + // When the user already supplied -r, jjf calls command_line with empty + // ids — no extra flag should be appended. + let s = command_line(&target, &[]); + assert_eq!(s, "jj describe -r @"); + } +} diff --git a/src/main.rs b/src/main.rs index 06a5b3b..498d07e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,19 +25,19 @@ fn real_main() -> Result { match first.as_str() { "init" => run_init(args), - "--emit" => match args.next() { - Some(sub) => { - let passthrough: Vec = args.collect(); - run_pick(&sub, &passthrough, /* emit = */ true) - } - None => { + "--emit" => { + let rest: Vec = args.collect(); + if rest.is_empty() { eprintln!("usage: jjf --emit [args...]"); - Ok(2) + return Ok(2); } - }, - sub => { - let passthrough: Vec = args.collect(); - run_pick(sub, &passthrough, /* emit = */ false) + run_pick(&rest, /* emit = */ true) + } + _ => { + let mut all = Vec::with_capacity(1 + args.size_hint().0); + all.push(first); + all.extend(args); + run_pick(&all, /* emit = */ false) } } } @@ -65,10 +65,29 @@ fn run_init(mut args: impl Iterator) -> Result { } } -fn run_pick(subcommand: &str, passthrough: &[String], emit: bool) -> Result { - if !jj::supports_revisions(subcommand)? { +fn run_pick(args: &[String], emit: bool) -> Result { + let target = jj::resolve_target(args)?; + + // Case 1: user already supplied a revset flag — bypass the picker entirely + // and run their command verbatim. Avoids double-`-r` and respects what + // they explicitly asked for. + if has_user_revset_flag(&target.passthrough) { + if emit { + let cmd = jj::command_line(&target, &[]); + println!("{cmd}"); + return Ok(0); + } + echo_command(&target, &[]); + let status = jj::exec(&target, &[])?; + return Ok(status.code().unwrap_or(1) as u8); + } + + // Case 2: leaf accepts no revset flag at all (e.g. `jj operation`, + // `jj workspace list`). Refuse to run the picker. + if target.flag.is_none() { + let leaf = target.leaf.join(" "); eprintln!( - "jjf: 'jj {subcommand}' doesn't take revisions — run 'jj {subcommand}' directly." + "jjf: 'jj {leaf}' takes no revset flag (-r/--to) — run 'jj {leaf}' directly." ); return Ok(2); } @@ -89,7 +108,7 @@ fn run_pick(subcommand: &str, passthrough: &[String], emit: bool) -> Result None }; - let tui_result = tui::run(rows, subcommand, passthrough); + let tui_result = tui::run(rows, &target); if let Some(saved) = saved_stdout { restore_stdout(saved)?; @@ -103,16 +122,25 @@ fn run_pick(subcommand: &str, passthrough: &[String], emit: bool) -> Result } if emit { - let cmd = jj::command_line(subcommand, passthrough, &ids); + let cmd = jj::command_line(&target, &ids); println!("{cmd}"); Ok(0) } else { - echo_command(subcommand, passthrough, &ids); - let status = jj::exec(subcommand, passthrough, &ids)?; + echo_command(&target, &ids); + let status = jj::exec(&target, &ids)?; Ok(status.code().unwrap_or(1) as u8) } } +fn has_user_revset_flag(passthrough: &[String]) -> bool { + passthrough.iter().any(|a| { + matches!( + a.as_str(), + "-r" | "--revision" | "--revisions" | "-t" | "--to" + ) + }) +} + #[cfg(unix)] fn redirect_stdout_to_tty() -> Result { use std::os::fd::AsRawFd; @@ -142,9 +170,9 @@ fn restore_stdout(saved: libc::c_int) -> Result<()> { Ok(()) } -fn echo_command(subcommand: &str, passthrough: &[String], ids: &[String]) { +fn echo_command(target: &jj::RevsetTarget, ids: &[String]) { use std::io::IsTerminal; - let cmd = jj::command_line(subcommand, passthrough, ids); + let cmd = jj::command_line(target, ids); if std::io::stdout().is_terminal() { println!("\x1b[2m$\x1b[0m {cmd}"); } else { diff --git a/src/tui.rs b/src/tui.rs index 364b2b7..f979dce 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -278,8 +278,7 @@ fn score_row( pub fn run( rows: Vec, - subcommand: &str, - passthrough: &[String], + target: &jj::RevsetTarget, ) -> Result>> { install_panic_hook(); let mut tty = OpenOptions::new() @@ -295,7 +294,7 @@ pub fn run( tty.write_all(HIDE_CURSOR).context("hide cursor")?; tty.flush().ok(); - let result = run_loop(&mut tty, rows, subcommand, passthrough); + let result = run_loop(&mut tty, rows, target); let _ = tty.write_all(SHOW_CURSOR); let _ = tty.write_all(b"\x1b[?1049l"); @@ -320,15 +319,14 @@ fn install_panic_hook() { fn run_loop( tty: &mut File, rows: Vec, - subcommand: &str, - passthrough: &[String], + target: &jj::RevsetTarget, ) -> Result>> { let mut app = App::new(rows); let (mut term_cols, mut term_rows) = crossterm::terminal::size().unwrap_or((80, 24)); // Chrome rows: input(1) + cmd_preview(1) + hint(1) = 3. app.last_height = (term_rows as usize).saturating_sub(3); loop { - render(tty, &mut app, term_rows, term_cols, subcommand, passthrough)?; + render(tty, &mut app, term_rows, term_cols, target)?; if !event::poll(Duration::from_millis(250)).context("event poll failed")? { continue; } @@ -454,8 +452,7 @@ fn render( app: &mut App, term_rows: u16, term_cols: u16, - subcommand: &str, - passthrough: &[String], + target: &jj::RevsetTarget, ) -> Result<()> { let viewport_y: u16 = 0; let viewport_height = term_rows; @@ -601,7 +598,7 @@ fn render( let cmd_y = viewport_y + 1 + log_height as u16 + 1; write!(buf, "\x1b[{};1H", cmd_y)?; buf.extend_from_slice(ERASE_TO_EOL); - let cmd = jj::command_line(subcommand, passthrough, &app.pending_ids()); + let cmd = jj::command_line(target, &app.pending_ids()); let cmd_max = (term_cols as usize).saturating_sub(2); // for "▶ " let cmd_visible: String = if cmd.chars().count() <= cmd_max { cmd