Add a move binary (mv replacement) reusing the copy engine#11
Merged
Conversation
`move` is a second binary in the crate that renames in place when possible and
falls back to a copy + source removal only when the rename can't be atomic.
Engine (src/core/move_op.rs):
- Try `std::fs::rename` first (atomic, preserves everything).
- Fall back to the public `copy()` + source removal on `EXDEV` (cross-device),
or when `--exclude` means part of a directory must stay behind.
- `copy()` always nests a directory under its destination, so the directory
fallback stages the copy in a temp dir on the destination filesystem and
cheap-renames it into the final name. The source is never deleted unless the
copy succeeded; an interrupt leaves the source in place.
- Exclude-aware deletion removes only the entries that reached the target and
prunes emptied directories, leaving excluded files in the source.
CLI (src/cli/move_args.rs, src/bin/move.rs):
- Rich mv surface: -t, -i, -f, -n/--no-clobber, -u/--update, -b/--backup,
-v/--verbose, plus -j, --reflink, -e/--exclude, -p/--preserve on the
cross-device fallback. Optional-value flags use `require_equals` so they
don't swallow a following positional (an issue the copy CLI still has).
- Reuses the shared copyconfig.toml for general defaults; thin main mirrors
src/main.rs (parse -> validate -> signal abort -> dispatch).
Also fixes a pre-existing bug in utility/backup.rs: `find_max_backup_number`
treated a bare relative filename's empty parent as a directory and failed
`read_dir("")`; it now falls back to the cwd (affected `copy -b` too).
Tests: 6 unit + 13 integration (tests/move_integration.rs) covering rename,
into-dir, multi-source, -t, -i (accept/decline), -f, -n, -b, -v, exclude, and
the copy fallback path; plus tests/gnu/move-*.sh.
Packaging (ships both binaries): release.yml builds/uploads copy-* and move-*
per target; install.sh installs both; AUR PKGBUILD/.SRCINFO install both and
declare `provides=move`; guix.scm installs both; nix buildRustPackage installs
all bins automatically. READMEs and AGENTS.md/CLAUDE.md document `move`.
Verified: cargo build (copy + move), fmt, clippy --all-targets -D warnings,
cargo test (75 unit + 68 + 13 integration), reuse lint (60/60), nix build
(result/bin/{copy,move}), and the GNU move-*.sh scripts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Changesmove binary feature
Sequence Diagram(s)sequenceDiagram
participant User
participant main as src/bin/move.rs
participant MoveArgs as MoveArgs::validate
participant SignalThread
participant move_op as src/core/move_op.rs
User->>main: invoke move <sources> <dest>
main->>MoveArgs: parse() + validate()
MoveArgs-->>main: (sources, dest, MoveOptions)
main->>SignalThread: spawn(SIGINT/SIGTERM → AtomicBool abort)
alt single source
main->>move_op: move_path(source, dest, options)
else multiple sources
main->>move_op: move_multiple(sources, dest, options)
end
move_op->>move_op: check overwrite policy (no-clobber/update/interactive)
move_op->>move_op: try std::fs::rename
alt rename succeeds
move_op-->>main: Ok(())
else EXDEV or exclude-dir
move_op->>move_op: move_via_copy → stage_and_place_dir or copy file
move_op->>move_op: check abort flag
move_op->>move_op: remove source / exclude-aware prune
move_op-->>main: Ok(()) or Err
end
main-->>User: exit 0 / 1 / 130
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 37-46: The platform names published for aarch64 and armv7 targets
in the release workflow do not match what the installer is attempting to
download, causing 404 errors. Update the platform names for the
aarch64-unknown-linux-gnu and armv7-unknown-linux-gnueabihf targets to either
include the -musl suffix (linux-aarch64-musl and linux-armv7-musl) to match
installer expectations, or alternatively update the installer download URLs to
expect the current names (linux-aarch64 and linux-armv7). Choose one approach
and ensure consistency between both the workflow artifact publishing and the
installer download logic.
- Line 82: The `softprops/action-gh-release` action at line 82 (and also at line
24) is pinned to the mutable tag `@v3` instead of a specific commit SHA. Replace
both instances of `uses: softprops/action-gh-release@v3` with `uses:
softprops/action-gh-release@<specific-commit-sha>` where the commit SHA is the
full 40-character hash of a stable release commit. This prevents supply-chain
risks from upstream tag retargeting or compromise.
In `@src/core/move_op.rs`:
- Around line 28-36: The current check in the resolve_target and subsequent
error handling only catches cases where target equals source exactly, but fails
to prevent the case where the destination is a nested subdirectory within the
source tree. Before the copy operation fallback (between the current target
equality check and the actual copy logic that spans lines 61-84), add a
validation guard that checks if the target path is nested inside the source
directory tree and returns an appropriate error if it is. This prevents the
scenario where a staging directory created during copy can end up recursively
copying itself when the destination is a subdirectory of the source.
- Around line 124-138: The current implementation removes the target before the
copy operation completes, which leaves the original target destroyed if copy
fails or is interrupted. Remove the premature target deletion at line 127 (the
remove_path(target) call), and instead apply a staging pattern for both files
and directories: stage the copy to a temporary location first, and only after
the copy succeeds, remove the existing target and move the staged result into
the final target location. The directory path already uses stage_and_place_dir
for this pattern, so apply the same approach to the file copy branch in the else
block.
- Around line 170-173: The cleanup operation `std::fs::remove_dir_all(&staging)`
is currently ignoring its result with `let _`, which can mask failures in
staging directory cleanup. Instead of discarding the error, capture the result
from remove_dir_all and check if it failed. If the cleanup fails, propagate that
error rather than silently returning the original result from place_via_staging.
This ensures filesystem errors in the staging cleanup path are properly reported
instead of being hidden.
- Around line 254-258: The code unconditionally removes empty source directories
even when the corresponding target directory never existed, which deletes
excluded directories. In the directory handling block where file_type.is_dir()
is true, add a check to verify that tgt_child actually exists before attempting
to remove the empty src_child directory. This ensures only directories that
actually reached the target are pruned, preserving excluded empty source
directories.
- Around line 51-56: The current code uses `if let Some(mode) = options.backup
&& mode != BackupMode::None` syntax which requires Rust 1.88+ but the project
supports Rust 1.85 as the MSRV. Convert this into nested if statements for
compatibility: first check if options.backup is Some(mode) using if let, and
then inside that block add a separate if statement to check if mode is not equal
to BackupMode::None. Apply this same fix to both occurrences mentioned (around
lines 51-56 and 200-204) where the generate_backup_path and create_backup
functions are called, ensuring the backup creation logic remains intact within
the properly nested conditional blocks.
In `@tests/move_integration.rs`:
- Around line 45-64: In the moves_multiple_sources_into_a_directory test
function, add a missing assertion to verify that the second source file was also
removed after the move operation. After the existing assertion that checks
one.assert(predicate::path::missing()), add a similar assertion for the two
variable using the same predicate::path::missing() pattern to ensure both source
files were properly removed from their original locations.
- Around line 67-86: The test `target_directory_flag_moves_sources_into_dir`
verifies that files are moved to the destination directory but does not verify
that the source files were removed after the move operation. After the
`.assert().success()` call confirms the command succeeded, add assertions to
verify that the original source files `one` and `two` no longer exist in the
temp directory, ensuring the move operation fully completes by removing sources
from their original locations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6689c3f7-40b0-4aa0-b033-2df64f8997f5
📒 Files selected for processing (20)
.github/workflows/release.ymlAGENTS.mdCLAUDE.mdCargo.tomlREADME.mdREADME_CRATES.mdguix.scminstall.shnix/package.nixpackaging/aur/.SRCINFOpackaging/aur/PKGBUILDsrc/bin/move.rssrc/cli/mod.rssrc/cli/move_args.rssrc/core/mod.rssrc/core/move_op.rssrc/utility/backup.rstests/gnu/move-i.shtests/gnu/move-into-dir.shtests/move_integration.rs
Comment on lines
+37
to
+46
| platform: linux-x86_64 | ||
| - os: ubuntu-latest | ||
| target: x86_64-unknown-linux-musl | ||
| artifact_name: copy | ||
| asset_name: copy-linux-x86_64-musl | ||
| platform: linux-x86_64-musl | ||
| - os: ubuntu-latest | ||
| target: aarch64-unknown-linux-gnu | ||
| artifact_name: copy | ||
| asset_name: copy-linux-aarch64 | ||
| platform: linux-aarch64 | ||
| - os: ubuntu-latest | ||
| target: armv7-unknown-linux-gnueabihf | ||
| artifact_name: copy | ||
| asset_name: copy-linux-armv7 | ||
| platform: linux-armv7 |
There was a problem hiding this comment.
Artifact platform names no longer match installer expectations.
Line 37-46 publishes linux-aarch64 / linux-armv7, but installer downloads *-aarch64-musl / *-armv7-musl names. That causes release download 404s on those architectures.
Please align the naming contract between workflow artifacts and installer URLs (either side can be the source of truth).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml around lines 37 - 46, The platform names
published for aarch64 and armv7 targets in the release workflow do not match
what the installer is attempting to download, causing 404 errors. Update the
platform names for the aarch64-unknown-linux-gnu and
armv7-unknown-linux-gnueabihf targets to either include the -musl suffix
(linux-aarch64-musl and linux-armv7-musl) to match installer expectations, or
alternatively update the installer download URLs to expect the current names
(linux-aarch64 and linux-armv7). Choose one approach and ensure consistency
between both the workflow artifact publishing and the installer download logic.
|
|
||
| - name: Upload Release Asset | ||
| - name: Upload Release Assets | ||
| uses: softprops/action-gh-release@v3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only check: list workflow actions that are referenced by tag instead of SHA.
rg -n '^\s*uses:\s*[^@]+@v?[0-9][^ ]*$' .github/workflowsRepository: UnbreakableMJ/copy
Length of output: 470
Pin softprops/action-gh-release to a commit SHA instead of the mutable tag.
Lines 24 and 82 use @v3, which is a mutable tag. Pinning to a specific commit SHA prevents supply-chain drift if the upstream tag is retargeted or compromised.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 82-82: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 82-82: action functionality is already included by the runner (superfluous-actions): use gh release in a script step
(superfluous-actions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/release.yml at line 82, The `softprops/action-gh-release`
action at line 82 (and also at line 24) is pinned to the mutable tag `@v3`
instead of a specific commit SHA. Replace both instances of `uses:
softprops/action-gh-release@v3` with `uses:
softprops/action-gh-release@<specific-commit-sha>` where the commit SHA is the
full 40-character hash of a stable release commit. This prevents supply-chain
risks from upstream tag retargeting or compromise.
Source: Linters/SAST tools
Comment on lines
+28
to
+36
| let target = resolve_target(source, destination); | ||
|
|
||
| if target == source { | ||
| return Err(CopyError::CopyFailed { | ||
| source: source.to_path_buf(), | ||
| destination: target, | ||
| reason: "'source' and 'destination' are the same path".to_string(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Reject directory targets inside the source before copy fallback.
target == source only catches identical spelling. With --exclude, Line 63 skips rename, so a destination under the source tree can create the staging directory inside the tree being copied and recursively copy itself.
Proposed guard
let target = resolve_target(source, destination);
+ let is_dir = source_metadata.is_dir();
if target == source {
return Err(CopyError::CopyFailed {
source: source.to_path_buf(),
destination: target,
reason: "'source' and 'destination' are the same path".to_string(),
});
}
+
+ if is_dir && target_is_source_or_child(source, &target)? {
+ return Err(CopyError::CopyFailed {
+ source: source.to_path_buf(),
+ destination: target,
+ reason: "cannot move a directory into itself".to_string(),
+ });
+ }
// Overwrite policy is decided before any rename, since rename overwrites- let is_dir = source_metadata.is_dir();
-
// Excludes can't be honored by an atomic rename of a whole directory, so a+fn target_is_source_or_child(source: &Path, target: &Path) -> CopyResult<bool> {
+ let source = std::fs::canonicalize(source).map_err(CopyError::Io)?;
+ let target_anchor = if target_exists(target) {
+ target
+ } else {
+ target.parent().unwrap_or_else(|| Path::new("."))
+ };
+ let target_anchor = std::fs::canonicalize(target_anchor).map_err(CopyError::Io)?;
+ Ok(target_anchor == source || target_anchor.starts_with(&source))
+}
+
fn target_exists(path: &Path) -> bool {Also applies to: 61-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/move_op.rs` around lines 28 - 36, The current check in the
resolve_target and subsequent error handling only catches cases where target
equals source exactly, but fails to prevent the case where the destination is a
nested subdirectory within the source tree. Before the copy operation fallback
(between the current target equality check and the actual copy logic that spans
lines 61-84), add a validation guard that checks if the target path is nested
inside the source directory tree and returns an appropriate error if it is. This
prevents the scenario where a staging directory created during copy can end up
recursively copying itself when the destination is a subdirectory of the source.
Comment on lines
+51
to
+56
| if let Some(mode) = options.backup | ||
| && mode != BackupMode::None | ||
| { | ||
| let backup_path = generate_backup_path(&target, mode)?; | ||
| create_backup(&target, &backup_path)?; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the documented Rust floor and locate let-chain syntax.
rg -n 'Rust 1\.85|1\.85|rust-version|edition' README.md Cargo.toml
rg -n 'if let Some\(mode\) = options\.backup|&& let Some\(name\)' src/core/move_op.rsRepository: UnbreakableMJ/copy
Length of output: 250
🌐 Web query:
Rust 1.80.0 if-let chains stabilization release date
💡 Result:
If-let chains (also known as "let chains") were not stabilized in Rust 1.80.0 [1][2]. This feature was stabilized in Rust 1.88.0, but only for use in the 2024 Edition [3][4][5]. If you are working in a codebase using an older Rust edition, let chains remain unavailable without using a nightly compiler and the associated feature gate [3][6][7].
Citations:
- 1: https://github.com/rust-lang/rust/releases/tag/1.80.0
- 2: https://blog.rust-lang.org/2024/07/25/Rust-1.80.0/
- 3: Tracking issue for eRFC 2497, "if- and while-let-chains, take 2" rust-lang/rust#53667
- 4: Tracking issue for eRFC 2497, "if- and while-let-chains, take 2" rust-lang/rust#53667
- 5: https://releases.rs/docs/1.88.0/
- 6: https://doc.rust-lang.org/edition-guide/rust-2024/let-chains.html
- 7: Stabilize let chains in the 2024 edition rust-lang/rust#132833
Use nested if statements instead of if let chains to maintain MSRV compatibility.
The if let ... && ... syntax at lines 51–56 and 200–204 requires Rust 1.88 or later with the 2024 Edition, but the README documents a minimum Rust version of 1.85. Rewrite using nested if statements:
Proposed fixes
Line 51:
- if let Some(mode) = options.backup
- && mode != BackupMode::None
- {
- let backup_path = generate_backup_path(&target, mode)?;
- create_backup(&target, &backup_path)?;
+ if let Some(mode) = options.backup {
+ if mode != BackupMode::None {
+ let backup_path = generate_backup_path(&target, mode)?;
+ create_backup(&target, &backup_path)?;
+ }
}Line 200:
- if destination.is_dir()
- && let Some(name) = source.file_name()
- {
- return destination.join(name);
+ if destination.is_dir() {
+ if let Some(name) = source.file_name() {
+ return destination.join(name);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(mode) = options.backup | |
| && mode != BackupMode::None | |
| { | |
| let backup_path = generate_backup_path(&target, mode)?; | |
| create_backup(&target, &backup_path)?; | |
| } | |
| if let Some(mode) = options.backup { | |
| if mode != BackupMode::None { | |
| let backup_path = generate_backup_path(&target, mode)?; | |
| create_backup(&target, &backup_path)?; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/move_op.rs` around lines 51 - 56, The current code uses `if let
Some(mode) = options.backup && mode != BackupMode::None` syntax which requires
Rust 1.88+ but the project supports Rust 1.85 as the MSRV. Convert this into
nested if statements for compatibility: first check if options.backup is
Some(mode) using if let, and then inside that block add a separate if statement
to check if mode is not equal to BackupMode::None. Apply this same fix to both
occurrences mentioned (around lines 51-56 and 200-204) where the
generate_backup_path and create_backup functions are called, ensuring the backup
creation logic remains intact within the properly nested conditional blocks.
Source: Learnings
Comment on lines
+124
to
+138
| // The overwrite policy has already been applied, so clear any existing | ||
| // target first to reproduce the source exactly at `target`. | ||
| if target_exists(target) { | ||
| remove_path(target)?; | ||
| } | ||
|
|
||
| if is_dir { | ||
| // `copy` always nests a directory under its destination (dest/name), | ||
| // so it can't rename. Stage the copy on the destination filesystem, | ||
| // then cheap-rename the nested result into the final target. | ||
| stage_and_place_dir(source, target, options)?; | ||
| } else { | ||
| // A file copies directly to the target path. | ||
| copy(source, target, &options.to_copy_options())?; | ||
| } |
There was a problem hiding this comment.
Do not remove the existing target before fallback copy succeeds.
On EXDEV fallback, Line 127 deletes the current destination before copy() completes. A copy error or interrupt then leaves the source intact but destroys the existing target; stage the file/directory first, then swap into place after a successful copy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/move_op.rs` around lines 124 - 138, The current implementation
removes the target before the copy operation completes, which leaves the
original target destroyed if copy fails or is interrupted. Remove the premature
target deletion at line 127 (the remove_path(target) call), and instead apply a
staging pattern for both files and directories: stage the copy to a temporary
location first, and only after the copy succeeds, remove the existing target and
move the staged result into the final target location. The directory path
already uses stage_and_place_dir for this pattern, so apply the same approach to
the file copy branch in the else block.
Comment on lines
+170
to
+173
| let result = place_via_staging(source, target, &staging, options); | ||
|
|
||
| let _ = std::fs::remove_dir_all(&staging); | ||
| result |
There was a problem hiding this comment.
Propagate staging cleanup failures.
Ignoring remove_dir_all can report success while leaving partial copied data in .copy-move-staging-*. As per coding guidelines, src/core/**/*.rs must not ignore filesystem errors in production copy paths.
Proposed cleanup handling
let result = place_via_staging(source, target, &staging, options);
- let _ = std::fs::remove_dir_all(&staging);
- result
+ let cleanup = std::fs::remove_dir_all(&staging).map_err(CopyError::Io);
+ match (result, cleanup) {
+ (Ok(()), Ok(())) => Ok(()),
+ (Ok(()), Err(error)) => Err(error),
+ (Err(error), Ok(())) => Err(error),
+ (Err(error), Err(cleanup_error)) => Err(CopyError::CopyFailed {
+ source: staging,
+ destination: target.to_path_buf(),
+ reason: format!(
+ "{error}; additionally failed to clean staging directory: {cleanup_error}"
+ ),
+ }),
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let result = place_via_staging(source, target, &staging, options); | |
| let _ = std::fs::remove_dir_all(&staging); | |
| result | |
| let result = place_via_staging(source, target, &staging, options); | |
| let cleanup = std::fs::remove_dir_all(&staging).map_err(CopyError::Io); | |
| match (result, cleanup) { | |
| (Ok(()), Ok(())) => Ok(()), | |
| (Ok(()), Err(error)) => Err(error), | |
| (Err(error), Ok(())) => Err(error), | |
| (Err(error), Err(cleanup_error)) => Err(CopyError::CopyFailed { | |
| source: staging, | |
| destination: target.to_path_buf(), | |
| reason: format!( | |
| "{error}; additionally failed to clean staging directory: {cleanup_error}" | |
| ), | |
| }), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/move_op.rs` around lines 170 - 173, The cleanup operation
`std::fs::remove_dir_all(&staging)` is currently ignoring its result with `let
_`, which can mask failures in staging directory cleanup. Instead of discarding
the error, capture the result from remove_dir_all and check if it failed. If the
cleanup fails, propagate that error rather than silently returning the original
result from place_via_staging. This ensures filesystem errors in the staging
cleanup path are properly reported instead of being hidden.
Source: Coding guidelines
Comment on lines
+254
to
+258
| if file_type.is_dir() { | ||
| remove_copied_entries(&src_child, &tgt_child)?; | ||
| if is_empty_dir(&src_child)? { | ||
| std::fs::remove_dir(&src_child)?; | ||
| } |
There was a problem hiding this comment.
Preserve excluded empty directories.
This recurses into every source directory and removes it when empty even if tgt_child never existed, so an empty excluded directory is deleted from the source. Only prune directories that actually reached the target.
Proposed target-existence check
if file_type.is_dir() {
+ let target_metadata = match std::fs::symlink_metadata(&tgt_child) {
+ Ok(metadata) => metadata,
+ Err(error) if error.kind() == io::ErrorKind::NotFound => continue,
+ Err(error) => return Err(error),
+ };
+ if !target_metadata.is_dir() {
+ continue;
+ }
remove_copied_entries(&src_child, &tgt_child)?;
if is_empty_dir(&src_child)? {
std::fs::remove_dir(&src_child)?;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if file_type.is_dir() { | |
| remove_copied_entries(&src_child, &tgt_child)?; | |
| if is_empty_dir(&src_child)? { | |
| std::fs::remove_dir(&src_child)?; | |
| } | |
| if file_type.is_dir() { | |
| let target_metadata = match std::fs::symlink_metadata(&tgt_child) { | |
| Ok(metadata) => metadata, | |
| Err(error) if error.kind() == io::ErrorKind::NotFound => continue, | |
| Err(error) => return Err(error), | |
| }; | |
| if !target_metadata.is_dir() { | |
| continue; | |
| } | |
| remove_copied_entries(&src_child, &tgt_child)?; | |
| if is_empty_dir(&src_child)? { | |
| std::fs::remove_dir(&src_child)?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/move_op.rs` around lines 254 - 258, The code unconditionally removes
empty source directories even when the corresponding target directory never
existed, which deletes excluded directories. In the directory handling block
where file_type.is_dir() is true, add a check to verify that tgt_child actually
exists before attempting to remove the empty src_child directory. This ensures
only directories that actually reached the target are pruned, preserving
excluded empty source directories.
Comment on lines
+45
to
+64
| fn moves_multiple_sources_into_a_directory() { | ||
| let temp = assert_fs::TempDir::new().unwrap(); | ||
| let one = temp.child("one.txt"); | ||
| let two = temp.child("two.txt"); | ||
| let dest_dir = temp.child("dest"); | ||
| one.write_str("1").unwrap(); | ||
| two.write_str("2").unwrap(); | ||
| dest_dir.create_dir_all().unwrap(); | ||
|
|
||
| move_cmd() | ||
| .arg(one.path()) | ||
| .arg(two.path()) | ||
| .arg(dest_dir.path()) | ||
| .assert() | ||
| .success(); | ||
|
|
||
| dest_dir.child("one.txt").assert("1"); | ||
| dest_dir.child("two.txt").assert("2"); | ||
| one.assert(predicate::path::missing()); | ||
| } |
There was a problem hiding this comment.
Verify both sources were removed.
The test checks that dest_dir contains both files (lines 61-62) and that one was removed (line 63), but doesn't verify that two was also removed.
✅ Proposed fix to add the missing assertion
dest_dir.child("one.txt").assert("1");
dest_dir.child("two.txt").assert("2");
one.assert(predicate::path::missing());
+ two.assert(predicate::path::missing());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/move_integration.rs` around lines 45 - 64, In the
moves_multiple_sources_into_a_directory test function, add a missing assertion
to verify that the second source file was also removed after the move operation.
After the existing assertion that checks one.assert(predicate::path::missing()),
add a similar assertion for the two variable using the same
predicate::path::missing() pattern to ensure both source files were properly
removed from their original locations.
Comment on lines
+67
to
+86
| fn target_directory_flag_moves_sources_into_dir() { | ||
| let temp = assert_fs::TempDir::new().unwrap(); | ||
| let one = temp.child("one.txt"); | ||
| let two = temp.child("two.txt"); | ||
| let dest_dir = temp.child("dest"); | ||
| one.write_str("1").unwrap(); | ||
| two.write_str("2").unwrap(); | ||
| dest_dir.create_dir_all().unwrap(); | ||
|
|
||
| move_cmd() | ||
| .arg("-t") | ||
| .arg(dest_dir.path()) | ||
| .arg(one.path()) | ||
| .arg(two.path()) | ||
| .assert() | ||
| .success(); | ||
|
|
||
| dest_dir.child("one.txt").assert("1"); | ||
| dest_dir.child("two.txt").assert("2"); | ||
| } |
There was a problem hiding this comment.
Verify sources were removed after -t move.
The test confirms both files reached the destination (lines 84-85), but doesn't verify the sources were removed. For consistency with other move tests, add assertions that one and two are missing after the move.
✅ Proposed fix to add source removal checks
dest_dir.child("one.txt").assert("1");
dest_dir.child("two.txt").assert("2");
+ one.assert(predicate::path::missing());
+ two.assert(predicate::path::missing());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn target_directory_flag_moves_sources_into_dir() { | |
| let temp = assert_fs::TempDir::new().unwrap(); | |
| let one = temp.child("one.txt"); | |
| let two = temp.child("two.txt"); | |
| let dest_dir = temp.child("dest"); | |
| one.write_str("1").unwrap(); | |
| two.write_str("2").unwrap(); | |
| dest_dir.create_dir_all().unwrap(); | |
| move_cmd() | |
| .arg("-t") | |
| .arg(dest_dir.path()) | |
| .arg(one.path()) | |
| .arg(two.path()) | |
| .assert() | |
| .success(); | |
| dest_dir.child("one.txt").assert("1"); | |
| dest_dir.child("two.txt").assert("2"); | |
| } | |
| fn target_directory_flag_moves_sources_into_dir() { | |
| let temp = assert_fs::TempDir::new().unwrap(); | |
| let one = temp.child("one.txt"); | |
| let two = temp.child("two.txt"); | |
| let dest_dir = temp.child("dest"); | |
| one.write_str("1").unwrap(); | |
| two.write_str("2").unwrap(); | |
| dest_dir.create_dir_all().unwrap(); | |
| move_cmd() | |
| .arg("-t") | |
| .arg(dest_dir.path()) | |
| .arg(one.path()) | |
| .arg(two.path()) | |
| .assert() | |
| .success(); | |
| dest_dir.child("one.txt").assert("1"); | |
| dest_dir.child("two.txt").assert("2"); | |
| one.assert(predicate::path::missing()); | |
| two.assert(predicate::path::missing()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/move_integration.rs` around lines 67 - 86, The test
`target_directory_flag_moves_sources_into_dir` verifies that files are moved to
the destination directory but does not verify that the source files were removed
after the move operation. After the `.assert().success()` call confirms the
command succeeded, add assertions to verify that the original source files `one`
and `two` no longer exist in the temp directory, ensuring the move operation
fully completes by removing sources from their original locations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a second binary,
move— a modernmvreplacement that reuses the existing copy engine. It renames in place when possible and falls back to a copy + source removal only when the rename can't be atomic.Engine (
src/core/move_op.rs)std::fs::renamefirst (atomic; preserves everything).copy()+ source removal onEXDEV(cross-device) or when--excludemust leave part of a directory behind.copy()always nests a directory under its destination, so the directory fallback stages the copy in a temp dir on the destination filesystem and cheap-renames it into place. The source is deleted only after a successful copy; an interrupt leaves it intact.CLI (
src/cli/move_args.rs,src/bin/move.rs)-t -i -f -n -u -b -v, plus-j --reflink -e -pon the cross-device fallback.require_equalsso they don't swallow a following positional.copyconfig.toml; thinmainmirrorssrc/main.rs.Bonus fix
utility/backup.rs::find_max_backup_numberfailed on bare relative filenames (read_dir("")); now falls back to the cwd. This affectedcopy -btoo.Tests
6 unit + 13 integration (
tests/move_integration.rs): rename, into-dir, multi-source,-t,-iaccept/decline,-f,-n,-b,-v,-eexclude, copy-fallback path; plustests/gnu/move-*.sh.Packaging (ships both binaries)
release.ymlbuilds/uploadscopy-*andmove-*per target;install.shinstalls both; AUR PKGBUILD/.SRCINFO install both +provides=move;guix.scminstalls both; NixbuildRustPackageinstalls all bins automatically. READMEs + AGENTS.md/CLAUDE.md documentmove.Verification (all green)
cargo build(copy + move),fmt --check,clippy --all-targets -D warnings,cargo test(75 unit + 68 + 13 integration),reuse lint(60/60),nix build→result/bin/{copy,move}, and the GNUmove-*.shscripts.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
movecommand as a modern mv replacement that reuses the copy engine with atomic rename behavior and automatic cross-filesystem copy-and-remove fallback.copyandmovebinaries now ship and install together.Documentation
Tests