lib/versioner: Purge the empty directories in .stversions (fixes #4406)

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4514
LGTM: AudriusButkevicius, imsodin

AUTHORS

@@ -41,6 +41,7 @@ Darshil Chanpura (dtchanpura) <dtchanpura@gmail.com> <dcprime314@gmail.com>
 David Rimmer (dinosore) <dinosore@dbrsoftware.co.uk>
 Denis A. (dva) <denisva@gmail.com>
 Dennis Wilson (snnd) <dw@risu.io>
+Dmitry Saveliev (dsaveliev) <d.e.saveliev@gmail.com>
 Dominik Heidler (asdil12) <dominik@heidler.eu>
 Elias Jarlebring (jarlebring) <jarlebring@gmail.com>
 Emil Hessman (ceh) <emil@hessman.se>

NICKS

@@ -36,6 +36,7 @@ ceh <emil@hessman.se>
 cqcallaw <enlightened.despot@gmail.com>
 damajor <damajor@gmail.com>
 dinosore <dinosore@dbrsoftware.co.uk>
+dsaveliev <d.e.saveliev@gmail.com>
 dtchanpura <dtchanpura@gmail.com>
 dtchanpura <dcprime314@gmail.com>
 dva <denisva@gmail.com>

gui/default/syncthing/core/aboutModalView.html

@@ -12,7 +12,7 @@
     <p translate>Copyright &copy; 2014-2017 the following Contributors:</p>
     <div class="row">
       <div class="col-md-12" id="contributor-list">
-Jakob Borg, Audrius Butkevicius, Alexander Graf, Anderson Mesquita, Antony Male, Ben Schulz, Caleb Callaway, Daniel Harte, Lars K.W. Gohlke, Lode Hoste, Michael Ploujnikov, Nate Morrison, Philippe Schommers, Ryan Sullivan, Sergey Mishin, Simon Frei, Stefan Tatschner, Aaron Bieber, Adam Piggott, Adel Qalieh, Alessandro G., Alexandre Viau, Andrew Dunham, Andrey D, Antoine Lamielle, Arthur Axel fREW Schmidt, Bart De Vries, Ben Curthoys, Ben Shepherd, Ben Sidhom, Benny Ng, Brandon Philips, Brendan Long, Brian R. Becker, Carsten Hagemann, Cathryne Linenweaver, Cedric Staniewski, Chris Howie, Chris Joel, Colin Kennedy, Daniel Bergmann, Daniel Martí, Darshil Chanpura, David Rimmer, Denis A., Dennis Wilson, Dominik Heidler, Elias Jarlebring, Emil Hessman, Erik Meitner, Federico Castagnini, Felix Ableitner, Felix Unterpaintner, Francois-Xavier Gsell, Frank Isemann, Gilli Sigurdsson, Heiko Zuerker, Jaakko Hannikainen, Jacek Szafarkiewicz, Jake Peterson, James Patterson, Jaroslav Malec, Jaya Chithra, Jens Diemer, Jochen Voss, Johan Vromans, Jose Manuel Delicado, Karol Różycki, Kelong Cong, Ken'ichi Kamada, Kevin Allen, Kevin White, Jr., Kurt Fitzner, Laurent Etiemble, Leo Arias, Liu Siyuan, Lord Landon Agahnim, Majed Abdulaziz, Marc Laporte, Marc Pujol, Marcin Dziadus, Mark Pulford, Mateusz Naściszewski, Matt Burke, Max Schulze, Michael Jephcote, Michael Tilli, Niels Peter Roest, Pascal Jungblut, Peter Hoeg, Phill Luby, Piotr Bejda, Robert Carosi, Roman Zaynetdinov, Ross Smith II, Sacheendra Talluri, Scott Klupfel, Stefan Kuntz, Suhas Gundimeda, Taylor Khan, Tim Abell, Tim Howes, Tobias Nygren, Tobias Tom, Tomas Cerveny, Tully Robinson, Tyler Brazier, Unrud, Veeti Paananen, Victor Buinsky, Vil Brekin, William A. Kennington III, Wulf Weich, Xavier O., Yannic A.
+Jakob Borg, Audrius Butkevicius, Alexander Graf, Anderson Mesquita, Antony Male, Ben Schulz, Caleb Callaway, Daniel Harte, Lars K.W. Gohlke, Lode Hoste, Michael Ploujnikov, Nate Morrison, Philippe Schommers, Ryan Sullivan, Sergey Mishin, Simon Frei, Stefan Tatschner, Aaron Bieber, Adam Piggott, Adel Qalieh, Alessandro G., Alexandre Viau, Andrew Dunham, Andrey D, Antoine Lamielle, Arthur Axel fREW Schmidt, Bart De Vries, Ben Curthoys, Ben Shepherd, Ben Sidhom, Benny Ng, Brandon Philips, Brendan Long, Brian R. Becker, Carsten Hagemann, Cathryne Linenweaver, Cedric Staniewski, Chris Howie, Chris Joel, Colin Kennedy, Daniel Bergmann, Daniel Martí, Darshil Chanpura, David Rimmer, Denis A., Dennis Wilson, Dmitry Saveliev, Dominik Heidler, Elias Jarlebring, Emil Hessman, Erik Meitner, Federico Castagnini, Felix Ableitner, Felix Unterpaintner, Francois-Xavier Gsell, Frank Isemann, Gilli Sigurdsson, Heiko Zuerker, Jaakko Hannikainen, Jacek Szafarkiewicz, Jake Peterson, James Patterson, Jaroslav Malec, Jaya Chithra, Jens Diemer, Jochen Voss, Johan Vromans, Jose Manuel Delicado, Karol Różycki, Kelong Cong, Ken'ichi Kamada, Kevin Allen, Kevin White, Jr., Kurt Fitzner, Laurent Etiemble, Leo Arias, Liu Siyuan, Lord Landon Agahnim, Majed Abdulaziz, Marc Laporte, Marc Pujol, Marcin Dziadus, Mark Pulford, Mateusz Naściszewski, Matt Burke, Max Schulze, Michael Jephcote, Michael Tilli, Niels Peter Roest, Pascal Jungblut, Peter Hoeg, Phill Luby, Piotr Bejda, Robert Carosi, Roman Zaynetdinov, Ross Smith II, Sacheendra Talluri, Scott Klupfel, Stefan Kuntz, Suhas Gundimeda, Taylor Khan, Tim Abell, Tim Howes, Tobias Nygren, Tobias Tom, Tomas Cerveny, Tully Robinson, Tyler Brazier, Unrud, Veeti Paananen, Victor Buinsky, Vil Brekin, William A. Kennington III, Wulf Weich, Xavier O., Yannic A.
     </div>
     </div>
     <hr/>

lib/versioner/empty_dir_tracker.go

@@ -0,0 +1,51 @@
+// Copyright (C) 2017 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+package versioner
+
+import (
+	"path/filepath"
+	"sort"
+
+	"github.com/syncthing/syncthing/lib/fs"
+)
+
+type emptyDirTracker map[string]struct{}
+
+func (t emptyDirTracker) addDir(path string) {
+	if path == "." {
+		return
+	}
+	t[path] = struct{}{}
+}
+
+// Remove all dirs from the path to the file
+func (t emptyDirTracker) addFile(path string) {
+	dir := filepath.Dir(path)
+	for dir != "." {
+		delete(t, dir)
+		dir = filepath.Dir(dir)
+	}
+}
+
+func (t emptyDirTracker) emptyDirs() []string {
+	empty := []string{}
+	for dir := range t {
+		empty = append(empty, dir)
+	}
+	sort.Sort(sort.Reverse(sort.StringSlice(empty)))
+	return empty
+}
+
+func (t emptyDirTracker) deleteEmptyDirs(fs fs.Filesystem) {
+	for _, path := range t.emptyDirs() {
+		l.Debugln("Cleaner: deleting empty directory", path)
+		err := fs.Remove(path)
+		if err != nil {
+			l.Warnln("Versioner: can't remove directory", path, err)
+		}
+	}
+}

lib/versioner/empty_dir_tracker_test.go

@@ -0,0 +1,75 @@
+// Copyright (C) 2017 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at https://mozilla.org/MPL/2.0/.
+
+package versioner
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/d4l3k/messagediff"
+)
+
+// TestEmptyDirs models the following .stversions structure:
+// .stversions/
+// ├── keep1
+// │   └── file1
+// ├── keep2
+// │   └── keep21
+// │       └── keep22
+// │           └── file1
+// ├── remove1
+// └── remove2
+//     └── remove21
+//         └── remove22
+func TestEmptyDirs(t *testing.T) {
+	var paths = []struct {
+		path   string
+		isFile bool
+	}{
+		{".", false},
+		{"keep1", false},
+		{"keep1/file1", true},
+		{"keep2", false},
+		{"keep2/keep21", false},
+		{"keep2/keep21/keep22", false},
+		{"keep2/keep21/keep22/file1", true},
+		{"remove1", false},
+		{"remove2", false},
+		{"remove2/remove21", false},
+		{"remove2/remove21/remove22", false},
+	}
+
+	var expected = []string{
+		"remove2/remove21/remove22",
+		"remove2/remove21",
+		"remove2",
+		"remove1",
+	}
+
+	// For compatibility with Windows
+	for i, p := range paths {
+		paths[i].path = filepath.FromSlash(p.path)
+	}
+
+	for i, p := range expected {
+		expected[i] = filepath.FromSlash(p)
+	}
+
+	dirTracker := make(emptyDirTracker)
+	for _, p := range paths {
+		if p.isFile {
+			dirTracker.addFile(p.path)
+		} else {
+			dirTracker.addDir(p.path)
+		}
+	}
+
+	result := dirTracker.emptyDirs()
+	if diff, equal := messagediff.PrettyDiff(expected, result); !equal {
+		t.Errorf("Incorrect empty directories list; got %v, expected %v\n%v", result, expected, diff)
+	}
+}

