From 4ade5cdf2466ba26fc6cb864946ff9482dd153b8 Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sat, 26 Jul 2025 22:38:13 +0200 Subject: [PATCH 1/9] Make calcHash() a method of SharedBuffer This will make it easier to use calcHash() in other SharedBuffer methods. --- internal/buffer/buffer.go | 46 ++++++++++++++++++------------------- internal/buffer/save.go | 2 +- internal/buffer/settings.go | 2 +- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/internal/buffer/buffer.go b/internal/buffer/buffer.go index 5280a71f..ef9d2331 100644 --- a/internal/buffer/buffer.go +++ b/internal/buffer/buffer.go @@ -140,6 +140,26 @@ func (b *SharedBuffer) remove(start, end Loc) []byte { return b.LineArray.remove(start, end) } +// calcHash calculates md5 hash of all lines in the buffer +func (b *SharedBuffer) calcHash(out *[md5.Size]byte) { + h := md5.New() + + if len(b.lines) > 0 { + h.Write(b.lines[0].data) + + for _, l := range b.lines[1:] { + if b.Endings == FFDos { + h.Write([]byte{'\r', '\n'}) + } else { + h.Write([]byte{'\n'}) + } + h.Write(l.data) + } + } + + h.Sum((*out)[:0]) +} + // MarkModified marks the buffer as modified for this frame // and performs rehighlighting if syntax highlighting is enabled func (b *SharedBuffer) MarkModified(start, end int) { @@ -416,7 +436,7 @@ func NewBuffer(r io.Reader, size int64, path string, startcursor Loc, btype BufT } else if !hasBackup { // since applying a backup does not save the applied backup to disk, we should // not calculate the original hash based on the backup data - calcHash(b, &b.origHash) + b.calcHash(&b.origHash) } } @@ -558,7 +578,7 @@ func (b *Buffer) ReOpen() error { if len(data) > LargeFileThreshold { b.Settings["fastdirty"] = true } else { - calcHash(b, &b.origHash) + b.calcHash(&b.origHash) } } b.isModified = false @@ -643,7 +663,7 @@ func (b *Buffer) Modified() bool { var buff [md5.Size]byte - calcHash(b, &buff) + b.calcHash(&buff) return buff != b.origHash } @@ -663,26 +683,6 @@ func (b *Buffer) Size() int { return nb } -// calcHash calculates md5 hash of all lines in the buffer -func calcHash(b *Buffer, out *[md5.Size]byte) { - h := md5.New() - - if len(b.lines) > 0 { - h.Write(b.lines[0].data) - - for _, l := range b.lines[1:] { - if b.Endings == FFDos { - h.Write([]byte{'\r', '\n'}) - } else { - h.Write([]byte{'\n'}) - } - h.Write(l.data) - } - } - - h.Sum((*out)[:0]) -} - func parseDefFromFile(f config.RuntimeFile, header *highlight.Header) *highlight.Def { data, err := f.Data() if err != nil { diff --git a/internal/buffer/save.go b/internal/buffer/save.go index c7bed485..f3e05ad2 100644 --- a/internal/buffer/save.go +++ b/internal/buffer/save.go @@ -318,7 +318,7 @@ func (b *Buffer) saveToFile(filename string, withSudo bool, autoSave bool) error // For large files 'fastdirty' needs to be on b.Settings["fastdirty"] = true } else { - calcHash(b, &b.origHash) + b.calcHash(&b.origHash) } } diff --git a/internal/buffer/settings.go b/internal/buffer/settings.go index 4c2c2a1d..ed22eae8 100644 --- a/internal/buffer/settings.go +++ b/internal/buffer/settings.go @@ -73,7 +73,7 @@ func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) { b.Settings["fastdirty"] = true } else { if !b.isModified { - calcHash(b, &b.origHash) + b.calcHash(&b.origHash) } else { // prevent using an old stale origHash value b.origHash = [md5.Size]byte{} From f938f62e318ce83bb1b6ca869035712166fc9490 Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sun, 27 Jul 2025 00:24:02 +0200 Subject: [PATCH 2/9] Make isModified reflect actual modified/unmodified state of buffer Instead of calculating the hash of the buffer every time Modified() is called, do that every time b.isModified is updated (i.e. every time the buffer is modified) and set b.isModified value accordingly. This change means that the hash will be recalculated every time the user types or deletes a character. But that is what already happens anyway, since inserting or deleting characters triggers redrawing the display, in particular redrawing the status line, which triggers Modified() in order to show the up-to-date modified/unmodified status in the status line. And with this change, we will be able to check this status more than once during a single "handle event & redraw" cycle, while still recalculating the hash only once. --- internal/buffer/backup.go | 2 +- internal/buffer/buffer.go | 37 ++++++++++++++++++++----------------- internal/buffer/save.go | 4 +--- internal/buffer/settings.go | 4 ++-- 4 files changed, 24 insertions(+), 23 deletions(-) diff --git a/internal/buffer/backup.go b/internal/buffer/backup.go index cda7a0eb..32180c0a 100644 --- a/internal/buffer/backup.go +++ b/internal/buffer/backup.go @@ -125,7 +125,7 @@ func (b *Buffer) ApplyBackup(fsize int64) (bool, bool) { if choice%3 == 0 { // recover b.LineArray = NewLineArray(uint64(fsize), FFAuto, backup) - b.isModified = true + b.setModified() return true, true } else if choice%3 == 1 { // delete diff --git a/internal/buffer/buffer.go b/internal/buffer/buffer.go index ef9d2331..00056fc0 100644 --- a/internal/buffer/buffer.go +++ b/internal/buffer/buffer.go @@ -126,20 +126,36 @@ type SharedBuffer struct { } func (b *SharedBuffer) insert(pos Loc, value []byte) { - b.isModified = true b.HasSuggestions = false b.LineArray.insert(pos, value) + b.setModified() inslines := bytes.Count(value, []byte{'\n'}) b.MarkModified(pos.Y, pos.Y+inslines) } + func (b *SharedBuffer) remove(start, end Loc) []byte { - b.isModified = true b.HasSuggestions = false + defer b.setModified() defer b.MarkModified(start.Y, end.Y) return b.LineArray.remove(start, end) } +func (b *SharedBuffer) setModified() { + if b.Type.Scratch { + return + } + + if b.Settings["fastdirty"].(bool) { + b.isModified = true + } else { + var buff [md5.Size]byte + + b.calcHash(&buff) + b.isModified = buff != b.origHash + } +} + // calcHash calculates md5 hash of all lines in the buffer func (b *SharedBuffer) calcHash(out *[md5.Size]byte) { h := md5.New() @@ -653,18 +669,7 @@ func (b *Buffer) Shared() bool { // Modified returns if this buffer has been modified since // being opened func (b *Buffer) Modified() bool { - if b.Type.Scratch { - return false - } - - if b.Settings["fastdirty"].(bool) { - return b.isModified - } - - var buff [md5.Size]byte - - b.calcHash(&buff) - return buff != b.origHash + return b.isModified } // Size returns the number of bytes in the current buffer @@ -1233,7 +1238,6 @@ func (b *Buffer) FindMatchingBrace(start Loc) (Loc, bool, bool) { func (b *Buffer) Retab() { toSpaces := b.Settings["tabstospaces"].(bool) tabsize := util.IntOpt(b.Settings["tabsize"]) - dirty := false for i := 0; i < b.LinesNum(); i++ { l := b.LineBytes(i) @@ -1254,10 +1258,9 @@ func (b *Buffer) Retab() { b.Unlock() b.MarkModified(i, i) - dirty = true } - b.isModified = dirty + b.setModified() } // ParseCursorLocation turns a cursor location like 10:5 (LINE:COL) diff --git a/internal/buffer/save.go b/internal/buffer/save.go index f3e05ad2..3c79089a 100644 --- a/internal/buffer/save.go +++ b/internal/buffer/save.go @@ -206,9 +206,7 @@ func (b *Buffer) Save() error { // AutoSave saves the buffer to its default path func (b *Buffer) AutoSave() error { - // Doing full b.Modified() check every time would be costly, due to the hash - // calculation. So use just isModified even if fastdirty is not set. - if !b.isModified { + if !b.Modified() { return nil } return b.saveToFile(b.Path, false, true) diff --git a/internal/buffer/settings.go b/internal/buffer/settings.go index ed22eae8..9c8b3ce1 100644 --- a/internal/buffer/settings.go +++ b/internal/buffer/settings.go @@ -91,7 +91,7 @@ func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) { case "dos": b.Endings = FFDos } - b.isModified = true + b.setModified() } else if option == "syntax" { if !nativeValue.(bool) { b.ClearMatches() @@ -105,7 +105,7 @@ func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) { b.Settings["encoding"] = "utf-8" } b.encoding = enc - b.isModified = true + b.setModified() } else if option == "readonly" && b.Type.Kind == BTDefault.Kind { b.Type.Readonly = nativeValue.(bool) } else if option == "hlsearch" { From e84d44d451bb548df42b9fec048fc3fa7edcb804 Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sun, 27 Jul 2025 02:41:25 +0200 Subject: [PATCH 3/9] Move backup & save related stuff from Buffer to SharedBuffer Various methods of Buffer should be rather methods of SharedBuffer. This commit doesn't move all of them to SharedBuffer yet, only those that need to be moved to SharedBuffer in order to be able to request creating or removing backups in other SharedBuffer methods. --- internal/buffer/backup.go | 16 ++++++++-------- internal/buffer/buffer.go | 3 ++- internal/buffer/save.go | 12 ++++++------ 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/internal/buffer/backup.go b/internal/buffer/backup.go index 32180c0a..941ac04f 100644 --- a/internal/buffer/backup.go +++ b/internal/buffer/backup.go @@ -34,13 +34,13 @@ Options: [r]ecover, [i]gnore, [a]bort: ` const backupSeconds = 8 -var BackupCompleteChan chan *Buffer +var BackupCompleteChan chan *SharedBuffer func init() { - BackupCompleteChan = make(chan *Buffer, 10) + BackupCompleteChan = make(chan *SharedBuffer, 10) } -func (b *Buffer) RequestBackup() { +func (b *SharedBuffer) RequestBackup() { if !b.RequestedBackup { select { case backupRequestChan <- b: @@ -51,7 +51,7 @@ func (b *Buffer) RequestBackup() { } } -func (b *Buffer) backupDir() string { +func (b *SharedBuffer) backupDir() string { backupdir, err := util.ReplaceHome(b.Settings["backupdir"].(string)) if backupdir == "" || err != nil { backupdir = filepath.Join(config.ConfigDir, "backups") @@ -59,12 +59,12 @@ func (b *Buffer) backupDir() string { return backupdir } -func (b *Buffer) keepBackup() bool { +func (b *SharedBuffer) keepBackup() bool { return b.forceKeepBackup || b.Settings["permbackup"].(bool) } // Backup saves the current buffer to the backups directory -func (b *Buffer) Backup() error { +func (b *SharedBuffer) Backup() error { if !b.Settings["backup"].(bool) || b.Path == "" || b.Type != BTDefault { return nil } @@ -101,7 +101,7 @@ func (b *Buffer) Backup() error { } // RemoveBackup removes any backup file associated with this buffer -func (b *Buffer) RemoveBackup() { +func (b *SharedBuffer) RemoveBackup() { if !b.Settings["backup"].(bool) || b.keepBackup() || b.Path == "" || b.Type != BTDefault { return } @@ -111,7 +111,7 @@ func (b *Buffer) RemoveBackup() { // ApplyBackup applies the corresponding backup file to this buffer (if one exists) // Returns true if a backup was applied -func (b *Buffer) ApplyBackup(fsize int64) (bool, bool) { +func (b *SharedBuffer) ApplyBackup(fsize int64) (bool, bool) { if b.Settings["backup"].(bool) && !b.Settings["permbackup"].(bool) && len(b.Path) > 0 && b.Type == BTDefault { backupfile := util.DetermineEscapePath(b.backupDir(), b.AbsPath) if info, err := os.Stat(backupfile); err == nil { diff --git a/internal/buffer/buffer.go b/internal/buffer/buffer.go index 00056fc0..9bd7ecfa 100644 --- a/internal/buffer/buffer.go +++ b/internal/buffer/buffer.go @@ -123,6 +123,8 @@ type SharedBuffer struct { // Hash of the original buffer -- empty if fastdirty is on origHash [md5.Size]byte + + fini int32 } func (b *SharedBuffer) insert(pos Loc, value []byte) { @@ -223,7 +225,6 @@ type Buffer struct { *EventHandler *SharedBuffer - fini int32 cursors []*Cursor curCursor int StartCursor Loc diff --git a/internal/buffer/save.go b/internal/buffer/save.go index 3c79089a..aaab39ba 100644 --- a/internal/buffer/save.go +++ b/internal/buffer/save.go @@ -47,11 +47,11 @@ type saveRequest struct { } var saveRequestChan chan saveRequest -var backupRequestChan chan *Buffer +var backupRequestChan chan *SharedBuffer func init() { saveRequestChan = make(chan saveRequest, 10) - backupRequestChan = make(chan *Buffer, 10) + backupRequestChan = make(chan *SharedBuffer, 10) go func() { duration := backupSeconds * float64(time.Second) @@ -116,7 +116,7 @@ func openFile(name string, withSudo bool) (wrappedFile, error) { return wrappedFile{writeCloser, withSudo, screenb, cmd, sigChan}, nil } -func (wf wrappedFile) Write(b *Buffer) (int, error) { +func (wf wrappedFile) Write(b *SharedBuffer) (int, error) { file := bufio.NewWriter(transform.NewWriter(wf.writeCloser, b.encoding.NewEncoder())) b.Lock() @@ -184,7 +184,7 @@ func (wf wrappedFile) Close() error { return err } -func (b *Buffer) overwriteFile(name string) (int, error) { +func (b *SharedBuffer) overwriteFile(name string) (int, error) { file, err := openFile(name, false) if err != nil { return 0, err @@ -335,7 +335,7 @@ func (b *Buffer) saveToFile(filename string, withSudo bool, autoSave bool) error return err } -func (b *Buffer) writeBackup(path string) (string, error) { +func (b *SharedBuffer) writeBackup(path string) (string, error) { backupDir := b.backupDir() if _, err := os.Stat(backupDir); err != nil { if !errors.Is(err, fs.ErrNotExist) { @@ -360,7 +360,7 @@ func (b *Buffer) writeBackup(path string) (string, error) { // contents of the file if it fails to write the new contents. // This means that the file is not overwritten directly but by writing to the // backup file first. -func (b *Buffer) safeWrite(path string, withSudo bool, newFile bool) (int, error) { +func (b *SharedBuffer) safeWrite(path string, withSudo bool, newFile bool) (int, error) { file, err := openFile(path, withSudo) if err != nil { return 0, err From 2c010afbe4c2b2373277b4c43c87cec47ba5d470 Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sat, 2 Aug 2025 22:13:35 +0200 Subject: [PATCH 4/9] Fix races between removing backups and creating periodic backups Micro's logic for periodic backup creation is racy and may cause spurious backups of unmodified buffers, at least for the following reasons: 1. When a buffer is closed, its backup is removed by the main goroutine, without any synchronization with the backup/save goroutine which creates periodic backups in the background. A part of the problem here is that the main goroutine removes the backup before setting b.fini to true, not after it, so the backup/save goroutine may start creating a new backup even after it has been removed by the main goroutine. But even if we move the b.RemoveBackup() call after setting b.fini, it will not solve the problem, since the backup/save goroutine may have already started creating a new periodic backup just before b.fini was set to true. 2. When a buffer is successfully saved and thus its backup is removed, if there was a periodic backup for this buffer requested by the main goroutine but not saved by the backup/save goroutine yet (i.e. this request is still pending in backupRequestChan), micro doesn't cancel this pending request, so a backup is unexpectedly saved a couple of seconds after the file itself was saved. Although usually this erroneous backup is removed later, when the buffer is closed. But if micro terminates abnormally and the buffer is not properly closed, this backup is not removed. Also if this issue occurs in combination with the race issue #1 described above, this backup may not be successfully removed either. So, to fix these issues: 1. Do the backup removal in the backup/save goroutine (at requests from the main goroutine), not directly in the main goroutine. 2. Make the communication between these goroutines fully synchronous: 2a. Instead of using the buffered channel backupRequestChan as a storage for pending requests for periodic backups, let the backup/save goroutine itself store this information, in the requestesBackups map. Then, backupRequestChan can be made non-buffered. 2b. Make saveRequestChan a non-buffered channel as well. (There was no point in making it buffered in the first place, actually.) Once both channels are non-buffered, the backup/save goroutine receives both backup and save requests from the main goroutine in exactly the same order as the main goroutine sends them, so we can guarantee that saving the buffer will cancel the previous pending backup request for this buffer. --- cmd/micro/micro.go | 2 -- internal/buffer/backup.go | 50 +++++++++++++++++++++++++++++---------- internal/buffer/buffer.go | 8 +------ internal/buffer/save.go | 23 +++++++++--------- 4 files changed, 50 insertions(+), 33 deletions(-) diff --git a/cmd/micro/micro.go b/cmd/micro/micro.go index 70b940bf..775301ee 100644 --- a/cmd/micro/micro.go +++ b/cmd/micro/micro.go @@ -489,8 +489,6 @@ func DoEvent() { } case f := <-timerChan: f() - case b := <-buffer.BackupCompleteChan: - b.RequestedBackup = false case <-sighup: exit(0) case <-util.Sigterm: diff --git a/internal/buffer/backup.go b/internal/buffer/backup.go index 941ac04f..8b728ec6 100644 --- a/internal/buffer/backup.go +++ b/internal/buffer/backup.go @@ -34,20 +34,49 @@ Options: [r]ecover, [i]gnore, [a]bort: ` const backupSeconds = 8 -var BackupCompleteChan chan *SharedBuffer +type backupRequestType int + +const ( + backupCreate = iota + backupRemove +) + +type backupRequest struct { + buf *SharedBuffer + reqType backupRequestType +} + +var requestedBackups map[*SharedBuffer]bool func init() { - BackupCompleteChan = make(chan *SharedBuffer, 10) + requestedBackups = make(map[*SharedBuffer]bool) } func (b *SharedBuffer) RequestBackup() { - if !b.RequestedBackup { - select { - case backupRequestChan <- b: - default: - // channel is full + backupRequestChan <- backupRequest{buf: b, reqType: backupCreate} +} + +func (b *SharedBuffer) CancelBackup() { + backupRequestChan <- backupRequest{buf: b, reqType: backupRemove} +} + +func handleBackupRequest(br backupRequest) { + switch br.reqType { + case backupCreate: + // schedule periodic backup + requestedBackups[br.buf] = true + case backupRemove: + br.buf.RemoveBackup() + delete(requestedBackups, br.buf) + } +} + +func periodicBackup() { + for buf := range requestedBackups { + err := buf.Backup() + if err == nil { + delete(requestedBackups, buf) } - b.RequestedBackup = true } } @@ -77,9 +106,6 @@ func (b *SharedBuffer) Backup() error { name := util.DetermineEscapePath(backupdir, b.AbsPath) if _, err := os.Stat(name); errors.Is(err, fs.ErrNotExist) { _, err = b.overwriteFile(name) - if err == nil { - BackupCompleteChan <- b - } return err } @@ -95,8 +121,6 @@ func (b *SharedBuffer) Backup() error { return err } - BackupCompleteChan <- b - return err } diff --git a/internal/buffer/buffer.go b/internal/buffer/buffer.go index 9bd7ecfa..bc33abfd 100644 --- a/internal/buffer/buffer.go +++ b/internal/buffer/buffer.go @@ -13,7 +13,6 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "time" luar "layeh.com/gopher-luar" @@ -101,7 +100,6 @@ type SharedBuffer struct { diffLock sync.RWMutex diff map[int]DiffStatus - RequestedBackup bool forceKeepBackup bool // ReloadDisabled allows the user to disable reloads if they @@ -123,8 +121,6 @@ type SharedBuffer struct { // Hash of the original buffer -- empty if fastdirty is on origHash [md5.Size]byte - - fini int32 } func (b *SharedBuffer) insert(pos Loc, value []byte) { @@ -495,13 +491,11 @@ func (b *Buffer) Fini() { if !b.Modified() { b.Serialize() } - b.RemoveBackup() + b.CancelBackup() if b.Type == BTStdout { fmt.Fprint(util.Stdout, string(b.Bytes())) } - - atomic.StoreInt32(&(b.fini), int32(1)) } // GetName returns the name that should be displayed in the statusline diff --git a/internal/buffer/save.go b/internal/buffer/save.go index aaab39ba..d2cd3533 100644 --- a/internal/buffer/save.go +++ b/internal/buffer/save.go @@ -11,7 +11,6 @@ import ( "os/signal" "path/filepath" "runtime" - "sync/atomic" "time" "unicode" @@ -47,11 +46,14 @@ type saveRequest struct { } var saveRequestChan chan saveRequest -var backupRequestChan chan *SharedBuffer +var backupRequestChan chan backupRequest func init() { - saveRequestChan = make(chan saveRequest, 10) - backupRequestChan = make(chan *SharedBuffer, 10) + // Both saveRequestChan and backupRequestChan need to be non-buffered + // so the save/backup goroutine receives both save and backup requests + // in the same order the main goroutine sends them. + saveRequestChan = make(chan saveRequest) + backupRequestChan = make(chan backupRequest) go func() { duration := backupSeconds * float64(time.Second) @@ -62,14 +64,10 @@ func init() { case sr := <-saveRequestChan: size, err := sr.buf.safeWrite(sr.path, sr.withSudo, sr.newFile) sr.saveResponseChan <- saveResponse{size, err} + case br := <-backupRequestChan: + handleBackupRequest(br) case <-backupTicker.C: - for len(backupRequestChan) > 0 { - b := <-backupRequestChan - bfini := atomic.LoadInt32(&(b.fini)) != 0 - if !bfini { - b.Backup() - } - } + periodicBackup() } } }() @@ -380,6 +378,9 @@ func (b *SharedBuffer) safeWrite(path string, withSudo bool, newFile bool) (int, return 0, err } + // Backup saved, so cancel pending periodic backup, if any + delete(requestedBackups, b) + b.forceKeepBackup = true size := 0 { From 7aa495fd3f824250e8e181247e6d79169b374c9d Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sun, 3 Aug 2025 16:17:53 +0200 Subject: [PATCH 5/9] Remove backup when buffer becomes unmodified We should cancel previously requested periodic backup (and remove this backup if it has already been created) not only when saving or closing the buffer but also in other cases when the buffer's state changes from modified to unmodified, i.e. when the user undoes all unsaved changes. Otherwise, if micro terminates abnormally before the buffer is closed, this backup will not get removed (so next time micro will suggest the user to recover this file), even though all changes to this file were successfully saved. --- internal/buffer/buffer.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/buffer/buffer.go b/internal/buffer/buffer.go index bc33abfd..72f6a8b8 100644 --- a/internal/buffer/buffer.go +++ b/internal/buffer/buffer.go @@ -152,6 +152,12 @@ func (b *SharedBuffer) setModified() { b.calcHash(&buff) b.isModified = buff != b.origHash } + + if b.isModified { + b.RequestBackup() + } else { + b.CancelBackup() + } } // calcHash calculates md5 hash of all lines in the buffer @@ -525,8 +531,6 @@ func (b *Buffer) Insert(start Loc, text string) { b.EventHandler.cursors = b.cursors b.EventHandler.active = b.curCursor b.EventHandler.Insert(start, text) - - b.RequestBackup() } } @@ -536,8 +540,6 @@ func (b *Buffer) Remove(start, end Loc) { b.EventHandler.cursors = b.cursors b.EventHandler.active = b.curCursor b.EventHandler.Remove(start, end) - - b.RequestBackup() } } From e127f08251cee5f96f84d4430e5d8406c3553317 Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sat, 2 Aug 2025 22:43:43 +0200 Subject: [PATCH 6/9] On panic, backup modified buffers only --- cmd/micro/micro.go | 6 ++++-- cmd/micro/micro_test.go | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/cmd/micro/micro.go b/cmd/micro/micro.go index 775301ee..159e646f 100644 --- a/cmd/micro/micro.go +++ b/cmd/micro/micro.go @@ -352,9 +352,11 @@ func main() { } else { fmt.Println("Micro encountered an error:", errors.Wrap(err, 2).ErrorStack(), "\nIf you can reproduce this error, please report it at https://github.com/zyedidia/micro/issues") } - // backup all open buffers + // immediately backup all buffers with unsaved changes for _, b := range buffer.OpenBuffers { - b.Backup() + if b.Modified() { + b.Backup() + } } exit(1) } diff --git a/cmd/micro/micro_test.go b/cmd/micro/micro_test.go index 31007cb1..7235bac1 100644 --- a/cmd/micro/micro_test.go +++ b/cmd/micro/micro_test.go @@ -55,9 +55,11 @@ func startup(args []string) (tcell.SimulationScreen, error) { if err := recover(); err != nil { screen.Screen.Fini() fmt.Println("Micro encountered an error:", err) - // backup all open buffers + // immediately backup all buffers with unsaved changes for _, b := range buffer.OpenBuffers { - b.Backup() + if b.Modified() { + b.Backup() + } } // Print the stack trace too log.Fatalf(errors.Wrap(err, 2).ErrorStack()) From 04b878bc2d1dae0c64cb62871f2a40df0565f2e9 Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sat, 2 Aug 2025 23:09:16 +0200 Subject: [PATCH 7/9] Ignore the `backup` option when removing backup When we need to remove existing backup for whatever reason (e.g. because we've just successfully saved the file), we should do that regardless of whether backups are enabled or not, since a backup may exist regardless (it could have been created before the `backup` option got disabled). --- internal/buffer/backup.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/buffer/backup.go b/internal/buffer/backup.go index 8b728ec6..9470c663 100644 --- a/internal/buffer/backup.go +++ b/internal/buffer/backup.go @@ -126,7 +126,7 @@ func (b *SharedBuffer) Backup() error { // RemoveBackup removes any backup file associated with this buffer func (b *SharedBuffer) RemoveBackup() { - if !b.Settings["backup"].(bool) || b.keepBackup() || b.Path == "" || b.Type != BTDefault { + if b.keepBackup() || b.Path == "" || b.Type != BTDefault { return } f := util.DetermineEscapePath(b.backupDir(), b.AbsPath) From a862c9709e0c328e7f754feae650dc98033ea71e Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sun, 3 Aug 2025 18:08:05 +0200 Subject: [PATCH 8/9] Unify backup write logic Use the same backup write helper function for both periodic background backups and for temporary backups in safeWrite(). Besides just removing code duplication, this brings the advantages of both together: - Temporary backups in safeWrite() now use the same atomic mechanism when replacing an already existing backup. So that if micro crashes in the middle of writing the backup in safeWrite(), this corrupted backup will not overwrite a previous good backup. - Better error handling for periodic backups. --- internal/buffer/backup.go | 64 ++++++++++++++++++++++++--------------- internal/buffer/save.go | 21 ------------- 2 files changed, 40 insertions(+), 45 deletions(-) diff --git a/internal/buffer/backup.go b/internal/buffer/backup.go index 9470c663..e93cb069 100644 --- a/internal/buffer/backup.go +++ b/internal/buffer/backup.go @@ -92,35 +92,51 @@ func (b *SharedBuffer) keepBackup() bool { return b.forceKeepBackup || b.Settings["permbackup"].(bool) } -// Backup saves the current buffer to the backups directory +func (b *SharedBuffer) writeBackup(path string) (string, error) { + backupdir := b.backupDir() + if _, err := os.Stat(backupdir); err != nil { + if !errors.Is(err, fs.ErrNotExist) { + return "", err + } + if err = os.Mkdir(backupdir, os.ModePerm); err != nil { + return "", err + } + } + + name := util.DetermineEscapePath(backupdir, path) + + // If no existing backup, just write the backup. + if _, err := os.Stat(name); errors.Is(err, fs.ErrNotExist) { + _, err = b.overwriteFile(name) + if err != nil { + os.Remove(name) + } + return name, err + } + + // If a backup already exists, replace it atomically. + tmp := util.AppendBackupSuffix(name) + _, err := b.overwriteFile(tmp) + if err != nil { + os.Remove(tmp) + return name, err + } + err = os.Rename(tmp, name) + if err != nil { + os.Remove(tmp) + return name, err + } + + return name, nil +} + +// Backup saves the buffer to the backups directory func (b *SharedBuffer) Backup() error { if !b.Settings["backup"].(bool) || b.Path == "" || b.Type != BTDefault { return nil } - backupdir := b.backupDir() - if _, err := os.Stat(backupdir); errors.Is(err, fs.ErrNotExist) { - os.Mkdir(backupdir, os.ModePerm) - } - - name := util.DetermineEscapePath(backupdir, b.AbsPath) - if _, err := os.Stat(name); errors.Is(err, fs.ErrNotExist) { - _, err = b.overwriteFile(name) - return err - } - - tmp := util.AppendBackupSuffix(name) - _, err := b.overwriteFile(tmp) - if err != nil { - os.Remove(tmp) - return err - } - err = os.Rename(tmp, name) - if err != nil { - os.Remove(tmp) - return err - } - + _, err := b.writeBackup(b.AbsPath) return err } diff --git a/internal/buffer/save.go b/internal/buffer/save.go index d2cd3533..7d943929 100644 --- a/internal/buffer/save.go +++ b/internal/buffer/save.go @@ -333,27 +333,6 @@ func (b *Buffer) saveToFile(filename string, withSudo bool, autoSave bool) error return err } -func (b *SharedBuffer) writeBackup(path string) (string, error) { - backupDir := b.backupDir() - if _, err := os.Stat(backupDir); err != nil { - if !errors.Is(err, fs.ErrNotExist) { - return "", err - } - if err = os.Mkdir(backupDir, os.ModePerm); err != nil { - return "", err - } - } - - backupName := util.DetermineEscapePath(backupDir, path) - _, err := b.overwriteFile(backupName) - if err != nil { - os.Remove(backupName) - return "", err - } - - return backupName, nil -} - // safeWrite writes the buffer to a file in a "safe" way, preventing loss of the // contents of the file if it fails to write the new contents. // This means that the file is not overwritten directly but by writing to the From 0a9fa4f2eaeb4f8d9689a6509dc5ab4a1c0959b9 Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sun, 3 Aug 2025 18:41:34 +0200 Subject: [PATCH 9/9] Always use temporary file when writing backup When writing a backup file, we should write it atomically (i.e. use a temporary file + rename) in all cases, not only when replacing an existing backup. Just like we do in util.SafeWrite(). Otherwise, if micro crashes while writing this backup, even if that doesn't result in corrupting an existing good backup, it still results in creating an undesired backup with invalid contents. --- internal/buffer/backup.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/internal/buffer/backup.go b/internal/buffer/backup.go index e93cb069..9920897d 100644 --- a/internal/buffer/backup.go +++ b/internal/buffer/backup.go @@ -104,18 +104,8 @@ func (b *SharedBuffer) writeBackup(path string) (string, error) { } name := util.DetermineEscapePath(backupdir, path) - - // If no existing backup, just write the backup. - if _, err := os.Stat(name); errors.Is(err, fs.ErrNotExist) { - _, err = b.overwriteFile(name) - if err != nil { - os.Remove(name) - } - return name, err - } - - // If a backup already exists, replace it atomically. tmp := util.AppendBackupSuffix(name) + _, err := b.overwriteFile(tmp) if err != nil { os.Remove(tmp)