feat: block git checkout/switch with --force and --discard-changes flags#36
feat: block git checkout/switch with --force and --discard-changes flags#36
Conversation
Extend extractShortOpts with a shortOptsWithValue option so that bundled chars after a value-consuming flag are treated as the flag's argument rather than additional flags. Apply this to git switch -c/-C so that `git switch -cfeature` is correctly allowed instead of being misread as containing -f.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #36 +/- ##
=======================================
Coverage 99.53% 99.53%
=======================================
Files 50 50
Lines 5330 5365 +35
=======================================
+ Hits 5305 5340 +35
Misses 25 25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR extends the safety net with two new protections — blocking Key changes:
Minor concerns found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["analyzeGit(tokens)"] --> B{subcommand?}
B -->|checkout| C["analyzeGitCheckout(rest)"]
B -->|switch| D["analyzeGitSwitch(rest)"]
B -->|other| E["existing handlers …"]
C --> C1["splitAtDoubleDash → before / after"]
C1 --> C2{"hasCheckoutForceFlag(before)?"}
C2 -->|yes| C3["🚫 REASON_CHECKOUT_FORCE"]
C2 -->|no| C4{"-b / -B / --orphan in tokens?"}
C4 -->|yes| C5["✅ null (allowed)"]
C4 -->|no| C6["existing double-dash / ambiguous checks …"]
D --> D1["splitAtDoubleDash → before"]
D1 --> D2{"before includes\n'--discard-changes'?"}
D2 -->|yes| D3["🚫 REASON_SWITCH_DISCARD_CHANGES"]
D2 -->|no| D4["extractShortOpts(before,\n{shortOptsWithValue: SWITCH_SHORT_OPTS})"]
D4 --> D5{"before includes '--force'\nor shortOpts has '-f'?"}
D5 -->|yes| D6["🚫 REASON_SWITCH_FORCE"]
D5 -->|no| D7["✅ null (allowed)"]
subgraph extractShortOpts ["extractShortOpts (updated)"]
S1["for each token"] --> S2{"starts with '-'\nnot '--' ?"}
S2 -->|no| S1
S2 -->|yes| S3["loop over chars"]
S3 --> S4{"char in\nshortOptsWithValue?"}
S4 -->|yes| S5["add opt → break cluster"]
S4 -->|no| S6["add opt → continue"]
S6 --> S3
end
Last reviewed commit: "fix(git): stop short..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/core/rules-git.ts
Outdated
| const CHECKOUT_SHORT_OPTS_WITH_VALUE = new Set(['b', 'B', 'U']); | ||
| const CHECKOUT_SHORT_OPTS_WITH_OPTIONAL_VALUE = new Set(['t']); | ||
| const SWITCH_SHORT_OPTS_WITH_VALUE = new Set(['-c', '-C']); |
There was a problem hiding this comment.
CHECKOUT_SHORT_OPTS_WITH_VALUE and CHECKOUT_SHORT_OPTS_WITH_OPTIONAL_VALUE store bare single characters (e.g. 'b', 't'), while SWITCH_SHORT_OPTS_WITH_VALUE stores dash-prefixed strings (e.g. '-c', '-C'). Both happen to be correct because they are consumed by different APIs:
- The bare-char sets are compared against
char(a single letter) insidehasCheckoutForceFlag()'s inner loop. - The dash-prefixed set is compared against the
shortOptstring (-${char}) produced insideextractShortOpts().
A future maintainer adding a new option to either set could easily choose the wrong format and introduce a silent bug where the lookup never matches. Consider aligning the naming or adding an inline comment explaining the format difference, e.g.:
// Chars WITHOUT the leading '-': compared against individual chars inside hasCheckoutForceFlag()
const CHECKOUT_SHORT_OPTS_WITH_VALUE = new Set(['b', 'B', 'U']);
const CHECKOUT_SHORT_OPTS_WITH_OPTIONAL_VALUE = new Set(['t']);
// Strings WITH the leading '-': passed to extractShortOpts() which builds '-<char>' before lookup
const SWITCH_SHORT_OPTS_WITH_VALUE = new Set(['-c', '-C']);
src/core/rules-git.ts
Outdated
| } | ||
| if (CHECKOUT_SHORT_OPTS_WITH_OPTIONAL_VALUE.has(char) && i < chars.length - 1) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
git checkout -tf <branch>
The optional-value guard breaks out of the inner character loop whenever -t is not the last character in a cluster:
if (CHECKOUT_SHORT_OPTS_WITH_OPTIONAL_VALUE.has(char) && i < chars.length - 1) {
break;
}For the token -tf, t is at index 0 and chars.length - 1 is 1, so the condition is true and the loop breaks — f is never inspected.
However, in POSIX semantics, an optional argument must be attached directly to the option character in the same cluster. This means -t f (space-separated) gives -t with no optional value and then a positional f, whereas -tf would indeed consume f as -t's optional value. So git would not see a force flag in -tf, and the break is correct.
If git's internal parser ever diverges from strict POSIX for this option (it has done so for others), the break would silently miss git checkout -tf <branch> as a force invocation. Adding a comment here explaining the POSIX rationale would help future readers:
// -t takes an *optional* value; in POSIX, optional args must be attached
// in the same cluster, so chars that follow '-t' within the token are its
// value, not separate flags.
if (CHECKOUT_SHORT_OPTS_WITH_OPTIONAL_VALUE.has(char) && i < chars.length - 1) {
break;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/shell.ts (1)
793-796: Accept a readonly token array here.
extractShortOpts()never mutatestokens, but the mutable signature forces callers like Line 264 insrc/core/rules-git.tsto allocate[...before]just to satisfy typing.readonly string[]removes that copy and better matches the rest of the parser APIs.As per coding guidelines, "Use readonly arrays where mutation isn't needed in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/shell.ts` around lines 793 - 796, Change the extractShortOpts signature to accept a readonly token array (e.g., tokens: readonly string[] or ReadonlyArray<string>) since the function does not mutate tokens; update the function declaration export function extractShortOpts(tokens: readonly string[], options?: { readonly shortOptsWithValue?: ReadonlySet<string> }): Set<string> and ensure any callers that currently do a defensive copy (e.g., using [...before]) can pass their readonly arrays directly; verify there are no internal mutations of tokens inside extractShortOpts and adjust any related type annotations to use readonly arrays for consistency with other parser APIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/rules-git.ts`:
- Line 58: Remove the 't' entry from the CHECKOUT_SHORT_OPTS_WITH_OPTIONAL_VALUE
Set so short-option clusters like "-tf" are parsed to reach the 'f' force flag;
then run/update the hasCheckoutForceFlag logic to ensure it now detects combined
force flags in clusters, and add a regression test in the rules-git test suite
that asserts detection for a command like "git checkout -tf main".
---
Nitpick comments:
In `@src/core/shell.ts`:
- Around line 793-796: Change the extractShortOpts signature to accept a
readonly token array (e.g., tokens: readonly string[] or ReadonlyArray<string>)
since the function does not mutate tokens; update the function declaration
export function extractShortOpts(tokens: readonly string[], options?: { readonly
shortOptsWithValue?: ReadonlySet<string> }): Set<string> and ensure any callers
that currently do a defensive copy (e.g., using [...before]) can pass their
readonly arrays directly; verify there are no internal mutations of tokens
inside extractShortOpts and adjust any related type annotations to use readonly
arrays for consistency with other parser APIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c733981e-db83-4159-ac22-e13865266677
⛔ Files ignored due to path filters (3)
dist/bin/cc-safety-net.jsis excluded by!**/dist/**dist/core/shell.d.tsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (5)
src/core/rules-git.tssrc/core/shell.tstests/bin/explain/command.test.tstests/core/analyze/parsing-helpers.test.tstests/core/rules-git.test.ts
kenryu42
changed the title
…as force CHECKOUT_SHORT_OPTS_WITH_OPTIONAL_VALUE treated 't' as consuming the next character in combined short opts, causing -tf to not be detected as --force. Also accept readonly arrays in extractShortOpts to avoid unnecessary spreads.
…LUE and replace hasCheckoutForceFlag with extractShortOpts
1 participant
Summary
git checkout --force/-fto prevent discarding uncommitted changesgit switch --discard-changesandgit switch --force/-fextractShortOptsto stop at options that consume a value (e.g.,-cfeatureno longer mis-identifies trailing chars as flags)Changes
src/core/rules-git.ts— AddedanalyzeGitCheckoutforce-flag detection (--force,-f, combined-qf) with a dedicatedhasCheckoutForceFlag()helper that correctly handles short-opt clusters. AddedanalyzeGitSwitch()to detect--discard-changes,--force, and-f. Added newREASON_*constants andCHECKOUT_SHORT_OPTS_WITH_VALUE/SWITCH_SHORT_OPTS_WITH_VALUEsets for accurate option parsing.src/core/shell.ts— ExtendedextractShortOpts()to accept an optional{ shortOptsWithValue }parameter; when a short option that consumes a value is encountered, parsing stops so the remainder is not misinterpreted as flags.tests/core/rules-git.test.ts— Added 66 lines of new tests coveringgit checkout --force/-f/-qf,git switch --discard-changes/--force/-f/-qf/-cfeature, and edge cases (branch creation with force,--detach, double-dash,-C repoprefix).tests/core/analyze/parsing-helpers.test.ts— Added tests for the newshortOptsWithValuebehavior inextractShortOpts.tests/bin/explain/command.test.ts— Added explain-command tests forgit switch --discard-changesandgit switch -f.dist/— Rebuilt dist bundles reflecting the above changes.Testing
Related Issue
None
PR Checklist
bun run checkpasses (lint, types, dead code, rules, tests)package.jsonSummary by CodeRabbit
New Features
git switchwith--discard-changes,--forceor-f; expandedgit checkoutforce detection.Tests
git switchand short-option parsing behaviors.