shell(mkdir): accept --verbose instead of the misspelling --vebose - #39221
shell(mkdir): accept --verbose instead of the misspelling --vebose#39221robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Reproduced with bun 1.4.0 on Linux and Windows: Fix and test are in this PR (#39221): |
There was a problem hiding this comment.
LGTM — the macOS EEXIST text is now branched on isMacOS, which resolves the prior finding.
What was reviewed:
parse_longchange is a one-string literal fix (--vebose→--verbose);Optsdoc comment already documents-v, --verbose.- Test file placement matches sibling per-builtin files in
test/js/bun/shell/commands/; array interpolation of${words}splits into separate argv words (verified againsthandle_template_valueinshell_body.rs), and the empty-flags case producesmkdir a/bwith the pre-createda/parent. - Path assertions use
join()so separators are platform-correct; the only per-OS string (EEXIST) now branches, andisMacOSis exported fromharness.
Extended reasoning...
Overview
Two files. src/runtime/shell/builtin/mkdir.rs changes one byte-string comparison in Opts::parse_long from b"--vebose" to b"--verbose" and deletes the comment that called the typo intentional. test/js/bun/shell/commands/mkdir.test.ts is a new 84-line test file with 15 parametrized cases covering -v/--verbose alone and with -p/--parents in every order, the silent (no-verbose) baseline, -p on an existing directory, EEXIST without -p, and rejection of the old --vebose spelling.
Prior review
The previous run flagged that the EEXIST assertion pinned File exists, which diverges on macOS where coreutils_error_map's DELTA table returns File or folder exists. Commit 110ffae added isMacOS from harness and branches the expected suffix — exactly the suggested fix, keeping full precision per platform rather than loosening to a regex. isMacOS is exported at test/harness.ts:20.
Security risks
None. The change tightens the accepted long-flag set for a shell builtin's option parser (accepts the documented spelling, stops accepting the undocumented typo). No new inputs reach any syscall; the verbose codepath was already reachable via -v.
Level of scrutiny
Low for the Rust change — a single literal correction with the doc comment three lines above already saying -v, --verbose. Medium for the test file, which is where the review effort went: verified array interpolation semantics (${words} with a string array spreads as space-separated argv per shell_body.rs:497-528; an empty array appends nothing so the "" case becomes mkdir a/b), that tempDir + describe.concurrent gives each case its own directory, that join() handles path separators, and that readdirSync assertions are single-entry/empty so ordering can't matter.
Other factors
Test placement follows the established test/js/bun/shell/commands/<builtin>.test.ts convention (13 siblings). $.nothrow() at module top is the same pattern used in 13 other files under test/js/bun/shell/. The --vebose rejection test asserts only the mkdir: illegal option -- prefix, which the PR description notes is deliberate (the trailing option text is being fixed separately) — that's a reasonable loosening with a stated reason. No bugs from the bug-hunting system this run.
There was a problem hiding this comment.
LGTM — the earlier macOS EEXIST-text concern is resolved (02fcc44 loosened it to a regex that also accommodates #39189's operand-vs-resolved-path change, with the rationale commented inline).
What was reviewed:
- The one-word
b"--vebose"→b"--verbose"change inparse_long;-vand--parentsarms are untouched. - Grepped the repo for other
--vebosereferences — none outside this PR, so no existing test relies on the old spelling. - Test file matches sibling
shell/commands/*.test.tsconventions ($.nothrow(),describe.concurrent,tempDir, array interpolation for flags); the two intentionally-loose stderr assertions each pin exit code, empty stdout, and a non-vacuous message shape.
Extended reasoning...
Overview
A one-word typo fix in src/runtime/shell/builtin/mkdir.rs: parse_long now matches b"--verbose" instead of the misspelled b"--vebose", and the misleading "intentional" comment is deleted. A new test/js/bun/shell/commands/mkdir.test.ts covers -v/--verbose alone and combined with -p/--parents in both orders, silent output without a verbose flag, -p on an existing dir, EEXIST without -p, and rejection of the old --vebose spelling.
Security risks
None. This only changes which byte string a flag parser matches; no new syscalls, no user-controlled data flowing anywhere new.
Level of scrutiny
Low. The Rust change is a two-line literal correction whose effect is fully described by the Opts doc comment directly above it and by GNU/BSD mkdir. The behavior it enables (self.verbose = true) is identical to the existing -v short flag, so all downstream verbose-output code is already exercised.
Other factors
- My previous review flagged that the EEXIST assertion pinned Linux-only text and would fail on macOS. The author fixed this in 110ffae (branched on
isMacOS), then in 02fcc44 relaxed it toexpect.stringMatching(/^mkdir: (?:.*[\\/])?dir: File (?:or folder )?exists\n$/)so it also survives #39189 (which switches the error to the operand as written) and the separate macOS EEXIST-wording fix. The comment above the assertion documents why; stdout and exitCode remain exact, which is the invariant this PR owns (verbose stays off on failure). The review thread is resolved. - Grepped the whole repo for
vebose— the only remaining hits are in this PR's own test (the rejection case), so dropping support for the misspelling breaks nothing. - The test file follows the same conventions as
ls.test.ts/rm.test.tsin the same directory: module-level$.nothrow(),describe.concurrentwith per-testtempDir,toEqualon a{stdout, stderr, exitCode}object. Array interpolation ($mkdir ${words} ${operand}``) spreads flags as separate args, andfilter(Boolean)handles the empty-flags case. - No CODEOWNERS entry covers
src/runtime/shell/.
Problem
mkdir --verbose direxits 1 withmkdir: illegal option -- verboseand creates nothing, whilemkdir -v dirandmkdir --parents dirwork.parse_longinsrc/runtime/shell/builtin/mkdir.rs:438matches the misspelling--veboseinstead of--verbose, so the real flag falls through to short-flag parsing of-verbose, which rejects the leading-.Optsdoc comment right above it says-v, --verbose. The comment calling the typo intentional was added when the file was ported and only reflects keeping the port faithful:--vebosewas never documented, and GNU and BSD mkdir reject it.Fix
parse_longmatches--verboseand setsverbose, the same thing-vdoes.--veboseis no longer accepted and is reported like any other unknown option.--verboseis the long form of-vboth in the builtin's ownOptsdocumentation and in the coreutilsmkdirthese builtins mirror. No other long flag undersrc/runtime/shell/builtin/is misspelled (checked every--literal there), so this is the only site.test/js/bun/shell/commands/mkdir.test.ts: 8 of its 15 cases fail on bun 1.4.0 (every--verbosecase plus the--veboserejection) and all 15 pass with this change, on Linux and on Windows.test/js/bun/shell/exec.test.ts, which pinsmkdir --helpas an illegal option, still passes.--verbosealone, combined with-p/--parentsin both orders, the silent output without a verbose flag,--verbose --parentson an existing directory (nothing created, nothing printed), the EEXIST failure (nothing printed, exit 1), and--vebosebeing rejected with nothing created.--vebosecase pins themkdir: illegal option --prefix (the option text echoed after it is being corrected separately), and the EEXIST cases accept the path either resolved or as written (shell: report the operand as written in cat, touch and mkdir errors #39189 switches mkdir's errors to the operand as written) and either wording of EEXIST (the macOS table inbun_core's coreutils error map saysFile or folder exists, which is being fixed separately). Empty stdout and the exit code, the part this PR is responsible for, stay exact.--mode/-mmessages) edits the sameimpl FlagParsera few lines away and, like shell: fail an empty operand with ENOENT instead of acting on the cwd #38002 and shell(mkdir, touch): report operands longer than the path buffers instead of aborting #38379, also createstest/js/bun/shell/commands/mkdir.test.ts. Themkdir.rshunks do not overlap; the test files share the same helper and wrapper, so whichever lands later keeps both blocks.Background
mkdir,touch,catandcpbuiltins parse their options throughparse_flagsinsrc/runtime/shell/interpreter.rs. For an argument starting with--it first offers the whole word to the builtin'sFlagParser::parse_long; if that returnsNone, the word is parsed again as a cluster of short flags (-verbosebecomes-,v,e, ...), and the first unknown letter produces theillegal optionerror. A missing long-flag arm therefore shows up as an "illegal option", not as a dedicated message.mkdirprints the absolute path of every directory it created, one per line (with-p, the intermediate directories too). That output code is shared by-vand--verbose; this change only affects which spellings turn it on.<builtin>: <path>: <message>, where the message comes from Bun's own errno-to-coreutils-text table (coreutils_error_mapinsrc/bun_core/result.rs), not from the host'sstrerror.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The mkdir builtin's long-flag parser matched the misspelled literal
--veboseinstead of--verbose, somkdir --verbosefell through to short-flag parsing of-verbose, failed on the first byte, and exited 1 without creating anything. The fix corrects the literal inparse_longto--verbose(dropping the misspelling and the comment that described it as intentional), so the flag now sets the same verbose state as-v, and a new test file covers-vand--verbosealone and combined with-p/--parents, along with rejection of the old spelling.