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 => "这个目录下已经有同名的东西了,替换后无法撤销。",