Merge pull request #1074 from syncthing/fix-1071

Handle symlinks in versioning (fixes #1071)

internal/versioner/simple.go

@@ -55,7 +55,7 @@ func NewSimple(folderID, folderPath string, params map[string]string) Versioner
 // Move away the named file to a version archive. If this function returns
 // nil, the named file does not exist any more (has been archived).
 func (v Simple) Archive(filePath string) error {
-	fileInfo, err := os.Stat(filePath)
+	fileInfo, err := os.Lstat(filePath)
 	if err != nil {
 		if os.IsNotExist(err) {
 			if debug {

internal/versioner/staggered.go

@@ -45,16 +45,6 @@ type Staggered struct {
 	mutex         *sync.Mutex
 }
 
-// Check if file or dir
-func isFile(path string) bool {
-	fileInfo, err := os.Stat(path)
-	if err != nil {
-		l.Infoln("versioner isFile:", err)
-		return false
-	}
-	return fileInfo.Mode().IsRegular()
-}
-
 // Rename versions with old version format
 func (v Staggered) renameOld() {
 	err := filepath.Walk(v.versionsPath, func(path string, f os.FileInfo, err error) error {
@@ -167,14 +157,16 @@ func (v Staggered) clean() {
 		if err != nil {
 			return err
 		}
-		switch mode := f.Mode(); {
+
-		case mode.IsDir():
+		if f.Mode().IsDir() && f.Mode()&os.ModeSymlink == 0 {
 			filesPerDir[path] = 0
 			if path != v.versionsPath {
 				dir := filepath.Dir(path)
 				filesPerDir[dir]++
 			}
-		case mode.IsRegular():
+		} else {
+			// Regular file, or possibly a symlink.
+
 			extension := filenameTag(path)
 			dir := filepath.Dir(path)
 			name := path[:len(path)-len(extension)-1]
@@ -227,56 +219,63 @@ func (v Staggered) expire(versions []string) {
 	var prevAge int64
 	firstFile := true
 	for _, file := range versions {
-		if isFile(file) {
+		fi, err := os.Stat(file)
-			versionTime, err := time.Parse(TimeFormat, filenameTag(file))
+		if err != nil {
-			if err != nil {
+			l.Warnln("versioner:", err)
-				l.Infof("Versioner: file name %q is invalid: %v", file, err)
+			continue
-				continue
-			}
-			age := int64(time.Since(versionTime).Seconds())
-
-			// If the file is older than the max age of the last interval, remove it
-			if lastIntv := v.interval[len(v.interval)-1]; lastIntv.end > 0 && age > lastIntv.end {
-				if debug {
-					l.Debugln("Versioner: File over maximum age -> delete ", file)
-				}
-				err = os.Remove(file)
-				if err != nil {
-					l.Warnf("Versioner: can't remove %q: %v", file, err)
-				}
-				continue
-			}
-
-			// If it's the first (oldest) file in the list we can skip the interval checks
-			if firstFile {
-				prevAge = age
-				firstFile = false
-				continue
-			}
-
-			// Find the interval the file fits in
-			var usedInterval Interval
-			for _, usedInterval = range v.interval {
-				if age < usedInterval.end {
-					break
-				}
-			}
-
-			if prevAge-age < usedInterval.step {
-				if debug {
-					l.Debugln("too many files in step -> delete", file)
-				}
-				err = os.Remove(file)
-				if err != nil {
-					l.Warnf("Versioner: can't remove %q: %v", file, err)
-				}
-				continue
-			}
-
-			prevAge = age
-		} else {
-			l.Infof("non-file %q is named like a file version", file)
 		}
+
+		if fi.IsDir() {
+			l.Infof("non-file %q is named like a file version", file)
+			continue
+		}
+
+		versionTime, err := time.Parse(TimeFormat, filenameTag(file))
+		if err != nil {
+			l.Infof("Versioner: file name %q is invalid: %v", file, err)
+			continue
+		}
+		age := int64(time.Since(versionTime).Seconds())
+
+		// If the file is older than the max age of the last interval, remove it
+		if lastIntv := v.interval[len(v.interval)-1]; lastIntv.end > 0 && age > lastIntv.end {
+			if debug {
+				l.Debugln("Versioner: File over maximum age -> delete ", file)
+			}
+			err = os.Remove(file)
+			if err != nil {
+				l.Warnf("Versioner: can't remove %q: %v", file, err)
+			}
+			continue
+		}
+
+		// If it's the first (oldest) file in the list we can skip the interval checks
+		if firstFile {
+			prevAge = age
+			firstFile = false
+			continue
+		}
+
+		// Find the interval the file fits in
+		var usedInterval Interval
+		for _, usedInterval = range v.interval {
+			if age < usedInterval.end {
+				break
+			}
+		}
+
+		if prevAge-age < usedInterval.step {
+			if debug {
+				l.Debugln("too many files in step -> delete", file)
+			}
+			err = os.Remove(file)
+			if err != nil {
+				l.Warnf("Versioner: can't remove %q: %v", file, err)
+			}
+			continue
+		}
+
+		prevAge = age
 	}
 }
 
@@ -289,7 +288,7 @@ func (v Staggered) Archive(filePath string) error {
 	v.mutex.Lock()
 	defer v.mutex.Unlock()
 
-	if _, err := os.Stat(filePath); err != nil {
+	if _, err := os.Lstat(filePath); err != nil {
 		if os.IsNotExist(err) {
 			if debug {
 				l.Debugln("not archiving nonexistent file", filePath)

test/common_test.go

@@ -379,9 +379,12 @@ func startWalker(dir string, res chan<- fileInfo, abort <-chan struct{}) {
 		if rn == "." || rn == ".stfolder" {
 			return nil
 		}
+		if rn == ".stversions" {
+			return filepath.SkipDir
+		}
 
 		var f fileInfo
-		if ok, err := symlinks.IsSymlink(path); err == nil && ok {
+		if info.Mode()&os.ModeSymlink != 0 {
 			f = fileInfo{
 				name: rn,
 				mode: os.ModeSymlink,

test/filetype_test.go

@@ -24,9 +24,53 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/syncthing/syncthing/internal/config"
+	"github.com/syncthing/syncthing/internal/protocol"
 )
 
-func TestFiletypeChange(t *testing.T) {
+func TestFileTypeChange(t *testing.T) {
+	// Use no versioning
+	id, _ := protocol.DeviceIDFromString(id2)
+	cfg, _ := config.Load("h2/config.xml", id)
+	fld := cfg.Folders()["default"]
+	fld.Versioning = config.VersioningConfiguration{}
+	cfg.SetFolder(fld)
+	cfg.Save()
+
+	testFileTypeChange(t)
+}
+
+func TestFileTypeChangeSimpleVersioning(t *testing.T) {
+	// Use simple versioning
+	id, _ := protocol.DeviceIDFromString(id2)
+	cfg, _ := config.Load("h2/config.xml", id)
+	fld := cfg.Folders()["default"]
+	fld.Versioning = config.VersioningConfiguration{
+		Type:   "simple",
+		Params: map[string]string{"keep": "5"},
+	}
+	cfg.SetFolder(fld)
+	cfg.Save()
+
+	testFileTypeChange(t)
+}
+
+func TestFileTypeChangeStaggeredVersioning(t *testing.T) {
+	// Use staggered versioning
+	id, _ := protocol.DeviceIDFromString(id2)
+	cfg, _ := config.Load("h2/config.xml", id)
+	fld := cfg.Folders()["default"]
+	fld.Versioning = config.VersioningConfiguration{
+		Type: "staggered",
+	}
+	cfg.SetFolder(fld)
+	cfg.Save()
+
+	testFileTypeChange(t)
+}
+
+func testFileTypeChange(t *testing.T) {
 	log.Println("Cleaning...")
 	err := removeAll("s1", "s2", "h1/index", "h2/index")
 	if err != nil {

test/symlink_test.go

@@ -24,10 +24,53 @@ import (
 	"testing"
 	"time"
 
+	"github.com/syncthing/syncthing/internal/config"
+	"github.com/syncthing/syncthing/internal/protocol"
 	"github.com/syncthing/syncthing/internal/symlinks"
 )
 
 func TestSymlinks(t *testing.T) {
+	// Use no versioning
+	id, _ := protocol.DeviceIDFromString(id2)
+	cfg, _ := config.Load("h2/config.xml", id)
+	fld := cfg.Folders()["default"]
+	fld.Versioning = config.VersioningConfiguration{}
+	cfg.SetFolder(fld)
+	cfg.Save()
+
+	testSymlinks(t)
+}
+
+func TestSymlinksSimpleVersioning(t *testing.T) {
+	// Use no versioning
+	id, _ := protocol.DeviceIDFromString(id2)
+	cfg, _ := config.Load("h2/config.xml", id)
+	fld := cfg.Folders()["default"]
+	fld.Versioning = config.VersioningConfiguration{
+		Type:   "simple",
+		Params: map[string]string{"keep": "5"},
+	}
+	cfg.SetFolder(fld)
+	cfg.Save()
+
+	testSymlinks(t)
+}
+
+func TestSymlinksStaggeredVersioning(t *testing.T) {
+	// Use no versioning
+	id, _ := protocol.DeviceIDFromString(id2)
+	cfg, _ := config.Load("h2/config.xml", id)
+	fld := cfg.Folders()["default"]
+	fld.Versioning = config.VersioningConfiguration{
+		Type: "staggered",
+	}
+	cfg.SetFolder(fld)
+	cfg.Save()
+
+	testSymlinks(t)
+}
+
+func testSymlinks(t *testing.T) {
 	log.Println("Cleaning...")
 	err := removeAll("s1", "s2", "h1/index", "h2/index")
 	if err != nil {
@@ -236,7 +279,7 @@ func TestSymlinks(t *testing.T) {
 		log.Fatal(err)
 	}
 
-	// Remove a symlink
+	// Remove a broken symlink
 
 	err = os.Remove("s1/removeLink")
 	if err != nil {