mv: add --exchange to atomically swap two paths#13215
Conversation
|
GNU testsuite comparison: |
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | mv_force_overwrite |
62.8 ms | 66 ms | -4.83% |
| ❌ | Simulation | mv_single_file |
62.6 ms | 65.4 ms | -4.26% |
| ⚡ | Simulation | du_max_depth_balanced_tree[(6, 4, 10)] |
26 ms | 25.2 ms | +3.37% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing sylvestre:mv-exchange (2c878bd) with main (c9ecfa0)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
There was a problem hiding this comment.
I would also test the scenario where source and target are the same file.
Should make test tests/mv/mv-exchange.sh pass
Matches GNU mv: 'mv --exchange a b c dir/' exchanges each source with the file of the same name inside dir/. Also add a same-file check (mirroring regular mv's behavior) and strengthen existing exchange tests with actual error-message assertions, per review feedback.
The test asserts a 'cannot move' stderr message that only occurs when --exchange actually attempts the renameat2 syscall; on other platforms it short-circuits to a 'not supported' error instead, matching the cfg gating already used by the other --exchange tests.
| let target_dir = paths.last().unwrap(); | ||
| if !target_dir.is_dir() { | ||
| return Err(MvError::TargetNotADirectory(target_dir.quote().to_string()).into()); | ||
| } | ||
| for source in &paths[..paths.len() - 1] { |
There was a problem hiding this comment.
I would use split_last as it allows a cleaner for loop:
| let target_dir = paths.last().unwrap(); | |
| if !target_dir.is_dir() { | |
| return Err(MvError::TargetNotADirectory(target_dir.quote().to_string()).into()); | |
| } | |
| for source in &paths[..paths.len() - 1] { | |
| let (target_dir, sources) = paths.split_last().unwrap(); | |
| if !target_dir.is_dir() { | |
| return Err(MvError::TargetNotADirectory(target_dir.quote().to_string()).into()); | |
| } | |
| for source in sources { |
| /// each of the other operands is exchanged with the file of the same name | ||
| /// inside that directory (matching GNU `mv --exchange a b c dir/`). | ||
| fn exchange_paths(paths: &[PathBuf], opts: &Options) -> UResult<()> { | ||
| match paths.len() { |
There was a problem hiding this comment.
Here it might be an option to match directly on paths.
| #[cfg(not(any(target_os = "linux", target_os = "android")))] | ||
| { | ||
| let _ = (from, to, opts); | ||
| Err(USimpleError::new( | ||
| 1, | ||
| translate!("mv-error-exchange-not-supported"), | ||
| )) | ||
| } |
There was a problem hiding this comment.
This snippet feels out of place: the user gets some of the other "--exchange"-related errors, but if he provides the correct params, he suddenly gets a "not supported" error. Maybe this error should be returned in uumain?
2 participants
Should make test tests/mv/mv-exchange.sh pass