From e6e89860e62447dd670154c98d6dd2d0201ce235 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Fri, 13 Nov 2026 15:32:53 +0800 Subject: [PATCH] fix(agent): don't overwrite a file at a case-only rename's new name on accept (#3095) On a case-sensitive disk, accepting an a.txt -> A.txt rename removed a.txt and wrote A.txt unchecked, overwriting a file the user had created there. Both names are now checked before either changes (old: the checkout's file; new: absent or already the task's), and the new name is written before the old one is removed. --- crates/ide-agent/src/worktree.rs | 70 ++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/crates/ide-agent/src/worktree.rs b/crates/ide-agent/src/worktree.rs index de5287deb..5f2a9ca63 100644 --- a/crates/ide-agent/src/worktree.rs +++ b/crates/ide-agent/src/worktree.rs @@ -411,25 +411,30 @@ pub(crate) fn accept( let copyable = |rel: &str| task_side.allows(rel) && checkout_side.allows(rel); let renamed_from: BTreeMap = repo.diff_with_worktree(base)?.into_iter().filter_map(|f| Some((f.path, f.old_path?))).collect(); - // Brings one path of the checkout to the task's version, unless the checkout changed it itself. - let apply = |rel: &str| -> IdeResult { - let local = local_root.join(rel); + // The task's version of one path of the checkout (`Some(None)`: absent there), or `None` when + // the checkout changed it itself. + let target = |rel: &str| -> IdeResult>>> { let in_task = repo.root().join(rel); - let base_bytes = repo.file_bytes_at(&in_task, Some(base))?; let current = std::fs::read(&in_task).ok(); - let local_bytes = std::fs::read(&local).ok(); + let local_bytes = std::fs::read(local_root.join(rel)).ok(); if local_bytes == current { - return Ok(true); + return Ok(Some(current)); } + let base_bytes = repo.file_bytes_at(&in_task, Some(base))?; // The checkout may hold the committed LF text with CRLF line ends (`core.autocrlf`). let unchanged = match (&local_bytes, &base_bytes) { (Some(now), Some(at_base)) => now == at_base || crlf_to_lf(now) == *at_base, (now, at_base) => now == at_base, }; - if !unchanged { - return Ok(false); + Ok(unchanged.then_some(current)) + }; + // Puts `bytes` (`None`: nothing) at `rel` of the checkout. + let write = |rel: &str, bytes: Option>| -> IdeResult<()> { + let local = local_root.join(rel); + if std::fs::read(&local).ok() == bytes { + return Ok(()); } - match current { + match bytes { Some(bytes) => { if let Some(parent) = local.parent() { std::fs::create_dir_all(parent)?; @@ -441,6 +446,12 @@ pub(crate) fn accept( _ => {} }, } + Ok(()) + }; + // Brings one path of the checkout to the task's version, unless the checkout changed it itself. + let apply = |rel: &str| -> IdeResult { + let Some(bytes) = target(rel)? else { return Ok(false) }; + write(rel, bytes)?; Ok(true) }; let mut accepted = Vec::with_capacity(changes.len()); @@ -466,8 +477,9 @@ pub(crate) fn accept( // A case-only rename (`a.txt` → `A.txt`). Where the disk resolves both // spellings to one file it is renamed in place and `apply` then refreshes // the content. On a case-sensitive disk the two names are different files: - // the old one's unchanged-ness (it is the file the checkout had) decides — - // then the task's version lands at the new name and the old one goes. + // both are checked before either changes — the old one must be the file the + // checkout had, the new one absent or already the task's — then the task's + // version lands at the new name before the old one goes. let case_only = old.is_some_and(|o| o != &change.rel_path && o.to_lowercase() == change.rel_path.to_lowercase()); if case_only { @@ -476,20 +488,18 @@ pub(crate) fn accept( if same_file(&old_path, &new_path) { std::fs::rename(&old_path, &new_path)?; } else { - if !apply(old)? { + let Some(old_target) = target(old)? else { skipped.push(skip(old)); continue; + }; + let task_bytes = std::fs::read(repo.root().join(&change.rel_path)).ok(); + let at_new = std::fs::read(&new_path).ok(); + if at_new.is_some() && at_new != task_bytes { + skipped.push(skip(&change.rel_path)); + continue; } - // `apply(old)` removed the old file (the task's tree has it only at - // the new name); now write that version under the new name. `apply` - // can't do it — it reads the new name's own base/absence — so the - // write is open-coded from the same task bytes `apply` would use. - if let Ok(bytes) = std::fs::read(repo.root().join(&change.rel_path)) { - if let Some(parent) = new_path.parent() { - std::fs::create_dir_all(parent)?; - } - std::fs::write(&new_path, bytes)?; - } + write(&change.rel_path, task_bytes)?; + write(old, old_target)?; accepted.push(change.rel_path.clone()); continue; } @@ -960,6 +970,22 @@ mod tests { assert_eq!(names, ["A.txt"]); } + #[test] + fn a_case_only_rename_keeps_a_file_the_checkout_has_at_the_new_name() { + let (_tmp, repo, created) = accept_repo(); + std::fs::write(repo.join("A.txt"), "mine\n").unwrap(); + if std::fs::read_to_string(repo.join("a.txt")).unwrap() == "mine\n" { + eprintln!("this disk doesn't tell a.txt from A.txt; nothing to check"); + return; + } + git(&created.path, &["mv", "a.txt", "A.txt"]); + let changes = listed(&created.path, &created.base_commit); + let outcome = accept(&repo, &created.path, &created.base_commit, &changes).unwrap(); + assert_eq!(outcome.skipped.iter().map(|s| s.path.as_str()).collect::>(), ["A.txt"]); + assert_eq!(std::fs::read_to_string(repo.join("A.txt")).unwrap(), "mine\n"); + assert_eq!(std::fs::read_to_string(repo.join("a.txt")).unwrap(), "one\ntwo\n"); + } + #[cfg(windows)] #[test] fn accept_does_not_write_into_the_checkouts_git_folder_through_its_short_name() {