Merge pull request #1013 from syncthing/timestamp-before-ext

Use file~timestamp.ext for version (fixes #1010)

internal/versioner/simple.go

@@ -98,7 +98,7 @@ func (v Simple) Archive(filePath string) error {
 		return err
 	}
 
-	ver := file + "~" + fileInfo.ModTime().Format("20060102-150405")
+	ver := taggedFilename(file, fileInfo.ModTime().Format(TimeFormat))
 	dst := filepath.Join(dir, ver)
 	if debug {
 		l.Debugln("moving to", dst)
@@ -108,12 +108,24 @@ func (v Simple) Archive(filePath string) error {
 		return err
 	}
 
-	versions, err := filepath.Glob(filepath.Join(dir, file+"~[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9]"))
+	// Glob according to the new file~timestamp.ext pattern.
+	newVersions, err := filepath.Glob(filepath.Join(dir, taggedFilename(file, TimeGlob)))
 	if err != nil {
 		l.Warnln("globbing:", err)
 		return nil
 	}
 
+	// Also according to the old file.ext~timestamp pattern.
+	oldVersions, err := filepath.Glob(filepath.Join(dir, file+"~"+TimeGlob))
+	if err != nil {
+		l.Warnln("globbing:", err)
+		return nil
+	}
+
+	// Use all the found filenames. "~" sorts after "." so all old pattern
+	// files will be deleted before any new, which is as it should be.
+	versions := append(oldVersions, newVersions...)
+
 	if len(versions) > v.keep {
 		sort.Strings(versions)
 		for _, toRemove := range versions[:len(versions)-v.keep] {

internal/versioner/staggered.go

@@ -56,17 +56,6 @@ func isFile(path string) bool {
 	return fileInfo.Mode().IsRegular()
 }
 
-const TimeLayout = "20060102-150405"
-
-func versionExt(path string) string {
-	pathSplit := strings.Split(path, "~")
-	if len(pathSplit) > 1 {
-		return pathSplit[len(pathSplit)-1]
-	} else {
-		return ""
-	}
-}
-
 // Rename versions with old version format
 func (v Staggered) renameOld() {
 	err := filepath.Walk(v.versionsPath, func(path string, f os.FileInfo, err error) error {
@@ -79,7 +68,7 @@ func (v Staggered) renameOld() {
 				l.Infoln("Renaming file", path, "from old to new version format")
 				versiondate := time.Unix(versionUnix, 0)
 				name := path[:len(path)-len(filepath.Ext(path))]
-				err = osutil.Rename(path, name+"~"+versiondate.Format(TimeLayout))
+				err = osutil.Rename(path, taggedFilename(name, versiondate.Format(TimeFormat)))
 				if err != nil {
 					l.Infoln("Error renaming to new format", err)
 				}
@@ -187,7 +176,7 @@ func (v Staggered) clean() {
 				filesPerDir[dir]++
 			}
 		case mode.IsRegular():
-			extension := versionExt(path)
+			extension := filenameTag(path)
 			dir := filepath.Dir(path)
 			name := path[:len(path)-len(extension)-1]
 
@@ -240,7 +229,7 @@ func (v Staggered) expire(versions []string) {
 	firstFile := true
 	for _, file := range versions {
 		if isFile(file) {
-			versionTime, err := time.Parse(TimeLayout, versionExt(file))
+			versionTime, err := time.Parse(TimeFormat, filenameTag(file))
 			if err != nil {
 				l.Infof("Versioner: file name %q is invalid: %v", file, err)
 				continue
@@ -342,7 +331,7 @@ func (v Staggered) Archive(filePath string) error {
 		return err
 	}
 
-	ver := file + "~" + fileInfo.ModTime().Format(TimeLayout)
+	ver := taggedFilename(file, fileInfo.ModTime().Format(TimeFormat))
 	dst := filepath.Join(dir, ver)
 	if debug {
 		l.Debugln("moving to", dst)
@@ -352,12 +341,23 @@ func (v Staggered) Archive(filePath string) error {
 		return err
 	}
 
-	versions, err := filepath.Glob(filepath.Join(dir, file+"~[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9]"))
+	// Glob according to the new file~timestamp.ext pattern.
+	newVersions, err := filepath.Glob(filepath.Join(dir, taggedFilename(file, TimeGlob)))
 	if err != nil {
-		l.Warnln("Versioner: error finding versions for", file, err)
+		l.Warnln("globbing:", err)
 		return nil
 	}
 
+	// Also according to the old file.ext~timestamp pattern.
+	oldVersions, err := filepath.Glob(filepath.Join(dir, file+"~"+TimeGlob))
+	if err != nil {
+		l.Warnln("globbing:", err)
+		return nil
+	}
+
+	// Use all the found filenames.
+	versions := append(oldVersions, newVersions...)
+
 	sort.Strings(versions)
 	v.expire(versions)
 

internal/versioner/util.go

@@ -0,0 +1,42 @@
+// Copyright (C) 2014 The Syncthing Authors.
+//
+// This program is free software: you can redistribute it and/or modify it
+// under the terms of the GNU General Public License as published by the Free
+// Software Foundation, either version 3 of the License, or (at your option)
+// any later version.
+//
+// This program is distributed in the hope that it will be useful, but WITHOUT
+// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+// more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this program. If not, see <http://www.gnu.org/licenses/>.
+
+package versioner
+
+import (
+	"path/filepath"
+	"regexp"
+)
+
+// Inserts ~tag just before the extension of the filename.
+func taggedFilename(name, tag string) string {
+	dir, file := filepath.Dir(name), filepath.Base(name)
+	ext := filepath.Ext(file)
+	withoutExt := file[:len(file)-len(ext)]
+	return filepath.Join(dir, withoutExt+"~"+tag+ext)
+}
+
+var tagExp = regexp.MustCompile(`~([^~.]+)(?:\.[^.]+)?$`)
+
+// Returns the tag from a filename, whether at the end or middle.
+func filenameTag(path string) string {
+	match := tagExp.FindStringSubmatch(path)
+	// match is []string{"whole match", "submatch"} when successfull
+
+	if len(match) != 2 {
+		return ""
+	}
+	return match[1]
+}

internal/versioner/versioner.go

@@ -22,3 +22,8 @@ type Versioner interface {
 }
 
 var Factories = map[string]func(folderID string, folderDir string, params map[string]string) Versioner{}
+
+const (
+	TimeFormat = "20060102-150405"
+	TimeGlob   = "[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]-[0-9][0-9][0-9][0-9][0-9][0-9]" // glob pattern matching TimeFormat
+)

internal/versioner/versioner_test.go

@@ -13,6 +13,34 @@
 // You should have received a copy of the GNU General Public License along
 // with this program. If not, see <http://www.gnu.org/licenses/>.
 
-package versioner_test
+package versioner
 
-// Empty test file to generate 0% coverage rather than no coverage
+import "testing"
+
+func TestTaggedFilename(t *testing.T) {
+	cases := [][3]string{
+		{"foo/bar.baz", "tag", "foo/bar~tag.baz"},
+		{"bar.baz", "tag", "bar~tag.baz"},
+		{"bar", "tag", "bar~tag"},
+
+		// Parsing test only
+		{"", "tag-only", "foo/bar.baz~tag-only"},
+		{"", "tag-only", "bar.baz~tag-only"},
+	}
+
+	for _, tc := range cases {
+		if tc[0] != "" {
+			// Test tagger
+			tf := taggedFilename(tc[0], tc[1])
+			if tf != tc[2] {
+				t.Errorf("%s != %s", tf, tc[2])
+			}
+		}
+
+		// Test parser
+		tag := filenameTag(tc[2])
+		if tag != tc[1] {
+			t.Errorf("%s != %s", tag, tc[1])
+		}
+	}
+}