lib/model: Consider existing file when handling symlink on windows (#6977)

lib/model/folder_sendrecv.go

@@ -387,6 +387,10 @@ func (f *sendReceiveFolder) processNeeded(snap *db.Snapshot, dbUpdateChan chan<-
 			}
 
 		case runtime.GOOS == "windows" && file.IsSymlink():
+			if err := f.handleSymlinkCheckExisting(file, snap, scanChan); err != nil {
+				f.newPullError(file.Name, fmt.Errorf("handling unsupported symlink: %w", err))
+				break
+			}
 			file.SetUnsupported(f.shortID)
 			l.Debugln(f, "Invalidating symlink (unsupported)", file.Name)
 			dbUpdateChan <- dbUpdateJob{file, dbUpdateInvalidate}
@@ -728,39 +732,9 @@ func (f *sendReceiveFolder) handleSymlink(file protocol.FileInfo, snap *db.Snaps
 		return
 	}
 
-	// There is already something under that name, we need to handle that.
+	if f.handleSymlinkCheckExisting(file, snap, scanChan); err != nil {
-	switch info, err := f.fs.Lstat(file.Name); {
+		f.newPullError(file.Name, fmt.Errorf("handling symlink: %w", err))
-	case err != nil && !fs.IsNotExist(err):
-		f.newPullError(file.Name, errors.Wrap(err, "checking for existing symlink"))
 		return
-	case err == nil:
-		// Check that it is what we have in the database.
-		curFile, hasCurFile := f.model.CurrentFolderFile(f.folderID, file.Name)
-		if err := f.scanIfItemChanged(file.Name, info, curFile, hasCurFile, scanChan); err != nil {
-			err = errors.Wrap(err, "handling symlink")
-			f.newPullError(file.Name, err)
-			return
-		}
-		// Remove it to replace with the symlink. This also handles the
-		// "change symlink type" path.
-		if !curFile.IsDirectory() && !curFile.IsSymlink() && f.inConflict(curFile.Version, file.Version) {
-			// The new file has been changed in conflict with the existing one. We
-			// should file it away as a conflict instead of just removing or
-			// archiving. Also merge with the version vector we had, to indicate
-			// we have resolved the conflict.
-			// Directories and symlinks aren't checked for conflicts.
-
-			file.Version = file.Version.Merge(curFile.Version)
-			err = f.inWritableDir(func(name string) error {
-				return f.moveForConflict(name, file.ModifiedBy.String(), scanChan)
-			}, curFile.Name)
-		} else {
-			err = f.deleteItemOnDisk(curFile, snap, scanChan)
-		}
-		if err != nil {
-			f.newPullError(file.Name, errors.Wrap(err, "symlink remove"))
-			return
-		}
 	}
 
 	// We declare a function that acts on only the path name, so
@@ -779,6 +753,38 @@ func (f *sendReceiveFolder) handleSymlink(file protocol.FileInfo, snap *db.Snaps
 	}
 }
 
+func (f *sendReceiveFolder) handleSymlinkCheckExisting(file protocol.FileInfo, snap *db.Snapshot, scanChan chan<- string) error {
+	// If there is already something under that name, we need to handle that.
+	info, err := f.fs.Lstat(file.Name)
+	if err != nil {
+		if fs.IsNotExist(err) {
+			return nil
+		}
+		return err
+	}
+	// Check that it is what we have in the database.
+	curFile, hasCurFile := f.model.CurrentFolderFile(f.folderID, file.Name)
+	if err := f.scanIfItemChanged(file.Name, info, curFile, hasCurFile, scanChan); err != nil {
+		return err
+	}
+	// Remove it to replace with the symlink. This also handles the
+	// "change symlink type" path.
+	if !curFile.IsDirectory() && !curFile.IsSymlink() && f.inConflict(curFile.Version, file.Version) {
+		// The new file has been changed in conflict with the existing one. We
+		// should file it away as a conflict instead of just removing or
+		// archiving. Also merge with the version vector we had, to indicate
+		// we have resolved the conflict.
+		// Directories and symlinks aren't checked for conflicts.
+
+		file.Version = file.Version.Merge(curFile.Version)
+		return f.inWritableDir(func(name string) error {
+			return f.moveForConflict(name, file.ModifiedBy.String(), scanChan)
+		}, curFile.Name)
+	} else {
+		return f.deleteItemOnDisk(curFile, snap, scanChan)
+	}
+}
+
 // deleteDir attempts to remove a directory that was deleted on a remote
 func (f *sendReceiveFolder) deleteDir(file protocol.FileInfo, snap *db.Snapshot, dbUpdateChan chan<- dbUpdateJob, scanChan chan<- string) {
 	// Used in the defer closure below, updated by the function body. Take

lib/model/folder_sendrecv_test.go

@@ -1281,6 +1281,50 @@ func TestPullCaseOnlyRename(t *testing.T) {
 	}
 }
 
+func TestPullSymlinkOverExistingWindows(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		t.Skip()
+	}
+
+	m, f := setupSendReceiveFolder()
+	defer cleanupSRFolder(f, m)
+
+	name := "foo"
+	if fd, err := f.fs.Create(name); err != nil {
+		t.Fatal(err)
+	} else {
+		if _, err := fd.Write([]byte("data")); err != nil {
+			t.Fatal(err)
+		}
+		fd.Close()
+	}
+
+	must(t, f.scanSubdirs(nil))
+
+	file, ok := m.CurrentFolderFile(f.ID, name)
+	if !ok {
+		t.Fatal("file missing")
+	}
+	m.Index(device1, f.ID, []protocol.FileInfo{{Name: name, Type: protocol.FileInfoTypeSymlink, Version: file.Version.Update(device1.Short())}})
+
+	scanChan := make(chan string)
+
+	changed := f.pullerIteration(scanChan)
+	if changed != 1 {
+		t.Error("Expected one change in pull, got", changed)
+	}
+	if file, ok := m.CurrentFolderFile(f.ID, name); !ok {
+		t.Error("symlink entry missing")
+	} else if !file.IsUnsupported() {
+		t.Error("symlink entry isn't marked as unsupported")
+	}
+	if _, err := f.fs.Lstat(name); err == nil {
+		t.Error("old file still exists on disk")
+	} else if !fs.IsNotExist(err) {
+		t.Error(err)
+	}
+}
+
 func cleanupSharedPullerState(s *sharedPullerState) {
 	s.mut.Lock()
 	defer s.mut.Unlock()