Skip to content

Commit 5053ea0

Browse files
committed
Bugfix: Copying or moving an empty folder now creates it
- The per-file copy loop creates directories only as FILE parents, so an empty directory — or a branch holding nothing but empty directories — completed "successfully" while never arriving at the destination - Cross-FS move was worse: the empty dir never staged, Phase 3's rename never moved it, and Phase 4's source delete then destroyed it — silent data loss - New `create_scanned_dirs_at_destination` pass (in `copy.rs`, shared by both pipelines) lands the scanned dirs the loop missed: runs on the copy's Completed arm and after the move's staging loop, maps via the same relative-path rule as `FileInfo::dest_path`, honors folder→file Rename redirects, records created dirs for rollback, and mirrors the loop's outcome semantics (Cancelled keeps copied files, other errors roll back) - Data-safety: a dest path that already holds anything (dir = merge, file = type clash) is left untouched — an empty source dir never replaces user data - The volume (MTP/SMB) pipeline never had the hole (`copy_directory_streaming` creates each dir before walking children); same-FS move renames whole sources, also fine - TDD: four red tests first (`copy_tests.rs` ×3, `move_op_tests.rs` ×1), verified live through MCP (top-level empty dir + nested empty dir both land now)
1 parent 7ab4b75 commit 5053ea0

5 files changed

Lines changed: 279 additions & 0 deletions

File tree

apps/desktop/src-tauri/src/file_system/write_operations/transfer/CLAUDE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ Frontend counterpart: [`apps/desktop/src/lib/file-operations/transfer/CLAUDE.md`
4444

4545
**Cross-FS move source-delete preserves Skipped sources.** `move_with_staging`'s Phase 3 (staging → final rename) resolves conflicts; a Skip discards the staged copy so the file never lands at the destination. Phase 4 (`delete_sources_after_move`) must therefore NOT delete that source — the user clicked Skip to keep both copies, and deleting the only original would be silent data loss. Phase 3 records every Skipped original in a `skipped_source_paths: HashSet<PathBuf>` (whole top-level source for a single-file / type-mismatch Skip; per-child paths remapped from the staging prefix back to the source prefix for a directory merge). Phase 4 skips whole sources in the set, removes a clean source dir wholesale (`remove_dir_all`), and for a source dir that holds a Skipped descendant walks it via `delete_dir_preserving_skipped` (deletes non-skipped children, removes a dir only once empty), so the Skipped child's original survives inside a surviving source directory. The same-FS path (`move_with_rename`) is inherently correct: it renames originals directly, and a Skipped child just leaves the source dir non-empty. Pinned by `move_op_tests.rs::{cross_fs_move_skip_preserves_source_and_dest, cross_fs_move_dir_merge_skip_child_preserves_source_child}`.
4646

47+
**Empty directories land via the scanned-dirs pass (`copy.rs::create_scanned_dirs_at_destination`).** The per-file loop creates directories only as FILE parents, so an empty directory — or a branch holding nothing but empty directories — has no file to hang its creation on and used to complete "successfully" while never arriving (and on a cross-FS move, Phase 4 then deleted the source: the empty dir was destroyed without ever landing). The pass runs over `ScanResult.dirs` on the local copy's Completed arm and after the move's staging loop (destination = the staging dir, so empty dirs ride the normal Phase-3 rename + cleanup). Mapping mirrors `FileInfo::dest_path`; created dirs are recorded for rollback. Data-safety: a dest path that already holds anything (dir = merge, file = type clash) is left untouched — an empty source dir never replaces user data. Pinned by `copy_tests.rs::{copy_creates_empty_directory_at_destination, copy_creates_nested_empty_directories, copy_empty_directory_does_not_clobber_same_named_dest_file}` and `move_op_tests.rs::cross_fs_move_preserves_empty_directories`. The volume (MTP/SMB) pipeline doesn't share the hole — `copy_directory_streaming` creates each dir before walking its children.
48+
4749
**Move rollback (same-FS).** `MoveTransaction` in `move_op.rs` tracks `(source, dest)` pairs for each rename. On cancellation, renames are reversed in reverse order. Same-FS rename rollback is instant (just another rename), so it runs synchronously. Cross-FS move rollback is handled by `CopyTransaction` (deletes the staging directory).
4850

4951
**Intentional duplication: `merge_move_directory` vs `copy_single_item`.** Both implement recursive merge with conflict resolution, but differ in every detail: copy has progress tracking, symlink handling, byte counting, strategy selection, and `CopyTransaction` recording. Move uses simple `fs::rename`. A shared abstraction would be forced and fragile. Cross-references are in the doc comments of both functions.

apps/desktop/src-tauri/src/file_system/write_operations/transfer/copy.rs

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -392,6 +392,40 @@ pub(in crate::file_system::write_operations) fn copy_files_with_progress_inner(
392392
return Ok(());
393393
}
394394

395+
// Land the scanned directories the per-file loop didn't create:
396+
// empty dirs (and branches of only empty dirs) have no files, so
397+
// without this they'd silently never arrive at the destination.
398+
// Outcomes mirror the loop's arms: Cancelled keeps what's copied
399+
// (commit, so the Drop safety-net can't roll it back), any other
400+
// error rolls back like `PostLoopIntent::Failed`.
401+
if let Err(e) = create_scanned_dirs_at_destination(
402+
&scan_result.dirs,
403+
sources,
404+
destination,
405+
state,
406+
&mut transaction,
407+
&mut created_dirs,
408+
&dir_remap,
409+
) {
410+
if matches!(e, WriteOperationError::Cancelled { .. }) {
411+
transaction.commit();
412+
events.emit_cancelled(WriteCancelledEvent {
413+
operation_id: operation_id.to_string(),
414+
operation_type: WriteOperationType::Copy,
415+
files_processed: files_done,
416+
rolled_back: false,
417+
});
418+
} else {
419+
transaction.rollback();
420+
events.emit_error(WriteErrorEvent::new(
421+
operation_id.to_string(),
422+
WriteOperationType::Copy,
423+
e.clone(),
424+
));
425+
}
426+
return Err(e);
427+
}
428+
395429
// Flush every created destination to disk before reporting
396430
// complete, so "complete" means durable. Reuses the transaction's
397431
// own `created_files`; skips paths the strategy already flushed.
@@ -580,6 +614,84 @@ fn apply_dir_remap(dest: &Path, dir_remap: &HashMap<PathBuf, PathBuf>) -> PathBu
580614
}
581615
}
582616