lib/versioner/staggered.go

@@ -111,34 +111,31 @@ func (v *Staggered) clean() {
 	}
 
 	versionsPerFile := make(map[string][]string)
-	filesPerDir := make(map[string]int)
+	dirTracker := make(emptyDirTracker)
 
-	err := v.versionsFs.Walk(".", func(path string, f fs.FileInfo, err error) error {
+	walkFn := func(path string, f fs.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
 
 		if f.IsDir() && !f.IsSymlink() {
-			filesPerDir[path] = 0
+			dirTracker.addDir(path)
-			if path != "." {
+			return nil
-				dir := filepath.Dir(path)
-				filesPerDir[dir]++
-			}
-		} else {
-			// Regular file, or possibly a symlink.
-			ext := filepath.Ext(path)
-			versionTag := filenameTag(path)
-			dir := filepath.Dir(path)
-			withoutExt := path[:len(path)-len(ext)-len(versionTag)-1]
-			name := withoutExt + ext
-
-			filesPerDir[dir]++
-			versionsPerFile[name] = append(versionsPerFile[name], path)
 		}
 
+		// Regular file, or possibly a symlink.
+		ext := filepath.Ext(path)
+		versionTag := filenameTag(path)
+		withoutExt := path[:len(path)-len(ext)-len(versionTag)-1]
+		name := withoutExt + ext
+
+		dirTracker.addFile(path)
+		versionsPerFile[name] = append(versionsPerFile[name], path)
+
 		return nil
-	})
+	}
-	if err != nil {
+
+	if err := v.versionsFs.Walk(".", walkFn); err != nil {
 		l.Warnln("Versioner: error scanning versions dir", err)
 		return
 	}
@@ -148,17 +145,7 @@ func (v *Staggered) clean() {
 		v.expire(versionList)
 	}
 
-	for path, numFiles := range filesPerDir {
+	dirTracker.deleteEmptyDirs(v.versionsFs)
-		if numFiles > 0 {
-			continue
-		}
-
-		l.Debugln("Cleaner: deleting empty directory", path)
-		err = v.versionsFs.Remove(path)
-		if err != nil {
-			l.Warnln("Versioner: can't remove directory", path, err)
-		}
-	}
 
 	l.Debugln("Cleaner: Finished cleaning", v.versionsFs)
 }

