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

@@ -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
+}

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)
+		}
+	}
+}