617+
/// Creates destination directories for the scanned source dirs the per-file
618+
/// loop didn't materialize. The loop creates directories only as FILE parents,
619+
/// so an empty directory — or a branch holding nothing but empty directories —
620+
/// used to complete "successfully" while never arriving at the destination
621+
/// (and on a cross-FS move, Phase 4 then deleted the source: the empty dir was
622+
/// destroyed without ever landing). The scan already collected every source
623+
/// dir; this pass lands the missing ones.
624+
///
625+
/// Mirrors `FileInfo::dest_path`'s mapping (the path relative to its top-level
626+
/// source's parent, joined onto `destination`), honors active folder→file
627+
/// Rename redirects via `dir_remap`, and records created dirs in the
628+
/// transaction for rollback.
629+
///
630+
/// Data-safety: a dest path that already holds ANYTHING is left untouched — a
631+
/// same-named dir is a merge (nothing to create), and a same-named file is a
632+
/// type clash where silently replacing user data with an empty directory would
633+
/// be worse than skipping.
634+
pub(super) fn create_scanned_dirs_at_destination(
635+
scanned_dirs: &[PathBuf],
636+
sources: &[PathBuf],
637+
destination: &Path,
638+
state: &Arc<WriteOperationState>,
639+
transaction: &mut CopyTransaction,
640+
created_dirs: &mut HashSet<PathBuf>,
641+
dir_remap: &HashMap<PathBuf, PathBuf>,
642+
) -> Result<(), WriteOperationError> {
643+
// `scanned_dirs` is deepest-first (the delete order); reverse so parents
644+
// come before children.
645+
for dir in scanned_dirs.iter().rev() {
646+
if is_cancelled(&state.intent) {
647+
return Err(WriteOperationError::Cancelled {
648+
message: "Operation cancelled by user".to_string(),
649+
});
650+
}
651+
let Some(dest) = dir_dest_path(dir, sources, destination) else {
652+
continue;
653+
};
654+
let dest = apply_dir_remap(&dest, dir_remap);
655+
if created_dirs.contains(&dest) || path_exists_or_is_symlink(&dest) {
656+
continue;
657+
}
658+
// Collect the missing ancestors first so rollback records exactly what
659+
// this pass created (same pattern as the file loop's parent creation).
660+
let mut dirs_to_create: Vec<PathBuf> = Vec::new();
661+
let mut walk = dest.clone();
662+
while !walk.exists() && !created_dirs.contains(&walk) {
663+
dirs_to_create.push(walk.clone());
664+
match walk.parent() {
665+
Some(p) => walk = p.to_path_buf(),
666+
None => break,
667+
}
668+
}
669+
fs::create_dir_all(&dest).map_err(|e| WriteOperationError::IoError {
670+
path: dest.display().to_string(),
671+
message: format!("Failed to create directory: {}", e),
672+
})?;
673+
for created in dirs_to_create.into_iter().rev() {
674+
transaction.record_dir(created.clone());
675+
created_dirs.insert(created);
676+
}
677+
}
678+
Ok(())
679+
}
680+
681+
/// Maps a scanned source directory to its destination path, mirroring
682+
/// `FileInfo::dest_path`: the path relative to its top-level source's parent,
683+
/// joined onto `destination`. `None` when the dir isn't under any source
684+
/// (can't happen for paths produced by the scan walker over these sources).
685+
fn dir_dest_path(dir: &Path, sources: &[PathBuf], destination: &Path) -> Option<PathBuf> {
686+
sources.iter().find_map(|source| {
687+
if !dir.starts_with(source) {
688+
return None;
689+
}
690+
let root = source.parent().unwrap_or(source);
691+
dir.strip_prefix(root).ok().map(|relative| destination.join(relative))
692+
})
693+
}
694+
583695
/// Copies a single file or symlink to its destination.
584696
/// Ensures parent directories exist before copying.
585697
/// Used by both copy and cross-filesystem move operations.