lib/versioner/trashcan.go

@@ -130,22 +130,15 @@ func (t *Trashcan) cleanoutArchive() error {
 	}
 
 	cutoff := time.Now().Add(time.Duration(-24*t.cleanoutDays) * time.Hour)
-	currentDir := ""
+	dirTracker := make(emptyDirTracker)
-	filesInDir := 0
+
 	walkFn := func(path string, info fs.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
 
-		if info.IsDir() {
+		if info.IsDir() && !info.IsSymlink() {
-			// We have entered a new directory. Lets check if the previous
+			dirTracker.addDir(path)
-			// directory was empty and try to remove it. We ignore failure for
-			// the time being.
-			if currentDir != "" && filesInDir == 0 {
-				t.fs.Remove(currentDir)
-			}
-			currentDir = path
-			filesInDir = 0
 			return nil
 		}
 
@@ -155,7 +148,7 @@ func (t *Trashcan) cleanoutArchive() error {
 		} else {
 			// Keep this file, and remember it so we don't unnecessarily try
 			// to remove this directory.
-			filesInDir++
+			dirTracker.addFile(path)
 		}
 		return nil
 	}
@@ -164,10 +157,7 @@ func (t *Trashcan) cleanoutArchive() error {
 		return err
 	}
 
-	// The last directory seen by the walkFn may not have been removed as it
+	dirTracker.deleteEmptyDirs(t.fs)
-	// should be.
+
-	if currentDir != "" && filesInDir == 0 {
-		t.fs.Remove(currentDir)
-	}
 	return nil
 }

lib/versioner/trashcan_test.go

@@ -31,8 +31,10 @@ func TestTrashcanCleanout(t *testing.T) {
 		{"testdata/.stversions/keep1/file2", false},
 		{"testdata/.stversions/keep2/file1", false},
 		{"testdata/.stversions/keep2/file2", true},
+		{"testdata/.stversions/keep3/keepsubdir/file1", false},
 		{"testdata/.stversions/remove/file1", true},
 		{"testdata/.stversions/remove/file2", true},
+		{"testdata/.stversions/remove/removesubdir/file1", true},
 	}
 
 	os.RemoveAll("testdata")
@@ -65,6 +67,10 @@ func TestTrashcanCleanout(t *testing.T) {
 		}
 	}
 
+	if _, err := os.Lstat("testdata/.stversions/keep3"); os.IsNotExist(err) {
+		t.Error("directory with non empty subdirs should not be removed")
+	}
+
 	if _, err := os.Lstat("testdata/.stversions/remove"); !os.IsNotExist(err) {
 		t.Error("empty directory should have been removed")
 	}