Repository navigation
Conversation
The cp builtin rejected `-f` as the first flag of a group, had no arm for `-r`, and took no long option. `-f` is now accepted and sets nothing: the copy already overwrites unless `-n` is given, and `-n` wins over `-f` in any order, as in GNU cp. `-r` means `-R` in any position. `--recursive`, `--verbose`, `--no-clobber` and `--force` run the arm of their letter. A flag that cp does not implement is still ignored after a leading `R`, `v` or `n`, as in the released versions. After `r` or `f` it is an error, except `p`, which is ignored after any flag. `-p` and `-a` name themselves in the unsupported-option error. Fixes #14595
|
Status of #47933 Reproduced on main b9f2ec3 (Linux x64, debug build, the builtin forced on with
With this PR both commands copy. Main b1007f1 has the same This PR replaces #35616, which can be closed. The check against #35616:
The diff is green. At 05fbdc8, |
…only test Two concurrent tests each took and released `unprivilegedSpawnOptions`. That helper changes the modes of the directories above the temporary directory, so one test could remove the access that the child of the other test needed. One test now runs the six commands under one call. A second test pins the time of a file after `cp -fp`: the `p` is ignored, and only the copy of Windows keeps the time.
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. WalkthroughThe Changescp Option Parsing and Copy Behavior
Suggested reviewers: Severity of issue fixed: Medium Merge Risk:
|
The previous commit ignored a `p` that followed another flag, so `cp -rp` and `cp -fp` exited 0 without the times that `-p` asks for. They now fail with `unsupported option -- -p`, as `cp -pr` and `cp -r -p` do. `cp -Rp` keeps working, because released versions accept it. The options test accepts both outputs of a verbose directory copy on macOS: no line when clonefile copies the directory, and two lines when that call fails.
There was a problem hiding this comment.
I re-reviewed after f3f8d01 and found no bugs; that push addresses both points from my earlier pass (-p after -r/-f now fails with unsupported option -- -p, and the macOS verbose directory row accepts either clonefile outcome). Because -f is accepted as a no-op rather than the POSIX unlink-and-recreate, and the "ignore later letters only after R/v/n" rule is a compatibility policy, a maintainer's call on those semantics is still worthwhile.
What was reviewed:
parse_longdelegating toparse_short(short, flag, 0): the four long names can only hit theR/v/n/farms, so thei > 0andIllegalOption(&smallflags[i..])arms are unreachable from that path.- The narrowed
_ if i > 0 && matches!(smallflags[0], b'R' | b'v' | b'n')rule: every cluster the base accepted still parses;r/f-led clusters admit onlyR r v n fand otherwise fail loudly, which the table pins. - New tests: child pipes drained with
Promise.all, temp dir and process released viausing/await using, read-only destinations restored infinally, andunprivilegedSpawnOptionsexists in test/harness.ts:829 with the Disposable shape the test uses.
Extended reasoning...
The diff touches only the FlagParser impl for Opts in src/runtime/shell/builtin/cp.rs (about 20 lines: long-name mapping, -r as -R, -f as a no-op, -a/-p error arms, and a narrowed ignore-after-first-letter rule) plus about 215 lines of new tests in test/js/bun/shell/commands/cp.test.ts. It touches no security-sensitive surface: no path handling, privilege, or network code changes, only option-letter dispatch. The latest commit resolves both optional findings from my previous review, no CODEOWNER covers these paths, and no other reviewer has requested changes. I deferred rather than approved because the change fixes user-facing shell compatibility policy (-f accepted without the POSIX force semantics, -Rp still silently ignoring p) that the PR itself says is a maintainer's choice.
`-Rv`, `-vR`, `-nrv` and `-frv` are the group orders in the tests of #35616. `-RL` and `-RP` complete the groups that stay accepted after a leading `R`.
Fixes #14595
Problem
cp -f a bprintscp: unsupported option, please open a GitHub issue -- -f.cp -r d eprintscp: illegal option -- r.impl FlagParser for Opts(src/runtime/shell/builtin/cp.rs:872) rejects a leadingfand has nor.Fix
-fis accepted and sets nothing.-rmeans-Rin any position. The title's long names map to their letters.-nis given, and-nwins over-f, as in GNU cp.test/js/bun/shell/commands/cp.test.ts(4 new tests fail on main). Alsotest/js/bun/shell.-rp(see Notes).Background
cpis a Bun Shell builtin, the onlycpon Windows. POSIX needsBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1.R,vorn, because released versions accept-Rp. Afterrorfsuch a letter is an error.-fthat removes a destination it cannot open. It belongs in the copyfs.cpshares.-r/--recursive/--verboseand fix clustered short flags #35616. Merge before or with shell: keep the last exit status on ShellExecEnv and return it from a bareexit#47887.Downsides
cp -f a bonto abthat cannot be opened still fails, where GNU cp replacesb.cp -rpstays an error, because-pis not implemented.-rand-finherit every result of-Rand plaincp.cp -r dist/. out/fillsout/dist/(shell(cp): copy the contents of a dir/. or dir/.. source into the target #38067).cp -vrT src dircopies todir/src.cpwith an option, 5 without. No new allocation, syscall or binary byte.Notes
Where this comes from
cp -r folder deston Windows. Its two repros are tests here.exit#47887 makes a bareexitreturn the last status. Its review found package.json scripts of the shapecp -f a b || exit && next. On Windows they fail atcptoday and continue without the file. With shell: keep the last exit status on ShellExecEnv and return it from a bareexit#47887 alone they stop with status 1. With this PR the copy works.-r/--recursive/--verboseand fix clustered short flags #35616 had the same goal. It is written against the parser that Hardening follow-up: fix regressions from #46021 and more crashes #46270 replaced: its arms reject-Rpand drop the-nthat is now real. This PR replaces it.-r/--recursive/--verboseand fix clustered short flags #35616 runs unchanged on a build of this PR. Linux: 13 pass, 58 skip. With its Windows-only block forced on: 71 pass. On main, 9 of the 13 fail.-R. The table has-r,-rf,-Rf,-fR,-rv,-vr,-Rv,-vR,-nrv,-frv,--recursive,--verboseand--force.-r/--recursive/--verboseand fix clustered short flags #35616 makes-Rp,-RH,-RL,-RPand-Rierrors. Here they exit 0, as on main and on 1.4.3.What changes
Linux x64, builtin forced on, debug builds of main b9f2ec3 and of this PR. Every row exits 1 on main.
cp -f a b,-ff,-fv,-v -funsupported option ... -- -fcp -fR d e,cp -R -f d e,cp -frv d ecp -fn a b,cp -n -f a b,cp -f -n a bb, exit 0cp -r d e,-rR,-rf,-rvillegal option -- r(and the rest of the group)cp -vr d e,cp -nr d e,cp -nrv d ed is a directory (not copied)cp --force a b,--verbose a b,--recursive d eillegal option -- -force(and so on)-f,-v,-Rcp --no-clobber a billegal option -- -no-clobberb, exit 0cp -p a b,-pR,-R -punsupported option ... -- -P-- -pcp -rp d e,cp -rfp d e,cp -fp a billegal option -- rp,-- rfp,unsupported option ... -- -funsupported option ... -- -pcp -a d e,-ar,-raillegal option -- aunsupported option ... -- -acp -fi a b,cp -rL d e-- -f,illegal option -- rLunsupported option ... -- -i,-- -Lcp -rT d e,cp -ru a billegal option -- rT,-- ruillegal option -- T,-- ucp -f,cp -f aunsupported option ... -- -fCommands that main accepts keep their result:
-Rf,-vf,-Rv,-vR,-Rp,-RH,-RL,-RP,-Ri,-Rz,-Ra,-vH,-nz,-nf,-Rnf,-Rfn,-vnf,-n -Rf,-Rf -n. The tests pin them.-nand-f-n -f,-f -n,-nfand-fnall keep the destination. BSD cp takes the last of the two. Bun'smvalso takes the last one, as GNU mv and BSD mv do.-nwins here. 1.4.4 already keeps the destination forcp -nf a b, because it ignores thef. The BSD rule would turn that command into an overwrite.What
-fdoes not do-fremove a destination that cannot be opened and create it again. The builtin does not.cp -f a bonto a mode 444bprintscp: Permission denied: <path>and exits 1.cp a bandcp -Rf a bdo the same on main. GNU cp 9.7 replacesb.cp -fof a read-only source works one time and fails on the next run. GNUcp -fworks each time.-f. There the destination is replaced.CopyFileWrefuses a read-only destination withACCESS_DENIED. That prints ascp: Operation not permitted.cp_open_dest_with_mkdirand in theCopyFileWarm ofcopy_single_file_sync(src/runtime/node/node_fs.rs).fs.cpshares both. No open PR has it: draft node:fs: replace an existing destination inside the native single-file cp #38226 givesfs.cpa replace policy and keeps the shell in place.-f copies onto a read-only file as plain cp doespins these results on each platform. uutils'test_cp_arg_forceexpects the replacement.Letters that
cpdoes not implementcp -RL,-Rp,-Rfand-Ri, and an error would break scripts that work.rorf. So the rule does not extend to them.cp -fi a bandcp -rT d estay errors. So does a file name such as-fonts.cssthatcp *.css distgets from the glob.pgets no exception.cp -rp,cp -rfpandcp -fpstay errors, and they now name-p, ascp -prandcp -r -pdo.cp -Rpstill works, because released versions accept it.pafter any letter, so that-rpworked as-Rpdoes. A review found that it exits 0 without the times that-pasks for. Such a choice is for a maintainer, so it is not in this PR."cp -rp" filename:package.jsongives 258 files. 195 spell itcp -rp, 22 spell itcp -Rp, 24 arescp -rp. A test in that commit showed what a copy with an ignoredpkeeps: Windows keeps the time of a file (CopyFileW), Linux and macOS do not.b'p' if i > 0 => {}, would accept-rp. After a release, the rule of Hardening follow-up: fix regressions from #46021 and more crashes #46270 keeps it at exit 0.-anow printsunsupported option, as-pdoes. The same search gives 2,436 files for"cp -a"and 2,008 for"cp -f".-ais not made an alias of-R, because it also asks for-p.R r v n f p a H i L P T t u x z(4,368 groups, with a file pair and with a directory pair):rorfand hold onlyR r v n f.rafter the first letter. It now copies the tree, as its-Rtwin does.R,vorn, a letter is still ignored, as on 1.4.4. Two effects stay:cp -vrT src dirnow copies todir/src. On main it exits 1, because therwas ignored too.-R,-vor-nis still read as options. With this PR, so is a file name that starts with-ror-fand holds onlyR r v n f. So is a file named--force,--recursive,--verboseor--no-clobber. Where the glob gives-ffirst,cp *f twith the files-fandkfcopies onlykfand exits 0. Main refuses that command, and GNU cp 9.7 does as this PR does../*prevents that. shell: accept--end-of-options delimiter in builtins #33995 adds--.Results that
-rand-finherit-Rand plaincpgive these results on main. The new spellings reach them. Run on Linux against GNU cp 9.7. Each row has an open PR. All of these PRs conflict with main now.cp -r dist/. out/,outexistsout/dist/index.htmlout/index.htmlcp -r dist/. ../index.htmlcp -r d o2,d/lflinks tofo2/lflinks to the absolute path ofd/fo2/lflinks tofcp -f link outoutis a link to the absolute targetoutis a regular filecp -f link out,outexistsoutkeeps its content, exit 0outis replacedcp -r d missing/parent/e,cp -f a missing/parent/bdir/.change of shell(cp): copy the contents of a dir/. or dir/.. source into the target #38067 or the exit status change of shell(cp): exit 1 when a held-back Windows error turns out to be real #37943. Each changes the result of a command that exits 0 on a release. shell(cp): copy the contents of a dir/. or dir/.. source into the target #38067 needs a re-cut for the currentrun_from_thread_pool_impl.Not in this PR
-a,-p,-H,-L,-Pand-istay unimplemented.c,l,s,Xandxfrom the usage line stayillegal option.--rec, and options after the operands. The builtin does not.rm --force(shell(rm): accept --force as the long form of -f #39224),mkdir --verbose(shell(mkdir): accept --verbose instead of the misspelling --vebose #39221),mv --force,ls --all.--(shell: accept--end-of-options delimiter in builtins #33995), a lone-(shell: treat a lone-as an operand in the builtin option parsers #39214),--name=value(shell: report --name=value spellings of unsupported builtin options as unsupported #39223), the text ofillegal optionerrors (shell: name the rejected flag in the builtins' illegal option errors #39230)..shfile in cp -r illegal option in bun shell on Windows #14595 has CRLF line ends. With LF the script works now. With CRLF the target becomesdest\r(Bun Shell on Windows fails to handle CRLF line endings in .sh files, causing command resolution failure #29669, PR Shell: handle CRLF, tab, comment, and backslash-newline line endings in the lexer #29670).Measurements
Release builds (LTO) of main b9f2ec3 and of this PR, linux-x64. Tools:
size,nm, gdb 16.3 (stepi,catch syscall). strace, perf, valgrind and bloaty are not installed here..text110,623,657 and.rodata26,772,460 bytes on both builds.Cp::startgoes from 2,164 to 1,813 bytes.parse_shortis now a function of its own (259 bytes), becauseparse_longalso calls it. No other symbol changes size.Cp::startuntil it returns. Secondcpof one script, 3 runs each, spread 0:cp a bcp -R d ecp -n a bcp -v a bcp -Rf d ecp -Rp d ecp -Rvn d ecp -f a bcp -r d ecp --force a bcp --recursive d ecp --verbose a bcp --no-clobber a bCp::startto exit, 3 runs each, identical:cp -r d e,cp --recursive d ecp -R d ecp --verbose a bcp -v a bcp --no-clobber a b,bexistscp -n a bcp -f a b,cp --force a bcp a bbTests
aarm: 3 rows failvin the leaders: 1 row failsfthat resets-n: 6 rows and the read-only test failtest/js/bun/shellandtest/cli/run/run-shell.test.ts: 1,055 pass, 2 fail. The same 2 fail on main on this machine:shell load > immediate exit, andbun exec > it works2when the whole directory runs in one process.clonefile. The table accepts no line (the clone worked) or both lines (it did not).no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/commands/cp.test.ts