Merge pull request #101 from levineuwirth/fix-save-clobber-guard

fix(save): refuse to silently clobber a file changed on disk

builtin/commands/default.lua

@@ -227,11 +227,26 @@ cmd { name = "buffer.save", description = "Save the current buffer to its backin
           ed.set_status("save vetoed by buffer.before-save")
           return
         end
+        -- `ed.save()` refuses when the file changed on disk since this
+        -- buffer read it, rather than clobbering the other writer. It
+        -- reports how to override; `buffer.after-save` must not fire.
         if ed.save() then
           pmacs.hook.run("buffer.after-save")
         end
       end }
 
+cmd { name = "buffer.save-anyway",
+      description = "Save, overwriting a file that changed on disk since it was read.",
+      fn = function()
+        if not pmacs.hook.run("buffer.before-save") then
+          ed.set_status("save vetoed by buffer.before-save")
+          return
+        end
+        if ed.save_ignoring_disk_changes() then
+          pmacs.hook.run("buffer.after-save")
+        end
+      end }
+
 -- Editor session -------------------------------------------------------------
 
 cmd { name = "editor.quit",   description = "Exit the editor.",

src/editor_core.rs

@@ -1229,11 +1229,61 @@ impl EditorCore {
     /// `buffer.save` Lua command) use the return value to gate
     /// `buffer.after-save` firing.
     pub fn save(&mut self) -> bool {
+        self.save_inner(false)
+    }
+
+    /// [`save`](Self::save), overwriting the file even though it changed on
+    /// disk since this buffer read it. The escape hatch for when the user
+    /// has looked and decided their buffer wins.
+    pub fn save_ignoring_disk_changes(&mut self) -> bool {
+        self.save_inner(true)
+    }
+
+    /// True when writing this buffer to `path` would destroy content the
+    /// buffer has never seen — i.e. a file exists there whose identity
+    /// differs from the [`FileMeta`] recorded when the buffer last read or
+    /// wrote it.
+    ///
+    /// Two cases count as "changed":
+    ///
+    /// * the buffer recorded a meta and the on-disk meta differs — someone
+    ///   else edited the file (another editor, a `git checkout`);
+    /// * the buffer recorded **no** meta (a `[new file]`, or a buffer whose
+    ///   path was set without reading) yet a file now exists — it was
+    ///   created underneath us, and we have never seen its contents.
+    ///
+    /// A **missing** file is not a clobber: there is nothing there to
+    /// destroy, so recreating a deleted file saves normally. An unstattable
+    /// path likewise falls through, and `save_atomic` reports the real
+    /// error.
+    #[must_use]
+    pub fn save_would_clobber(&self, id: BufferId, path: &Path) -> bool {
+        let Ok(current) = crate::file_io::current_meta(path) else {
+            return false; // absent, or we cannot stat it
+        };
+        let reg = self.registry.borrow();
+        let Ok(buffer) = reg.get(id) else {
+            return false;
+        };
+        buffer.file_meta() != Some(&current)
+    }
+
+    fn save_inner(&mut self, force: bool) -> bool {
         let id = self.active_buffer_id();
         let Some(path) = self.active_buffer_path() else {
             self.status = "no file (M1: open a file from argv)".into();
             return false;
         };
+        // Refuse to silently overwrite a file that changed underneath us.
+        // Without this, pmacs clobbers another editor's (or a `git
+        // checkout`'s) writes with a buffer that never saw them.
+        if !force && self.save_would_clobber(id, &path) {
+            self.status = format!(
+                "{} changed on disk since it was read --- M-x buffer.save-anyway to overwrite",
+                path.display()
+            );
+            return false;
+        }
         let len_and_bytes = {
             let reg = self.registry.borrow();
             let buffer = match reg.get(id) {

src/lua_bindings/mod.rs

@@ -10870,6 +10870,16 @@ fn install_session(editor: &Table, lua: &Lua, core: &SharedCore) -> mlua::Result
             lua.create_function(move |_, ()| Ok(cc.borrow_mut().save()))?,
         )?;
     }
+    {
+        // Overwrite even though the file changed on disk since this buffer
+        // read it. `save()` refuses that case rather than silently
+        // clobbering another writer; this is the deliberate override.
+        let cc = core.clone();
+        editor.set(
+            "save_ignoring_disk_changes",
+            lua.create_function(move |_, ()| Ok(cc.borrow_mut().save_ignoring_disk_changes()))?,
+        )?;
+    }
     register(editor, lua, core, "quit", |c| c.quit = true)?;
     register(editor, lua, core, "cancel", |c| {
         c.status = "Quit".into();

tests/save_clobber_guard_acceptance.rs

@@ -0,0 +1,202 @@
+//! `EditorCore::save()` must not silently overwrite a file that changed on
+//! disk since the buffer read it.
+//!
+//! Before this guard, pmacs wrote unconditionally and only *then* recorded
+//! the new `FileMeta` — so another editor's (or a `git checkout`'s) writes
+//! were destroyed without a word. The comparison seam
+//! (`FileMeta: PartialEq`, `file_io::current_meta`) already existed and no
+//! caller used it.
+
+use pmacs::editor::EditorState;
+use std::path::PathBuf;
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+fn tempdir() -> PathBuf {
+    static SEQ: AtomicUsize = AtomicUsize::new(0);
+    let d = std::env::temp_dir().join(format!(
+        "pmacs-clobber-{}-{}",
+        std::process::id(),
+        SEQ.fetch_add(1, Ordering::Relaxed)
+    ));
+    std::fs::create_dir_all(&d).unwrap();
+    d
+}
+
+fn exec(s: &EditorState, src: &str) {
+    s.lua_host.lua().load(src.to_string()).exec().unwrap();
+}
+
+fn eval<T: mlua::FromLuaMulti>(s: &EditorState, src: &str) -> T {
+    s.lua_host.lua().load(src.to_string()).eval().unwrap()
+}
+
+fn write(p: &std::path::Path, body: &str) {
+    std::fs::write(p, body).unwrap();
+}
+
+fn read(p: &std::path::Path) -> String {
+    std::fs::read_to_string(p).unwrap()
+}
+
+fn status(s: &EditorState) -> String {
+    s.core.borrow().status.clone()
+}
+
+/// Open `path` and dirty the buffer.
+fn open_and_dirty(s: &EditorState, path: &str) {
+    exec(
+        s,
+        &format!("pmacs.buffer.find_or_open({path:?}); pmacs.window.buffer():insert(0, 'mine ')"),
+    );
+}
+
+#[test]
+fn save_refuses_to_clobber_a_file_changed_on_disk() {
+    let dir = tempdir();
+    let f = dir.join("a.txt");
+    write(&f, "original\n");
+    let fs = f.display().to_string();
+
+    let s = EditorState::new();
+    open_and_dirty(&s, &fs);
+
+    // Another writer lands between our read and our save.
+    write(&f, "THEIRS -- do not destroy\n");
+
+    let saved: bool = eval(&s, "return pmacs.editor.save()");
+    assert!(!saved, "save must refuse");
+    assert_eq!(
+        read(&f),
+        "THEIRS -- do not destroy\n",
+        "their content is intact"
+    );
+    assert!(
+        status(&s).contains("changed on disk") && status(&s).contains("save-anyway"),
+        "the refusal says what happened and how to override: {:?}",
+        status(&s)
+    );
+    // The buffer keeps its unsaved edits — nothing was lost on our side.
+    let modified: bool = eval(&s, "return pmacs.window.buffer():is_modified()");
+    assert!(modified);
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn the_save_command_does_not_fire_after_save_when_it_refuses() {
+    let dir = tempdir();
+    let f = dir.join("a.txt");
+    write(&f, "original\n");
+    let fs = f.display().to_string();
+
+    let s = EditorState::new();
+    open_and_dirty(&s, &fs);
+    exec(
+        &s,
+        "_G.after = 0; pmacs.hook.add('buffer.after-save', function() _G.after = _G.after + 1 end)",
+    );
+    write(&f, "theirs\n");
+
+    exec(&s, "pmacs.command.invoke('buffer.save')");
+    let after: i64 = eval(&s, "return _G.after");
+    assert_eq!(after, 0, "after-save must not fire on a refused save");
+    assert_eq!(read(&f), "theirs\n");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn save_anyway_overwrites_deliberately_and_resyncs_meta() {
+    let dir = tempdir();
+    let f = dir.join("a.txt");
+    write(&f, "original\n");
+    let fs = f.display().to_string();
+
+    let s = EditorState::new();
+    open_and_dirty(&s, &fs);
+    write(&f, "theirs\n");
+    assert!(!eval::<bool>(&s, "return pmacs.editor.save()"));
+
+    // The user looked and decided their buffer wins.
+    exec(&s, "pmacs.command.invoke('buffer.save-anyway')");
+    assert_eq!(read(&f), "mine original\n", "overwritten on purpose");
+
+    // The buffer re-syncs to the file it just wrote, so an immediate
+    // ordinary save is allowed again.
+    exec(&s, "pmacs.window.buffer():insert(0, 'more ')");
+    assert!(
+        eval::<bool>(&s, "return pmacs.editor.save()"),
+        "meta was refreshed by the forced save"
+    );
+    assert_eq!(read(&f), "more mine original\n");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn an_unchanged_file_saves_normally_and_repeatedly() {
+    let dir = tempdir();
+    let f = dir.join("a.txt");
+    write(&f, "original\n");
+    let fs = f.display().to_string();
+
+    let s = EditorState::new();
+    open_and_dirty(&s, &fs);
+    assert!(eval::<bool>(&s, "return pmacs.editor.save()"));
+    assert_eq!(read(&f), "mine original\n");
+
+    // A save updates our recorded meta, so the next one is not a false
+    // positive — the guard must not trip on our own writes.
+    exec(&s, "pmacs.window.buffer():insert(0, 'again ')");
+    assert!(eval::<bool>(&s, "return pmacs.editor.save()"));
+    assert_eq!(read(&f), "again mine original\n");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn a_deleted_file_is_recreated_not_refused() {
+    let dir = tempdir();
+    let f = dir.join("a.txt");
+    write(&f, "original\n");
+    let fs = f.display().to_string();
+
+    let s = EditorState::new();
+    open_and_dirty(&s, &fs);
+    // Nothing on disk to clobber, so recreating it is not data loss.
+    std::fs::remove_file(&f).unwrap();
+    assert!(
+        eval::<bool>(&s, "return pmacs.editor.save()"),
+        "a vanished file is recreated, not refused"
+    );
+    assert_eq!(read(&f), "mine original\n");
+    std::fs::remove_dir_all(&dir).ok();
+}
+
+#[test]
+fn a_new_file_buffer_refuses_once_someone_else_creates_the_file() {
+    let dir = tempdir();
+    let missing = dir.join("draft.txt");
+
+    let s = EditorState::new();
+    // The argv `[new file]` shape: a path with nothing on disk, no meta.
+    exec(
+        &s,
+        "_G.nb = pmacs.buffer.create('draft.txt'); pmacs.window.switch_buffer(_G.nb)",
+    );
+    {
+        let id = s.core.borrow().active_buffer_id();
+        s.core
+            .borrow_mut()
+            .set_buffer_path(id, Some(missing.clone()));
+    }
+    exec(&s, "pmacs.window.buffer():insert(0, 'my draft')");
+
+    // While we drafted, someone created the file. We have never seen its
+    // contents, so writing over them is exactly the clobber we refuse.
+    write(&missing, "theirs\n");
+    assert!(!eval::<bool>(&s, "return pmacs.editor.save()"));
+    assert_eq!(read(&missing), "theirs\n");
+
+    // With the file still absent it would have saved cleanly.
+    std::fs::remove_file(&missing).unwrap();
+    assert!(eval::<bool>(&s, "return pmacs.editor.save()"));
+    assert_eq!(read(&missing), "my draft");
+    std::fs::remove_dir_all(&dir).ok();
+}