Fix git status parser to handle paths with spaces and renames#272
Conversation
lucieleblanc
left a comment
There was a problem hiding this comment.
Hi @nickeddy , thank you for the fix. Since this change touches generators, could you provide screenshots of the generators working in a local build of Warp? The patch process is explained in the test-local-warp skill. If you're testing on MacOS, you can also use cmd-shift-5 to start a screen recording of the app.
I'll be happy to review this again once a screen capture or a set of screenshots showing the changes are uploaded and displayed in the PR description or a comment. Thanks!
90ade7e to
506d3e0
Compare
lucieleblanc
left a comment
There was a problem hiding this comment.
I agree with the general direction of these changes, but there are a few details we should clarify and another generator we should avoid regressing.
There was a problem hiding this comment.
A few more changes. In general, please prefer concise comments and clearer code, and comment inline in GitHub instead of adding comments about the diff in the diff itself.
Also, the PR description should not be this long. Please shorten it to explain the core of the change. The "verification" section should not include lints/fmt/clippy. Those are prerequisites; describe newly added/modified test cases instead.
Once again, please include screenshots of all affected generators in a comment to show they work as intended with all these edge cases. Thanks!
208a88b to
6777e31
Compare
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
Next step: open or find a same-repo issue describing this change, then link it to this PR by adding Closes #123 to the PR description (or using the "Development" sidebar on GitHub). A maintainer will mark the issue ready-to-implement when it is ready. Once it is marked, comment /oz-review to re-trigger review.
Powered by Oz
|
/oz-review |
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
This PR is linked to #271, but no linked issue is marked ready-to-implement yet. Only repository maintainers apply that label, so please wait for a maintainer to mark the issue. Once it is marked, push a new commit or comment /oz-review to re-trigger review.
Powered by Oz
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR updates the git command-signature file completion parser to consume NUL-separated git status --short -z and git diff --name-only -z output, preserving paths with spaces and correctly handling rename/copy records. The added tests cover spaces, renames, copies, empty output, and fatal-error handling.
Concerns
No blocking correctness, security, or spec-drift concerns found in the attached diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Hi @nickeddy I'm sorry for the delay. I'm the team member who will take over overseeing this PR. |
|
Looks like we're conflicting with #279 which fixed a separate issue. You'll need to move your change into the |
…acters Rebased onto #279, which replaced the status-short files_for_staging generator with a token-aware `git ls-files` one. The -z fix now lands in that generator instead: - files_for_staging_command: add -z to both ls-files invocations so paths arrive NUL-separated and raw (core.quotePath otherwise C-quotes non-ASCII/special filenames); post_process_files_for_staging splits on NUL instead of lines. - get_changed_or_tracked_files: replace the `git status --short | sed` pipes with `git diff [--cached] --diff-filter=AM --name-only -z` and a NUL-splitting post_process_diff_name_only, preserving filenames with spaces (git does the filtering — portable, unlike GNU-only sed -z). - Drop post_process_tracked_files, now unused. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6777e31 to
c1abbd4
Compare
|
Reworked against #279 and retested against my warp#11796 PR, confirmed working: |
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
|
CLA verification got stuck. closing and re-opening to un-stick |
There was a problem hiding this comment.
Overview
This PR updates the Git file suggestion generators to consume NUL-separated path output for git ls-files and git diff --name-only, preserving paths with spaces and avoiding shell-specific sed parsing. It also adds focused unit coverage for NUL-separated diff output, empty output, fatal errors, and staging suggestions with spaces.
Concerns
- No blocking correctness issues found in the annotated diff.
- No security findings identified in the changed lines.
spec_context.mdcontains no approved or repository spec context, so there were no spec commitments to validate against.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
## Description Updates `warp-command-signatures` to ec1ae8e8. ### Merged PRs - Fix git status parser to handle paths with spaces and renames (warpdotdev/command-signatures#272) - fix(git): complete nested untracked files for git add (warpdotdev/command-signatures#279) - Resolve update-command-spec-version repo paths dynamically (warpdotdev/command-signatures#290) ## Changelog Entries for Stable CHANGELOG-BUG-FIX: Fixed `git status` completion parsing for paths with spaces and renames, and completion of nested untracked files for `git add`. <!-- factory-client: {"source":"factory-client","slack_channel":"C0BCE7AELJ2","slack_thread_ts":"1783976762.109799","slack_permalink":"https://warpdev.slack.com/archives/C0BCE7AELJ2/p1783976762109799?thread_ts=1783976762.109799&cid=C0BCE7AELJ2","oz_run_id":"019f5d4d-27c2-750d-823f-90a8b0a92b9c","repo":"warpdotdev/warp"} --> _Conversation: https://staging.warp.dev/conversation/b34001b4-4abb-4cb1-be9d-98f75676605e_ _Run: https://oz.staging.warp.dev/runs/019f5d4d-27c2-750d-823f-90a8b0a92b9c_ _This PR was generated with [Oz](https://warp.dev/oz)._ Co-authored-by: Oz <oz-agent@warp.dev>





Switch
files_for_staging_commandtogit ls-files -zso paths arrive NUL-separatedand unquoted; parse each record by taking everything after the 3-byte
XYprefixinstead of splitting on whitespace, preserving filenames with spaces. Rename (
R)and copy (
C) entries span two records — we keep the<to>path and skip the origin.get_changed_or_tracked_filesshared that post-processor but still emittednewline-separated
git status … | sedoutput, which the NUL parser collapsed into onegarbage suggestion. It now uses
git diff [--cached] --diff-filter=AM --name-only -z(git does the filtering — portable, unlike GNU-only
sed -z), with its own NULpath-splitting post-processor.
Tests added:
test_post_process_diff_name_only(NUL paths incl. one with spaces),_empty,_fatal_error. Existingfiles_for_stagingtests already cover renames,copies, spaces, and untracked dirs.
cargo fmt,cargo testboth green.