Merge pull request #1194 from syncthing/fix-1186

Use comma-ok idiom to signal files missing in database (fixes #1186)

internal/files/leveldb.go

@@ -594,11 +594,11 @@ func ldbWithAllFolderTruncated(db *leveldb.DB, folder []byte, fn func(device []b
 	}
 }
 
-func ldbGet(db *leveldb.DB, folder, device, file []byte) protocol.FileInfo {
+func ldbGet(db *leveldb.DB, folder, device, file []byte) (protocol.FileInfo, bool) {
 	nk := deviceKey(folder, device, file)
 	bs, err := db.Get(nk, nil)
 	if err == leveldb.ErrNotFound {
-		return protocol.FileInfo{}
+		return protocol.FileInfo{}, false
 	}
 	if err != nil {
 		panic(err)
@@ -609,10 +609,10 @@ func ldbGet(db *leveldb.DB, folder, device, file []byte) protocol.FileInfo {
 	if err != nil {
 		panic(err)
 	}
-	return f
+	return f, true
 }
 
-func ldbGetGlobal(db *leveldb.DB, folder, file []byte) protocol.FileInfo {
+func ldbGetGlobal(db *leveldb.DB, folder, file []byte) (protocol.FileInfo, bool) {
 	k := globalKey(folder, file)
 	snap, err := db.GetSnapshot()
 	if err != nil {
@@ -633,7 +633,7 @@ func ldbGetGlobal(db *leveldb.DB, folder, file []byte) protocol.FileInfo {
 	}
 	bs, err := snap.Get(k, nil)
 	if err == leveldb.ErrNotFound {
-		return protocol.FileInfo{}
+		return protocol.FileInfo{}, false
 	}
 	if err != nil {
 		panic(err)
@@ -663,7 +663,7 @@ func ldbGetGlobal(db *leveldb.DB, folder, file []byte) protocol.FileInfo {
 	if err != nil {
 		panic(err)
 	}
-	return f
+	return f, true
 }
 
 func ldbWithGlobal(db *leveldb.DB, folder []byte, truncate bool, fn fileIterator) {

internal/files/set.go

@@ -118,8 +118,8 @@ func (s *Set) Update(device protocol.DeviceID, fs []protocol.FileInfo) {
 		discards := make([]protocol.FileInfo, 0, len(fs))
 		updates := make([]protocol.FileInfo, 0, len(fs))
 		for _, newFile := range fs {
-			existingFile := ldbGet(s.db, []byte(s.folder), device[:], []byte(newFile.Name))
+			existingFile, ok := ldbGet(s.db, []byte(s.folder), device[:], []byte(newFile.Name))
-			if existingFile.Version <= newFile.Version {
+			if !ok || existingFile.Version <= newFile.Version {
 				discards = append(discards, existingFile)
 				updates = append(updates, newFile)
 			}
@@ -174,16 +174,16 @@ func (s *Set) WithGlobalTruncated(fn fileIterator) {
 	ldbWithGlobal(s.db, []byte(s.folder), true, nativeFileIterator(fn))
 }
 
-func (s *Set) Get(device protocol.DeviceID, file string) protocol.FileInfo {
+func (s *Set) Get(device protocol.DeviceID, file string) (protocol.FileInfo, bool) {
-	f := ldbGet(s.db, []byte(s.folder), device[:], []byte(osutil.NormalizedFilename(file)))
+	f, ok := ldbGet(s.db, []byte(s.folder), device[:], []byte(osutil.NormalizedFilename(file)))
 	f.Name = osutil.NativeFilename(f.Name)
-	return f
+	return f, ok
 }
 
-func (s *Set) GetGlobal(file string) protocol.FileInfo {
+func (s *Set) GetGlobal(file string) (protocol.FileInfo, bool) {
-	f := ldbGetGlobal(s.db, []byte(s.folder), []byte(osutil.NormalizedFilename(file)))
+	f, ok := ldbGetGlobal(s.db, []byte(s.folder), []byte(osutil.NormalizedFilename(file)))
 	f.Name = osutil.NativeFilename(f.Name)
-	return f
+	return f, ok
 }
 
 func (s *Set) Availability(file string) []protocol.DeviceID {

internal/files/set_test.go

@@ -209,27 +209,42 @@ func TestGlobalSet(t *testing.T) {
 		t.Errorf("Need incorrect;\n A: %v !=\n E: %v", n, expectedRemoteNeed)
 	}
 
-	f := m.Get(protocol.LocalDeviceID, "b")
+	f, ok := m.Get(protocol.LocalDeviceID, "b")
+	if !ok {
+		t.Error("Unexpectedly not OK")
+	}
 	if fmt.Sprint(f) != fmt.Sprint(localTot[1]) {
 		t.Errorf("Get incorrect;\n A: %v !=\n E: %v", f, localTot[1])
 	}
 
-	f = m.Get(remoteDevice0, "b")
+	f, ok = m.Get(remoteDevice0, "b")
+	if !ok {
+		t.Error("Unexpectedly not OK")
+	}
 	if fmt.Sprint(f) != fmt.Sprint(remote1[0]) {
 		t.Errorf("Get incorrect;\n A: %v !=\n E: %v", f, remote1[0])
 	}
 
-	f = m.GetGlobal("b")
+	f, ok = m.GetGlobal("b")
+	if !ok {
+		t.Error("Unexpectedly not OK")
+	}
 	if fmt.Sprint(f) != fmt.Sprint(remote1[0]) {
 		t.Errorf("GetGlobal incorrect;\n A: %v !=\n E: %v", f, remote1[0])
 	}
 
-	f = m.Get(protocol.LocalDeviceID, "zz")
+	f, ok = m.Get(protocol.LocalDeviceID, "zz")
+	if ok {
+		t.Error("Unexpectedly OK")
+	}
 	if f.Name != "" {
 		t.Errorf("Get incorrect;\n A: %v !=\n E: %v", f, protocol.FileInfo{})
 	}
 
-	f = m.GetGlobal("zz")
+	f, ok = m.GetGlobal("zz")
+	if ok {
+		t.Error("Unexpectedly OK")
+	}
 	if f.Name != "" {
 		t.Errorf("GetGlobal incorrect;\n A: %v !=\n E: %v", f, protocol.FileInfo{})
 	}

internal/model/model.go

@@ -440,13 +440,17 @@ func (m *Model) NeedFolderFiles(folder string, max int) ([]protocol.FileInfoTrun
 			seen = make(map[string]bool, len(progressNames)+len(queuedNames))
 
 			for i, name := range progressNames {
-				progress[i] = rf.GetGlobal(name).ToTruncated() /// XXX: Should implement GetGlobalTruncated directly
+				if f, ok := rf.GetGlobal(name); ok {
-				seen[name] = true
+					progress[i] = f.ToTruncated() /// XXX: Should implement GetGlobalTruncated directly
+					seen[name] = true
+				}
 			}
 
 			for i, name := range queuedNames {
-				queued[i] = rf.GetGlobal(name).ToTruncated() /// XXX: Should implement GetGlobalTruncated directly
+				if f, ok := rf.GetGlobal(name); ok {
-				seen[name] = true
+					queued[i] = f.ToTruncated() /// XXX: Should implement GetGlobalTruncated directly
+					seen[name] = true
+				}
 			}
 		}
 		left := max - len(progress) - len(queued)
@@ -704,7 +708,11 @@ func (m *Model) Request(deviceID protocol.DeviceID, folder, name string, offset
 		return nil, ErrNoSuchFile
 	}
 
-	lf := r.Get(protocol.LocalDeviceID, name)
+	lf, ok := r.Get(protocol.LocalDeviceID, name)
+	if !ok {
+		return nil, ErrNoSuchFile
+	}
+
 	if lf.IsInvalid() || lf.IsDeleted() {
 		if debug {
 			l.Debugf("%v REQ(in): %s: %q / %q o=%d s=%d; invalid: %v", m, deviceID, folder, name, offset, size, lf)
@@ -759,18 +767,18 @@ func (m *Model) ReplaceLocal(folder string, fs []protocol.FileInfo) {
 	m.fmut.RUnlock()
 }
 
-func (m *Model) CurrentFolderFile(folder string, file string) protocol.FileInfo {
+func (m *Model) CurrentFolderFile(folder string, file string) (protocol.FileInfo, bool) {
 	m.fmut.RLock()
-	f := m.folderFiles[folder].Get(protocol.LocalDeviceID, file)
+	f, ok := m.folderFiles[folder].Get(protocol.LocalDeviceID, file)
 	m.fmut.RUnlock()
-	return f
+	return f, ok
 }
 
-func (m *Model) CurrentGlobalFile(folder string, file string) protocol.FileInfo {
+func (m *Model) CurrentGlobalFile(folder string, file string) (protocol.FileInfo, bool) {
 	m.fmut.RLock()
-	f := m.folderFiles[folder].GetGlobal(file)
+	f, ok := m.folderFiles[folder].GetGlobal(file)
 	m.fmut.RUnlock()
-	return f
+	return f, ok
 }
 
 type cFiler struct {
@@ -779,7 +787,7 @@ type cFiler struct {
 }
 
 // Implements scanner.CurrentFiler
-func (cf cFiler) CurrentFile(file string) protocol.FileInfo {
+func (cf cFiler) CurrentFile(file string) (protocol.FileInfo, bool) {
 	return cf.m.CurrentFolderFile(cf.r, file)
 }
 
@@ -1309,8 +1317,8 @@ func (m *Model) Override(folder string) {
 			batch = batch[:0]
 		}
 
-		have := fs.Get(protocol.LocalDeviceID, need.Name)
+		have, ok := fs.Get(protocol.LocalDeviceID, need.Name)
-		if have.Name != need.Name {
+		if !ok || have.Name != need.Name {
 			// We are missing the file
 			need.Flags |= protocol.FlagDeleted
 			need.Blocks = nil

internal/model/puller.go

@@ -348,8 +348,12 @@ func (p *Puller) pullerIteration(ignores *ignore.Matcher) int {
 		if !ok {
 			break
 		}
-		f := p.model.CurrentGlobalFile(p.folder, fileName)
+		if f, ok := p.model.CurrentGlobalFile(p.folder, fileName); ok {
-		p.handleFile(f, copyChan, finisherChan)
+			p.handleFile(f, copyChan, finisherChan)
+		} else {
+			// File is no longer in the index. Mark it as done and drop it.
+			p.queue.Done(fileName)
+		}
 	}
 
 	// Signal copy and puller routines that we are done with the in data for
@@ -386,7 +390,7 @@ func (p *Puller) handleDir(file protocol.FileInfo) {
 	}
 
 	if debug {
-		curFile := p.model.CurrentFolderFile(p.folder, file.Name)
+		curFile, _ := p.model.CurrentFolderFile(p.folder, file.Name)
 		l.Debugf("need dir\n\t%v\n\t%v", file, curFile)
 	}
 
@@ -480,9 +484,9 @@ func (p *Puller) deleteFile(file protocol.FileInfo) {
 // handleFile queues the copies and pulls as necessary for a single new or
 // changed file.
 func (p *Puller) handleFile(file protocol.FileInfo, copyChan chan<- copyBlocksState, finisherChan chan<- *sharedPullerState) {
-	curFile := p.model.CurrentFolderFile(p.folder, file.Name)
+	curFile, ok := p.model.CurrentFolderFile(p.folder, file.Name)
 
-	if len(curFile.Blocks) == len(file.Blocks) && scanner.BlocksEqual(curFile.Blocks, file.Blocks) {
+	if ok && len(curFile.Blocks) == len(file.Blocks) && scanner.BlocksEqual(curFile.Blocks, file.Blocks) {
 		// We are supposed to copy the entire file, and then fetch nothing. We
 		// are only updating metadata, so we don't actually *need* to make the
 		// copy.

internal/scanner/walk.go

@@ -60,7 +60,7 @@ type TempNamer interface {
 
 type CurrentFiler interface {
 	// CurrentFile returns the file as seen at last scan.
-	CurrentFile(name string) protocol.FileInfo
+	CurrentFile(name string) (protocol.FileInfo, bool)
 }
 
 // Walk returns the list of files found in the local folder by scanning the
@@ -183,13 +183,14 @@ func (w *Walker) walkAndHashFiles(fchan chan protocol.FileInfo) filepath.WalkFun
 
 			if w.CurrentFiler != nil {
 				// A symlink is "unchanged", if
+				//  - it exists
 				//  - it wasn't deleted (because it isn't now)
 				//  - it was a symlink
 				//  - it wasn't invalid
 				//  - the symlink type (file/dir) was the same
 				//  - the block list (i.e. hash of target) was the same
-				cf := w.CurrentFiler.CurrentFile(rn)
+				cf, ok := w.CurrentFiler.CurrentFile(rn)
-				if !cf.IsDeleted() && cf.IsSymlink() && !cf.IsInvalid() && SymlinkTypeEqual(flags, cf.Flags) && BlocksEqual(cf.Blocks, blocks) {
+				if ok && !cf.IsDeleted() && cf.IsSymlink() && !cf.IsInvalid() && SymlinkTypeEqual(flags, cf.Flags) && BlocksEqual(cf.Blocks, blocks) {
 					return rval
 				}
 			}
@@ -214,14 +215,15 @@ func (w *Walker) walkAndHashFiles(fchan chan protocol.FileInfo) filepath.WalkFun
 		if info.Mode().IsDir() {
 			if w.CurrentFiler != nil {
 				// A directory is "unchanged", if it
+				//  - exists
 				//  - has the same permissions as previously, unless we are ignoring permissions
 				//  - was not marked deleted (since it apparently exists now)
 				//  - was a directory previously (not a file or something else)
 				//  - was not a symlink (since it's a directory now)
 				//  - was not invalid (since it looks valid now)
-				cf := w.CurrentFiler.CurrentFile(rn)
+				cf, ok := w.CurrentFiler.CurrentFile(rn)
 				permUnchanged := w.IgnorePerms || !cf.HasPermissionBits() || PermsEqual(cf.Flags, uint32(info.Mode()))
-				if permUnchanged && !cf.IsDeleted() && cf.IsDirectory() && !cf.IsSymlink() && !cf.IsInvalid() {
+				if ok && permUnchanged && !cf.IsDeleted() && cf.IsDirectory() && !cf.IsSymlink() && !cf.IsInvalid() {
 					return nil
 				}
 			}
@@ -248,6 +250,7 @@ func (w *Walker) walkAndHashFiles(fchan chan protocol.FileInfo) filepath.WalkFun
 		if info.Mode().IsRegular() {
 			if w.CurrentFiler != nil {
 				// A file is "unchanged", if it
+				//  - exists
 				//  - has the same permissions as previously, unless we are ignoring permissions
 				//  - was not marked deleted (since it apparently exists now)
 				//  - had the same modification time as it has now
@@ -255,9 +258,9 @@ func (w *Walker) walkAndHashFiles(fchan chan protocol.FileInfo) filepath.WalkFun
 				//  - was not a symlink (since it's a file now)
 				//  - was not invalid (since it looks valid now)
 				//  - has the same size as previously
-				cf := w.CurrentFiler.CurrentFile(rn)
+				cf, ok := w.CurrentFiler.CurrentFile(rn)
 				permUnchanged := w.IgnorePerms || !cf.HasPermissionBits() || PermsEqual(cf.Flags, uint32(info.Mode()))
-				if permUnchanged && !cf.IsDeleted() && cf.Modified == info.ModTime().Unix() && !cf.IsDirectory() &&
+				if ok && permUnchanged && !cf.IsDeleted() && cf.Modified == info.ModTime().Unix() && !cf.IsDirectory() &&
 					!cf.IsSymlink() && !cf.IsInvalid() && cf.Size() == info.Size() {
 					return nil
 				}