From 9471b9f6af2aa84720ae92f4f0b748acb84ff306 Mon Sep 17 00:00:00 2001 From: Dmitry Saveliev Date: Sat, 18 Nov 2017 15:56:53 +0000 Subject: [PATCH] lib/versioner: Purge the empty directories in .stversions (fixes #4406) GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4514 LGTM: AudriusButkevicius, imsodin --- AUTHORS | 1 + NICKS | 1 + .../syncthing/core/aboutModalView.html | 2 +- lib/versioner/empty_dir_tracker.go | 51 +++++++++++++ lib/versioner/empty_dir_tracker_test.go | 75 +++++++++++++++++++ lib/versioner/staggered.go | 47 +++++------- lib/versioner/trashcan.go | 24 ++---- lib/versioner/trashcan_test.go | 6 ++ 8 files changed, 159 insertions(+), 48 deletions(-) create mode 100644 lib/versioner/empty_dir_tracker.go create mode 100644 lib/versioner/empty_dir_tracker_test.go diff --git a/AUTHORS b/AUTHORS index eea052725..e76ea0690 100644 --- a/AUTHORS +++ b/AUTHORS @@ -41,6 +41,7 @@ Darshil Chanpura (dtchanpura) David Rimmer (dinosore) Denis A. (dva) Dennis Wilson (snnd) +Dmitry Saveliev (dsaveliev) Dominik Heidler (asdil12) Elias Jarlebring (jarlebring) Emil Hessman (ceh) diff --git a/NICKS b/NICKS index c573f7834..03733b267 100644 --- a/NICKS +++ b/NICKS @@ -36,6 +36,7 @@ ceh cqcallaw damajor dinosore +dsaveliev dtchanpura dtchanpura dva diff --git a/gui/default/syncthing/core/aboutModalView.html b/gui/default/syncthing/core/aboutModalView.html index 09066b157..225afe2c4 100644 --- a/gui/default/syncthing/core/aboutModalView.html +++ b/gui/default/syncthing/core/aboutModalView.html @@ -12,7 +12,7 @@

Copyright © 2014-2017 the following Contributors:

-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.
diff --git a/lib/versioner/empty_dir_tracker.go b/lib/versioner/empty_dir_tracker.go new file mode 100644 index 000000000..0e4a27b2b --- /dev/null +++ b/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) + } + } +} diff --git a/lib/versioner/empty_dir_tracker_test.go b/lib/versioner/empty_dir_tracker_test.go new file mode 100644 index 000000000..107a7fc2c --- /dev/null +++ b/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) + } +} diff --git a/lib/versioner/staggered.go b/lib/versioner/staggered.go index 1f69b6bdf..a3cecb6c2 100644 --- a/lib/versioner/staggered.go +++ b/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 - if path != "." { - 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) + dirTracker.addDir(path) + return nil } + // 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 { - 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) - } - } + dirTracker.deleteEmptyDirs(v.versionsFs) l.Debugln("Cleaner: Finished cleaning", v.versionsFs) } diff --git a/lib/versioner/trashcan.go b/lib/versioner/trashcan.go index 7f9efa685..70e65ac83 100644 --- a/lib/versioner/trashcan.go +++ b/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 := "" - filesInDir := 0 + dirTracker := make(emptyDirTracker) + walkFn := func(path string, info fs.FileInfo, err error) error { if err != nil { return err } - if info.IsDir() { - // We have entered a new directory. Lets check if the previous - // 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 + if info.IsDir() && !info.IsSymlink() { + dirTracker.addDir(path) 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 - // should be. - if currentDir != "" && filesInDir == 0 { - t.fs.Remove(currentDir) - } + dirTracker.deleteEmptyDirs(t.fs) + return nil } diff --git a/lib/versioner/trashcan_test.go b/lib/versioner/trashcan_test.go index 1be3c8512..023e5806b 100644 --- a/lib/versioner/trashcan_test.go +++ b/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") }