lib/fs: Correct wrapping order for meaningful log-caller (#7209)

lib/db/set.go

@@ -388,7 +388,7 @@ func (s *FileSet) SetIndexID(device protocol.DeviceID, id protocol.IndexID) {
 	}
 }
 
-func (s *FileSet) MtimeFS() *fs.MtimeFS {
+func (s *FileSet) MtimeFS() fs.Filesystem {
 	opStr := fmt.Sprintf("%s MtimeFS()", s.folder)
 	l.Debugf(opStr)
 	prefix, err := s.db.keyer.GenerateMtimesKey(nil, []byte(s.folder))

lib/fs/casefs.go

@@ -52,7 +52,7 @@ type caseFilesystemRegistry struct {
 	startCleaner sync.Once
 }
 
-func (r *caseFilesystemRegistry) get(fs Filesystem) *caseFilesystem {
+func (r *caseFilesystemRegistry) get(fs Filesystem) Filesystem {
 	r.mut.Lock()
 	defer r.mut.Unlock()
 
@@ -98,7 +98,7 @@ type caseFilesystem struct {
 // case-sensitive one. However it will add some overhead and thus shouldn't be
 // used if the filesystem is known to already behave case-sensitively.
 func NewCaseFilesystem(fs Filesystem) Filesystem {
-	return globalCaseFilesystemRegistry.get(fs)
+	return wrapFilesystem(fs, globalCaseFilesystemRegistry.get)
 }
 
 func (f *caseFilesystem) Chmod(name string, mode FileMode) error {

lib/fs/filesystem.go

@@ -260,6 +260,21 @@ func Canonicalize(file string) (string, error) {
 	return file, nil
 }
 
+// wrapFilesystem should always be used when wrapping a Filesystem.
+// It ensures proper wrapping order, which right now means:
+// `logFilesystem` needs to be the outermost wrapper for caller lookup.
+func wrapFilesystem(fs Filesystem, wrapFn func(Filesystem) Filesystem) Filesystem {
+	logFs, ok := fs.(*logFilesystem)
+	if ok {
+		fs = logFs.Filesystem
+	}
+	fs = wrapFn(fs)
+	if ok {
+		fs = &logFilesystem{fs}
+	}
+	return fs
+}
+
 // unwrapFilesystem removes "wrapping" filesystems to expose the underlying filesystem.
 func unwrapFilesystem(fs Filesystem) Filesystem {
 	for {
@@ -268,7 +283,7 @@ func unwrapFilesystem(fs Filesystem) Filesystem {
 			fs = sfs.Filesystem
 		case *walkFilesystem:
 			fs = sfs.Filesystem
-		case *MtimeFS:
+		case *mtimeFS:
 			fs = sfs.Filesystem
 		default:
 			return sfs

lib/fs/mtimefs.go

@@ -17,41 +17,38 @@ type database interface {
 	Delete(key string) error
 }
 
-// The MtimeFS is a filesystem with nanosecond mtime precision, regardless
-// of what shenanigans the underlying filesystem gets up to. A nil MtimeFS
-// just does the underlying operations with no additions.
-type MtimeFS struct {
+type mtimeFS struct {
 	Filesystem
 	chtimes         func(string, time.Time, time.Time) error
 	db              database
 	caseInsensitive bool
 }
 
-type MtimeFSOption func(*MtimeFS)
+type MtimeFSOption func(*mtimeFS)
 
 func WithCaseInsensitivity(v bool) MtimeFSOption {
-	return func(f *MtimeFS) {
+	return func(f *mtimeFS) {
 		f.caseInsensitive = v
 	}
 }
 
-func NewMtimeFS(underlying Filesystem, db database, options ...MtimeFSOption) *MtimeFS {
-	f := &MtimeFS{
-		Filesystem: underlying,
-		chtimes:    underlying.Chtimes, // for mocking it out in the tests
-		db:         db,
-	}
-	for _, opt := range options {
-		opt(f)
-	}
-	return f
+// NewMtimeFS returns a filesystem with nanosecond mtime precision, regardless
+// of what shenanigans the underlying filesystem gets up to.
+func NewMtimeFS(fs Filesystem, db database, options ...MtimeFSOption) Filesystem {
+	return wrapFilesystem(fs, func(underlying Filesystem) Filesystem {
+		f := &mtimeFS{
+			Filesystem: underlying,
+			chtimes:    underlying.Chtimes, // for mocking it out in the tests
+			db:         db,
+		}
+		for _, opt := range options {
+			opt(f)
+		}
+		return f
+	})
 }
 
-func (f *MtimeFS) Chtimes(name string, atime, mtime time.Time) error {
-	if f == nil {
-		return f.chtimes(name, atime, mtime)
-	}
-
+func (f *mtimeFS) Chtimes(name string, atime, mtime time.Time) error {
 	// Do a normal Chtimes call, don't care if it succeeds or not.
 	f.chtimes(name, atime, mtime)
 
@@ -66,7 +63,7 @@ func (f *MtimeFS) Chtimes(name string, atime, mtime time.Time) error {
 	return nil
 }
 
-func (f *MtimeFS) Stat(name string) (FileInfo, error) {
+func (f *mtimeFS) Stat(name string) (FileInfo, error) {
 	info, err := f.Filesystem.Stat(name)
 	if err != nil {
 		return nil, err
@@ -86,7 +83,7 @@ func (f *MtimeFS) Stat(name string) (FileInfo, error) {
 	return info, nil
 }
 
-func (f *MtimeFS) Lstat(name string) (FileInfo, error) {
+func (f *mtimeFS) Lstat(name string) (FileInfo, error) {
 	info, err := f.Filesystem.Lstat(name)
 	if err != nil {
 		return nil, err
@@ -106,7 +103,7 @@ func (f *MtimeFS) Lstat(name string) (FileInfo, error) {
 	return info, nil
 }
 
-func (f *MtimeFS) Walk(root string, walkFn WalkFunc) error {
+func (f *mtimeFS) Walk(root string, walkFn WalkFunc) error {
 	return f.Filesystem.Walk(root, func(path string, info FileInfo, err error) error {
 		if info != nil {
 			real, virtual, loadErr := f.load(path)
@@ -125,7 +122,7 @@ func (f *MtimeFS) Walk(root string, walkFn WalkFunc) error {
 	})
 }
 
-func (f *MtimeFS) Create(name string) (File, error) {
+func (f *mtimeFS) Create(name string) (File, error) {
 	fd, err := f.Filesystem.Create(name)
 	if err != nil {
 		return nil, err
@@ -133,7 +130,7 @@ func (f *MtimeFS) Create(name string) (File, error) {
 	return mtimeFile{fd, f}, nil
 }
 
-func (f *MtimeFS) Open(name string) (File, error) {
+func (f *mtimeFS) Open(name string) (File, error) {
 	fd, err := f.Filesystem.Open(name)
 	if err != nil {
 		return nil, err
@@ -141,7 +138,7 @@ func (f *MtimeFS) Open(name string) (File, error) {
 	return mtimeFile{fd, f}, nil
 }
 
-func (f *MtimeFS) OpenFile(name string, flags int, mode FileMode) (File, error) {
+func (f *mtimeFS) OpenFile(name string, flags int, mode FileMode) (File, error) {
 	fd, err := f.Filesystem.OpenFile(name, flags, mode)
 	if err != nil {
 		return nil, err
@@ -152,7 +149,7 @@ func (f *MtimeFS) OpenFile(name string, flags int, mode FileMode) (File, error)
 // "real" is the on disk timestamp
 // "virtual" is what want the timestamp to be
 
-func (f *MtimeFS) save(name string, real, virtual time.Time) {
+func (f *mtimeFS) save(name string, real, virtual time.Time) {
 	if f.caseInsensitive {
 		name = UnicodeLowercase(name)
 	}
@@ -172,7 +169,7 @@ func (f *MtimeFS) save(name string, real, virtual time.Time) {
 	f.db.PutBytes(name, bs)
 }
 
-func (f *MtimeFS) load(name string) (real, virtual time.Time, err error) {
+func (f *mtimeFS) load(name string) (real, virtual time.Time, err error) {
 	if f.caseInsensitive {
 		name = UnicodeLowercase(name)
 	}
@@ -205,7 +202,7 @@ func (m mtimeFileInfo) ModTime() time.Time {
 
 type mtimeFile struct {
 	File
-	fs *MtimeFS
+	fs *mtimeFS
 }
 
 func (f mtimeFile) Stat() (FileInfo, error) {

lib/fs/mtimefs_test.go

@@ -27,7 +27,7 @@ func TestMtimeFS(t *testing.T) {
 	// a random time with nanosecond precision
 	testTime := time.Unix(1234567890, 123456789)
 
-	mtimefs := NewMtimeFS(newBasicFilesystem("."), make(mapStore))
+	mtimefs := newMtimeFS(newBasicFilesystem("."), make(mapStore))
 
 	// Do one Chtimes call that will go through to the normal filesystem
 	mtimefs.chtimes = os.Chtimes
@@ -90,7 +90,7 @@ func TestMtimeFSWalk(t *testing.T) {
 	defer func() { _ = os.RemoveAll(dir) }()
 
 	underlying := NewFilesystem(FilesystemTypeBasic, dir)
-	mtimefs := NewMtimeFS(underlying, make(mapStore))
+	mtimefs := newMtimeFS(underlying, make(mapStore))
 	mtimefs.chtimes = failChtimes
 
 	if err := ioutil.WriteFile(filepath.Join(dir, "file"), []byte("hello"), 0644); err != nil {
@@ -144,7 +144,7 @@ func TestMtimeFSOpen(t *testing.T) {
 	defer func() { _ = os.RemoveAll(dir) }()
 
 	underlying := NewFilesystem(FilesystemTypeBasic, dir)
-	mtimefs := NewMtimeFS(underlying, make(mapStore))
+	mtimefs := newMtimeFS(underlying, make(mapStore))
 	mtimefs.chtimes = failChtimes
 
 	if err := ioutil.WriteFile(filepath.Join(dir, "file"), []byte("hello"), 0644); err != nil {
@@ -196,7 +196,7 @@ func TestMtimeFSInsensitive(t *testing.T) {
 		t.Skip("need case insensitive FS")
 	}
 
-	theTest := func(t *testing.T, fs *MtimeFS, shouldSucceed bool) {
+	theTest := func(t *testing.T, fs *mtimeFS, shouldSucceed bool) {
 		os.RemoveAll("testdata")
 		defer os.RemoveAll("testdata")
 		os.Mkdir("testdata", 0755)
@@ -223,12 +223,12 @@ func TestMtimeFSInsensitive(t *testing.T) {
 
 	// The test should fail with a case sensitive mtimefs
 	t.Run("with case sensitive mtimefs", func(t *testing.T) {
-		theTest(t, NewMtimeFS(newBasicFilesystem("."), make(mapStore)), false)
+		theTest(t, newMtimeFS(newBasicFilesystem("."), make(mapStore)), false)
 	})
 
 	// And succeed with a case insensitive one.
 	t.Run("with case insensitive mtimefs", func(t *testing.T) {
-		theTest(t, NewMtimeFS(newBasicFilesystem("."), make(mapStore), WithCaseInsensitivity(true)), true)
+		theTest(t, newMtimeFS(newBasicFilesystem("."), make(mapStore), WithCaseInsensitivity(true)), true)
 	})
 }
 
@@ -261,3 +261,7 @@ func failChtimes(name string, mtime, atime time.Time) error {
 func evilChtimes(name string, mtime, atime time.Time) error {
 	return os.Chtimes(name, mtime.Add(300*time.Hour).Truncate(time.Hour), atime.Add(300*time.Hour).Truncate(time.Hour))
 }
+
+func newMtimeFS(fs Filesystem, db database, options ...MtimeFSOption) *mtimeFS {
+	return NewMtimeFS(fs, db, options...).(*mtimeFS)
+}