lib/model: Fix child-check when deleting dirs in pull (#7236)

lib/model/folder_sendrecv.go

@@ -1878,61 +1878,103 @@ func (f *sendReceiveFolder) deleteDirOnDisk(dir string, snap *db.Snapshot, scanC
 		return err
 	}
 
-	files, _ := f.mtimefs.DirNames(dir)
+	if err := f.deleteDirOnDiskHandleChildren(dir, snap, scanChan); err != nil {
+		return err
+	}
+
+	err := f.inWritableDir(f.mtimefs.Remove, dir)
+	if err == nil || fs.IsNotExist(err) {
+		// It was removed or it doesn't exist to start with
+		return nil
+	}
+	if _, serr := f.mtimefs.Lstat(dir); serr != nil && !fs.IsPermission(serr) {
+		// We get an error just looking at the directory, and it's not a
+		// permission problem. Lets assume the error is in fact some variant
+		// of "file does not exist" (possibly expressed as some parent being a
+		// file and not a directory etc) and that the delete is handled.
+		return nil
+	}
 
-	toBeDeleted := make([]string, 0, len(files))
+	return err
+}
 
+func (f *sendReceiveFolder) deleteDirOnDiskHandleChildren(dir string, snap *db.Snapshot, scanChan chan<- string) error {
+	var dirsToDelete []string
 	var hasIgnored, hasKnown, hasToBeScanned, hasReceiveOnlyChanged bool
+	var delErr error
 
-	for _, dirFile := range files {
-		fullDirFile := filepath.Join(dir, dirFile)
-		switch {
-		case fs.IsTemporary(dirFile) || f.ignores.Match(fullDirFile).IsDeletable():
-			toBeDeleted = append(toBeDeleted, fullDirFile)
-			continue
-		case f.ignores != nil && f.ignores.Match(fullDirFile).IsIgnored():
+	err := f.mtimefs.Walk(dir, func(path string, info fs.FileInfo, err error) error {
+		if path == dir {
+			return nil
+		}
+		if err != nil {
+			return err
+		}
+		switch match := f.ignores.Match(path); {
+		case match.IsDeletable():
+			if info.IsDir() {
+				dirsToDelete = append(dirsToDelete, path)
+				return nil
+			}
+			fallthrough
+		case fs.IsTemporary(path):
+			if err := f.mtimefs.Remove(path); err != nil && delErr == nil {
+				delErr = err
+			}
+			return nil
+		case match.IsIgnored():
 			hasIgnored = true
-			continue
+			return nil
 		}
-		cf, ok := snap.Get(protocol.LocalDeviceID, fullDirFile)
+		cf, ok := snap.Get(protocol.LocalDeviceID, path)
 		switch {
 		case !ok || cf.IsDeleted():
 			// Something appeared in the dir that we either are not
 			// aware of at all or that we think should be deleted
 			// -> schedule scan.
-			scanChan <- fullDirFile
+			scanChan <- path
 			hasToBeScanned = true
-			continue
+			return nil
 		case ok && f.Type == config.FolderTypeReceiveOnly && cf.IsReceiveOnlyChanged():
 			hasReceiveOnlyChanged = true
-			continue
-		}
-		info, err := f.mtimefs.Lstat(fullDirFile)
-		var diskFile protocol.FileInfo
-		if err == nil {
-			diskFile, err = scanner.CreateFileInfo(info, fullDirFile, f.mtimefs)
+			return nil
 		}
+		diskFile, err := scanner.CreateFileInfo(info, path, f.mtimefs)
 		if err != nil {
 			// Lets just assume the file has changed.
-			scanChan <- fullDirFile
+			scanChan <- path
 			hasToBeScanned = true
-			continue
+			return nil
 		}
 		if !cf.IsEquivalentOptional(diskFile, f.modTimeWindow, f.IgnorePerms, true, protocol.LocalAllFlags) {
 			// File on disk changed compared to what we have in db
 			// -> schedule scan.
-			scanChan <- fullDirFile
+			scanChan <- path
 			hasToBeScanned = true
-			continue
+			return nil
 		}
 		// Dir contains file that is valid according to db and
 		// not ignored -> something weird is going on
 		hasKnown = true
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+
+	for i := range dirsToDelete {
+		if err := f.mtimefs.Remove(dirsToDelete[len(dirsToDelete)-1-i]); err != nil && delErr == nil {
+			delErr = err
+		}
 	}
 
+	// "Error precedence":
+	// Something changed on disk, check that and maybe all else gets resolved
 	if hasToBeScanned {
 		return errDirHasToBeScanned
 	}
+	// Ignored files will never be touched, i.e. this will keep failing until
+	// user acts.
 	if hasIgnored {
 		return errDirHasIgnored
 	}
@@ -1945,25 +1987,8 @@ func (f *sendReceiveFolder) deleteDirOnDisk(dir string, snap *db.Snapshot, scanC
 	if hasKnown {
 		return errDirNotEmpty
 	}
-
-	for _, del := range toBeDeleted {
-		f.mtimefs.RemoveAll(del)
-	}
-
-	err := f.inWritableDir(f.mtimefs.Remove, dir)
-	if err == nil || fs.IsNotExist(err) {
-		// It was removed or it doesn't exist to start with
-		return nil
-	}
-	if _, serr := f.mtimefs.Lstat(dir); serr != nil && !fs.IsPermission(serr) {
-		// We get an error just looking at the directory, and it's not a
-		// permission problem. Lets assume the error is in fact some variant
-		// of "file does not exist" (possibly expressed as some parent being a
-		// file and not a directory etc) and that the delete is handled.
-		return nil
-	}
-
-	return err
+	// All good, except maybe failing to remove a (?d) ignored item
+	return delErr
 }
 
 // scanIfItemChanged schedules the given file for scanning and returns errModified

lib/model/folder_sendrecv_test.go

@@ -11,6 +11,7 @@ import (
 	"context"
 	"crypto/rand"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
@@ -1350,6 +1351,32 @@ func TestPullDeleteCaseConflict(t *testing.T) {
 	}
 }
 
+func TestPullDeleteIgnoreChildDir(t *testing.T) {
+	m, f := setupSendReceiveFolder(t)
+	defer cleanupSRFolder(f, m)
+
+	parent := "parent"
+	del := "ignored"
+	child := "keep"
+	matcher := ignore.New(f.mtimefs)
+	must(t, matcher.Parse(bytes.NewBufferString(fmt.Sprintf(`
+!%v
+(?d)%v
+`, child, del)), ""))
+	f.ignores = matcher
+
+	must(t, f.mtimefs.Mkdir(parent, 0777))
+	must(t, f.mtimefs.Mkdir(filepath.Join(parent, del), 0777))
+	must(t, f.mtimefs.Mkdir(filepath.Join(parent, del, child), 0777))
+
+	scanChan := make(chan string, 2)
+
+	err := f.deleteDirOnDisk(parent, f.fset.Snapshot(), scanChan)
+	if err == nil {
+		t.Error("no error")
+	}
+}
+
 func cleanupSharedPullerState(s *sharedPullerState) {
 	s.mut.Lock()
 	defer s.mut.Unlock()