apps/desktop/src-tauri/src/file_system/write_operations/transfer/copy_tests.rs

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,3 +116,103 @@ fn local_copy_emits_flushing_phase_before_complete() {
116116
let complete = events.complete.lock().unwrap();
117117
assert_eq!(complete.len(), 1, "exactly one write-complete");
118118
}
119+
120+
/// Copying an EMPTY directory must create it at the destination. The copy
121+
/// loop iterates `scan_result.files` only and creates directories lazily as
122+
/// file parents, so a source with zero files used to complete "successfully"
123+
/// while creating nothing — the empty dir silently never arrived.
124+
#[test]
125+
fn copy_creates_empty_directory_at_destination() {
126+
let tmp = tempfile::tempdir().expect("tempdir");
127+
let src_dir = tmp.path().join("src");
128+
let dst_dir = tmp.path().join("dst");
129+
fs::create_dir_all(src_dir.join("empty-dir")).unwrap();
130+
fs::create_dir_all(&dst_dir).unwrap();
131+
132+
let events = Arc::new(CollectorEventSink::new());
133+
let state = make_state(200);
134+
let config = WriteOperationConfig::default();
135+
136+
let source = src_dir.join("empty-dir");
137+
let result = copy_files_with_progress_inner(
138+
&*events,
139+
"op-copy-empty-dir",
140+
&state,
141+
std::slice::from_ref(&source),
142+
&dst_dir,
143+
&config,
144+
);
145+
assert!(result.is_ok(), "expected Ok, got {:?}", result);
146+
assert!(
147+
dst_dir.join("empty-dir").is_dir(),
148+
"the empty directory must exist at the destination"
149+
);
150+
}
151+
152+
/// Empty directories NESTED inside a populated source must land too: the file
153+
/// loop creates only ancestors of files, so `tree/sub-empty` (no files
154+
/// anywhere under it) needs the explicit scanned-dirs pass.
155+
#[test]
156+
fn copy_creates_nested_empty_directories() {
157+
let tmp = tempfile::tempdir().expect("tempdir");
158+
let src_dir = tmp.path().join("src");
159+
let dst_dir = tmp.path().join("dst");
160+
fs::create_dir_all(src_dir.join("tree/populated")).unwrap();
161+
fs::create_dir_all(src_dir.join("tree/sub-empty/deeper-empty")).unwrap();
162+
fs::write(src_dir.join("tree/populated/file.txt"), b"content").unwrap();
163+
fs::create_dir_all(&dst_dir).unwrap();
164+
165+
let events = Arc::new(CollectorEventSink::new());
166+
let state = make_state(200);
167+
let config = WriteOperationConfig::default();
168+
169+
let source = src_dir.join("tree");
170+
let result = copy_files_with_progress_inner(
171+
&*events,
172+
"op-copy-nested-empty-dirs",
173+
&state,
174+
std::slice::from_ref(&source),
175+
&dst_dir,
176+
&config,
177+
);
178+
assert!(result.is_ok(), "expected Ok, got {:?}", result);
179+
assert!(
180+
dst_dir.join("tree/populated/file.txt").is_file(),
181+
"the regular file must arrive"
182+
);
183+
assert!(
184+
dst_dir.join("tree/sub-empty/deeper-empty").is_dir(),
185+
"nested empty directories must exist at the destination"
186+
);
187+
}
188+
189+
/// An empty source dir whose destination already holds a same-named FILE must
190+
/// not destroy that file (folders merge; a type clash on an empty dir is left
191+
/// alone rather than silently replacing user data).
192+
#[test]
193+
fn copy_empty_directory_does_not_clobber_same_named_dest_file() {
194+
let tmp = tempfile::tempdir().expect("tempdir");
195+
let src_dir = tmp.path().join("src");
196+
let dst_dir = tmp.path().join("dst");
197+
fs::create_dir_all(src_dir.join("clash")).unwrap();
198+
fs::create_dir_all(&dst_dir).unwrap();
199+
fs::write(dst_dir.join("clash"), b"existing user data").unwrap();
200+
201+
let events = Arc::new(CollectorEventSink::new());
202+
let state = make_state(200);
203+
let config = WriteOperationConfig::default();
204+
205+
let source = src_dir.join("clash");
206+
let result = copy_files_with_progress_inner(
207+
&*events,
208+
"op-copy-empty-dir-clash",
209+
&state,
210+
std::slice::from_ref(&source),
211+
&dst_dir,
212+
&config,
213+
);
214+
assert!(result.is_ok(), "expected Ok, got {:?}", result);
215+
let dest = dst_dir.join("clash");
216+
assert!(dest.is_file(), "the existing dest file must survive");
217+
assert_eq!(fs::read(&dest).unwrap(), b"existing user data");
218+
}

