diff --git a/docs/usage/FEATURES.md b/docs/usage/FEATURES.md index 19d722632e..8d646693b7 100644 --- a/docs/usage/FEATURES.md +++ b/docs/usage/FEATURES.md @@ -215,17 +215,19 @@ rtk grep [chemin] [options] | Option | Court | Defaut | Description | |--------|-------|--------|-------------| -| `--max-len` | `-l` | 80 | Longueur maximale de ligne | -| `--max` | | 200 | Nombre maximum de resultats (pas de raccourci, `-m` est reserve a `grep --max-count`) | -| `--context-only` | | non | Afficher uniquement le contexte du match (pas de raccourci, `-c` est reserve a `grep --count`) | -| `--file-type` | `-t` | tous | Filtrer par type (ts, py, rust, etc.) | -| `--line-numbers` | `-n` | oui | Numeros de ligne (toujours actif) | +| `--max-len` | | 80 | Longueur maximale de ligne | +| `--max` | | 200 | Nombre maximum de resultats | +| `--context-only` | | non | Afficher uniquement le contexte du match | -Les arguments supplementaires sont transmis a `rg` (ripgrep). Les flags qui changent le format de sortie (`-c`, `-l`, `-L`, `-o`, `-Z`) passent directement a `rg`/`grep` sans filtrage RTK. +Aucune de ces options n'a de raccourci : `-l`, `-m` et `-c` sont reserves aux flags natifs de meme lettre (`--files-with-matches`, `--max-count`, `--count`) et sont transmis au moteur. + +Les arguments supplementaires sont transmis au moteur reellement invoque -- `grep` pour `rtk grep`, `rg` pour `rtk rg`, RTK ne substituant jamais l'un a l'autre. Un flag propre a ripgrep (`--glob`, `-t`) n'est donc valide que sous `rtk rg`. Les flags qui changent le format de sortie (`-c`, `-l`, `-L`, `-o`, `-Z`) passent directement au moteur sans filtrage RTK. + +Il n'y a pas d'option `--file-type` : `rtk rg -t rust` filtre par type, `rtk grep -t rust` remonte l'erreur de grep (`invalid option -- 't'`). **Avant / Apres :** ``` -# rg "fn run" (20 lignes) # rtk grep "fn run" (10 lignes) +# grep -rn "fn run" (20 lignes) # rtk grep -rn "fn run" (10 lignes) src/git.rs:45:pub fn run(...) src/git.rs src/git.rs:120:fn run_status(...) 45: pub fn run(...) src/ls.rs:12:pub fn run(...) 120: fn run_status(...) diff --git a/scripts/benchmark.sh b/scripts/benchmark.sh index ffc7301426..5d5f91caf0 100755 --- a/scripts/benchmark.sh +++ b/scripts/benchmark.sh @@ -215,7 +215,7 @@ bench "git show" "git show HEAD --stat 2>/dev/null || true" "$RTK git show HEAD section "grep" bench "grep fn" "grep -rn 'fn ' src/ || true" "$RTK grep -rn 'fn ' src/" bench "grep struct" "grep -rn 'struct ' src/ || true" "$RTK grep -rn 'struct ' src/" -bench "grep -l 40" "grep -rn 'fn ' src/ || true" "$RTK grep -rn 'fn ' src/ -l 40" +bench "grep --max-len 40" "grep -rn 'fn ' src/ || true" "$RTK grep --max-len 40 -rn 'fn ' src/" bench "grep -c" "grep -ron 'fn ' src/ || true" "$RTK grep -rc 'fn ' src/" # =================== diff --git a/scripts/test-all.sh b/scripts/test-all.sh index 8be6e8deca..ecba1d9a4b 100755 --- a/scripts/test-all.sh +++ b/scripts/test-all.sh @@ -257,9 +257,20 @@ fi section "Grep" -assert_ok "rtk grep pattern" rtk grep "pub fn" src/ -assert_contains "rtk grep finds results" "pub fn" rtk grep "pub fn" src/ -assert_ok "rtk grep with file type" rtk grep "pub fn" src/ -t rust +# All three need -r: `grep PATTERN ` without it exits 2 ("Is a directory"), +# which made the first two assertions red and the third one inert. +assert_ok "rtk grep pattern" rtk grep -r "pub fn" src/ +assert_contains "rtk grep finds results" "pub fn" rtk grep -r "pub fn" src/ +# `-t` is rg-only. The form matters: the pattern must match, the path must be a +# single file, and `-t` must come first, so the only reason to fail is `-t` +# itself -- a directory, a non-matching pattern, or `-t` after another flag all +# make this assertion pass either way. +assert_fails "rtk grep -t rejected by grep" rtk grep -t rust "fn main" src/main.rs +if command -v rg >/dev/null 2>&1; then + assert_ok "rtk rg with file type" rtk rg "pub fn" src/ -t rust +else + skip_test "rtk rg with file type" "rg not installed" +fi section "Grep (extra args passthrough)" diff --git a/scripts/test-aristote.sh b/scripts/test-aristote.sh index 371ce90332..b93aa3ef06 100755 --- a/scripts/test-aristote.sh +++ b/scripts/test-aristote.sh @@ -121,9 +121,16 @@ assert_ok "rtk read --max-lines 10" rtk read --max-lines 10 "$ARISTO section "Grep" -assert_ok "rtk grep import" rtk grep "import" "$ARISTOTE/src" -assert_ok "rtk grep with type filter" rtk grep "useState" "$ARISTOTE/src" -t tsx -assert_contains "rtk grep finds components" "import" rtk grep "import" "$ARISTOTE/src" +# `grep PATTERN ` without -r exits 2 ("Is a directory"). +assert_ok "rtk grep import" rtk grep -r "import" "$ARISTOTE/src" +assert_contains "rtk grep finds components" "import" rtk grep -r "import" "$ARISTOTE/src" + +# `-t` is rg-only, so the type filter belongs to the rg engine. +if command -v rg >/dev/null 2>&1; then + assert_ok "rtk rg with type filter" rtk rg "useState" "$ARISTOTE/src" -t tsx +else + skip_test "rtk rg with type filter" "rg not installed" +fi # ── 4. Git ─────────────────────────────────────────── diff --git a/src/main.rs b/src/main.rs index 4520e13062..c2f9e67327 100644 --- a/src/main.rs +++ b/src/main.rs @@ -313,8 +313,16 @@ enum Commands { /// Compact grep - strips whitespace, truncates, groups by file Grep { + // rtk's own options here are long-only: a short form shadows the native + // grep/rg flag of the same letter and captures it before it can reach + // src/cmds/system/search.rs, the shared filter behind `rtk grep` and + // `rtk rg`. /// Max line length - #[arg(short = 'l', long, default_value = "80")] + // No short: `-l` is grep's --files-with-matches. Bound to a `usize` it + // accepted numeric patterns -- `rtk grep -l 8080 a.txt b.txt` set + // max_len=8080, took a.txt as the pattern, and returned empty output + // with no error. + #[arg(long, default_value = "80")] max_len: usize, /// Max results to show // No short: `-m` is GNU grep's --max-count (stop after N matches per @@ -328,9 +336,9 @@ enum Commands { /// Show only match context (not full line) #[arg(long)] context_only: bool, - /// Filter by file type (e.g., ts, py, rust) - #[arg(short = 't', long)] - file_type: Option, + // No --file-type: `-t TYPE` is rg's type filter and the option never + // reached the engine, so `rtk rg -t rust` filters by type while + // `rtk grep -t rust` surfaces grep's own error. /// Pattern, path, and any grep/rg flags (e.g. -v, -i, -A 3, --glob, --version) #[arg(trailing_var_arg = true, allow_hyphen_values = true)] extra_args: Vec, @@ -2005,7 +2013,6 @@ fn run_cli() -> Result { max_len, max, context_only, - file_type: _, extra_args, } => search::run( search::Engine::Grep, @@ -3695,4 +3702,164 @@ mod tests { _ => panic!("Expected Init command"), } } + + // --- grep argument routing (clap layer) --- + // + // The `Grep` variant funnels the pattern, path, and every native grep/rg flag + // into one trailing slot so `src/cmds/system/search.rs` parses them with full + // grep/rg semantics. These tests pin that routing. + + /// Parse `rtk grep …` and return the captured `extra_args`, or `None` if + /// clap rejects the invocation (e.g. a colliding short option mis-binds). + fn grep_extra_args(args: &[&str]) -> Option> { + match Cli::try_parse_from(args).ok()?.command { + Commands::Grep { extra_args, .. } => Some(extra_args), + _ => None, + } + } + + #[test] + fn test_grep_parse_simple_pattern_path() { + assert_eq!( + grep_extra_args(&["rtk", "grep", "FOO", "src/"]).unwrap(), + vec!["FOO", "src/"] + ); + } + + #[test] + fn test_grep_parse_combined_short_cluster() { + // `-rn` is a native grep cluster (recursive + line-numbers); it must + // reach search.rs intact, not be intercepted by clap. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-rn", "FOO", "src/"]).unwrap(), + vec!["-rn", "FOO", "src/"] + ); + } + + #[test] + fn test_grep_parse_value_flag_after_context() { + // `-A 3` (after-context) — the value must not be stolen by clap. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-A", "3", "FOO", "file"]).unwrap(), + vec!["-A", "3", "FOO", "file"] + ); + } + + #[test] + fn test_grep_parse_invert_match_v_not_shadowed() { + // `-v` (invert-match) must reach grep, not be captured as rtk's + // top-level verbose flag. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-v", "FOO", "file"]).unwrap(), + vec!["-v", "FOO", "file"] + ); + } + + #[test] + fn test_grep_parse_alternation_pattern_intact() { + // Extended-regex alternation must pass through untouched. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-nE", "a|b", "file"]).unwrap(), + vec!["-nE", "a|b", "file"] + ); + } + + #[test] + fn test_grep_parse_files_with_matches_l() { + // Native grep `-l` (files-with-matches). Must parse and pass `-l` through. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-l", "FOO", "src/"]).unwrap(), + vec!["-l", "FOO", "src/"] + ); + } + + #[test] + fn test_grep_parse_type_t_forwarded() { + // `-t TYPE` is rg-only and must reach the engine: `rtk rg` filters by + // type, `rtk grep` surfaces grep's own `invalid option -- 't'`. + assert_eq!( + grep_extra_args(&["rtk", "grep", "-t", "rust", "FOO", "src/"]).unwrap(), + vec!["-t", "rust", "FOO", "src/"] + ); + } + + #[test] + fn test_grep_file_type_option_is_gone() { + // `--file-type` was removed, not merely un-shorted: an unknown long flag + // routes to extra_args and is the engine's problem. Re-adding the option + // would bind it here and cost extra_args these two tokens. + assert_eq!( + grep_extra_args(&["rtk", "grep", "--file-type", "rust", "FOO", "src/"]).unwrap(), + vec!["--file-type", "rust", "FOO", "src/"] + ); + } + + /// Parse `rtk grep …` into the full `Grep` field set for inspection. + fn parse_grep(args: &[&str]) -> Result<(usize, usize, bool, Vec), clap::Error> { + match Cli::try_parse_from(args)?.command { + Commands::Grep { + max_len, + max, + context_only, + extra_args, + } => Ok((max_len, max, context_only, extra_args)), + _ => unreachable!("parsed a grep command"), + } + } + + #[test] + fn test_grep_long_options_bind_before_pattern() { + // Long-only knobs must still bind when placed before the pattern, with + // only the pattern/path landing in extra_args. + let (max_len, max, context_only, extra_args) = parse_grep(&[ + "rtk", + "grep", + "--max-len", + "40", + "--max", + "5", + "--context-only", + "FOO", + "path", + ]) + .unwrap(); + assert_eq!(max_len, 40); + assert_eq!(max, 5); + assert!(context_only); + assert_eq!(extra_args, vec!["FOO", "path"]); + } + + #[test] + fn test_grep_options_after_pattern_stay_in_extra_args() { + // Once the pattern starts the trailing slot, later tokens — even rtk's + // own `--max-len` — pass through verbatim rather than binding as options. + // The mechanism is `allow_hyphen_values`, not `trailing_var_arg`. + let (max_len, _, _, extra_args) = + parse_grep(&["rtk", "grep", "FOO", "--max-len", "40"]).unwrap(); + assert_eq!( + max_len, 80, + "option after pattern must NOT bind (default kept)" + ); + assert_eq!(extra_args, vec!["FOO", "--max-len", "40"]); + } + + #[test] + fn test_grep_version_routes_to_extra_args() { + // `--version` is not a subcommand flag (version isn't propagated), so it + // lands in extra_args, and search::run's --version/--help check execs it + // against the engine unfiltered. + assert_eq!( + grep_extra_args(&["rtk", "grep", "--version"]).unwrap(), + vec!["--version"] + ); + } + + #[test] + fn test_grep_double_dash_consumed_by_clap() { + // clap strips the `--` separator before populating trailing_var_arg. + // core::args_utils::restore_double_dash re-inserts it at runtime — this + // test pins the premise that helper depends on. + let (_, _, _, extra_args) = parse_grep(&["rtk", "grep", "--", "-v", "file"]).unwrap(); + assert_eq!(extra_args, vec!["-v", "file"], "clap must consume the --"); + } } diff --git a/tests/search_compress_test.rs b/tests/search_compress_test.rs index 74c55aed8c..702bfd925b 100644 --- a/tests/search_compress_test.rs +++ b/tests/search_compress_test.rs @@ -17,6 +17,17 @@ fn rg_available() -> bool { .unwrap_or(false) } +/// `rtk grep` shells out to `grep`, never `rg` (`search.rs`: `Engine::Grep => "grep"`). +/// Guarding a grep-engine test on `rg_available()` makes it silently skip -- and +/// report success -- on a box without ripgrep, which is a false green. +fn grep_available() -> bool { + Command::new("grep") + .arg("--version") + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + fn write_temp(content: &str) -> (tempfile::TempDir, std::path::PathBuf) { let dir = tempfile::tempdir().expect("tempdir"); let path = dir.path().join("test.txt"); @@ -28,7 +39,7 @@ fn write_temp(content: &str) -> (tempfile::TempDir, std::path::PathBuf) { #[test] fn single_file_context_shown_without_header() { - if !rg_available() { + if !grep_available() { return; } let long = "x".repeat(120); @@ -53,7 +64,7 @@ fn single_file_context_shown_without_header() { #[test] fn after_context_uses_dash_separator_for_context_lines() { - if !rg_available() { + if !grep_available() { return; } let (_dir, path) = write_temp("MATCH\nafter1\n"); @@ -72,7 +83,7 @@ fn after_context_uses_dash_separator_for_context_lines() { #[test] fn capped_single_file_shows_header() { - if !rg_available() { + if !grep_available() { return; } let filler: String = (0..40).map(|i| format!("w{i} ")).collect(); @@ -92,7 +103,7 @@ fn capped_single_file_shows_header() { #[test] fn true_no_match_exits_1() { - if !rg_available() { + if !grep_available() { return; } let (_dir, path) = write_temp("hello world\n"); @@ -134,7 +145,7 @@ fn no_line_number_flag_produces_output_not_zero_matches() { #[test] fn no_filename_flag_produces_output_not_zero_matches() { - if !rg_available() { + if !grep_available() { return; } let (_dir, path) = write_temp("hello world\n"); @@ -231,7 +242,7 @@ fn count_tokens(s: &str) -> usize { // Covers #545: grep savings are measured against the real grep output. #[test] fn bulky_grep_yields_token_savings() { - if !rg_available() { + if !rg_available() || !grep_available() { return; } let filler: String = (0..50).map(|i| format!("word{i} ")).collect(); @@ -264,7 +275,7 @@ fn bulky_grep_yields_token_savings() { #[test] fn grep_only_flags_fall_back_to_system_grep() { - if !rg_available() { + if !grep_available() { return; } let dir = tempfile::tempdir().expect("tempdir"); @@ -296,7 +307,7 @@ fn grep_only_flags_fall_back_to_system_grep() { #[test] fn small_grep_not_worse_than_plain() { - if !rg_available() { + if !grep_available() { return; } let (_dir, path) = write_temp("foo\n"); @@ -316,11 +327,108 @@ fn small_grep_not_worse_than_plain() { ); } +// --- a numeric pattern must not bind to rtk's removed `-l` short --- + +#[test] +fn numeric_pattern_with_files_with_matches_flag() { + if !grep_available() { + return; + } + // The pattern MUST parse as a usize for this test to gate anything. With + // `-l` bound to `--max-len: usize`, a non-numeric pattern makes clap fail, + // and run_fallback then re-runs raw grep and prints the right answer -- so + // a word pattern passes with or without the fix. A numeric pattern instead + // binds silently: `-l 8080` sets max_len=8080, leaving the first filename + // as the pattern and the second as the only path. No error, no fallback, + // wrong answer. + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::write(dir.path().join("hit.txt"), "listen on port 8080 today\n").expect("write"); + std::fs::write(dir.path().join("miss.txt"), "nothing numeric here\n").expect("write"); + let hit = dir.path().join("hit.txt"); + let miss = dir.path().join("miss.txt"); + + let out = rtk() + .args([ + "grep", + "-l", + "8080", + hit.to_str().unwrap(), + miss.to_str().unwrap(), + ]) + .output() + .expect("rtk grep"); + let stdout = String::from_utf8_lossy(&out.stdout); + + assert!( + out.status.success(), + "`grep -l ` must find the match (#2628); status={:?} stderr={}", + out.status.code(), + String::from_utf8_lossy(&out.stderr) + ); + assert!( + stdout.contains("hit.txt"), + "-l must report the file containing 8080:\n{stdout}" + ); + assert!( + !stdout.contains("miss.txt"), + "-l must not report the non-matching file:\n{stdout}" + ); +} + +// --- #2628: rtk's own long options still bind after the short forms were removed --- + +#[test] +fn max_len_long_option_still_binds_to_rtk() { + if !grep_available() { + return; + } + // The other half of #2628: dropping the `-l` short must not stop the long + // form from reaching rtk's truncation. Needs a bulky match set -- a small + // result passes through uncompressed under the never-worse-than-plain + // guard, so a one-line file would prove nothing either way. + let body: String = (0..60) + .map(|i| format!("MATCH {i} {}\n", "x".repeat(400))) + .collect(); + let (_dir, path) = write_temp(&body); + let file = path.to_str().unwrap(); + + // Measure matched lines only. rtk's own trailer ("+N more ... [see + // remaining: tail ...]") is fixed-width chrome that --max-len does not + // govern, and it is the longest line in either run. + let widest_match = |args: &[&str]| -> usize { + let out = rtk().args(args).output().expect("rtk grep"); + assert!( + out.status.success(), + "stderr={}", + String::from_utf8_lossy(&out.stderr) + ); + String::from_utf8_lossy(&out.stdout) + .lines() + .filter(|l| l.starts_with("MATCH ")) + .map(|l| l.chars().count()) + .max() + .unwrap_or(0) + }; + + let narrow = widest_match(&["grep", "--max-len", "40", "MATCH", file]); + let default = widest_match(&["grep", "MATCH", file]); + + assert!( + narrow > 0 && default > 0, + "expected matched lines in both runs" + ); + assert!( + narrow < default, + "--max-len must still bind after the short form was removed \ + (narrow={narrow}, default={default})" + ); +} + // --- #2543: bundled files-with-matches cluster (-rln / -ln) lists files, not "0 matches" --- #[test] fn bundled_files_with_matches_cluster_lists_files() { - if !rg_available() { + if !grep_available() { return; } let dir = tempfile::tempdir().expect("tempdir");