lib/versioner: Reduce surface area (#6186)

* lib/versioner: Reduce surface area

This is a refactor while I was anyway rooting around in the versioner.
Instead of exporting every possible implementation and the factory and
letting the caller do whatever, this now encapsulates all that and
exposes a New() that takes a config.VersioningConfiguration.

Given that and that we don't know (from the outside) how a versioner
works or what state it keeps, we now just construct it once per folder
and keep it around. Previously it was recreated for each restore
request.

* unparam

* wip

lib/config/folderconfiguration.go

@@ -18,7 +18,6 @@ import (
 	"github.com/syncthing/syncthing/lib/fs"
 	"github.com/syncthing/syncthing/lib/protocol"
 	"github.com/syncthing/syncthing/lib/util"
-	"github.com/syncthing/syncthing/lib/versioner"
 )
 
 var (
@@ -105,18 +104,6 @@ func (f FolderConfiguration) Filesystem() fs.Filesystem {
 	return f.cachedFilesystem
 }
 
-func (f FolderConfiguration) Versioner() versioner.Versioner {
-	if f.Versioning.Type == "" {
-		return nil
-	}
-	versionerFactory, ok := versioner.Factories[f.Versioning.Type]
-	if !ok {
-		panic(fmt.Sprintf("Requested versioning type %q that does not exist", f.Versioning.Type))
-	}
-
-	return versionerFactory(f.ID, f.Filesystem(), f.Versioning.Params)
-}
-
 func (f FolderConfiguration) ModTimeWindow() time.Duration {
 	return f.cachedModTimeWindow
 }

lib/model/model.go

@@ -140,6 +140,7 @@ type model struct {
 	folderRunners      map[string]service                                     // folder -> puller or scanner
 	folderRunnerTokens map[string][]suture.ServiceToken                       // folder -> tokens for puller or scanner
 	folderRestartMuts  syncMutexMap                                           // folder -> restart mutex
+	folderVersioners   map[string]versioner.Versioner                         // folder -> versioner (may be nil)
 
 	pmut                sync.RWMutex // protects the below
 	conn                map[protocol.DeviceID]connections.Connection
@@ -166,6 +167,7 @@ var (
 	errFolderNotRunning  = errors.New("folder is not running")
 	errFolderMissing     = errors.New("no such folder")
 	errNetworkNotAllowed = errors.New("network not allowed")
+	errNoVersioner       = errors.New("folder has no versioner")
 	// errors about why a connection is closed
 	errIgnoredFolderRemoved = errors.New("folder no longer ignored")
 	errReplacingConnection  = errors.New("replacing connection")
@@ -200,6 +202,7 @@ func NewModel(cfg config.Wrapper, id protocol.DeviceID, clientName, clientVersio
 		folderIgnores:       make(map[string]*ignore.Matcher),
 		folderRunners:       make(map[string]service),
 		folderRunnerTokens:  make(map[string][]suture.ServiceToken),
+		folderVersioners:    make(map[string]versioner.Versioner),
 		conn:                make(map[protocol.DeviceID]connections.Connection),
 		connRequestLimiters: make(map[protocol.DeviceID]*byteSemaphore),
 		closed:              make(map[protocol.DeviceID]chan struct{}),
@@ -318,7 +321,20 @@ func (m *model) startFolderLocked(cfg config.FolderConfiguration) {
 		}
 	}
 
-	ver := cfg.Versioner()
+	ffs := fset.MtimeFS()
+
+	// These are our metadata files, and they should always be hidden.
+	_ = ffs.Hide(config.DefaultMarkerName)
+	_ = ffs.Hide(".stversions")
+	_ = ffs.Hide(".stignore")
+
+	var ver versioner.Versioner
+	if cfg.Versioning.Type != "" {
+		var err error
+		ver, err = versioner.New(ffs, cfg.Versioning)
+		if err != nil {
+			panic(fmt.Errorf("creating versioner: %v", err))
+		}
 		if service, ok := ver.(suture.Service); ok {
 			// The versioner implements the suture.Service interface, so
 			// expects to be run in the background in addition to being called
@@ -326,13 +342,8 @@ func (m *model) startFolderLocked(cfg config.FolderConfiguration) {
 			token := m.Add(service)
 			m.folderRunnerTokens[folder] = append(m.folderRunnerTokens[folder], token)
 		}
-
+	}
-	ffs := fset.MtimeFS()
+	m.folderVersioners[folder] = ver
-
-	// These are our metadata files, and they should always be hidden.
-	ffs.Hide(config.DefaultMarkerName)
-	ffs.Hide(".stversions")
-	ffs.Hide(".stignore")
 
 	ignores := m.folderIgnores[folder]
 
@@ -459,6 +470,7 @@ func (m *model) removeFolderLocked(cfg config.FolderConfiguration) {
 	delete(m.folderIgnores, cfg.ID)
 	delete(m.folderRunners, cfg.ID)
 	delete(m.folderRunnerTokens, cfg.ID)
+	delete(m.folderVersioners, cfg.ID)
 }
 
 func (m *model) restartFolder(from, to config.FolderConfiguration) {
@@ -2444,14 +2456,14 @@ func (m *model) GlobalDirectoryTree(folder, prefix string, levels int, dirsonly
 }
 
 func (m *model) GetFolderVersions(folder string) (map[string][]versioner.FileVersion, error) {
-	fcfg, ok := m.cfg.Folder(folder)
+	m.fmut.RLock()
+	ver, ok := m.folderVersioners[folder]
+	m.fmut.RUnlock()
 	if !ok {
 		return nil, errFolderMissing
 	}
-
-	ver := fcfg.Versioner()
 	if ver == nil {
-		return nil, errors.New("no versioner configured")
+		return nil, errNoVersioner
 	}
 
 	return ver.GetVersions()
@@ -2463,7 +2475,15 @@ func (m *model) RestoreFolderVersions(folder string, versions map[string]time.Ti
 		return nil, errFolderMissing
 	}
 
-	ver := fcfg.Versioner()
+	m.fmut.RLock()
+	ver := m.folderVersioners[folder]
+	m.fmut.RUnlock()
+	if !ok {
+		return nil, errFolderMissing
+	}
+	if ver == nil {
+		return nil, errNoVersioner
+	}
 
 	restoreErrors := make(map[string]string)
 

lib/versioner/external.go

@@ -21,22 +21,22 @@ import (
 
 func init() {
 	// Register the constructor for this type of versioner with the name "external"
-	Factories["external"] = NewExternal
+	factories["external"] = newExternal
 }
 
-type External struct {
+type external struct {
 	command    string
 	filesystem fs.Filesystem
 }
 
-func NewExternal(folderID string, filesystem fs.Filesystem, params map[string]string) Versioner {
+func newExternal(filesystem fs.Filesystem, params map[string]string) Versioner {
 	command := params["command"]
 
 	if runtime.GOOS == "windows" {
 		command = strings.Replace(command, `\`, `\\`, -1)
 	}
 
-	s := External{
+	s := external{
 		command:    command,
 		filesystem: filesystem,
 	}
@@ -47,7 +47,7 @@ func NewExternal(folderID string, filesystem fs.Filesystem, params map[string]st
 
 // Archive moves the named file away to a version archive. If this function
 // returns nil, the named file does not exist any more (has been archived).
-func (v External) Archive(filePath string) error {
+func (v external) Archive(filePath string) error {
 	info, err := v.filesystem.Lstat(filePath)
 	if fs.IsNotExist(err) {
 		l.Debugln("not archiving nonexistent file", filePath)
@@ -107,10 +107,10 @@ func (v External) Archive(filePath string) error {
 	return errors.New("Versioner: file was not removed by external script")
 }
 
-func (v External) GetVersions() (map[string][]FileVersion, error) {
+func (v external) GetVersions() (map[string][]FileVersion, error) {
 	return nil, ErrRestorationNotSupported
 }
 
-func (v External) Restore(filePath string, versionTime time.Time) error {
+func (v external) Restore(filePath string, versionTime time.Time) error {
 	return ErrRestorationNotSupported
 }

lib/versioner/external_test.go

@@ -29,7 +29,7 @@ func TestExternalNoCommand(t *testing.T) {
 
 	// The versioner should fail due to missing command.
 
-	e := External{
+	e := external{
 		filesystem: fs.NewFilesystem(fs.FilesystemTypeBasic, "."),
 		command:    "nonexistent command",
 	}
@@ -62,7 +62,7 @@ func TestExternal(t *testing.T) {
 
 	// The versioner should run successfully.
 
-	e := External{
+	e := external{
 		filesystem: fs.NewFilesystem(fs.FilesystemTypeBasic, "."),
 		command:    cmd,
 	}

lib/versioner/simple.go

@@ -15,22 +15,22 @@ import (
 
 func init() {
 	// Register the constructor for this type of versioner with the name "simple"
-	Factories["simple"] = NewSimple
+	factories["simple"] = newSimple
 }
 
-type Simple struct {
+type simple struct {
 	keep       int
 	folderFs   fs.Filesystem
 	versionsFs fs.Filesystem
 }
 
-func NewSimple(folderID string, folderFs fs.Filesystem, params map[string]string) Versioner {
+func newSimple(folderFs fs.Filesystem, params map[string]string) Versioner {
 	keep, err := strconv.Atoi(params["keep"])
 	if err != nil {
 		keep = 5 // A reasonable default
 	}
 
-	s := Simple{
+	s := simple{
 		keep:       keep,
 		folderFs:   folderFs,
 		versionsFs: fsFromParams(folderFs, params),
@@ -42,7 +42,7 @@ func NewSimple(folderID string, folderFs fs.Filesystem, params map[string]string
 
 // Archive moves the named file away 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 {
+func (v simple) Archive(filePath string) error {
 	err := archiveFile(v.folderFs, v.versionsFs, filePath, TagFilename)
 	if err != nil {
 		return err
@@ -63,10 +63,10 @@ func (v Simple) Archive(filePath string) error {
 	return nil
 }
 
-func (v Simple) GetVersions() (map[string][]FileVersion, error) {
+func (v simple) GetVersions() (map[string][]FileVersion, error) {
 	return retrieveVersions(v.versionsFs)
 }
 
-func (v Simple) Restore(filepath string, versionTime time.Time) error {
+func (v simple) Restore(filepath string, versionTime time.Time) error {
 	return restoreFile(v.versionsFs, v.folderFs, filepath, versionTime, TagFilename)
 }

lib/versioner/simple_test.go

@@ -41,7 +41,7 @@ func TestTaggedFilename(t *testing.T) {
 		}
 
 		// Test parser
-		tag := ExtractTag(tc[2])
+		tag := extractTag(tc[2])
 		if tag != tc[1] {
 			t.Errorf("%s != %s", tag, tc[1])
 		}
@@ -61,7 +61,7 @@ func TestSimpleVersioningVersionCount(t *testing.T) {
 
 	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, dir)
 
-	v := NewSimple("", fs, map[string]string{"keep": "2"})
+	v := newSimple(fs, map[string]string{"keep": "2"})
 
 	path := "test"
 

lib/versioner/staggered.go

@@ -22,26 +22,26 @@ import (
 
 func init() {
 	// Register the constructor for this type of versioner with the name "staggered"
-	Factories["staggered"] = NewStaggered
+	factories["staggered"] = newStaggered
 }
 
-type Interval struct {
+type interval struct {
 	step int64
 	end  int64
 }
 
-type Staggered struct {
+type staggered struct {
 	suture.Service
 	cleanInterval int64
 	folderFs      fs.Filesystem
 	versionsFs    fs.Filesystem
-	interval      [4]Interval
+	interval      [4]interval
 	mutex         sync.Mutex
 
 	testCleanDone chan struct{}
 }
 
-func NewStaggered(folderID string, folderFs fs.Filesystem, params map[string]string) Versioner {
+func newStaggered(folderFs fs.Filesystem, params map[string]string) Versioner {
 	maxAge, err := strconv.ParseInt(params["maxAge"], 10, 0)
 	if err != nil {
 		maxAge = 31536000 // Default: ~1 year
@@ -55,11 +55,11 @@ func NewStaggered(folderID string, folderFs fs.Filesystem, params map[string]str
 	params["fsPath"] = params["versionsPath"]
 	versionsFs := fsFromParams(folderFs, params)
 
-	s := &Staggered{
+	s := &staggered{
 		cleanInterval: cleanInterval,
 		folderFs:      folderFs,
 		versionsFs:    versionsFs,
-		interval: [4]Interval{
+		interval: [4]interval{
 			{30, 3600},       // first hour -> 30 sec between versions
 			{3600, 86400},    // next day -> 1 h between versions
 			{86400, 592000},  // next 30 days -> 1 day between versions
@@ -73,7 +73,7 @@ func NewStaggered(folderID string, folderFs fs.Filesystem, params map[string]str
 	return s
 }
 
-func (v *Staggered) serve(ctx context.Context) {
+func (v *staggered) serve(ctx context.Context) {
 	v.clean()
 	if v.testCleanDone != nil {
 		close(v.testCleanDone)
@@ -91,7 +91,7 @@ func (v *Staggered) serve(ctx context.Context) {
 	}
 }
 
-func (v *Staggered) clean() {
+func (v *staggered) clean() {
 	l.Debugln("Versioner clean: Waiting for lock on", v.versionsFs)
 	v.mutex.Lock()
 	defer v.mutex.Unlock()
@@ -142,7 +142,7 @@ func (v *Staggered) clean() {
 	l.Debugln("Cleaner: Finished cleaning", v.versionsFs)
 }
 
-func (v *Staggered) expire(versions []string) {
+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 {
@@ -159,7 +159,7 @@ func (v *Staggered) expire(versions []string) {
 	}
 }
 
-func (v *Staggered) toRemove(versions []string, now time.Time) []string {
+func (v *staggered) toRemove(versions []string, now time.Time) []string {
 	var prevAge int64
 	firstFile := true
 	var remove []string
@@ -168,7 +168,7 @@ func (v *Staggered) toRemove(versions []string, now time.Time) []string {
 	sort.Strings(versions)
 
 	for _, version := range versions {
-		versionTime, err := time.ParseInLocation(TimeFormat, ExtractTag(version), time.Local)
+		versionTime, err := time.ParseInLocation(TimeFormat, extractTag(version), time.Local)
 		if err != nil {
 			l.Debugf("Versioner: file name %q is invalid: %v", version, err)
 			continue
@@ -190,7 +190,7 @@ func (v *Staggered) toRemove(versions []string, now time.Time) []string {
 		}
 
 		// Find the interval the file fits in
-		var usedInterval Interval
+		var usedInterval interval
 		for _, usedInterval = range v.interval {
 			if age < usedInterval.end {
 				break
@@ -211,7 +211,7 @@ func (v *Staggered) toRemove(versions []string, now time.Time) []string {
 
 // Archive moves the named file away to a version archive. If this function
 // returns nil, the named file does not exist any more (has been archived).
-func (v *Staggered) Archive(filePath string) error {
+func (v *staggered) Archive(filePath string) error {
 	l.Debugln("Waiting for lock on ", v.versionsFs)
 	v.mutex.Lock()
 	defer v.mutex.Unlock()
@@ -225,14 +225,14 @@ func (v *Staggered) Archive(filePath string) error {
 	return nil
 }
 
-func (v *Staggered) GetVersions() (map[string][]FileVersion, error) {
+func (v *staggered) GetVersions() (map[string][]FileVersion, error) {
 	return retrieveVersions(v.versionsFs)
 }
 
-func (v *Staggered) Restore(filepath string, versionTime time.Time) error {
+func (v *staggered) Restore(filepath string, versionTime time.Time) error {
 	return restoreFile(v.versionsFs, v.folderFs, filepath, versionTime, TagFilename)
 }
 
-func (v *Staggered) String() string {
+func (v *staggered) String() string {
 	return fmt.Sprintf("Staggered/@%p", v)
 }

lib/versioner/staggered_test.go

@@ -57,9 +57,9 @@ func TestStaggeredVersioningVersionCount(t *testing.T) {
 	}
 	sort.Strings(delete)
 
-	v := NewStaggered("", fs.NewFilesystem(fs.FilesystemTypeFake, "testdata"), map[string]string{
+	v := newStaggered(fs.NewFilesystem(fs.FilesystemTypeFake, "testdata"), map[string]string{
 		"maxAge": strconv.Itoa(365 * 86400),
-	}).(*Staggered)
+	}).(*staggered)
 	rem := v.toRemove(versionsWithMtime, now)
 	sort.Strings(rem)
 

lib/versioner/trashcan.go

@@ -20,21 +20,21 @@ import (
 
 func init() {
 	// Register the constructor for this type of versioner
-	Factories["trashcan"] = NewTrashcan
+	factories["trashcan"] = newTrashcan
 }
 
-type Trashcan struct {
+type trashcan struct {
 	suture.Service
 	folderFs     fs.Filesystem
 	versionsFs   fs.Filesystem
 	cleanoutDays int
 }
 
-func NewTrashcan(folderID string, folderFs fs.Filesystem, params map[string]string) Versioner {
+func newTrashcan(folderFs fs.Filesystem, params map[string]string) Versioner {
 	cleanoutDays, _ := strconv.Atoi(params["cleanoutDays"])
 	// On error we default to 0, "do not clean out the trash can"
 
-	s := &Trashcan{
+	s := &trashcan{
 		folderFs:     folderFs,
 		versionsFs:   fsFromParams(folderFs, params),
 		cleanoutDays: cleanoutDays,
@@ -47,13 +47,13 @@ func NewTrashcan(folderID string, folderFs fs.Filesystem, params map[string]stri
 
 // Archive moves the named file away to a version archive. If this function
 // returns nil, the named file does not exist any more (has been archived).
-func (t *Trashcan) Archive(filePath string) error {
+func (t *trashcan) Archive(filePath string) error {
 	return archiveFile(t.folderFs, t.versionsFs, filePath, func(name, tag string) string {
 		return name
 	})
 }
 
-func (t *Trashcan) serve(ctx context.Context) {
+func (t *trashcan) serve(ctx context.Context) {
 	l.Debugln(t, "starting")
 	defer l.Debugln(t, "stopping")
 
@@ -79,11 +79,11 @@ func (t *Trashcan) serve(ctx context.Context) {
 	}
 }
 
-func (t *Trashcan) String() string {
+func (t *trashcan) String() string {
 	return fmt.Sprintf("trashcan@%p", t)
 }
 
-func (t *Trashcan) cleanoutArchive() error {
+func (t *trashcan) cleanoutArchive() error {
 	if _, err := t.versionsFs.Lstat("."); fs.IsNotExist(err) {
 		return nil
 	}
@@ -121,11 +121,11 @@ func (t *Trashcan) cleanoutArchive() error {
 	return nil
 }
 
-func (t *Trashcan) GetVersions() (map[string][]FileVersion, error) {
+func (t *trashcan) GetVersions() (map[string][]FileVersion, error) {
 	return retrieveVersions(t.versionsFs)
 }
 
-func (t *Trashcan) Restore(filepath string, versionTime time.Time) error {
+func (t *trashcan) Restore(filepath string, versionTime time.Time) error {
 	// If we have an untagged file A and want to restore it on top of existing file A, we can't first archive the
 	// existing A as we'd overwrite the old A version, therefore when we archive existing file, we archive it with a
 	// tag but when the restoration is finished, we rename it (untag it). This is only important if when restoring A,

lib/versioner/trashcan_test.go

@@ -53,7 +53,7 @@ func TestTrashcanCleanout(t *testing.T) {
 		}
 	}
 
-	versioner := NewTrashcan("default", fs.NewFilesystem(fs.FilesystemTypeBasic, "testdata"), map[string]string{"cleanoutDays": "7"}).(*Trashcan)
+	versioner := newTrashcan(fs.NewFilesystem(fs.FilesystemTypeBasic, "testdata"), map[string]string{"cleanoutDays": "7"}).(*trashcan)
 	if err := versioner.cleanoutArchive(); err != nil {
 		t.Fatal(err)
 	}
@@ -95,7 +95,7 @@ func TestTrashcanArchiveRestoreSwitcharoo(t *testing.T) {
 
 	writeFile(t, folderFs, "file", "A")
 
-	versioner := NewTrashcan("", folderFs, map[string]string{
+	versioner := newTrashcan(folderFs, map[string]string{
 		"fsType": "basic",
 		"fsPath": tmpDir2,
 	})

lib/versioner/util.go

@@ -24,7 +24,7 @@ var errDirectory = fmt.Errorf("cannot restore on top of a directory")
 var errNotFound = fmt.Errorf("version not found")
 var errFileAlreadyExists = fmt.Errorf("file already exists")
 
-// Inserts ~tag just before the extension of the filename.
+// TagFilename inserts ~tag just before the extension of the filename.
 func TagFilename(name, tag string) string {
 	dir, file := filepath.Dir(name), filepath.Base(name)
 	ext := filepath.Ext(file)
@@ -34,8 +34,8 @@ func TagFilename(name, tag string) string {
 
 var tagExp = regexp.MustCompile(`.*~([^~.]+)(?:\.[^.]+)?$`)
 
-// Returns the tag from a filename, whether at the end or middle.
+// extractTag returns the tag from a filename, whether at the end or middle.
-func ExtractTag(path string) string {
+func extractTag(path string) string {
 	match := tagExp.FindStringSubmatch(path)
 	// match is []string{"whole match", "submatch"} when successful
 
@@ -45,9 +45,10 @@ func ExtractTag(path string) string {
 	return match[1]
 }
 
+// UntagFilename returns the filename without tag, and the extracted tag
 func UntagFilename(path string) (string, string) {
 	ext := filepath.Ext(path)
-	versionTag := ExtractTag(path)
+	versionTag := extractTag(path)
 
 	// Files tagged with old style tags cannot be untagged.
 	if versionTag == "" {
@@ -276,7 +277,7 @@ func findAllVersions(fs fs.Filesystem, filePath string) []string {
 	file := filepath.Base(filePath)
 
 	// Glob according to the new file~timestamp.ext pattern.
-	pattern := filepath.Join(inFolderPath, TagFilename(file, TimeGlob))
+	pattern := filepath.Join(inFolderPath, TagFilename(file, timeGlob))
 	versions, err := fs.Glob(pattern)
 	if err != nil {
 		l.Warnln("globbing:", err, "for", pattern)

lib/versioner/versioner.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/fs"
 )
 
@@ -27,10 +28,22 @@ type FileVersion struct {
 	Size        int64     `json:"size"`
 }
 
-var Factories = map[string]func(folderID string, filesystem fs.Filesystem, params map[string]string) Versioner{}
+type factory func(filesystem fs.Filesystem, params map[string]string) Versioner
+
+var factories = make(map[string]factory)
+
 var ErrRestorationNotSupported = fmt.Errorf("version restoration not supported with the current 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
+	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
 )
+
+func New(fs fs.Filesystem, cfg config.VersioningConfiguration) (Versioner, error) {
+	fac, ok := factories[cfg.Type]
+	if !ok {
+		return nil, fmt.Errorf("requested versioning type %q does not exist", cfg.Type)
+	}
+
+	return fac(fs, cfg.Params), nil
+}