Repository navigation
Implement rename - #632
Implement rename#632
rename#632Conversation
rename replaces a substring in filenames. The option surface follows rename(1): -v, -s, -n, -a, -l, -o and -i, with all three operands required. Behavior throughout is what util-linux 2.42.2 and 2.40.4 were both observed doing. The parts a reader should not have to reverse engineer: - The exit status is two counters rather than bit flags. 0 means something was renamed, 1 that everything failed, 2 a mix, and 4 that nothing matched. An operand that matched nothing, that -o skipped, or that was declined at an -i prompt counts as neither, so it can neither degrade a success nor promote a failure. - Only the final path component is rewritten, unless either operand holds a separator, which widens the scope to the whole path. Trailing separators are stripped from the operand, but the existence check still sees the operand exactly as it was typed. - An empty substring inserts at every code-unit boundary rather than matching nowhere, and a substring equal to its replacement exits 4 before any syscall runs. - The two overwrite guards ask different questions deliberately. The default path uses a check that follows symlinks, so a dangling destination is clobbered; -s lstats the new target name, so a dangling entry blocks it. - Filenames are carried as OsStr and the substitution engine is generic over the code unit: bytes on unix, UTF-16 units on Windows. A name that is not valid Unicode survives the report and the diagnostics without being replaced. Known divergences, each structural to clap and Rust rather than a defect in the port: the --help and --version layout, ANSI escapes on a terminal, the eager mutual-exclusion check, the rpmatch answer set, and SIGPIPE, which Rust ignores and the reference dies of.
Sixty-four tests: the exit-status tally, the three substitution modes, the path-scope rule, both overwrite guards, symlink mode, byte-oriented names, and the test_invalid_arg smoke test every other util carries. They defend the decisions a refactor would plausibly undo rather than restating what clap already guarantees. The two overwrite predicates and their opposite treatment of a dangling destination, the short circuit that runs before any syscall, and the order of stripping against the existence check each have a test whose only job is to fail if someone tidies them away. Diagnostics are asserted only as far as we write them. The errno text after our half belongs to libc, so the suite does not pin strerror strings on platforms it cannot run. The substitution engine keeps its unit tests beside it in subst.rs. `cargo test` from the workspace root does not run them, because the root package is the only default member; `cargo test -p uu_rename` does.
44bba04 to
d4627bd
Compare
|
Fixed the initial macOS CI failure.
So on macOS the call succeeded, The fix was trivial: gated that test to Linux, utility unchanged. All checks now pass. |
|
|
||
| const TERMINATOR: &str = "--"; | ||
|
|
||
| pub(crate) fn collect_getopt_argv(args: impl uucore::Args) -> Vec<OsString> { |
There was a problem hiding this comment.
you said dropping it costs nothing, so let's drop it :)
no other utility rewrites argv, i'd rather keep clap's behavior everywhere
There was a problem hiding this comment.
Understood. Dropped in 464d2f8. With POSIXLY_CORRECT set, a dash-leading argument after the first operand is now read as an option, like the other utilities.
| // buffered anywhere else. Rust's is line buffered everywhere, and the | ||
| // difference is not only a matter of syscall counts - under a write limit | ||
| // it stops our loop part way through a run C util-linux finishes. | ||
| let sink: Box<dyn Write> = if stdout.is_terminal() { |
There was a problem hiding this comment.
do we need the Box?
could we just always use BufWriter? we already flush before the prompt
There was a problem hiding this comment.
Yes, done in 9efef02: one BufWriter, no Box. I kept one flush per operand when stdout is a terminal, the same idea as the flush before the prompt. C's stdout is line buffered there, and without the flush the errors print ahead of the -v lines they followed:
$ rename -v s z s1 s9 s2 s8 s3
rename: s9: not accessible: No such file or directory
rename: s8: not accessible: No such file or directory
`s1' -> `z1'
`s2' -> `z2'
`s3' -> `z3'
Off a terminal nothing changes.
| } | ||
|
|
||
| #[test] | ||
| fn test_first_replaces_only_the_leading_occurrence() { |
There was a problem hiding this comment.
most of these unit tests duplicate the ones in tests/by-util/test_rename.rs
could you please trim them to what the integration tests can't reach?
There was a problem hiding this comment.
Trimmed to two in 1692735: an empty name, which only a link with an empty target can supply (macOS only), and an all-separator name, which would mean pointing rename at /. I also corrected the comment on the first, which wrongly called it unreachable from the CLI.
| } | ||
|
|
||
| #[test] | ||
| fn test_posixly_correct_is_read_for_presence_and_not_for_value() { |
There was a problem hiding this comment.
almost the same as the previous test, could be merged, no?
| /// unix::test_every_failing_operand_is_reported_once_and_in_order, because the | ||
| /// text it has to assert is the platform's strerror. | ||
| #[test] | ||
| fn test_a_failure_in_the_middle_does_not_stop_the_operands_after_it() { |
There was a problem hiding this comment.
this is a subset of unix::test_every_failing_operand_is_reported_once_and_in_order, please keep only one
No other utility in the tree adjusts argv before clap parses it, and keeping rename on clap's behavior like the rest is worth more than matching getopt in this one corner. Without POSIXLY_CORRECT the rewrite handed argv back untouched, so only runs with the variable set change. There, an argument after the first operand that begins with `-` is now read as an option, as it already is without the variable, where C util-linux reads it as a filename or as the replacement. `--` still ends the options in both.
The ungated test ran the same argv as unix::test_every_failing_operand_is_reported_once_and_in_order and asserted a subset of what it does. It was kept for the Windows leg, but the one thing it added there, that an operand after a failure is still renamed, is already pinned on every platform by the tally table: its `s9 s1` row can only exit 2 if the rename after the failure happened.
Two remain, for inputs the integration suite cannot supply portably: an empty name, which only a link with an empty target provides and only macOS lets exist, and a name made only of separators, which means pointing rename at `/`. The comment on the first said the CLI could not reach it at all; it says when it can.
The report stream was a boxed writer so that it could be a BufWriter off a terminal and the bare, line buffered stdout on one. A single BufWriter does for both once the loop flushes it after each operand on a terminal, which needs no trait object. The flush is what keeps a terminal reading the way C util-linux's line buffered stdout does, each -v line in order with the diagnostics around it. Without it every diagnostic would print ahead of the lines that came before it, and the report would only appear at exit. Nothing in the test suite drives a terminal, so that order is not pinned by a test. I compared it by hand under script(1) against util-linux 2.42.2: a run mixing renames with failures, the -i prompt, the -n -o skip report and -i at end of input are all identical. Off a terminal nothing changes.
|
Each comment and the commit that addresses it:
One note, I updated the PR description: the |
|
Thanks for your PR |
2 participants
Closes #629.
renamereplaces a substring in filenames. This is the whole of rename(1):-v,-s,-n,-a,-l,-o,-i, the three required operands, and the documented exit statuses. No option is half implemented.I built this against util-linux 2.42.2 and cross checked it against 2.40.4, comparing stdout, stderr, exit status and the resulting directory tree over roughly a thousand invocations.
Decisions I made inside the port
POSIXLY_CORRECTis not honored. With it set, C util-linux stops reading options at the first operand; here clap keeps permuting as it does for every other utility in this project, so a dash-leading argument after the first operand is read as an option.Filenames are
OsStrend to end, and the substitution engine is generic over the code unit:u8on unix,u16on Windows. That is whatencoding.rsis for. A name that is not valid Unicode is exactly the kind of name people reach for rename to fix, so it has to survive the match, the rename, the-vline and the error message; carrying it through aStringwould replace it with U+FFFD.Diagnostics are written as bytes rather than through
show_error!, for the same reason.Displaywrites&str, so a message that carries a filename losslessly and aDisplayimpl are mutually exclusive.RenameErrortherefore has noDisplayand writes its ownrename:prefix.stdout is block buffered when it is not a terminal. C util-linux's stdout is line buffered on a tty and block buffered anywhere else, and Rust's is line buffered everywhere. That is not only a syscall count: under a write limit, line buffering stops our loop part way through a run C util-linux completes.
-iaccepts the C locale answer set. C util-linux takes the answers it accepts from the locale'sYESEXPR; we accept an answer whose first character isyorY. It is two sided: under a locale whoseYESEXPRis^[qQ]C util-linux acceptsqand rejectsy. UnderLC_ALL=Cthe two are identical. Matching it means declaring an rpmatch extern ourselves and callingsetlocale, which is process global state inside a utility that is supposed to stay embeddable.Exit status 64
rename(1) documents 64 as "unanticipated error occurred" and names nothing that produces it. This port cannot return it: exit status is set in four places and the reachable values are 0, 1, 2 and 4. I also never saw C util-linux return it, including from a deliberate hunt (memory exhaustion, a fifo, a dangling link,
/proc/self/mem), so there was no case to map onto it. I am not claiming C util-linux cannot produce a 64, only that I could not find what does. If someone knows, I will wire it up.What the tests do not cover
60 integration tests, plus 2 unit tests on the substitution engine. Three deficiencies worth naming:
cargo testfrom the workspace root, because the root package is the only default member.cargo test -p uu_renameruns them. This is not specific to rename;lscpuandlsmemhave the same blind spot.#[cfg(unix)], because uutests' symlink helpersunwrapand Windows needsSeCreateSymbolicLinkPrivilegeor Developer Mode, so on a runner without it the fixture would panic rather than skip.-sbehavior and the Windows arm ofencoding::symlinkare compiled but have never been executed anywhere.-iprompt decides how to read stdin from atcgetattrprobe at startup. Nothing exercises that probe, because it needs a pty and there is no test in this project that drives one.