From 506d3e0b07031f81044a3f8661ae9a83c6633f51 Mon Sep 17 00:00:00 2001 From: Nick Eddy Date: Fri, 15 May 2026 15:20:07 -0700 Subject: [PATCH 1/4] Fix git status parser to handle paths with spaces and renames (#271) Switch `files_for_staging` to `git status --short -z` so paths arrive NUL-separated and unquoted, and parse each record by taking everything after the 3-byte `XY ` status prefix instead of splitting on whitespace. This preserves filenames containing spaces, and the parser now skips the source record of rename (`R`) and copy (`C`) entries since only the destination exists on disk. Co-Authored-By: Claude Opus 4.7 (1M context) --- command-signatures/src/generators/git.rs | 177 ++++++++++++++++++----- 1 file changed, 139 insertions(+), 38 deletions(-) diff --git a/command-signatures/src/generators/git.rs b/command-signatures/src/generators/git.rs index 1057c0b3..693fce44 100644 --- a/command-signatures/src/generators/git.rs +++ b/command-signatures/src/generators/git.rs @@ -440,16 +440,29 @@ fn post_process_tracked_files(output: &str) -> GeneratorResults { return GeneratorResults::default(); } - output - .lines() - // The first non-whitespace string is just a character indicating the type of indexed file. - .filter_map(|file| file.split_whitespace().nth(1)) - .map(|file| { - Suggestion::with_description(file, "Changed file") + // `git status --short -z` emits NUL-separated records of the form `XY ` + // (two-byte status code + space + raw pathname, no C-style quoting). Renames + // and copies are emitted as two records: `R \0\0` — the source + // path follows the destination and we skip it because that file no longer + // exists on disk. + let mut suggestions: Vec = Vec::new(); + let mut records = output.split('\0').filter(|r| !r.is_empty()); + while let Some(record) = records.next() { + let Some(path) = record.get(3..) else { continue }; + if matches!(record.as_bytes().first(), Some(b'R') | Some(b'C')) { + records.next(); + } + suggestions.push( + Suggestion::with_description(path, "Changed file") .with_priority(Priority::Global(Importance::More(Order(100)))) - .with_icon(IconType::File) - }) - .collect_unordered_results() + .with_icon(IconType::File), + ); + } + + GeneratorResults { + suggestions, + is_ordered: false, + } } fn post_process_git_for_each_ref(output: &str) -> GeneratorResults { @@ -770,7 +783,7 @@ pub fn generator() -> CommandSignatureGenerators { .add_generator( "files_for_staging", Generator::script( - CommandBuilder::single_command("git --no-optional-locks status --short"), + CommandBuilder::single_command("git --no-optional-locks status --short -z"), post_process_tracked_files, ), ) @@ -965,47 +978,135 @@ mod tests { ); } + fn changed_file(path: &str) -> Suggestion { + Suggestion { + exact_string: path.to_owned(), + display_name: None, + description: Some("Changed file".to_owned()), + priority: Priority::Global(Importance::More(Order(100))), + icon: Some(IconType::File), + is_hidden: false, + } + } + #[test] fn test_post_process_tracked_files() { - let command_output = r" - M app/src/features.rs - M app/src/launch_config_palette.rs - M app/src/workspace/mod.rs"; + // `git status --short -z` output: NUL-separated records, each `XY `. + let command_output = + " M app/src/features.rs\0M app/src/launch_config_palette.rs\0 M app/src/workspace/mod.rs\0"; assert_eq!( post_process_tracked_files(command_output), GeneratorResults { suggestions: vec![ - Suggestion { - exact_string: "app/src/features.rs".to_owned(), - display_name: None, - description: Some("Changed file".to_owned()), - priority: Priority::Global(Importance::More(Order(100))), - icon: Some(IconType::File), - is_hidden: false, - }, - Suggestion { - exact_string: "app/src/launch_config_palette.rs".to_owned(), - display_name: None, - description: Some("Changed file".to_owned()), - priority: Priority::Global(Importance::More(Order(100))), - icon: Some(IconType::File), - is_hidden: false, - }, - Suggestion { - exact_string: "app/src/workspace/mod.rs".to_owned(), - display_name: None, - description: Some("Changed file".to_owned()), - priority: Priority::Global(Importance::More(Order(100))), - icon: Some(IconType::File), - is_hidden: false, - }, + changed_file("app/src/features.rs"), + changed_file("app/src/launch_config_palette.rs"), + changed_file("app/src/workspace/mod.rs"), ], is_ordered: false, } ); } + /// Filenames with spaces must be preserved intact. Under `-z` git emits raw + /// bytes with no C-style quoting, so the parser must take everything after + /// the 3-byte `XY ` prefix rather than splitting on whitespace. + #[test] + fn test_post_process_tracked_files_with_spaces_in_path() { + // Untracked file `new file test.csv` under `-z`: + let command_output = "?? new file test.csv\0"; + + assert_eq!( + post_process_tracked_files(command_output), + GeneratorResults { + suggestions: vec![changed_file("new file test.csv")], + is_ordered: false, + } + ); + } + + /// Renames under `-z` are emitted as two records: `R \0\0`. + /// We surface the destination only — the source no longer exists on disk. + #[test] + fn test_post_process_tracked_files_rename() { + let command_output = "R new name.txt\0old name.txt\0 M other.rs\0"; + + assert_eq!( + post_process_tracked_files(command_output), + GeneratorResults { + suggestions: vec![changed_file("new name.txt"), changed_file("other.rs")], + is_ordered: false, + } + ); + } + + /// Copies (`C`) are formatted the same way as renames (`\0\0`) + /// and must skip the source record just like renames. + #[test] + fn test_post_process_tracked_files_copy() { + let command_output = "C copied.txt\0source.txt\0"; + + assert_eq!( + post_process_tracked_files(command_output), + GeneratorResults { + suggestions: vec![changed_file("copied.txt")], + is_ordered: false, + } + ); + } + + /// Two renames in a row exercise the iterator-state interaction between + /// successive skip-source decisions. + #[test] + fn test_post_process_tracked_files_back_to_back_renames() { + let command_output = "R a.rs\0a-old.rs\0R b.rs\0b-old.rs\0"; + + assert_eq!( + post_process_tracked_files(command_output), + GeneratorResults { + suggestions: vec![changed_file("a.rs"), changed_file("b.rs")], + is_ordered: false, + } + ); + } + + /// `git status --short -z` emits untracked directories with a trailing slash. + #[test] + fn test_post_process_tracked_files_untracked_directory() { + let command_output = "?? dir with space/\0"; + + assert_eq!( + post_process_tracked_files(command_output), + GeneratorResults { + suggestions: vec![changed_file("dir with space/")], + is_ordered: false, + } + ); + } + + /// Empty output yields no suggestions. + #[test] + fn test_post_process_tracked_files_empty() { + assert_eq!( + post_process_tracked_files(""), + GeneratorResults { + suggestions: vec![], + is_ordered: false, + } + ); + } + + /// Fatal errors short-circuit to the default (empty, ordered) result. + #[test] + fn test_post_process_tracked_files_fatal_error() { + let command_output = "fatal: not a git repository\n"; + + assert_eq!( + post_process_tracked_files(command_output), + GeneratorResults::default() + ); + } + #[test] fn test_post_process_tags() { let command_output = "v1.0.0\nv2.0.0\nv0.1.0"; From 36cdf5ee90e208a100be1f5548f9068a0af9f64b Mon Sep 17 00:00:00 2001 From: Nick Eddy Date: Wed, 27 May 2026 12:23:43 -0700 Subject: [PATCH 2/4] cargo fmt --- command-signatures/src/generators/git.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/command-signatures/src/generators/git.rs b/command-signatures/src/generators/git.rs index 693fce44..f14550e9 100644 --- a/command-signatures/src/generators/git.rs +++ b/command-signatures/src/generators/git.rs @@ -448,7 +448,9 @@ fn post_process_tracked_files(output: &str) -> GeneratorResults { let mut suggestions: Vec = Vec::new(); let mut records = output.split('\0').filter(|r| !r.is_empty()); while let Some(record) = records.next() { - let Some(path) = record.get(3..) else { continue }; + let Some(path) = record.get(3..) else { + continue; + }; if matches!(record.as_bytes().first(), Some(b'R') | Some(b'C')) { records.next(); } From 31302bdfb62f980aa4eb01b9d8ffd6d2070711ae Mon Sep 17 00:00:00 2001 From: Nick Eddy Date: Fri, 29 May 2026 10:32:04 -0700 Subject: [PATCH 3/4] Fix get_changed_or_tracked_files regression and clarify rename/copy handling Switching files_for_staging to git status --short -z made post_process_tracked_files a NUL-record parser, but get_changed_or_tracked_files still fed it newline-separated output (git status --short | sed). split('\0') then yields a single record, collapsing the whole blob into one garbage suggestion with embedded newlines. Drop the sed pipe (its -z flag is GNU-only and breaks on macOS) and let git filter directly: git diff [--cached] --diff-filter=AM --name-only -z. --cached inspects the index, without it the working tree; --diff-filter=AM keeps the original M/A-only behavior; --name-only -z emits bare NUL-separated paths handled by a dedicated post_process_diff_name_only. Also correct the post_process_tracked_files comment: for copies the rename/copy origin still exists on disk, so we skip it because it is unchanged, not because it is gone. Add tests covering the new get_changed_or_tracked_files path (incl. spaces, empty, fatal-error) so it cannot silently regress again. Co-Authored-By: Claude Opus 4.8 (1M context) --- command-signatures/src/generators/git.rs | 107 +++++++++++++++++++++-- 1 file changed, 99 insertions(+), 8 deletions(-) diff --git a/command-signatures/src/generators/git.rs b/command-signatures/src/generators/git.rs index f14550e9..58e992df 100644 --- a/command-signatures/src/generators/git.rs +++ b/command-signatures/src/generators/git.rs @@ -442,9 +442,11 @@ fn post_process_tracked_files(output: &str) -> GeneratorResults { // `git status --short -z` emits NUL-separated records of the form `XY ` // (two-byte status code + space + raw pathname, no C-style quoting). Renames - // and copies are emitted as two records: `R \0\0` — the source - // path follows the destination and we skip it because that file no longer - // exists on disk. + // (`R`) and copies (`C`) span two records — `R \0\0` — where the + // second record is the rename/copy *origin*. We surface only the first + // (the resulting `` path) and skip the origin: for a rename the origin + // is gone, and for a copy the origin still exists but is unchanged, so in + // neither case is it a freshly changed file worth suggesting. let mut suggestions: Vec = Vec::new(); let mut records = output.split('\0').filter(|r| !r.is_empty()); while let Some(record) = records.next() { @@ -467,6 +469,36 @@ fn post_process_tracked_files(output: &str) -> GeneratorResults { } } +/// Post-processes `git diff [--cached] --diff-filter=AM --name-only -z` output +/// for the `get_changed_or_tracked_files` generator. +/// +/// That generator can't share the `git status --short -z` path used by +/// `files_for_staging`: it needs to distinguish staged (index) changes from +/// working-tree changes, and the only place that distinction is visible is the +/// command itself (the post-processor receives stdout alone). Doing the +/// distinction with `git status` would mean filtering NUL records in the shell, +/// which requires GNU-only `sed -z`/`grep -z` and breaks on macOS. Letting git +/// filter via `--diff-filter` keeps everything portable. +/// +/// `--name-only -z` emits bare NUL-separated pathnames (no `XY ` status prefix, +/// no C-style quoting), so each non-empty record is already a complete path. +fn post_process_diff_name_only(output: &str) -> GeneratorResults { + let output = filter_messages(output); + if output.starts_with("fatal:") { + return GeneratorResults::default(); + } + + output + .split('\0') + .filter(|path| !path.is_empty()) + .map(|path| { + Suggestion::with_description(path, "Changed file") + .with_priority(Priority::Global(Importance::More(Order(100)))) + .with_icon(IconType::File) + }) + .collect_unordered_results() +} + fn post_process_git_for_each_ref(output: &str) -> GeneratorResults { output .split('\n') @@ -826,13 +858,22 @@ pub fn generator() -> CommandSignatureGenerators { "get_changed_or_tracked_files", Generator::command_from_tokens( |tokens, _, _| { + // Filter to Added/Modified paths with git itself (`--diff-filter`) + // and emit NUL-separated bare pathnames (`--name-only -z`). This + // avoids a `sed`/`grep` NUL filter, whose `-z` flag is GNU-only and + // breaks on the BSD tools shipped with macOS. `--cached` inspects + // the index (staged changes); without it, the working tree. if tokens.contains(&"--staged") || tokens.contains(&"--cached") { - CommandBuilder::pipe( CommandBuilder::single_command(r#"git --no-optional-locks status --short"#), CommandBuilder::single_command(r#"sed -ne '/^M /p' -e '/A /p'"#)) + CommandBuilder::single_command( + "git --no-optional-locks diff --cached --diff-filter=AM --name-only -z", + ) } else { - CommandBuilder::pipe(CommandBuilder::single_command(r#"git --no-optional-locks status --short"#), CommandBuilder::single_command(r#"sed -ne '/M /p' -e '/A /p'"#)) + CommandBuilder::single_command( + "git --no-optional-locks diff --diff-filter=AM --name-only -z", + ) } }, - post_process_tracked_files, + post_process_diff_name_only, ), ) .add_generator( @@ -915,8 +956,9 @@ pub fn generator() -> CommandSignatureGenerators { #[cfg(test)] mod tests { use crate::generators::git::{ - detect_refspec_prefix, post_process_branches, post_process_push_refspec_branches, - post_process_push_refspec_tags, post_process_tags, post_process_tracked_files, + detect_refspec_prefix, post_process_branches, post_process_diff_name_only, + post_process_push_refspec_branches, post_process_push_refspec_tags, post_process_tags, + post_process_tracked_files, }; use warp_completion_metadata::{ GeneratorResults, IconType, Importance, Order, Priority, Suggestion, @@ -1109,6 +1151,55 @@ mod tests { ); } + /// Regression coverage for the `get_changed_or_tracked_files` generator. + /// + /// It can't share the `git status --short -z` path used by + /// `files_for_staging`, because it needs to distinguish staged from + /// working-tree changes and that requires filtering NUL records — for which + /// the only portable tool is git itself (`sed -z`/`grep -z` are GNU-only and + /// break on macOS). It therefore runs `git diff [--cached] --diff-filter=AM + /// --name-only -z`, which emits bare NUL-separated pathnames with no `XY ` + /// status prefix. The post-processor must yield one suggestion per record + /// and preserve spaces in pathnames. + #[test] + fn test_post_process_diff_name_only() { + // What `git diff --diff-filter=AM --name-only -z` emits. + let command_output = "app/src/features.rs\0app/src/new.rs\0dir with space/some file.rs\0"; + + assert_eq!( + post_process_diff_name_only(command_output), + GeneratorResults { + suggestions: vec![ + changed_file("app/src/features.rs"), + changed_file("app/src/new.rs"), + changed_file("dir with space/some file.rs"), + ], + is_ordered: false, + } + ); + } + + /// Empty output (no changed files) yields no suggestions. + #[test] + fn test_post_process_diff_name_only_empty() { + assert_eq!( + post_process_diff_name_only(""), + GeneratorResults { + suggestions: vec![], + is_ordered: false, + } + ); + } + + /// Fatal errors short-circuit to the default (empty, ordered) result. + #[test] + fn test_post_process_diff_name_only_fatal_error() { + assert_eq!( + post_process_diff_name_only("fatal: not a git repository\n"), + GeneratorResults::default() + ); + } + #[test] fn test_post_process_tags() { let command_output = "v1.0.0\nv2.0.0\nv0.1.0"; From 208a88b8f4e65feb72618735326c8ecffc1e7f59 Mon Sep 17 00:00:00 2001 From: Nick Eddy Date: Fri, 29 May 2026 11:59:05 -0700 Subject: [PATCH 4/4] Trim comments per review Cut the verbose doc/inline comments down to concise descriptions on the relevant functions; the git-diff rationale moves to a PR review comment. Co-Authored-By: Claude Opus 4.8 (1M context) --- command-signatures/src/generators/git.rs | 43 ++++-------------------- 1 file changed, 7 insertions(+), 36 deletions(-) diff --git a/command-signatures/src/generators/git.rs b/command-signatures/src/generators/git.rs index 58e992df..b8f87107 100644 --- a/command-signatures/src/generators/git.rs +++ b/command-signatures/src/generators/git.rs @@ -440,13 +440,10 @@ fn post_process_tracked_files(output: &str) -> GeneratorResults { return GeneratorResults::default(); } - // `git status --short -z` emits NUL-separated records of the form `XY ` - // (two-byte status code + space + raw pathname, no C-style quoting). Renames - // (`R`) and copies (`C`) span two records — `R \0\0` — where the - // second record is the rename/copy *origin*. We surface only the first - // (the resulting `` path) and skip the origin: for a rename the origin - // is gone, and for a copy the origin still exists but is unchanged, so in - // neither case is it a freshly changed file worth suggesting. + // Records are NUL-separated `XY ` (status code + space + raw pathname, + // no quoting). Renames (`R`) and copies (`C`) span two records, + // `R \0\0`; we keep the `` and skip the origin, which isn't a + // changed file (renamed away, or an unchanged copy source). let mut suggestions: Vec = Vec::new(); let mut records = output.split('\0').filter(|r| !r.is_empty()); while let Some(record) = records.next() { @@ -469,19 +466,8 @@ fn post_process_tracked_files(output: &str) -> GeneratorResults { } } -/// Post-processes `git diff [--cached] --diff-filter=AM --name-only -z` output -/// for the `get_changed_or_tracked_files` generator. -/// -/// That generator can't share the `git status --short -z` path used by -/// `files_for_staging`: it needs to distinguish staged (index) changes from -/// working-tree changes, and the only place that distinction is visible is the -/// command itself (the post-processor receives stdout alone). Doing the -/// distinction with `git status` would mean filtering NUL records in the shell, -/// which requires GNU-only `sed -z`/`grep -z` and breaks on macOS. Letting git -/// filter via `--diff-filter` keeps everything portable. -/// -/// `--name-only -z` emits bare NUL-separated pathnames (no `XY ` status prefix, -/// no C-style quoting), so each non-empty record is already a complete path. +/// Parses `git diff --name-only -z` output: bare NUL-separated pathnames, one +/// per changed file, with no status prefix or quoting. fn post_process_diff_name_only(output: &str) -> GeneratorResults { let output = filter_messages(output); if output.starts_with("fatal:") { @@ -858,11 +844,6 @@ pub fn generator() -> CommandSignatureGenerators { "get_changed_or_tracked_files", Generator::command_from_tokens( |tokens, _, _| { - // Filter to Added/Modified paths with git itself (`--diff-filter`) - // and emit NUL-separated bare pathnames (`--name-only -z`). This - // avoids a `sed`/`grep` NUL filter, whose `-z` flag is GNU-only and - // breaks on the BSD tools shipped with macOS. `--cached` inspects - // the index (staged changes); without it, the working tree. if tokens.contains(&"--staged") || tokens.contains(&"--cached") { CommandBuilder::single_command( "git --no-optional-locks diff --cached --diff-filter=AM --name-only -z", @@ -1151,19 +1132,9 @@ mod tests { ); } - /// Regression coverage for the `get_changed_or_tracked_files` generator. - /// - /// It can't share the `git status --short -z` path used by - /// `files_for_staging`, because it needs to distinguish staged from - /// working-tree changes and that requires filtering NUL records — for which - /// the only portable tool is git itself (`sed -z`/`grep -z` are GNU-only and - /// break on macOS). It therefore runs `git diff [--cached] --diff-filter=AM - /// --name-only -z`, which emits bare NUL-separated pathnames with no `XY ` - /// status prefix. The post-processor must yield one suggestion per record - /// and preserve spaces in pathnames. + /// NUL-separated paths, including one with spaces, each become a suggestion. #[test] fn test_post_process_diff_name_only() { - // What `git diff --diff-filter=AM --name-only -z` emits. let command_output = "app/src/features.rs\0app/src/new.rs\0dir with space/some file.rs\0"; assert_eq!(