lib/ignore: Centralize handling of temporary filenames (fixes #3899)

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3901
LGTM: calmh, AudriusButkevicius

lib/ignore/ignore.go

@@ -249,6 +249,22 @@ func (m *Matcher) clean(d time.Duration) {
 	}
 }
 
+// ShouldIgnore returns true when a file is temporary, internal or ignored
+func (m *Matcher) ShouldIgnore(filename string) bool {
+	switch {
+	case IsTemporary(filename):
+		return true
+
+	case IsInternal(filename):
+		return true
+
+	case m.Match(filename).IsIgnored():
+		return true
+	}
+
+	return false
+}
+
 func hashPatterns(patterns []Pattern) string {
 	h := md5.New()
 	for _, pat := range patterns {

lib/model/tempname.go → lib/ignore/tempname.go

@@ -4,7 +4,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this file,
 // You can obtain one at http://mozilla.org/MPL/2.0/.
 
-package model
+package ignore
 
 import (
 	"crypto/md5"
@@ -14,45 +14,39 @@ import (
 	"strings"
 )
 
-type tempNamer struct {
-	prefix    string
-	recognize []string
-}
-
 const (
-	windowsTempPrefix = "~syncthing~"
-	unixTempPrefix    = ".syncthing."
+	WindowsTempPrefix = "~syncthing~"
+	UnixTempPrefix    = ".syncthing."
 )
 
-var defTempNamer tempNamer
+var TempPrefix string
 
 // Real filesystems usually handle 255 bytes. encfs has varying and
 // confusing file name limits. We take a safe way out and switch to hashing
 // quite early.
-const maxFilenameLength = 160 - len(unixTempPrefix) - len(".tmp")
+const maxFilenameLength = 160 - len(UnixTempPrefix) - len(".tmp")
 
 func init() {
 	if runtime.GOOS == "windows" {
-		defTempNamer = tempNamer{windowsTempPrefix, []string{unixTempPrefix, windowsTempPrefix}}
+		TempPrefix = WindowsTempPrefix
 	} else {
-		defTempNamer = tempNamer{unixTempPrefix, []string{unixTempPrefix, windowsTempPrefix}}
+		TempPrefix = UnixTempPrefix
 	}
 }
 
 // IsTemporary is true if the file name has the temporary prefix. Regardless
 // of the normally used prefix, the standard Windows and Unix temp prefixes
 // are always recognized as temp files.
-func (t tempNamer) IsTemporary(name string) bool {
+func IsTemporary(name string) bool {
 	name = filepath.Base(name)
-	for _, prefix := range t.recognize {
-		if strings.HasPrefix(name, prefix) {
-			return true
-		}
+	if strings.HasPrefix(name, WindowsTempPrefix) ||
+		strings.HasPrefix(name, UnixTempPrefix) {
+		return true
 	}
 	return false
 }
 
-func (t tempNamer) TempName(name string) string {
+func TempName(name string) string {
 	tdir := filepath.Dir(name)
 	tbase := filepath.Base(name)
 	if len(tbase) > maxFilenameLength {
@@ -60,6 +54,6 @@ func (t tempNamer) TempName(name string) string {
 		hash.Write([]byte(name))
 		tbase = fmt.Sprintf("%x", hash.Sum(nil))
 	}
-	tname := fmt.Sprintf("%s%s.tmp", t.prefix, tbase)
+	tname := fmt.Sprintf("%s%s.tmp", TempPrefix, tbase)
 	return filepath.Join(tdir, tname)
 }

lib/model/tempname_test.go → lib/ignore/tempname_test.go

@@ -4,7 +4,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this file,
 // You can obtain one at http://mozilla.org/MPL/2.0/.
 
-package model
+package ignore
 
 import (
 	"strings"
@@ -16,11 +16,11 @@ func TestLongTempFilename(t *testing.T) {
 	for i := 0; i < 300; i++ {
 		filename += "l"
 	}
-	tFile := defTempNamer.TempName(filename)
+	tFile := TempName(filename)
 	if len(tFile) < 10 || len(tFile) > 200 {
 		t.Fatal("Invalid long filename")
 	}
-	if !strings.HasSuffix(defTempNamer.TempName("short"), "short.tmp") {
-		t.Fatal("Invalid short filename", defTempNamer.TempName("short"))
+	if !strings.HasSuffix(TempName("short"), "short.tmp") {
+		t.Fatal("Invalid short filename", TempName("short"))
 	}
 }

lib/model/model.go

@@ -594,7 +594,7 @@ func (m *Model) NeedSize(folder string) db.Counts {
 		ignores := m.folderIgnores[folder]
 		cfg := m.folderCfgs[folder]
 		rf.WithNeedTruncated(protocol.LocalDeviceID, func(f db.FileIntf) bool {
-			if shouldIgnore(f, ignores, cfg.IgnoreDelete, defTempNamer) {
+			if shouldIgnore(f, ignores, cfg.IgnoreDelete) {
 				return true
 			}
 
@@ -658,7 +658,7 @@ func (m *Model) NeedFolderFiles(folder string, page, perpage int) ([]db.FileInfo
 	ignores := m.folderIgnores[folder]
 	cfg := m.folderCfgs[folder]
 	rf.WithNeedTruncated(protocol.LocalDeviceID, func(f db.FileIntf) bool {
-		if shouldIgnore(f, ignores, cfg.IgnoreDelete, defTempNamer) {
+		if shouldIgnore(f, ignores, cfg.IgnoreDelete) {
 			return true
 		}
 
@@ -1166,7 +1166,7 @@ func (m *Model) Request(deviceID protocol.DeviceID, folder, name string, offset
 	// Only check temp files if the flag is set, and if we are set to advertise
 	// the temp indexes.
 	if fromTemporary && !folderCfg.DisableTempIndexes {
-		tempFn := filepath.Join(folderPath, defTempNamer.TempName(name))
+		tempFn := filepath.Join(folderPath, ignore.TempName(name))
 
 		if info, err := osutil.Lstat(tempFn); err != nil || !info.Mode().IsRegular() {
 			// Reject reads for anything that doesn't exist or is something
@@ -1804,7 +1804,6 @@ func (m *Model) internalScanFolderSubdirs(folder string, subDirs []string) error
 		Subs:                  subDirs,
 		Matcher:               ignores,
 		BlockSize:             protocol.BlockSize,
-		TempNamer:             defTempNamer,
 		TempLifetime:          time.Duration(m.cfg.Options().KeepTemporariesH) * time.Hour,
 		CurrentFiler:          cFiler{m, folder},
 		Lstater:               mtimefs,
@@ -2625,7 +2624,7 @@ func makeForgetUpdate(files []protocol.FileInfo) []protocol.FileDownloadProgress
 }
 
 // shouldIgnore returns true when a file should be excluded from processing
-func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool, tmpNamer tempNamer) bool {
+func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool) bool {
 	switch {
 	case ignoreDelete && file.IsDeleted():
 		// ignoreDelete first because it's a very cheap test so a win if it
@@ -2633,13 +2632,7 @@ func shouldIgnore(file db.FileIntf, matcher *ignore.Matcher, ignoreDelete bool,
 		// deleted files.
 		return true
 
-	case tmpNamer.IsTemporary(file.FileName()):
-		return true
-
-	case ignore.IsInternal(file.FileName()):
-		return true
-
-	case matcher.Match(file.FileName()).IsIgnored():
+	case matcher.ShouldIgnore(file.FileName()):
 		return true
 	}
 

lib/model/rwfolder.go

@@ -376,7 +376,7 @@ func (f *sendReceiveFolder) pullerIteration(ignores *ignore.Matcher) int {
 	// pile.
 
 	folderFiles.WithNeed(protocol.LocalDeviceID, func(intf db.FileIntf) bool {
-		if shouldIgnore(intf, ignores, f.IgnoreDelete, defTempNamer) {
+		if shouldIgnore(intf, ignores, f.IgnoreDelete) {
 			return true
 		}
 
@@ -795,7 +795,8 @@ func (f *sendReceiveFolder) deleteDir(file protocol.FileInfo, matcher *ignore.Ma
 		files, _ := dir.Readdirnames(-1)
 		for _, dirFile := range files {
 			fullDirFile := filepath.Join(file.Name, dirFile)
-			if defTempNamer.IsTemporary(dirFile) || (matcher != nil && matcher.Match(fullDirFile).IsDeletable()) {
+			if ignore.IsTemporary(dirFile) || (matcher != nil &&
+				matcher.Match(fullDirFile).IsDeletable()) {
 				os.RemoveAll(filepath.Join(f.dir, fullDirFile))
 			}
 		}
@@ -1041,7 +1042,7 @@ func (f *sendReceiveFolder) handleFile(file protocol.FileInfo, copyChan chan<- c
 	}
 
 	// Figure out the absolute filenames we need once and for all
-	tempName, err := rootedJoinedPath(f.dir, defTempNamer.TempName(file.Name))
+	tempName, err := rootedJoinedPath(f.dir, ignore.TempName(file.Name))
 	if err != nil {
 		f.newError(file.Name, err)
 		return

lib/model/rwfolder_test.go

@@ -16,21 +16,29 @@ import (
 
 	"github.com/syncthing/syncthing/lib/db"
 	"github.com/syncthing/syncthing/lib/fs"
+	"github.com/syncthing/syncthing/lib/ignore"
 	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/scanner"
 	"github.com/syncthing/syncthing/lib/sync"
 )
 
-func init() {
+func TestMain(m *testing.M) {
 	// We do this to make sure that the temp file required for the tests
 	// does not get removed during the tests. Also set the prefix so it's
 	// found correctly regardless of platform.
-	defTempNamer.prefix = windowsTempPrefix
+	if ignore.TempPrefix != ignore.WindowsTempPrefix {
+		originalPrefix := ignore.TempPrefix
+		ignore.TempPrefix = ignore.WindowsTempPrefix
+		defer func() {
+			ignore.TempPrefix = originalPrefix
+		}()
+	}
 	future := time.Now().Add(time.Hour)
-	err := os.Chtimes(filepath.Join("testdata", defTempNamer.TempName("file")), future, future)
+	err := os.Chtimes(filepath.Join("testdata", ignore.TempName("file")), future, future)
 	if err != nil {
 		panic(err)
 	}
+	os.Exit(m.Run())
 }
 
 var blocks = []protocol.BlockInfo{
@@ -176,14 +184,14 @@ func TestCopierFinder(t *testing.T) {
 	// After dropping out blocks found locally:
 	// Pull: 1, 5, 6, 8
 
-	tempFile := filepath.Join("testdata", defTempNamer.TempName("file2"))
+	tempFile := filepath.Join("testdata", ignore.TempName("file2"))
 	err := os.Remove(tempFile)
 	if err != nil && !os.IsNotExist(err) {
 		t.Error(err)
 	}
 
 	existingBlocks := []int{0, 2, 3, 4, 0, 0, 7, 0}
-	existingFile := setUpFile(defTempNamer.TempName("file"), existingBlocks)
+	existingFile := setUpFile(ignore.TempName("file"), existingBlocks)
 	requiredFile := existingFile
 	requiredFile.Blocks = blocks[1:]
 	requiredFile.Name = "file2"
@@ -246,7 +254,7 @@ func TestCopierFinder(t *testing.T) {
 }
 
 func TestWeakHash(t *testing.T) {
-	tempFile := filepath.Join("testdata", defTempNamer.TempName("weakhash"))
+	tempFile := filepath.Join("testdata", ignore.TempName("weakhash"))
 	var shift int64 = 10
 	var size int64 = 1 << 20
 	expectBlocks := int(size / protocol.BlockSize)
@@ -451,12 +459,12 @@ func TestLastResortPulling(t *testing.T) {
 	}
 
 	(<-finisherChan).fd.Close()
-	os.Remove(filepath.Join("testdata", defTempNamer.TempName("newfile")))
+	os.Remove(filepath.Join("testdata", ignore.TempName("newfile")))
 }
 
 func TestDeregisterOnFailInCopy(t *testing.T) {
 	file := setUpFile("filex", []int{0, 2, 0, 0, 5, 0, 0, 8})
-	defer os.Remove("testdata/" + defTempNamer.TempName("filex"))
+	defer os.Remove("testdata/" + ignore.TempName("filex"))
 
 	db := db.OpenMemory()
 
@@ -530,7 +538,7 @@ func TestDeregisterOnFailInCopy(t *testing.T) {
 
 func TestDeregisterOnFailInPull(t *testing.T) {
 	file := setUpFile("filex", []int{0, 2, 0, 0, 5, 0, 0, 8})
-	defer os.Remove("testdata/" + defTempNamer.TempName("filex"))
+	defer os.Remove("testdata/" + ignore.TempName("filex"))
 
 	db := db.OpenMemory()
 	m := NewModel(defaultConfig, protocol.LocalDeviceID, "device", "syncthing", "dev", db, nil)

lib/scanner/walk.go

@@ -50,8 +50,6 @@ type Config struct {
 	BlockSize int
 	// If Matcher is not nil, it is used to identify files to ignore which were specified by the user.
 	Matcher *ignore.Matcher
-	// If TempNamer is not nil, it is used to ignore temporary files when walking.
-	TempNamer TempNamer
 	// Number of hours to keep temporary files for
 	TempLifetime time.Duration
 	// If CurrentFiler is not nil, it is queried for the current file before rescanning.
@@ -76,11 +74,6 @@ type Config struct {
 	Cancel chan struct{}
 }
 
-type TempNamer interface {
-	// IsTemporary returns true if path refers to the name of temporary file.
-	IsTemporary(path string) bool
-}
-
 type CurrentFiler interface {
 	// CurrentFile returns the file as seen at last scan.
 	CurrentFile(name string) (protocol.FileInfo, bool)
@@ -96,9 +89,6 @@ func Walk(cfg Config) (chan protocol.FileInfo, error) {
 	if w.CurrentFiler == nil {
 		w.CurrentFiler = noCurrentFiler{}
 	}
-	if w.TempNamer == nil {
-		w.TempNamer = noTempNamer{}
-	}
 	if w.Lstater == nil {
 		w.Lstater = defaultLstater{}
 	}
@@ -247,7 +237,7 @@ func (w *walker) walkAndHashFiles(fchan, dchan chan protocol.FileInfo) filepath.
 			return skip
 		}
 
-		if w.TempNamer.IsTemporary(relPath) {
+		if ignore.IsTemporary(relPath) {
 			l.Debugln("temporary:", relPath)
 			if info.Mode().IsRegular() && info.ModTime().Add(w.TempLifetime).Before(now) {
 				os.Remove(absPath)
@@ -582,14 +572,6 @@ func (noCurrentFiler) CurrentFile(name string) (protocol.FileInfo, bool) {
 	return protocol.FileInfo{}, false
 }
 
-// A no-op TempNamer
-
-type noTempNamer struct{}
-
-func (noTempNamer) IsTemporary(path string) bool {
-	return false
-}
-
 // A no-op Lstater
 
 type defaultLstater struct{}