apps/desktop/src-tauri/src/file_system/write_operations/transfer/move_op.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,29 @@ fn move_with_staging(
595595
return Err(e);
596596
}
597597

598+
// Stage the scanned directories the per-file loop didn't create: an empty
599+
// dir has no files, so it never staged, Phase 3's rename never moved it,
600+
// and Phase 4's source delete then DESTROYED it — gone from the source
601+
// without ever arriving at the destination. Staging it here lets it ride
602+
// the normal rename + cleanup machinery.
603+
if let Err(e) = super::copy::create_scanned_dirs_at_destination(
604+
&scan_result.dirs,
605+
sources,
606+
&staging_dir,
607+
state,
608+
&mut transaction,
609+
&mut created_dirs,
610+
&dir_remap,
611+
) {
612+
remove_dir_all_in_background(staging_dir.clone());
613+
events.emit_error(WriteErrorEvent::new(
614+
operation_id.to_string(),
615+
WriteOperationType::Move,
616+
e.clone(),
617+
));
618+
return Err(e);
619+
}
620+
598621
// Original source paths whose staged copy was discarded on Skip (the file
599622
// never reached the destination). Phase 4 consults this so it deletes ONLY
600623
// sources that actually landed — deleting a skipped source's original would

apps/desktop/src-tauri/src/file_system/write_operations/transfer/move_op_tests.rs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,3 +645,45 @@ fn matrix_dir_overwrite_older_skips_newer_child_preserves_source() {
645645
assert_dir_one_skipped_child(false, ConflictResolution::OverwriteOlder, false);
646646
assert_dir_one_skipped_child(true, ConflictResolution::OverwriteOlder, false);
647647
}
648+
649+
/// A cross-FS move of a tree containing an EMPTY directory must land that
650+
/// directory at the destination — and, critically, must not destroy it. The
651+
/// staging copy iterates `scan_result.files` only, so an empty dir never
652+
/// staged, never renamed into place, and then Phase 4 deleted the source:
653+
/// the directory vanished entirely. That's silent data loss, the worst case
654+
/// of the empty-dir hole (the copy sibling merely failed to create it).
655+
#[test]
656+
fn cross_fs_move_preserves_empty_directories() {
657+
let tmp = tempfile::tempdir().expect("tempdir");
658+
let src_dir = tmp.path().join("src");
659+
let dst_dir = tmp.path().join("dst");
660+
fs::create_dir_all(src_dir.join("tree/populated")).unwrap();
661+
fs::create_dir_all(src_dir.join("tree/sub-empty")).unwrap();
662+
fs::write(src_dir.join("tree/populated/file.txt"), b"content").unwrap();
663+
fs::create_dir_all(&dst_dir).unwrap();
664+
665+
let events = Arc::new(CollectorEventSink::new());
666+
let state = make_state(200);
667+
let config = WriteOperationConfig::default();
668+
669+
let source = src_dir.join("tree");
670+
let result = move_with_staging(
671+
&*events,
672+
"op-cross-fs-move-empty-dir",
673+
&state,
674+
std::slice::from_ref(&source),
675+
&dst_dir,
676+
&config,
677+
);
678+
assert!(result.is_ok(), "expected Ok, got {:?}", result);
679+
680+
assert!(
681+
dst_dir.join("tree/sub-empty").is_dir(),
682+
"the empty directory must arrive at the destination"
683+
);
684+
assert!(
685+
dst_dir.join("tree/populated/file.txt").is_file(),
686+
"the regular file must arrive at the destination"
687+
);
688+
assert!(!source.exists(), "the source tree should be removed after the move");
689+
}

0 commit comments

Comments
 (0)