From 2df001fe5cd0cf715a7d5a4228497ab2e3a95b34 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Fri, 18 Mar 2016 08:28:44 +0000 Subject: [PATCH] lib/model: Correct handling of multiple subs when scanning (fixes #2851) Previously the code failed in that it would return top-level plus a sub, i.e. ["", "foo"], and it would consider "usr/lib" a prefix of "usr/libexec" which it is not. --- lib/model/model.go | 69 +++++++++++++++++++++----------- lib/model/model_test.go | 88 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 22 deletions(-) diff --git a/lib/model/model.go b/lib/model/model.go index ad34056ef..ca5bd4969 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -18,6 +18,7 @@ import ( "path/filepath" "reflect" "runtime" + "sort" "strings" stdsync "sync" "time" @@ -1319,28 +1320,13 @@ func (m *Model) internalScanFolderSubs(folder string, subs []string) error { return err } - // Required to make sure that we start indexing at a directory we're already - // aware off. - var unifySubs []string -nextSub: - for _, sub := range subs { - for sub != "" && sub != ".stfolder" && sub != ".stignore" { - if _, ok = fs.Get(protocol.LocalDeviceID, sub); ok { - break - } - sub = filepath.Dir(sub) - if sub == "." || sub == string(filepath.Separator) { - sub = "" - } - } - for _, us := range unifySubs { - if strings.HasPrefix(sub, us) { - continue nextSub - } - } - unifySubs = append(unifySubs, sub) - } - subs = unifySubs + // Clean the list of subitems to ensure that we start at a known + // directory, and don't scan subdirectories of things we've already + // scanned. + subs = unifySubs(subs, func(f string) bool { + _, ok := fs.Get(protocol.LocalDeviceID, f) + return ok + }) // The cancel channel is closed whenever we return (such as from an error), // to signal the potentially still running walker to stop. @@ -2086,3 +2072,42 @@ func stringSliceWithout(ss []string, s string) []string { } return ss } + +// unifySubs takes a list of files or directories and trims them down to +// themselves or the closest parent that exists() returns true for, while +// removing duplicates and subdirectories. That is, if we have foo/ in the +// list, we don't also need foo/bar/ because that's already covered. +func unifySubs(dirs []string, exists func(dir string) bool) []string { + var subs []string + + // Trim each item to itself or its closest known parent + for _, sub := range dirs { + for sub != "" && sub != ".stfolder" && sub != ".stignore" { + if exists(sub) { + break + } + sub = filepath.Dir(sub) + if sub == "." || sub == string(filepath.Separator) { + // Shortcut. We are going to scan the full folder, so we can + // just return an empty list of subs at this point. + return nil + } + } + subs = append(subs, sub) + } + + // Remove any paths that are already covered by their parent + sort.Strings(subs) + var cleaned []string +next: + for _, sub := range subs { + for _, existing := range cleaned { + if sub == existing || strings.HasPrefix(sub, existing+string(os.PathSeparator)) { + continue next + } + } + cleaned = append(cleaned, sub) + } + + return cleaned +} diff --git a/lib/model/model_test.go b/lib/model/model_test.go index 5b1d588ef..c83ccfce7 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -15,10 +15,12 @@ import ( "net" "os" "path/filepath" + "runtime" "strconv" "testing" "time" + "github.com/d4l3k/messagediff" "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/db" "github.com/syncthing/syncthing/lib/protocol" @@ -1210,3 +1212,89 @@ func TestIgnoreDelete(t *testing.T) { t.Fatal("foo should not be marked for deletion") } } + +func TestUnifySubs(t *testing.T) { + cases := []struct { + in []string // input to unifySubs + exists []string // paths that exist in the database + out []string // expected output + }{ + { + // trailing slashes are cleaned, known paths are just passed on + []string{"foo/", "bar//"}, + []string{"foo", "bar"}, + []string{"bar", "foo"}, // the output is sorted + }, + { + // "foo/bar" gets trimmed as it's covered by foo + []string{"foo", "bar/", "foo/bar/"}, + []string{"foo", "bar"}, + []string{"bar", "foo"}, + }, + { + // "bar" gets trimmed to "" as it's unknown, + // "" gets simplified to the empty list + []string{"foo", "bar", "foo/bar"}, + []string{"foo"}, + nil, + }, + { + // two independent known paths, both are kept + // "usr/lib" is not a prefix of "usr/libexec" + []string{"usr/lib", "usr/libexec"}, + []string{"usr/lib", "usr/libexec"}, + []string{"usr/lib", "usr/libexec"}, + }, + { + // "usr/lib" is a prefix of "usr/lib/exec" + []string{"usr/lib", "usr/lib/exec"}, + []string{"usr/lib", "usr/libexec"}, + []string{"usr/lib"}, + }, + { + // .stignore and .stfolder are special and are passed on + // verbatim even though they are unknown + []string{".stfolder", ".stignore"}, + []string{}, + []string{".stfolder", ".stignore"}, + }, + { + // but the presense of something else unknown forces an actual + // scan + []string{".stfolder", ".stignore", "foo/bar"}, + []string{}, + nil, + }, + } + + if runtime.GOOS == "windows" { + // Fixup path separators + for i := range cases { + for j, p := range cases[i].in { + cases[i].in[j] = filepath.FromSlash(p) + } + for j, p := range cases[i].exists { + cases[i].exists[j] = filepath.FromSlash(p) + } + for j, p := range cases[i].out { + cases[i].out[j] = filepath.FromSlash(p) + } + } + } + + for i, tc := range cases { + exists := func(f string) bool { + for _, e := range tc.exists { + if f == e { + return true + } + } + return false + } + + out := unifySubs(tc.in, exists) + if diff, equal := messagediff.PrettyDiff(tc.out, out); !equal { + t.Errorf("Case %d failed; got %v, expected %v, diff:\n%s", i, out, tc.out, diff) + } + } +}