lib/versioner: Revert naming change (fixes #5807) (#5808)

lib/model/model_test.go

@@ -2852,9 +2852,10 @@ func TestVersionRestore(t *testing.T) {
 	defer cleanupModel(m)
 	m.ScanFolder("default")
 
-	sentinel, err := time.ParseInLocation(versioner.TimeFormat, "20200101-010101", time.Local)
-	must(t, err)
-	sentinelTag := sentinel.Format(versioner.TimeFormat)
+	sentinel, err := time.ParseInLocation(versioner.TimeFormat, "20180101-010101", time.Local)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	for _, file := range []string{
 		// Versions directory
@@ -2866,7 +2867,6 @@ func TestVersionRestore(t *testing.T) {
 		".stversions/dir/file~20171210-040406.txt",
 		".stversions/very/very/deep/one~20171210-040406.txt", // lives deep down, no directory exists.
 		".stversions/dir/existing~20171210-040406.txt",       // exists, should expect to be archived.
-		".stversions/dir/file.txt~20171210-040405",           // old tag format, supported
 		".stversions/dir/cat",                                // untagged which was used by trashcan, supported
 
 		// "file.txt" will be restored
@@ -2897,7 +2897,7 @@ func TestVersionRestore(t *testing.T) {
 		"file.txt":               1,
 		"existing":               1,
 		"something":              1,
-		"dir/file.txt":           4,
+		"dir/file.txt":           3,
 		"dir/existing.txt":       1,
 		"very/very/deep/one.txt": 1,
 		"dir/cat":                1,
@@ -2942,6 +2942,8 @@ func TestVersionRestore(t *testing.T) {
 		"very/very/deep/one.txt": makeTime("20171210-040406"),
 	}
 
+	beforeRestore := time.Now().Truncate(time.Second)
+
 	ferr, err := m.RestoreFolderVersions("default", restore)
 	must(t, err)
 
@@ -2977,51 +2979,48 @@ func TestVersionRestore(t *testing.T) {
 		}
 	}
 
-	// Simple versioner uses modtime for timestamp generation, so we can check
-	// if existing stuff was correctly archived as we restored.
+	// Simple versioner uses now for timestamp generation, so we can check
+	// if existing stuff was correctly archived as we restored (oppose to deleteD), and version time as after beforeRestore
 	expectArchived := map[string]struct{}{
 		"existing":         {},
 		"dir/file.txt":     {},
 		"dir/existing.txt": {},
 	}
 
-	// Even if they are at the archived path, content should have the non
-	// archived name.
-	for file := range expectArchived {
+	allFileVersions, err := m.GetFolderVersions("default")
+	must(t, err)
+	for file, versions := range allFileVersions {
+		key := file
 		if runtime.GOOS == "windows" {
 			file = filepath.FromSlash(file)
 		}
-		taggedName := versioner.TagFilename(file, sentinelTag)
-		taggedArchivedName := filepath.Join(".stversions", taggedName)
+		for _, version := range versions {
+			if version.VersionTime.Equal(beforeRestore) || version.VersionTime.After(beforeRestore) {
+				fd, err := filesystem.Open(".stversions/" + versioner.TagFilename(file, version.VersionTime.Format(versioner.TimeFormat)))
+				must(t, err)
+				defer fd.Close()
 
-		fd, err := filesystem.Open(taggedArchivedName)
-		must(t, err)
-		defer fd.Close()
-
-		content, err := ioutil.ReadAll(fd)
-		if err != nil {
-			t.Error(err)
-		}
-		if !bytes.Equal(content, []byte(file)) {
-			t.Errorf("%s: %s != %s", file, string(content), file)
+				content, err := ioutil.ReadAll(fd)
+				if err != nil {
+					t.Error(err)
+				}
+				// Even if they are at the archived path, content should have the non
+				// archived name.
+				if !bytes.Equal(content, []byte(file)) {
+					t.Errorf("%s (%s): %s != %s", file, fd.Name(), string(content), file)
+				}
+				_, ok := expectArchived[key]
+				if !ok {
+					t.Error("unexpected archived file with future timestamp", file, version.VersionTime)
+				}
+				delete(expectArchived, key)
+			}
 		}
 	}
 
-	// Check for other unexpected things that are tagged.
-	filesystem.Walk(".", func(path string, f fs.FileInfo, err error) error {
-		if !f.IsRegular() {
-			return nil
-		}
-		if strings.Contains(path, sentinelTag) {
-			path = osutil.NormalizedFilename(path)
-			name, _ := versioner.UntagFilename(path)
-			name = strings.TrimPrefix(name, ".stversions/")
-			if _, ok := expectArchived[name]; !ok {
-				t.Errorf("unexpected file with sentinel tag: %s", name)
-			}
-		}
-		return nil
-	})
+	if len(expectArchived) != 0 {
+		t.Fatal("missed some archived files", expectArchived)
+	}
 }
 
 func TestPausedFolders(t *testing.T) {

lib/versioner/simple.go

@@ -7,12 +7,10 @@
 package versioner
 
 import (
-	"path/filepath"
 	"strconv"
 	"time"
 
 	"github.com/syncthing/syncthing/lib/fs"
-	"github.com/syncthing/syncthing/lib/util"
 )
 
 func init() {
@@ -50,35 +48,12 @@ func (v Simple) Archive(filePath string) error {
 		return err
 	}
 
-	file := filepath.Base(filePath)
-	dir := filepath.Dir(filePath)
-
-	// Glob according to the new file~timestamp.ext pattern.
-	pattern := filepath.Join(dir, TagFilename(file, TimeGlob))
-	newVersions, err := v.versionsFs.Glob(pattern)
-	if err != nil {
-		l.Warnln("globbing:", err, "for", pattern)
-		return nil
-	}
-
-	// Also according to the old file.ext~timestamp pattern.
-	pattern = filepath.Join(dir, file+"~"+TimeGlob)
-	oldVersions, err := v.versionsFs.Glob(pattern)
-	if err != nil {
-		l.Warnln("globbing:", err, "for", pattern)
-		return nil
-	}
-
-	// Use all the found filenames.
-	versions := util.UniqueTrimmedStrings(append(oldVersions, newVersions...))
-
-	// Amend with mtime, sort on mtime, delete the oldest first. Mtime,
-	// nowadays at least, is the time when the archiving happened.
-	versionsWithMtimes := versionsToVersionsWithMtime(v.versionsFs, versions)
-	if len(versionsWithMtimes) > v.keep {
-		for _, toRemove := range versionsWithMtimes[:len(versionsWithMtimes)-v.keep] {
+	// Versions are sorted by timestamp in the file name, oldest first.
+	versions := findAllVersions(v.versionsFs, filePath)
+	if len(versions) > v.keep {
+		for _, toRemove := range versions[:len(versions)-v.keep] {
 			l.Debugln("cleaning out", toRemove)
-			err = v.versionsFs.Remove(toRemove.name)
+			err = v.versionsFs.Remove(toRemove)
 			if err != nil {
 				l.Warnln("removing old version:", err)
 			}

lib/versioner/staggered.go

@@ -7,14 +7,12 @@
 package versioner
 
 import (
-	"path/filepath"
 	"sort"
 	"strconv"
 	"time"
 
 	"github.com/syncthing/syncthing/lib/fs"
 	"github.com/syncthing/syncthing/lib/sync"
-	"github.com/syncthing/syncthing/lib/util"
 )
 
 func init() {
@@ -103,7 +101,7 @@ func (v *Staggered) clean() {
 		return
 	}
 
-	versionsPerFile := make(map[string][]versionWithMtime)
+	versionsPerFile := make(map[string][]string)
 	dirTracker := make(emptyDirTracker)
 
 	walkFn := func(path string, f fs.FileInfo, err error) error {
@@ -124,10 +122,7 @@ func (v *Staggered) clean() {
 			return nil
 		}
 
-		versionsPerFile[name] = append(versionsPerFile[name], versionWithMtime{
-			name:  name,
-			mtime: f.ModTime(),
-		})
+		versionsPerFile[name] = append(versionsPerFile[name], path)
 
 		return nil
 	}
@@ -146,7 +141,7 @@ func (v *Staggered) clean() {
 	l.Debugln("Cleaner: Finished cleaning", v.versionsFs)
 }
 
-func (v *Staggered) expire(versions []versionWithMtime) {
+func (v *Staggered) expire(versions []string) {
 	l.Debugln("Versioner: Expiring versions", versions)
 	for _, file := range v.toRemove(versions, time.Now()) {
 		if fi, err := v.versionsFs.Lstat(file); err != nil {
@@ -163,24 +158,26 @@ func (v *Staggered) expire(versions []versionWithMtime) {
 	}
 }
 
-func (v *Staggered) toRemove(versions []versionWithMtime, now time.Time) []string {
+func (v *Staggered) toRemove(versions []string, now time.Time) []string {
 	var prevAge int64
 	firstFile := true
 	var remove []string
 
-	// The list of versions may or may not be properly sorted. Let's take
-	// off and nuke from orbit, it's the only way to be sure.
-	sort.Slice(versions, func(i, j int) bool {
-		return versions[i].mtime.Before(versions[j].mtime)
-	})
+	// The list of versions may or may not be properly sorted.
+	sort.Strings(versions)
 
 	for _, version := range versions {
-		age := int64(now.Sub(version.mtime).Seconds())
+		versionTime, err := time.ParseInLocation(TimeFormat, ExtractTag(version), time.Local)
+		if err != nil {
+			l.Debugf("Versioner: file name %q is invalid: %v", version, err)
+			continue
+		}
+		age := int64(now.Sub(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 {
-			l.Debugln("Versioner: File over maximum age -> delete ", version.name)
-			remove = append(remove, version.name)
+			l.Debugln("Versioner: File over maximum age -> delete ", version)
+			remove = append(remove, version)
 			continue
 		}
 
@@ -200,8 +197,8 @@ func (v *Staggered) toRemove(versions []versionWithMtime, now time.Time) []strin
 		}
 
 		if prevAge-age < usedInterval.step {
-			l.Debugln("too many files in step -> delete", version.name)
-			remove = append(remove, version.name)
+			l.Debugln("too many files in step -> delete", version)
+			remove = append(remove, version)
 			continue
 		}
 
@@ -222,31 +219,7 @@ func (v *Staggered) Archive(filePath string) error {
 		return err
 	}
 
-	file := filepath.Base(filePath)
-	inFolderPath := filepath.Dir(filePath)
-
-	// Glob according to the new file~timestamp.ext pattern.
-	pattern := filepath.Join(inFolderPath, TagFilename(file, TimeGlob))
-	newVersions, err := v.versionsFs.Glob(pattern)
-	if err != nil {
-		l.Warnln("globbing:", err, "for", pattern)
-		return nil
-	}
-
-	// Also according to the old file.ext~timestamp pattern.
-	pattern = filepath.Join(inFolderPath, file+"~"+TimeGlob)
-	oldVersions, err := v.versionsFs.Glob(pattern)
-	if err != nil {
-		l.Warnln("globbing:", err, "for", pattern)
-		return nil
-	}
-
-	// Use all the found filenames.
-	versions := append(oldVersions, newVersions...)
-	versions = util.UniqueTrimmedStrings(versions)
-
-	versionsWithMtimes := versionsToVersionsWithMtime(v.versionsFs, versions)
-	v.expire(versionsWithMtimes)
+	v.expire(findAllVersions(v.versionsFs, filePath))
 
 	return nil
 }

lib/versioner/staggered_test.go

@@ -26,25 +26,25 @@ func TestStaggeredVersioningVersionCount(t *testing.T) {
 	*/
 
 	now := parseTime("20160415-140000")
-	versionsWithMtime := []versionWithMtime{
+	versionsWithMtime := []string{
 		// 14:00:00 is "now"
-		{"test~20160415-140000", parseTime("20160415-140000")}, // 0 seconds ago
-		{"test~20160415-135959", parseTime("20160415-135959")}, // 1 second ago
-		{"test~20160415-135958", parseTime("20160415-135958")}, // 2 seconds ago
-		{"test~20160415-135900", parseTime("20160415-135900")}, // 1 minute ago
-		{"test~20160415-135859", parseTime("20160415-135859")}, // 1 minute 1 second ago
-		{"test~20160415-135830", parseTime("20160415-135830")}, // 1 minute 30 seconds ago
-		{"test~20160415-135829", parseTime("20160415-135829")}, // 1 minute 31 seconds ago
-		{"test~20160415-135700", parseTime("20160415-135700")}, // 3 minutes ago
-		{"test~20160415-135630", parseTime("20160415-135630")}, // 3 minutes 30 seconds ago
-		{"test~20160415-133000", parseTime("20160415-133000")}, // 30 minutes ago
-		{"test~20160415-132900", parseTime("20160415-132900")}, // 31 minutes ago
-		{"test~20160415-132500", parseTime("20160415-132500")}, // 35 minutes ago
-		{"test~20160415-132000", parseTime("20160415-132000")}, // 40 minutes ago
-		{"test~20160415-130000", parseTime("20160415-130000")}, // 60 minutes ago
-		{"test~20160415-124000", parseTime("20160415-124000")}, // 80 minutes ago
-		{"test~20160415-122000", parseTime("20160415-122000")}, // 100 minutes ago
-		{"test~20160415-110000", parseTime("20160415-110000")}, // 120 minutes ago
+		"test~20160415-140000", // 0 seconds ago
+		"test~20160415-135959", // 1 second ago
+		"test~20160415-135958", // 2 seconds ago
+		"test~20160415-135900", // 1 minute ago
+		"test~20160415-135859", // 1 minute 1 second ago
+		"test~20160415-135830", // 1 minute 30 seconds ago
+		"test~20160415-135829", // 1 minute 31 seconds ago
+		"test~20160415-135700", // 3 minutes ago
+		"test~20160415-135630", // 3 minutes 30 seconds ago
+		"test~20160415-133000", // 30 minutes ago
+		"test~20160415-132900", // 31 minutes ago
+		"test~20160415-132500", // 35 minutes ago
+		"test~20160415-132000", // 40 minutes ago
+		"test~20160415-130000", // 60 minutes ago
+		"test~20160415-124000", // 80 minutes ago
+		"test~20160415-122000", // 100 minutes ago
+		"test~20160415-110000", // 120 minutes ago
 	}
 
 	delete := []string{

lib/versioner/trashcan.go

@@ -133,9 +133,15 @@ func (t *Trashcan) Restore(filepath string, versionTime time.Time) error {
 
 	taggedName := ""
 	tagger := func(name, tag string) string {
-		// We can't use TagFilename here, as restoreFii would discover that as a valid version and restore that instead.
+		// We also abuse the fact that tagger gets called twice, once for tagging the restoration version, which
+		// should just return the plain name, and second time by archive which archives existing file in the folder.
+		// We can't use TagFilename here, as restoreFile would discover that as a valid version and restore that instead.
+		if taggedName != "" {
+			return taggedName
+		}
+
 		taggedName = fs.TempName(name)
-		return taggedName
+		return name
 	}
 
 	err := restoreFile(t.versionsFs, t.folderFs, filepath, versionTime, tagger)

lib/versioner/trashcan_test.go

@@ -108,18 +108,39 @@ func TestTrashcanArchiveRestoreSwitcharoo(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	versionInfo, err := versionsFs.Stat("file")
+	// Check versions
+	versions, err := versioner.GetVersions()
 	if err != nil {
 		t.Fatal(err)
 	}
 
+	fileVersions := versions["file"]
+	if len(fileVersions) != 1 {
+		t.Fatalf("unexpected number of versions: %d != 1", len(fileVersions))
+	}
+
+	fileVersion := fileVersions[0]
+
+	if !fileVersion.ModTime.Equal(fileVersion.VersionTime) {
+		t.Error("time mismatch")
+	}
+
 	if content := readFile(t, versionsFs, "file"); content != "A" {
 		t.Errorf("expected A got %s", content)
 	}
 
 	writeFile(t, folderFs, "file", "B")
 
-	if err := versioner.Restore("file", versionInfo.ModTime().Truncate(time.Second)); err != nil {
+	versionInfo, err := versionsFs.Stat("file")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if !versionInfo.ModTime().Truncate(time.Second).Equal(fileVersion.ModTime) {
+		t.Error("time mismatch")
+	}
+
+	if err := versioner.Restore("file", fileVersion.VersionTime); err != nil {
 		t.Fatal(err)
 	}
 

lib/versioner/util.go

@@ -17,6 +17,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/syncthing/syncthing/lib/fs"
 	"github.com/syncthing/syncthing/lib/osutil"
+	"github.com/syncthing/syncthing/lib/util"
 )
 
 var errDirectory = fmt.Errorf("cannot restore on top of a directory")
@@ -87,15 +88,16 @@ func retrieveVersions(fileSystem fs.Filesystem) (map[string][]FileVersion, error
 			return nil
 		}
 
+		modTime := f.ModTime().Truncate(time.Second)
+
 		path = osutil.NormalizedFilename(path)
 
 		name, tag := UntagFilename(path)
-		// Something invalid, assume it's an untagged file
+		// Something invalid, assume it's an untagged file (trashcan versioner stuff)
 		if name == "" || tag == "" {
-			versionTime := f.ModTime().Truncate(time.Second)
 			files[path] = append(files[path], FileVersion{
-				VersionTime: versionTime,
-				ModTime:     versionTime,
+				VersionTime: modTime,
+				ModTime:     modTime,
 				Size:        f.Size(),
 			})
 			return nil
@@ -107,15 +109,11 @@ func retrieveVersions(fileSystem fs.Filesystem) (map[string][]FileVersion, error
 			return nil
 		}
 
-		if err == nil {
-			files[name] = append(files[name], FileVersion{
-				// This looks backwards, but mtime of the file is when we archived it, making that the version time
-				// The mod time of the file before archiving is embedded in the file name.
-				VersionTime: f.ModTime().Truncate(time.Second),
-				ModTime:     versionTime.Truncate(time.Second),
-				Size:        f.Size(),
-			})
-		}
+		files[name] = append(files[name], FileVersion{
+			VersionTime: versionTime,
+			ModTime:     modTime,
+			Size:        f.Size(),
+		})
 
 		return nil
 	})
@@ -156,30 +154,38 @@ func archiveFile(srcFs, dstFs fs.Filesystem, filePath string, tagger fileTagger)
 		}
 	}
 
-	l.Debugln("archiving", filePath)
-
 	file := filepath.Base(filePath)
 	inFolderPath := filepath.Dir(filePath)
 
 	err = dstFs.MkdirAll(inFolderPath, 0755)
 	if err != nil && !fs.IsExist(err) {
+		l.Debugln("archiving", filePath, err)
 		return err
 	}
 
-	ver := tagger(file, info.ModTime().Format(TimeFormat))
+	now := time.Now()
+
+	ver := tagger(file, now.Format(TimeFormat))
 	dst := filepath.Join(inFolderPath, ver)
-	l.Debugln("moving to", dst)
+	l.Debugln("archiving", filePath, "moving to", dst)
 	err = osutil.RenameOrCopy(srcFs, dstFs, filePath, dst)
 
-	// Set the mtime to the time the file was deleted. This can be used by the
-	// cleanout routine. If this fails things won't work optimally but there's
-	// not much we can do about it so we ignore the error.
-	_ = dstFs.Chtimes(dst, time.Now(), time.Now())
+	mtime := info.ModTime()
+	// If it's a trashcan versioner type thing, then it does not have version time in the name
+	// so use mtime for that.
+	if ver == file {
+		mtime = now
+	}
+
+	_ = dstFs.Chtimes(dst, mtime, mtime)
 
 	return err
 }
 
 func restoreFile(src, dst fs.Filesystem, filePath string, versionTime time.Time, tagger fileTagger) error {
+	tag := versionTime.In(time.Local).Truncate(time.Second).Format(TimeFormat)
+	taggedFilePath := tagger(filePath, tag)
+
 	// If the something already exists where we are restoring to, archive existing file for versioning
 	// remove if it's a symlink, or fail if it's a directory
 	if info, err := dst.Lstat(filePath); err == nil {
@@ -203,28 +209,27 @@ func restoreFile(src, dst fs.Filesystem, filePath string, versionTime time.Time,
 	}
 
 	filePath = osutil.NativeFilename(filePath)
-	tag := versionTime.In(time.Local).Truncate(time.Second).Format(TimeFormat)
-
-	taggedFilename := TagFilename(filePath, tag)
-	oldTaggedFilename := filePath + tag
-	untaggedFileName := filePath
 
-	// Check that the thing we've been asked to restore is actually a file
-	// and that it exists.
+	// Try and find a file that has the correct mtime
 	sourceFile := ""
-	for _, candidate := range []string{taggedFilename, oldTaggedFilename, untaggedFileName} {
-		if info, err := src.Lstat(candidate); fs.IsNotExist(err) || !info.IsRegular() {
-			continue
-		} else if err != nil {
-			// All other errors are fatal
-			return err
-		} else if candidate == untaggedFileName && !info.ModTime().Truncate(time.Second).Equal(versionTime) {
-			// No error, and untagged file, but mtime does not match, skip
-			continue
+	sourceMtime := time.Time{}
+	if info, err := src.Lstat(taggedFilePath); err == nil && info.IsRegular() {
+		sourceFile = taggedFilePath
+		sourceMtime = info.ModTime()
+	} else if err == nil {
+		l.Debugln("restore:", taggedFilePath, "not regular")
+	} else {
+		l.Debugln("restore:", taggedFilePath, err.Error())
+	}
+
+	// Check for untagged file
+	if sourceFile == "" {
+		info, err := src.Lstat(filePath)
+		if err == nil && info.IsRegular() && info.ModTime().Truncate(time.Second).Equal(versionTime) {
+			sourceFile = filePath
+			sourceMtime = info.ModTime()
 		}
 
-		sourceFile = candidate
-		break
 	}
 
 	if sourceFile == "" {
@@ -240,7 +245,9 @@ func restoreFile(src, dst fs.Filesystem, filePath string, versionTime time.Time,
 	}
 
 	_ = dst.MkdirAll(filepath.Dir(filePath), 0755)
-	return osutil.RenameOrCopy(src, dst, sourceFile, filePath)
+	err := osutil.RenameOrCopy(src, dst, sourceFile, filePath)
+	_ = dst.Chtimes(filePath, sourceMtime, sourceMtime)
+	return err
 }
 
 func fsFromParams(folderFs fs.Filesystem, params map[string]string) (versionsFs fs.Filesystem) {
@@ -260,33 +267,23 @@ func fsFromParams(folderFs fs.Filesystem, params map[string]string) (versionsFs
 		_ = fsType.UnmarshalText([]byte(params["fsType"]))
 		versionsFs = fs.NewFilesystem(fsType, params["fsPath"])
 	}
-	l.Debugln("%s (%s) folder using %s (%s) versioner dir", folderFs.URI(), folderFs.Type(), versionsFs.URI(), versionsFs.Type())
+	l.Debugf("%s (%s) folder using %s (%s) versioner dir", folderFs.URI(), folderFs.Type(), versionsFs.URI(), versionsFs.Type())
 	return
 }
 
-type versionWithMtime struct {
-	name  string
-	mtime time.Time
-}
-
-func versionsToVersionsWithMtime(fs fs.Filesystem, versions []string) []versionWithMtime {
-	versionsWithMtimes := make([]versionWithMtime, 0, len(versions))
+func findAllVersions(fs fs.Filesystem, filePath string) []string {
+	inFolderPath := filepath.Dir(filePath)
+	file := filepath.Base(filePath)
 
-	for _, version := range versions {
-		if stat, err := fs.Stat(version); err != nil {
-			// Welp, assume it's gone?
-			continue
-		} else {
-			versionsWithMtimes = append(versionsWithMtimes, versionWithMtime{
-				name:  version,
-				mtime: stat.ModTime(),
-			})
-		}
+	// Glob according to the new file~timestamp.ext pattern.
+	pattern := filepath.Join(inFolderPath, TagFilename(file, TimeGlob))
+	versions, err := fs.Glob(pattern)
+	if err != nil {
+		l.Warnln("globbing:", err, "for", pattern)
+		return nil
 	}
+	versions = util.UniqueTrimmedStrings(versions)
+	sort.Strings(versions)
 
-	sort.Slice(versionsWithMtimes, func(i, j int) bool {
-		return versionsWithMtimes[i].mtime.Before(versionsWithMtimes[j].mtime)
-	})
-
-	return versionsWithMtimes
+	return versions
 }