From 7bd26390d75f026533cf4f101b64ba368ed42dbc Mon Sep 17 00:00:00 2001 From: webdev <86946125+biztex@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:06:39 +0700 Subject: [PATCH] fix(files): replace what a drop lands on only once the copy is whole (#490) (#512) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(files): replace what a drop lands on only once the copy is whole A drop onto a name already taken destroyed what was there before it knew it could put anything in its place: `copy_into_dir` removed the destination and only then started the copy. A copy that fails partway — a full disk, a control connection that drops mid-tree on a remote workspace — therefore left the destination holding neither the old thing nor a whole new one, and the panel reported the failure over a hole where the file used to be. "Replace" gave up the one guarantee it had: that saying no to the copy still leaves you what you had. The copy now lands on a working name beside the destination, what is there is moved aside rather than removed, and a rename — one metadata operation, not a tree walk — puts the new copy in its place; the old one goes only once the new one is there. A failure anywhere puts it back, and in the one case where even that fails it is still on disk under `.tty7-replaced-` instead of not at all. The other half of the same drop is a collision the destination cannot see. Two sources of one drop can carry the same name — `~/a/notes.md` and `~/b/notes.md` dragged in together — and neither is a conflict, because the destination has never heard of either. Both were planned onto the same path, so the second was written straight over the first while the panel reported both as copied: a file the drop said it had copied, gone, with nothing said. One name is one file. The first claim on it stands and the rest are refused with a reason. Three tests, two of which fail on the code they replace: a same-name pair reports one copy and one refusal rather than two copies of a single file, a replacement whose copy cannot finish leaves the original readable, and one that does finish leaves no working files behind. The drop is not in a release yet, so the changelog says this in the entry for the feature rather than in a Fixed line about a bug nobody met. Closes #490 * fix(files): stop the pass that carries the answer from asking again `conflicts` was filled on every pass, including the one that runs after the user has already answered "Replace". The panel prompts on any non-empty `conflicts` (`file_tree.rs:1281`), so the dialog re-opened over a replacement that had already happened — and since that branch is an `else if`, `report.errors` is never read while `conflicts` is set, so the same pass silently swallowed every error it produced: the new `FileDropNameTaken` refusal when a drop both collides with itself and lands on an existing name, and a replacement that failed — the case the staging exists to survive. The field already documented the invariant the code did not keep: "non-empty only when the caller asked without `overwrite`". Keep it, in the one place that fills it. The remaining silence is the narrow one: both renames are a control round trip each on a remote host, and a link that drops between them leaves what was there under the name it was moved aside to. That was a `log::warn!` and nothing else. The panel now names it, so the answer to "where did my file go" is on screen rather than in a log nobody opens. `free_name_beside` hands back the bare name for it — the sentence wants a name, not somebody else's whole remote path. The added test walks the real sequence: ask, answer, replace. It fails on the previous revision with `conflicts == ["note.txt"]` after the replace. --------- Co-authored-by: l0ng-ai --- CHANGELOG.md | 11 +- src/ui/file_copy.rs | 247 ++++++++++++++++++++++++++++++++++++++++++-- src/ui/i18n/en.rs | 5 + src/ui/i18n/ja.rs | 5 + src/ui/i18n/mod.rs | 3 + src/ui/i18n/zh.rs | 3 + 6 files changed, 262 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c92f52c3..5beb57ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,9 +16,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 belongs to the top of it. The landing is highlighted while the drag is in flight. Folders come in whole, the executable bit survives the copy, and a name already taken is asked about rather than silently replaced — a "no" - leaves every file in the drop untouched. It works over a remote workspace - too, reading here and writing there, up to the size one control frame can - carry; past that the panel says to use SFTP. + leaves every file in the drop untouched, and a "yes" copies beside what is + there and swaps the two only once the copy is whole, so a copy that fails + partway leaves the original where it was. Two dropped items of the same + name are one name and one file: the first keeps it and the second is + refused, rather than landing on top of it with both reported as copied. It + works over a remote workspace too, reading here and writing there, up to + the size one control frame can carry; past that the panel says to use + SFTP. - **Rearrange splits by dragging a pane** — hovering a pane now floats a small grip along its top edge; dragging it moves that pane elsewhere in the tab. diff --git a/src/ui/file_copy.rs b/src/ui/file_copy.rs index 5eba4b81..49bee6e7 100644 --- a/src/ui/file_copy.rs +++ b/src/ui/file_copy.rs @@ -31,6 +31,15 @@ const MAX_DEPTH: usize = 64; /// The megabyte of slack is for that JSON and the framing around it. const REMOTE_FILE_MAX: u64 = (crate::daemon::protocol::MAX_FRAME - 1024 * 1024) as u64; +/// How many working names beside a destination are tried before a replacement +/// gives up. +/// +/// The first choice is normally free. It is not free when a copy was killed +/// outright and left its half-written tree behind, and — the reason these are +/// probed rather than cleared out of the way — it is not free when the name +/// happens to belong to a file of somebody's own. +const WORKING_NAME_TRIES: usize = 16; + /// What one drop did, in the terms the panel has to answer in: rows to /// refresh, names to ask about, failures to report. #[derive(Default)] @@ -88,10 +97,27 @@ pub(crate) fn copy_into_dir( report.fail(&name, t(L10nKey::FileDropNotHere).to_string()); continue; } - if host.exists(&host.join(dir, &name)) { + let dest = host.join(dir, &name); + // Two sources of one drop can carry the same name: `~/a/notes.md` and + // `~/b/notes.md` dragged in together. Nothing in the destination + // objects to either of them, so neither is a conflict — they collide + // with each other, and the second would be written straight over the + // first with the panel reporting both as copied. One name is one file: + // the first claim on it stands, and the rest are refused out loud + // rather than allowed to eat it. + if planned.iter().any(|(_, taken, _)| *taken == dest) { + report.fail(&name, t(L10nKey::FileDropNameTaken).to_string()); + continue; + } + // Only ever on the pass that asks. `conflicts` is what re-opens the + // "replace?" dialog, so filling it on the pass that carries the answer + // asks the same question a second time — and because the panel reports + // conflicts *or* errors and never both, it also swallows every error + // that pass produced, including a replacement that failed. + if !overwrite && host.exists(&dest) { report.conflicts.push(name.clone()); } - planned.push((src.clone(), host.join(dir, &name), name)); + planned.push((src.clone(), dest, name)); } if !overwrite && !report.conflicts.is_empty() { @@ -102,13 +128,18 @@ pub(crate) fn copy_into_dir( // Replace rather than merge: a folder dropped onto a folder of the // same name should end up as what was dropped, not as the union of the // two, which is what writing into it one file at a time would leave. - if host.exists(&dest) - && let Err(e) = host.remove(&dest, true) - { - report.errors.push((name, e)); - continue; - } - match copy_tree(host, &src, &dest, 0) { + // + // What is already there is not cleared away to make room, though. A + // copy can fail halfway — a full disk, a control connection that drops + // mid-tree — and clearing the way first is what turns that into a + // destination holding neither the old thing nor a whole new one. The + // copy lands beside it instead, and only takes its place once it is + // whole. + let done = match host.exists(&dest) { + true => copy_over(host, &src, dir, &dest, &name), + false => copy_tree(host, &src, &dest, 0), + }; + match done { Ok(()) => report.copied.push(name), Err(e) => report.errors.push((name, e)), } @@ -116,6 +147,93 @@ pub(crate) fn copy_into_dir( report } +/// Copy `src` onto a `dest` that is already there, without `dest` ever being +/// the thing that is missing. +/// +/// Three steps, and what was there survives all of them: the copy lands on a +/// working name beside it, the old thing is moved aside rather than removed, +/// and a rename — one metadata operation, not a tree walk — puts the new copy +/// in its place. A failure anywhere puts the old thing back, and in the one +/// case where even that fails it is still on disk under the name it was moved +/// to — which the panel is told, so it is a name somebody can find rather than +/// one they have to go looking for. +fn copy_over(host: &dyn Host, src: &Path, dir: &Path, dest: &Path, name: &str) -> io::Result<()> { + let (staged, _) = free_name_beside(host, dir, "partial", name)?; + if let Err(e) = copy_tree(host, src, &staged, 0) { + let _ = host.remove(&staged, true); + return Err(e); + } + let (aside, aside_name) = match free_name_beside(host, dir, "replaced", name) { + Ok(aside) => aside, + Err(e) => { + let _ = host.remove(&staged, true); + return Err(e); + } + }; + if let Err(e) = host.rename(dest, &aside) { + let _ = host.remove(&staged, true); + return Err(e); + } + if let Err(e) = host.rename(&staged, dest) { + let _ = host.remove(&staged, true); + if let Err(back) = host.rename(&aside, dest) { + // Both renames are one control round trip each on a remote host, so + // a link that drops between them lands here. Nothing is lost, but + // it is under a name nobody chose — which is only better than lost + // if the panel says so rather than the log. + log::warn!( + "{} could not be put back after a failed replacement and is at {}: {back}", + dest.display(), + aside.display() + ); + return Err(io::Error::other(t_fmt( + L10nKey::FileDropLeftAside, + &[("name", &aside_name)], + ))); + } + return Err(e); + } + if let Err(e) = host.remove(&aside, true) { + // The copy is in place and the drop succeeded; all that is left is the + // old thing under a name nobody asked for. + log::warn!( + "{} outlived the copy that replaced it: {e}", + aside.display() + ); + } + Ok(()) +} + +/// A name in `dir` that nothing is using yet, for a copy to land on before it +/// takes the destination's place. Returned with the bare name as well as the +/// path, because the one message that has to name it is a sentence in the +/// panel, where a whole path — on a remote host, someone else's whole path — +/// is not what the sentence wants. +/// +/// The leading dot keeps the working copy out of the way of a tree that hides +/// dotfiles, and the tag says what it is to anyone who finds one that outlived +/// the copy that made it. +fn free_name_beside( + host: &dyn Host, + dir: &Path, + tag: &str, + name: &str, +) -> io::Result<(PathBuf, String)> { + for n in 0..WORKING_NAME_TRIES { + let candidate = match n { + 0 => format!(".tty7-{tag}-{name}"), + n => format!(".tty7-{tag}-{n}-{name}"), + }; + let path = host.join(dir, &candidate); + if !host.exists(&path) { + return Ok((path, candidate)); + } + } + Err(io::Error::other( + t(L10nKey::FileDropNoWorkingName).to_string(), + )) +} + fn copy_tree(host: &dyn Host, src: &Path, dest: &Path, depth: usize) -> io::Result<()> { if depth > MAX_DEPTH { return Err(io::Error::other(t_fmt( @@ -181,6 +299,18 @@ mod tests { std::fs::write(path, body).unwrap(); } + /// The working files a replacement makes, and is supposed to take away + /// again whichever way it ends. + fn leavings(dir: &Path) -> Vec { + let mut names: Vec = std::fs::read_dir(dir) + .unwrap() + .map(|e| e.unwrap().file_name().to_string_lossy().to_string()) + .filter(|name| name.starts_with(".tty7-")) + .collect(); + names.sort(); + names + } + #[test] fn a_dropped_file_lands_in_the_directory_it_was_dropped_on() { let root = scratch("one-file"); @@ -294,6 +424,105 @@ mod tests { ); } + #[test] + fn two_dropped_items_of_the_same_name_do_not_land_on_top_of_each_other() { + let root = scratch("same-name"); + let first = root.join("from/a/notes.md"); + let second = root.join("from/b/notes.md"); + write(&first, "first"); + write(&second, "second"); + let dest_dir = root.join("into"); + std::fs::create_dir_all(&dest_dir).unwrap(); + + let report = copy_into_dir(&*LocalHost::shared(), &[first, second], &dest_dir, false); + + assert_eq!(report.copied, vec!["notes.md".to_string()]); + assert_eq!( + report.errors.len(), + 1, + "the one that could not be written has to be said out loud" + ); + assert_eq!(report.errors[0].0, "notes.md"); + assert_eq!( + std::fs::read_to_string(dest_dir.join("notes.md")).unwrap(), + "first", + "the first claim on the name stands" + ); + } + + #[test] + fn the_pass_that_carries_the_answer_does_not_ask_again() { + let root = scratch("no-re-ask"); + let src = root.join("from/note.txt"); + write(&src, "new"); + let dest_dir = root.join("into"); + write(&dest_dir.join("note.txt"), "old"); + + let asked = copy_into_dir(&*LocalHost::shared(), &[src.clone()], &dest_dir, false); + assert_eq!(asked.conflicts, vec!["note.txt".to_string()]); + + let answered = copy_into_dir(&*LocalHost::shared(), &[src], &dest_dir, true); + + assert_eq!(answered.copied, vec!["note.txt".to_string()]); + assert!( + answered.conflicts.is_empty(), + "the question was already answered: asking it again re-opens the dialog, \ + and the panel reports conflicts instead of errors, so it also hides \ + whatever went wrong on this pass" + ); + } + + #[test] + fn a_finished_replacement_leaves_no_working_files_behind() { + let root = scratch("replace-clean"); + let src = root.join("from/note.txt"); + write(&src, "new"); + let dest_dir = root.join("into"); + write(&dest_dir.join("note.txt"), "old"); + + let report = copy_into_dir(&*LocalHost::shared(), &[src], &dest_dir, true); + + assert_eq!(report.copied, vec!["note.txt".to_string()]); + assert_eq!( + std::fs::read_to_string(dest_dir.join("note.txt")).unwrap(), + "new" + ); + assert!( + leavings(&dest_dir).is_empty(), + "the replacement left its working files behind: {:?}", + leavings(&dest_dir) + ); + } + + #[cfg(unix)] + #[test] + fn a_replacement_that_fails_partway_leaves_what_was_there() { + let root = scratch("replace-fails"); + // A walk that cannot finish: the copy gets deep enough to have written + // part of itself before it gives up, which is the shape of the full + // disk and the dropped connection this path exists for. + let src = root.join("from/pkg"); + write(&src.join("a.txt"), "a"); + std::os::unix::fs::symlink(&src, src.join("loop")).unwrap(); + let dest_dir = root.join("into"); + write(&dest_dir.join("pkg/keep.txt"), "keep"); + + let report = copy_into_dir(&*LocalHost::shared(), &[src], &dest_dir, true); + + assert_eq!(report.errors.len(), 1); + assert!(report.copied.is_empty()); + assert_eq!( + std::fs::read_to_string(dest_dir.join("pkg/keep.txt")).unwrap(), + "keep", + "a copy that failed took the destination down with it" + ); + assert!( + leavings(&dest_dir).is_empty(), + "the failed copy was left behind: {:?}", + leavings(&dest_dir) + ); + } + #[test] fn a_row_dropped_back_on_its_own_folder_does_nothing() { let root = scratch("same-dir"); diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 7426f517..ac581ac2 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -890,8 +890,13 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::FileTreeContextShowDotfiles => "Show Dotfiles", L10nKey::FileDropIntoItself => "A folder cannot be copied into itself.", L10nKey::FileDropNotHere => "Not on this machine.", + L10nKey::FileDropNameTaken => "Another item in the same drop already has that name.", L10nKey::FileDropTooDeep => "Nested more than {n} folders deep.", L10nKey::FileDropTooLarge => "Larger than {limit} MB — send it over SFTP instead.", + L10nKey::FileDropNoWorkingName => "No free name beside it to copy onto first.", + L10nKey::FileDropLeftAside => { + "The copy could not be put in place; what was there is now named \"{name}\" in the same folder." + } L10nKey::FileDropReplaceTitle => "Replace \"{name}\"?", L10nKey::FileDropReplaceManyTitle => "Replace {n} items?", L10nKey::FileDropReplaceBody => { diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 8357f3bd..78979cd7 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -938,8 +938,13 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::FileTreeContextShowDotfiles => "ドットファイルを表示", L10nKey::FileDropIntoItself => "フォルダを自分自身の中にはコピーできません", L10nKey::FileDropNotHere => "このマシンにはありません", + L10nKey::FileDropNameTaken => "同じドロップ内の別の項目がすでにこの名前を使っています", L10nKey::FileDropTooDeep => "フォルダの入れ子が {n} 階層を超えています", L10nKey::FileDropTooLarge => "{limit} MB を超えています。SFTP で転送してください", + L10nKey::FileDropNoWorkingName => "隣に空いている一時的な名前がなく、先にコピーできません", + L10nKey::FileDropLeftAside => { + "新しいコピーを所定の位置に移せませんでした。元のものは同じフォルダの「{name}」になっています" + } L10nKey::FileDropReplaceTitle => "「{name}」を置き換えますか？", L10nKey::FileDropReplaceManyTitle => "{n} 項目を置き換えますか？", L10nKey::FileDropReplaceBody => { diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index a97bf2a8..aabcea66 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -678,8 +678,11 @@ l10n_keys! { FileTreeContextShowDotfiles, FileDropIntoItself, FileDropNotHere, + FileDropNameTaken, FileDropTooDeep, FileDropTooLarge, + FileDropNoWorkingName, + FileDropLeftAside, FileDropReplaceTitle, FileDropReplaceManyTitle, FileDropReplaceBody, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index 1209a79c..f87b2b09 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -849,8 +849,11 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::FileTreeContextShowDotfiles => "显示点文件", L10nKey::FileDropIntoItself => "文件夹不能复制到它自己里面。", L10nKey::FileDropNotHere => "不在这台机器上。", + L10nKey::FileDropNameTaken => "同一次拖放里已经有另一项用了这个名字。", L10nKey::FileDropTooDeep => "文件夹嵌套超过 {n} 层。", L10nKey::FileDropTooLarge => "超过 {limit} MB，请改用 SFTP 传输。", + L10nKey::FileDropNoWorkingName => "旁边找不到可用的临时名称，无法先复制再替换。", + L10nKey::FileDropLeftAside => "新副本没能就位，原来的东西现在在同一目录下叫“{name}”。", L10nKey::FileDropReplaceTitle => "替换“{name}”？", L10nKey::FileDropReplaceManyTitle => "替换 {n} 个项目？", L10nKey::FileDropReplaceBody => "这个目录下已经有同名的东西了，替换后无法撤销。",