zyedidia / micro

A modern and intuitive terminal-based text editor
https://micro-editor.github.io
MIT License
24.39k stars 1.16k forks source link

save: Save files atomically #3273

Open JoeKar opened 2 months ago

JoeKar commented 2 months ago

The target situation shall be, that micro checks if the file to be stored already exists and if so it shall work with a temporary file first, before the target file is overwritten respective the temporary file renamed to the target file. Possible symlinks pointing to the target file will be resolved, before the save takes place. This shall guarantee that this symlink isn't renamed by the new approach.

TODOs:

Fixes #1916 Fixes #3148 Fixes #3196

JoeKar commented 1 month ago

Still a lot of rework left, but thank your for your time reviewing this! :+1:

JoeKar commented 1 month ago

I did a lot of (commit) refactoring, but still I'm not fully happy with it. The backup approach is still messy (duplicated code, no single point of truth, etc.), but currently I'm running out of ideas.

dmaluka commented 1 month ago

I did a lot of (commit) refactoring, but still I'm not fully happy with it. The backup approach is still messy (duplicated code, no single point of truth, etc.), but currently I'm running out of ideas.

IIUC, basically, all these troubles are due to the need to avoid circular dependencies between packages?

If so, it seems we could avoid these troubles by implementing overwrite & backup stuff as a part of the config package. OTOH, that sounds like an abuse of the config package: it is supposed to implement stuff related specifically to configuration, not stuff like buffer backups.

So maybe instead, simply provide separate implementations of overwrite & backup stuff for different kinds of files, in both buffer and config packages (especially since, as I noted in another comment, it seems better to have separate implementations of Overwrite() anyway). Maybe make them implement some common interface (if there is a need for that).

A very rough idea:

type FileWriter interface {
    Overwrite(name string) error
    BackupName(name string) string
    KeepBackup() bool
}

and a common Write() implementation using this interface:

func Write(name string, fwriter FileWriter) error

Or maybe even Write() implementations need to be different for different files.

P.S. Side note: maybe let's name it SafeWrite()?

JoeKar commented 1 month ago

Yes, I know https://github.com/zyedidia/micro/pull/3273/commits/dc701a15eff200eb68732a3eac947c23e4c2c6da doesn't really belong to the PR in the first place, but I touched some WriteFile's anyway. It was not a big deal to do this cleanup.