Merge pull request #3822 from dmaluka/fix-spurious-backups

Fix spurious backups of unmodified files

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)
 		}
@@ -489,8 +491,6 @@ func DoEvent() {
 		}
 	case f := <-timerChan:
 		f()
-	case b := <-buffer.BackupCompleteChan:
-		b.RequestedBackup = false
 	case <-sighup:
 		exit(0)
 	case <-util.Sigterm:

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())

internal/buffer/backup.go

@@ -34,24 +34,53 @@ Options: [r]ecover, [i]gnore, [a]bort: `
 
 const backupSeconds = 8
 
-var BackupCompleteChan chan *Buffer
+type backupRequestType int
 
-func init() {
-	BackupCompleteChan = make(chan *Buffer, 10)
+const (
+	backupCreate = iota
+	backupRemove
+)
+
+type backupRequest struct {
+	buf     *SharedBuffer
+	reqType backupRequestType
 }
 
-func (b *Buffer) RequestBackup() {
-	if !b.RequestedBackup {
-		select {
-		case backupRequestChan <- b:
-		default:
-			// channel is full
-		}
-		b.RequestedBackup = true
+var requestedBackups map[*SharedBuffer]bool
+
+func init() {
+	requestedBackups = make(map[*SharedBuffer]bool)
+}
+
+func (b *SharedBuffer) RequestBackup() {
+	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 (b *Buffer) backupDir() string {
+func periodicBackup() {
+	for buf := range requestedBackups {
+		err := buf.Backup()
+		if err == nil {
+			delete(requestedBackups, buf)
+		}
+	}
+}
+
+func (b *SharedBuffer) backupDir() string {
 	backupdir, err := util.ReplaceHome(b.Settings["backupdir"].(string))
 	if backupdir == "" || err != nil {
 		backupdir = filepath.Join(config.ConfigDir, "backups")
@@ -59,50 +88,51 @@ 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 {
-	if !b.Settings["backup"].(bool) || b.Path == "" || b.Type != BTDefault {
-		return nil
-	}
-
+func (b *SharedBuffer) writeBackup(path string) (string, error) {
 	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)
-		if err == nil {
-			BackupCompleteChan <- b
+	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
 		}
-		return err
 	}
 
+	name := util.DetermineEscapePath(backupdir, path)
 	tmp := util.AppendBackupSuffix(name)
+
 	_, err := b.overwriteFile(tmp)
 	if err != nil {
 		os.Remove(tmp)
-		return err
+		return name, err
 	}
 	err = os.Rename(tmp, name)
 	if err != nil {
 		os.Remove(tmp)
-		return err
+		return name, err
 	}
 
-	BackupCompleteChan <- b
+	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
+	}
+
+	_, err := b.writeBackup(b.AbsPath)
 	return err
 }
 
 // RemoveBackup removes any backup file associated with this buffer
-func (b *Buffer) RemoveBackup() {
-	if !b.Settings["backup"].(bool) || b.keepBackup() || b.Path == "" || b.Type != BTDefault {
+func (b *SharedBuffer) RemoveBackup() {
+	if b.keepBackup() || b.Path == "" || b.Type != BTDefault {
 		return
 	}
 	f := util.DetermineEscapePath(b.backupDir(), b.AbsPath)
@@ -111,7 +141,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 {
@@ -125,7 +155,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

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
@@ -126,20 +124,62 @@ 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
+	}
+
+	if b.isModified {
+		b.RequestBackup()
+	} else {
+		b.CancelBackup()
+	}
+}
+
+// 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) {
@@ -187,7 +227,6 @@ type Buffer struct {
 	*EventHandler
 	*SharedBuffer
 
-	fini        int32
 	cursors     []*Cursor
 	curCursor   int
 	StartCursor Loc
@@ -416,7 +455,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)
 		}
 	}
 
@@ -458,13 +497,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
@@ -494,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()
 	}
 }
 
@@ -505,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()
 	}
 }
 
@@ -558,7 +591,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
@@ -633,18 +666,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
-
-	calcHash(b, &buff)
-	return buff != b.origHash
+	return b.isModified
 }
 
 // Size returns the number of bytes in the current buffer
@@ -663,26 +685,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 {
@@ -1233,7 +1235,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 +1255,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)

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 *Buffer
+var backupRequestChan chan backupRequest
 
 func init() {
-	saveRequestChan = make(chan saveRequest, 10)
-	backupRequestChan = make(chan *Buffer, 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()
 			}
 		}
 	}()
@@ -116,7 +114,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 +182,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
@@ -206,9 +204,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)
@@ -318,7 +314,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)
 		}
 	}
 
@@ -337,32 +333,11 @@ func (b *Buffer) saveToFile(filename string, withSudo bool, autoSave bool) error
 	return err
 }
 
-func (b *Buffer) 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
 // 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
@@ -382,6 +357,9 @@ func (b *Buffer) safeWrite(path string, withSudo bool, newFile bool) (int, error
 		return 0, err
 	}
 
+	// Backup saved, so cancel pending periodic backup, if any
+	delete(requestedBackups, b)
+
 	b.forceKeepBackup = true
 	size := 0
 	{

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{}
@@ -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" {