lib/ignore, lib/scanner: Catch included items below ignored ones (#4811)

lib/fs/basicfs_watch.go

@@ -29,10 +29,6 @@ func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context
 		return nil, err
 	}
 
-	absShouldIgnore := func(absPath string) bool {
-		return ignore.ShouldIgnore(f.unrootedChecked(absPath))
-	}
-
 	outChan := make(chan Event)
 	backendChan := make(chan notify.EventInfo, backendBuffer)
 
@@ -41,7 +37,15 @@ func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context
 		eventMask |= permEventMask
 	}
 
-	if err := notify.WatchWithFilter(filepath.Join(absName, "..."), backendChan, absShouldIgnore, eventMask); err != nil {
+	if ignore.SkipIgnoredDirs() {
+		absShouldIgnore := func(absPath string) bool {
+			return ignore.ShouldIgnore(f.unrootedChecked(absPath))
+		}
+		err = notify.WatchWithFilter(filepath.Join(absName, "..."), backendChan, absShouldIgnore, eventMask)
+	} else {
+		err = notify.Watch(filepath.Join(absName, "..."), backendChan, eventMask)
+	}
+	if err != nil {
 		notify.Stop(backendChan)
 		if reachedMaxUserWatches(err) {
 			err = errors.New("failed to setup inotify handler. Please increase inotify limits, see https://docs.syncthing.net/users/faq.html#inotify-limits")

lib/fs/basicfs_watch_test.go

@@ -41,15 +41,17 @@ func TestMain(m *testing.M) {
 	testFs = NewFilesystem(FilesystemTypeBasic, testDirAbs)
 
 	backendBuffer = 10
-	defer func() {
+
+	exitCode := m.Run()
+
 	backendBuffer = 500
 	os.RemoveAll(testDir)
-	}()
-	os.Exit(m.Run())
+
+	os.Exit(exitCode)
 }
 
 const (
-	testDir = "temporary_test_root"
+	testDir = "testdata"
 )
 
 var (
@@ -75,7 +77,31 @@ func TestWatchIgnore(t *testing.T) {
 		{name, NonRemove},
 	}
 
-	testScenario(t, name, testCase, expectedEvents, allowedEvents, ignored)
+	testScenario(t, name, testCase, expectedEvents, allowedEvents, fakeMatcher{ignore: filepath.Join(name, ignored), skipIgnoredDirs: true})
+}
+
+func TestWatchInclude(t *testing.T) {
+	name := "include"
+
+	file := "file"
+	ignored := "ignored"
+	testFs.MkdirAll(filepath.Join(name, ignored), 0777)
+	included := filepath.Join(ignored, "included")
+
+	testCase := func() {
+		createTestFile(name, file)
+		createTestFile(name, included)
+	}
+
+	expectedEvents := []Event{
+		{file, NonRemove},
+		{included, NonRemove},
+	}
+	allowedEvents := []Event{
+		{name, NonRemove},
+	}
+
+	testScenario(t, name, testCase, expectedEvents, allowedEvents, fakeMatcher{ignore: filepath.Join(name, ignored), include: filepath.Join(name, included)})
 }
 
 func TestWatchRename(t *testing.T) {
@@ -104,7 +130,7 @@ func TestWatchRename(t *testing.T) {
 
 	// set the "allow others" flag because we might get the create of
 	// "oldfile" initially
-	testScenario(t, name, testCase, expectedEvents, allowedEvents, "")
+	testScenario(t, name, testCase, expectedEvents, allowedEvents, fakeMatcher{})
 }
 
 // TestWatchOutside checks that no changes from outside the folder make it in
@@ -120,14 +146,25 @@ func TestWatchOutside(t *testing.T) {
 	go func() {
 		defer func() {
 			if recover() == nil {
+				select {
+				case <-ctx.Done(): // timed out
+				default:
 					t.Fatalf("Watch did not panic on receiving event outside of folder")
 				}
+			}
 			cancel()
 		}()
 		fs.watchLoop(".", backendChan, outChan, fakeMatcher{}, ctx)
 	}()
 
 	backendChan <- fakeEventInfo(filepath.Join(filepath.Dir(testDirAbs), "outside"))
+
+	select {
+	case <-time.After(10 * time.Second):
+		cancel()
+		t.Errorf("Timed out before panicing")
+	case <-ctx.Done():
+	}
 }
 
 func TestWatchSubpath(t *testing.T) {
@@ -178,7 +215,7 @@ func TestWatchOverflow(t *testing.T) {
 		}
 	}
 
-	testScenario(t, name, testCase, expectedEvents, allowedEvents, "")
+	testScenario(t, name, testCase, expectedEvents, allowedEvents, fakeMatcher{})
 }
 
 func TestWatchErrorLinuxInterpretation(t *testing.T) {
@@ -283,7 +320,7 @@ func TestWatchIssue4877(t *testing.T) {
 		testFs = origTestFs
 	}()
 
-	testScenario(t, name, testCase, expectedEvents, allowedEvents, "")
+	testScenario(t, name, testCase, expectedEvents, allowedEvents, fakeMatcher{})
 }
 
 // path relative to folder root, also creates parent dirs if necessary
@@ -312,7 +349,7 @@ func sleepMs(ms int) {
 	time.Sleep(time.Duration(ms) * time.Millisecond)
 }
 
-func testScenario(t *testing.T, name string, testCase func(), expectedEvents, allowedEvents []Event, ignored string) {
+func testScenario(t *testing.T, name string, testCase func(), expectedEvents, allowedEvents []Event, fm fakeMatcher) {
 	if err := testFs.MkdirAll(name, 0755); err != nil {
 		panic(fmt.Sprintf("Failed to create directory %s: %s", name, err))
 	}
@@ -321,11 +358,7 @@ func testScenario(t *testing.T, name string, testCase func(), expectedEvents, al
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	if ignored != "" {
-		ignored = filepath.Join(name, ignored)
-	}
-
-	eventChan, err := testFs.Watch(name, fakeMatcher{ignored}, ctx, false)
+	eventChan, err := testFs.Watch(name, fm, ctx, false)
 	if err != nil {
 		panic(err)
 	}
@@ -335,7 +368,7 @@ func testScenario(t *testing.T, name string, testCase func(), expectedEvents, al
 	testCase()
 
 	select {
-	case <-time.NewTimer(time.Minute).C:
+	case <-time.After(time.Minute):
 		t.Errorf("Timed out before receiving all expected events")
 
 	case <-ctx.Done():
@@ -382,10 +415,19 @@ func testWatchOutput(t *testing.T, name string, in <-chan Event, expectedEvents,
 	}
 }
 
-type fakeMatcher struct{ match string }
+// Matches are done via direct comparison against both ignore and include
+type fakeMatcher struct {
+	ignore          string
+	include         string
+	skipIgnoredDirs bool
+}
 
 func (fm fakeMatcher) ShouldIgnore(name string) bool {
-	return name == fm.match
+	return name != fm.include && name == fm.ignore
+}
+
+func (fm fakeMatcher) SkipIgnoredDirs() bool {
+	return fm.skipIgnoredDirs
 }
 
 type fakeEventInfo string

lib/fs/filesystem.go

@@ -87,6 +87,7 @@ type Usage struct {
 
 type Matcher interface {
 	ShouldIgnore(name string) bool
+	SkipIgnoredDirs() bool
 }
 
 type MatchResult interface {

lib/ignore/ignore.go

@@ -85,6 +85,7 @@ type Matcher struct {
 	curHash         string
 	stop            chan struct{}
 	changeDetector  ChangeDetector
+	skipIgnoredDirs bool
 	mut             sync.Mutex
 }
 
@@ -111,6 +112,7 @@ func New(fs fs.Filesystem, opts ...Option) *Matcher {
 		fs:              fs,
 		stop:            make(chan struct{}),
 		mut:             sync.NewMutex(),
+		skipIgnoredDirs: true,
 	}
 	for _, opt := range opts {
 		opt(m)
@@ -169,6 +171,14 @@ func (m *Matcher) parseLocked(r io.Reader, file string) error {
 		return err
 	}
 
+	m.skipIgnoredDirs = true
+	for _, p := range patterns {
+		if p.result&resultInclude == resultInclude {
+			m.skipIgnoredDirs = false
+			break
+		}
+	}
+
 	m.curHash = newHash
 	m.patterns = patterns
 	if m.withCache {
@@ -291,6 +301,12 @@ func (m *Matcher) ShouldIgnore(filename string) bool {
 	return false
 }
 
+func (m *Matcher) SkipIgnoredDirs() bool {
+	m.mut.Lock()
+	defer m.mut.Unlock()
+	return m.skipIgnoredDirs
+}
+
 func hashPatterns(patterns []Pattern) string {
 	h := md5.New()
 	for _, pat := range patterns {

lib/scanner/testdata/.stignore

@@ -2,3 +2,4 @@
 
 bfile
 dir1/cfile
+/dir2/dir21

lib/scanner/walk.go

@@ -8,7 +8,9 @@ package scanner
 
 import (
 	"context"
+	"path/filepath"
 	"runtime"
+	"strings"
 	"sync/atomic"
 	"time"
 	"unicode/utf8"
@@ -196,6 +198,8 @@ func (w *walker) walk(ctx context.Context) chan protocol.FileInfo {
 
 func (w *walker) walkAndHashFiles(ctx context.Context, fchan, dchan chan protocol.FileInfo) fs.WalkFunc {
 	now := time.Now()
+	ignoredParent := ""
+
 	return func(path string, info fs.FileInfo, err error) error {
 		select {
 		case <-ctx.Done():
@@ -220,12 +224,6 @@ func (w *walker) walkAndHashFiles(ctx context.Context, fchan, dchan chan protoco
 			return nil
 		}
 
-		info, err = w.Filesystem.Lstat(path)
-		// An error here would be weird as we've already gotten to this point, but act on it nonetheless
-		if err != nil {
-			return skip
-		}
-
 		if fs.IsTemporary(path) {
 			l.Debugln("temporary:", path)
 			if info.IsRegular() && info.ModTime().Add(w.TempLifetime).Before(now) {
@@ -240,13 +238,59 @@ func (w *walker) walkAndHashFiles(ctx context.Context, fchan, dchan chan protoco
 			return skip
 		}
 
-		if w.Matcher.Match(path).IsIgnored() {
-			l.Debugln("ignored (patterns):", path)
+		if !utf8.ValidString(path) {
+			l.Warnf("File name %q is not in UTF8 encoding; skipping.", path)
 			return skip
 		}
 
-		if !utf8.ValidString(path) {
-			l.Warnf("File name %q is not in UTF8 encoding; skipping.", path)
+		if w.Matcher.Match(path).IsIgnored() {
+			l.Debugln("ignored (patterns):", path)
+			// Only descend if matcher says so and the current file is not a symlink.
+			if w.Matcher.SkipIgnoredDirs() || (info.IsSymlink() && info.IsDir()) {
+				return fs.SkipDir
+			}
+			// If the parent wasn't ignored already, set this path as the "highest" ignored parent
+			if info.IsDir() && (ignoredParent == "" || !strings.HasPrefix(path, ignoredParent+string(fs.PathSeparator))) {
+				ignoredParent = path
+			}
+			return nil
+		}
+
+		if ignoredParent == "" {
+			// parent isn't ignored, nothing special
+			return w.handleItem(ctx, path, fchan, dchan, skip)
+		}
+
+		// Part of current path below the ignored (potential) parent
+		rel := strings.TrimPrefix(path, ignoredParent+string(fs.PathSeparator))
+
+		// ignored path isn't actually a parent of the current path
+		if rel == path {
+			ignoredParent = ""
+			return w.handleItem(ctx, path, fchan, dchan, skip)
+		}
+
+		// The previously ignored parent directories of the current, not
+		// ignored path need to be handled as well.
+		if err = w.handleItem(ctx, ignoredParent, fchan, dchan, skip); err != nil {
+			return err
+		}
+		for _, name := range strings.Split(rel, string(fs.PathSeparator)) {
+			ignoredParent = filepath.Join(ignoredParent, name)
+			if err = w.handleItem(ctx, ignoredParent, fchan, dchan, skip); err != nil {
+				return err
+			}
+		}
+		ignoredParent = ""
+
+		return nil
+	}
+}
+
+func (w *walker) handleItem(ctx context.Context, path string, fchan, dchan chan protocol.FileInfo, skip error) error {
+	info, err := w.Filesystem.Lstat(path)
+	// An error here would be weird as we've already gotten to this point, but act on it nonetheless
+	if err != nil {
 		return skip
 	}
 
@@ -274,7 +318,6 @@ func (w *walker) walkAndHashFiles(ctx context.Context, fchan, dchan chan protoco
 	}
 
 	return err
-	}
 }
 
 func (w *walker) walkRegular(ctx context.Context, relPath string, info fs.FileInfo, fchan chan protocol.FileInfo) error {

lib/scanner/walk_test.go

@@ -38,6 +38,8 @@ type testfile struct {
 
 type testfileList []testfile
 
+var testFs fs.Filesystem
+
 var testdata = testfileList{
 	{"afile", 4, "b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c"},
 	{"dir1", 128, ""},
@@ -53,17 +55,19 @@ func init() {
 	// Limit the stack size to 10 megs to crash early in that case instead of
 	// potentially taking down the box...
 	rdebug.SetMaxStack(10 * 1 << 20)
+
+	testFs = fs.NewFilesystem(fs.FilesystemTypeBasic, "testdata")
 }
 
 func TestWalkSub(t *testing.T) {
-	ignores := ignore.New(fs.NewFilesystem(fs.FilesystemTypeBasic, "."))
-	err := ignores.Load("testdata/.stignore")
+	ignores := ignore.New(testFs)
+	err := ignores.Load(".stignore")
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	fchan := Walk(context.TODO(), Config{
-		Filesystem: fs.NewFilesystem(fs.FilesystemTypeBasic, "testdata"),
+		Filesystem: testFs,
 		Subs:       []string{"dir2"},
 		Matcher:    ignores,
 		Hashers:    2,
@@ -88,15 +92,15 @@ func TestWalkSub(t *testing.T) {
 }
 
 func TestWalk(t *testing.T) {
-	ignores := ignore.New(fs.NewFilesystem(fs.FilesystemTypeBasic, "."))
-	err := ignores.Load("testdata/.stignore")
+	ignores := ignore.New(testFs)
+	err := ignores.Load(".stignore")
 	if err != nil {
 		t.Fatal(err)
 	}
 	t.Log(ignores)
 
 	fchan := Walk(context.TODO(), Config{
-		Filesystem: fs.NewFilesystem(fs.FilesystemTypeBasic, "testdata"),
+		Filesystem: testFs,
 		Matcher:    ignores,
 		Hashers:    2,
 	})
@@ -192,11 +196,9 @@ func TestNormalization(t *testing.T) {
 
 	numValid := len(tests) - numInvalid
 
-	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, ".")
-
 	for _, s1 := range tests {
 		// Create a directory for each of the interesting strings above
-		if err := fs.MkdirAll(filepath.Join("testdata/normalization", s1), 0755); err != nil {
+		if err := testFs.MkdirAll(filepath.Join("normalization", s1), 0755); err != nil {
 			t.Fatal(err)
 		}
 
@@ -205,7 +207,7 @@ func TestNormalization(t *testing.T) {
 			// file names. Ensure that the file doesn't exist when it's
 			// created. This detects and fails if there's file name
 			// normalization stuff at the filesystem level.
-			if fd, err := fs.OpenFile(filepath.Join("testdata/normalization", s1, s2), os.O_CREATE|os.O_EXCL, 0644); err != nil {
+			if fd, err := testFs.OpenFile(filepath.Join("normalization", s1, s2), os.O_CREATE|os.O_EXCL, 0644); err != nil {
 				t.Fatal(err)
 			} else {
 				fd.Write([]byte("test"))
@@ -219,14 +221,8 @@ func TestNormalization(t *testing.T) {
 	// make sure it all gets done. In production, things will be correct
 	// eventually...
 
-	_, err := walkDir(fs, "testdata/normalization", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-	tmp, err := walkDir(fs, "testdata/normalization", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
+	walkDir(testFs, "normalization", nil, nil)
+	tmp := walkDir(testFs, "normalization", nil, nil)
 
 	files := fileList(tmp).testfiles()
 
@@ -268,9 +264,10 @@ func TestWalkSymlinkUnix(t *testing.T) {
 	defer os.RemoveAll("_symlinks")
 	os.Symlink("../testdata", "_symlinks/link")
 
+	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, "_symlinks")
 	for _, path := range []string{".", "link"} {
 		// Scan it
-		files, _ := walkDir(fs.NewFilesystem(fs.FilesystemTypeBasic, "_symlinks"), path, nil)
+		files := walkDir(fs, path, nil, nil)
 
 		// Verify that we got one symlink and with the correct attributes
 		if len(files) != 1 {
@@ -291,9 +288,11 @@ func TestWalkSymlinkWindows(t *testing.T) {
 	}
 
 	// Create a folder with a symlink in it
-	os.RemoveAll("_symlinks")
-	os.Mkdir("_symlinks", 0755)
-	defer os.RemoveAll("_symlinks")
+	name := "_symlinks-win"
+	os.RemoveAll(name)
+	os.Mkdir(name, 0755)
+	defer os.RemoveAll(name)
+	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, name)
 	if err := osutil.DebugSymlinkForTestsOnly("../testdata", "_symlinks/link"); err != nil {
 		// Probably we require permissions we don't have.
 		t.Skip(err)
@@ -301,7 +300,7 @@ func TestWalkSymlinkWindows(t *testing.T) {
 
 	for _, path := range []string{".", "link"} {
 		// Scan it
-		files, _ := walkDir(fs.NewFilesystem(fs.FilesystemTypeBasic, "_symlinks"), path, nil)
+		files := walkDir(fs, path, nil, nil)
 
 		// Verify that we got zero symlinks
 		if len(files) != 0 {
@@ -330,10 +329,8 @@ func TestWalkRootSymlink(t *testing.T) {
 	}
 
 	// Scan it
-	files, err := walkDir(fs.NewFilesystem(fs.FilesystemTypeBasic, link), ".", nil)
-	if err != nil {
-		t.Fatal("Expected no error when root folder path is provided via a symlink: " + err.Error())
-	}
+	files := walkDir(fs.NewFilesystem(fs.FilesystemTypeBasic, link), ".", nil, nil)
+
 	// Verify that we got two files
 	if len(files) != 2 {
 		t.Errorf("expected two files, not %d", len(files))
@@ -352,10 +349,7 @@ func TestBlocksizeHysteresis(t *testing.T) {
 	current := make(fakeCurrentFiler)
 
 	runTest := func(expectedBlockSize int) {
-		files, err := walkDir(sf, ".", current)
-		if err != nil {
-			t.Fatal(err)
-		}
+		files := walkDir(sf, ".", current, nil)
 		if len(files) != 1 {
 			t.Fatalf("expected one file, not %d", len(files))
 		}
@@ -409,7 +403,7 @@ func TestBlocksizeHysteresis(t *testing.T) {
 	runTest(512 << 10)
 }
 
-func walkDir(fs fs.Filesystem, dir string, cfiler CurrentFiler) ([]protocol.FileInfo, error) {
+func walkDir(fs fs.Filesystem, dir string, cfiler CurrentFiler, matcher *ignore.Matcher) []protocol.FileInfo {
 	fchan := Walk(context.TODO(), Config{
 		Filesystem:     fs,
 		Subs:           []string{dir},
@@ -417,6 +411,7 @@ func walkDir(fs fs.Filesystem, dir string, cfiler CurrentFiler) ([]protocol.File
 		Hashers:        2,
 		UseLargeBlocks: true,
 		CurrentFiler:   cfiler,
+		Matcher:        matcher,
 	})
 
 	var tmp []protocol.FileInfo
@@ -425,7 +420,7 @@ func walkDir(fs fs.Filesystem, dir string, cfiler CurrentFiler) ([]protocol.File
 	}
 	sort.Sort(fileList(tmp))
 
-	return tmp, nil
+	return tmp
 }
 
 type fileList []protocol.FileInfo
@@ -580,15 +575,53 @@ func TestIssue4799(t *testing.T) {
 	}
 	fd.Close()
 
-	files, err := walkDir(fs, "/foo", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
+	files := walkDir(fs, "/foo", nil, nil)
 	if len(files) != 1 || files[0].Name != "foo" {
 		t.Error(`Received unexpected file infos when walking "/foo"`, files)
 	}
 }
 
+func TestRecurseInclude(t *testing.T) {
+	stignore := `
+	!/dir1/cfile
+	!efile
+	!ffile
+	*
+	`
+	ignores := ignore.New(testFs, ignore.WithCache(true))
+	if err := ignores.Parse(bytes.NewBufferString(stignore), ".stignore"); err != nil {
+		t.Fatal(err)
+	}
+
+	files := walkDir(testFs, ".", nil, ignores)
+
+	expected := []string{
+		filepath.Join("dir1"),
+		filepath.Join("dir1", "cfile"),
+		filepath.Join("dir2"),
+		filepath.Join("dir2", "dir21"),
+		filepath.Join("dir2", "dir21", "dir22"),
+		filepath.Join("dir2", "dir21", "dir22", "dir23"),
+		filepath.Join("dir2", "dir21", "dir22", "dir23", "efile"),
+		filepath.Join("dir2", "dir21", "dir22", "efile"),
+		filepath.Join("dir2", "dir21", "dir22", "efile", "efile"),
+		filepath.Join("dir2", "dir21", "dira"),
+		filepath.Join("dir2", "dir21", "dira", "efile"),
+		filepath.Join("dir2", "dir21", "dira", "ffile"),
+		filepath.Join("dir2", "dir21", "efile"),
+		filepath.Join("dir2", "dir21", "efile", "ign"),
+		filepath.Join("dir2", "dir21", "efile", "ign", "efile"),
+	}
+	if len(files) != len(expected) {
+		t.Fatalf("Got %d files %v, expected %d files at %v", len(files), files, len(expected), expected)
+	}
+	for i := range files {
+		if files[i].Name != expected[i] {
+			t.Errorf("Got %v, expected file at %v", files[i], expected[i])
+		}
+	}
+}
+
 func TestIssue4841(t *testing.T) {
 	tmp, err := ioutil.TempDir("", "")
 	if err != nil {