Merge branch 'fix-1373'

* fix-1373:
  fixup alterFiles
  Ensure progress when delete-by-rename fails (fixes #1373)
  Handle weird Lstat() returns for disappeared items (ref #1373)
  Alter files into directories and the other way around

internal/model/model.go

@@ -1242,8 +1242,16 @@ func (m *Model) ScanFolderSub(folder, sub string) error {
 					"size":     f.Size(),
 				})
 				batch = append(batch, nf)
-			} else if _, err := os.Lstat(filepath.Join(folderCfg.Path, f.Name)); err != nil && os.IsNotExist(err) {
-				// File has been deleted
+			} else if _, err := os.Lstat(filepath.Join(folderCfg.Path, f.Name)); err != nil {
+				// File has been deleted.
+
+				// We don't specifically verify that the error is
+				// os.IsNotExist because there is a corner case when a
+				// directory is suddenly transformed into a file. When that
+				// happens, files that were in the directory (that is now a
+				// file) are deleted but will return a confusing error ("not a
+				// directory") when we try to Lstat() them.
+
 				nf := protocol.FileInfo{
 					Name:     f.Name,
 					Flags:    f.Flags | protocol.FlagDeleted,

internal/model/puller.go

@@ -614,23 +614,32 @@ func (p *Puller) renameFile(source, target protocol.FileInfo) {
 		err = osutil.TryRename(from, to)
 	}
 
-	if err != nil {
-		l.Infof("Puller (folder %q, file %q): rename from %q: %v", p.folder, target.Name, source.Name, err)
-		return
-	}
+	if err == nil {
+		// The file was renamed, so we have handled both the necessary delete
+		// of the source and the creation of the target. Fix-up the metadata,
+		// and update the local index of the target file.
+
+		p.model.updateLocal(p.folder, source)
 
-	// Fix-up the metadata, and update the local index of the target file
 		err = p.shortcutFile(target)
 		if err != nil {
 			l.Infof("Puller (folder %q, file %q): rename from %q metadata: %v", p.folder, target.Name, source.Name, err)
 			return
 		}
+	} else {
+		// We failed the rename so we have a source file that we still need to
+		// get rid of. Attempt to delete it instead so that we make *some*
+		// progress. The target is unhandled.
+
+		err = osutil.InWritableDir(os.Remove, from)
+		if err != nil {
+			l.Infof("Puller (folder %q, file %q): delete %q after failed rename: %v", p.folder, target.Name, source.Name, err)
+			return
+		}
 
-	// Source file already has the delete bit set.
-	// Because we got rid of the file (by renaming it), we just need to update
-	// the index, and we're done with it.
 		p.model.updateLocal(p.folder, source)
 	}
+}
 
 // handleFile queues the copies and pulls as necessary for a single new or
 // changed file.

test/filetype_test.go

@@ -91,10 +91,22 @@ func testFileTypeChange(t *testing.T) {
 
 	// A directory that we will replace with a file later
 
+	err = os.Mkdir("s1/emptyDirToReplace", 0755)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// A directory with files that we will replace with a file later
+
 	err = os.Mkdir("s1/dirToReplace", 0755)
 	if err != nil {
 		t.Fatal(err)
 	}
+	fd, err = os.Create("s1/dirToReplace/emptyFile")
+	if err != nil {
+		t.Fatal(err)
+	}
+	fd.Close()
 
 	// Verify that the files and directories sync to the other side
 
@@ -165,15 +177,33 @@ func testFileTypeChange(t *testing.T) {
 
 	// Replace file with directory
 
-	os.RemoveAll("s1/fileToReplace")
+	err = os.RemoveAll("s1/fileToReplace")
+	if err != nil {
+		t.Fatal(err)
+	}
 	err = os.Mkdir("s1/fileToReplace", 0755)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	// Replace directory with file
+	// Replace empty directory with file
 
-	os.RemoveAll("s1/dirToReplace")
+	err = os.RemoveAll("s1/emptyDirToReplace")
+	if err != nil {
+		t.Fatal(err)
+	}
+	fd, err = os.Create("s1/emptyDirToReplace")
+	if err != nil {
+		t.Fatal(err)
+	}
+	fd.Close()
+
+	// Clear directory and replace with file
+
+	err = os.RemoveAll("s1/dirToReplace")
+	if err != nil {
+		t.Fatal(err)
+	}
 	fd, err = os.Create("s1/dirToReplace")
 	if err != nil {
 		t.Fatal(err)

test/util.go

@@ -23,6 +23,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"log"
 	"math/rand"
 	"os"
@@ -113,8 +114,10 @@ func alterFiles(dir string) error {
 			return nil
 		}
 
+		info, err = os.Stat(path)
 		if err != nil {
-			return err
+			// Something we deleted while walking. Ignore.
+			return nil
 		}
 
 		if strings.HasPrefix(filepath.Base(path), "test-") {
@@ -128,17 +131,24 @@ func alterFiles(dir string) error {
 			return nil
 		}
 
-		r := rand.Float64()
+		// File structure is base/x/xy/xyz12345...
+		// comps == 1: base (don't touch)
+		// comps == 2: base/x (must be dir)
+		// comps == 3: base/x/xy (must be dir)
+		// comps  > 3: base/x/xy/xyz12345... (can be dir or file)
+
 		comps := len(strings.Split(path, string(os.PathSeparator)))
+
+		r := rand.Intn(10)
 		switch {
-		case r < 0.1 && comps > 2:
+		case r == 0 && comps > 2:
 			// Delete every tenth file or directory, except top levels
 			err := removeAll(path)
 			if err != nil {
 				return err
 			}
 
-		case r < 0.2 && info.Mode().IsRegular():
+		case r == 1 && info.Mode().IsRegular():
 			if info.Mode()&0200 != 0200 {
 				// Not owner writable. Fix.
 				err = os.Chmod(path, 0644)
@@ -166,7 +176,31 @@ func alterFiles(dir string) error {
 			if err != nil {
 				return err
 			}
-		case r < 0.3 && comps > 1 && (info.Mode().IsRegular() || rand.Float64() < 0.2):
+
+		case r == 2 && comps > 3 && rand.Float64() < 0.2:
+			if !info.Mode().IsRegular() {
+				err = removeAll(path)
+				if err != nil {
+					return err
+				}
+				d1 := []byte("I used to be a dir: " + path)
+				err := ioutil.WriteFile(path, d1, 0644)
+				if err != nil {
+					return err
+				}
+			} else {
+				err := os.Remove(path)
+				if err != nil {
+					return err
+				}
+				err = os.MkdirAll(path, 0755)
+				if err != nil {
+					return err
+				}
+				generateFiles(path, 10, 20, "../LICENSE")
+			}
+
+		case r == 3 && comps > 2 && (info.Mode().IsRegular() || rand.Float64() < 0.2):
 			rpath := filepath.Dir(path)
 			if rand.Float64() < 0.2 {
 				for move := rand.Intn(comps - 1); move > 0; move-- {
@@ -184,8 +218,7 @@ func alterFiles(dir string) error {
 		return err
 	}
 
-	// Create 100 new files
-	return generateFiles(dir, 100, 20, "../LICENSE")
+	return generateFiles(dir, 25, 20, "../LICENSE")
 }
 
 func ReadRand(bs []byte) (int, error) {