Skip to content

fix(git): inject -- for bare-word-with-extension paths in git diff#1679

Closed
truffle-dev wants to merge 1 commit into
rtk-ai:developfrom
truffle-dev:fix/git-diff-bare-word-extension-1669
Closed

fix(git): inject -- for bare-word-with-extension paths in git diff#1679
truffle-dev wants to merge 1 commit into
rtk-ai:developfrom
truffle-dev:fix/git-diff-bare-word-extension-1669

Conversation

@truffle-dev
Copy link
Copy Markdown

Closes #1669.

Reporter ran:

rtk git diff -- "package.json" "playwright.config.ts" ".harness/e2e_pipeline.yaml" "e2e-tests/config/loadEnv.ts" "e2e-tests/config/test.env"

and got fatal: bad revision 'package.json' while raw git diff -- ... worked.

What was happening

clap's trailing_var_arg = true drops -- when it's the first positional argument (existing comment at src/cmds/git/git.rs:74-83). normalize_diff_args (added for issue #1215) re-inserts it, walking the args until it finds the first one that looks path-like. Three rules existed before this PR:

  1. Explicit prefix . or ~ → always a path.
  2. Contains / or \ → use path_exists.
  3. Bare word → never a path (added for issue rtk git diff produces empty output when branch name contains / #1431 to keep rtk git diff main resolving as a branch).

For the reporter's command, the first path-looking arg by rule 1 is .harness/e2e_pipeline.yaml at index 2. -- lands at index 2, leaving package.json (index 0) and playwright.config.ts (index 1) before the separator, where git treats them as revisions.

The fix

A new rule between the slash-path rule and the plain-bare-word rule: a bare word with a mid-string . (looks like a filename with extension) uses path_exists to distinguish real files from refs/tags.

if arg.find('.').is_some_and(|i| i > 0) {
    return path_exists(arg);
}

package.json and playwright.config.ts resolve as paths because the filesystem has them, so -- lands at index 0 instead of index 2. Plain bare words like main continue to be treated as refs even when a same-named file exists (issue #1431's deliberate behavior is preserved by the existing rule sitting after this one). A version-tag-shape arg like v1.2.3 that doesn't exist as a file remains a ref candidate.

Verification

Stash-bisect on a five-file repro that mirrors the reporter's exact command:

Before the patch:

$ rtk git diff -- "package.json" "playwright.config.ts" ".harness/e2e_pipeline.yaml" "e2e-tests/config/loadEnv.ts" "e2e-tests/config/test.env"
fatal: bad revision 'package.json'

After the patch:

$ rtk git diff -- "package.json" "playwright.config.ts" ".harness/e2e_pipeline.yaml" "e2e-tests/config/loadEnv.ts" "e2e-tests/config/test.env"
.harness/e2e_pipeline.yaml  | 2 +-
 e2e-tests/config/loadEnv.ts | 2 +-
 e2e-tests/config/test.env   | 2 +-
 package.json                | 2 +-
 playwright.config.ts        | 1 +
 5 files changed, 5 insertions(+), 4 deletions(-)

Tests

Four new unit tests next to the existing ones in mod tests:

  • bare_word_with_extension_treated_as_path — single package.json that exists.
  • bare_word_with_extension_no_injection_when_not_filev1.2.3 tag pattern that doesn't exist on disk stays a ref.
  • mixed_bare_extension_and_explicit_path — the reporter's exact five-file case.
  • ref_before_bare_word_extension["HEAD", "Cargo.toml"]["HEAD", "--", "Cargo.toml"].

cargo test --release --bin rtk normalize_diff_args runs all 14 (10 prior + 4 new) green.

Scope notes

  • run_log and run_show (src/cmds/git/git.rs:447, :240) do not call normalize_diff_args, so rtk git log -- file.json and rtk git show -- file.json likely have the same shape. Kept this PR narrow to the diff path the reporter hit; happy to extend the normalization to log/show in a follow-up if you prefer a sweep.
  • The wshm AI-triage comment on the issue diagnosed this as "arg-forwarding bug ... arg splitting logic" at 95% confidence. The actual cause is more specific — the splitting works, but the rule for which leading args qualify as paths missed the bare-word-with-extension category.

clap's `trailing_var_arg = true` drops `--` when it's the first
positional. `normalize_diff_args` re-inserts it, but the bare-word
rule (deliberately conservative since rtk-ai#1431) skipped over args
like `package.json` and `playwright.config.ts`. When mixed with an
explicit-path arg (`.harness/...`), `--` landed at the explicit
path's index, leaving the bare-word-with-extension args before it
to be misinterpreted as revisions, producing
`fatal: bad revision 'package.json'`.

This adds a rule between the existing slash-path rule and the
plain-bare-word rule: a bare word with a mid-string `.` (looks
like a filename with extension) uses `path_exists` to distinguish
real files (`package.json`) from refs/tags (`v1.2.3`). Plain bare
words like `main` continue to be treated as refs even when a
same-named file exists, preserving rtk-ai#1431's behavior.

Closes rtk-ai#1669
@KuSh KuSh self-assigned this May 4, 2026
@KuSh
Copy link
Copy Markdown
Collaborator

KuSh commented May 4, 2026

Hi, thanks for your PR, I'm willing to wait to see if clap allows us to keep escapes -- to allow transmitting it transparently. There seems to be traction outside our own needs.

@truffle-dev
Copy link
Copy Markdown
Author

Sounds good. Subscribed to clap-rs/clap#5055. Happy to keep this rebased as a fallback, or close whenever you prefer.

@KuSh
Copy link
Copy Markdown
Collaborator

KuSh commented May 10, 2026

@truffle-dev After thinking more about this and seeing that clap's issue has not seen any updates since the spring of 2024, I pushed a more generic solution than stacking more heuristic on already existing ones, in #1823. If you want to give it a try, it would help, thanks

@truffle-dev
Copy link
Copy Markdown
Author

Tried it. Built #1823, ran the args_utils tests (15/15 pass), and walked through the original cases:

  • rtk cargo clippy -p test_pkg -- -D warnings — pass-through clean.
  • rtk cargo test -- --nocapture — pass-through clean.
  • rtk git diff -- existing.rs-- survives.
  • rtk git diff existing.rs — no -- injected, which is the deliberate scope cut.

Closing this in favor of #1823. The preserve-what-the-user-typed shape is cleaner than the path-existence heuristic; users who want a bare-word-with-extension treated as a path can still spell -- themselves, same as upstream git.

@truffle-dev
Copy link
Copy Markdown
Author

Superseded by #1823.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rtk does not handle git diff correctly in some cases

2 participants