Correctly handle ro dirs in ro dirs etc

internal/model/puller.go

@@ -272,25 +272,50 @@ func (p *Puller) handleDir(file protocol.FileInfo) {
 		l.Debugf("need dir\n\t%v\n\t%v", file, curFile)
 	}
 
-	var err error
-	if info, err := os.Stat(realName); err != nil && os.IsNotExist(err) {
-		err = os.MkdirAll(realName, mode)
+	if info, err := os.Stat(realName); err != nil {
+		if os.IsNotExist(err) {
+			// The directory doesn't exist, so we create it with the right
+			// mode bits from the start.
+
+			mkdir := func(path string) error {
+				// We declare a function that acts on only the path name, so
+				// we can pass it to InWritableDir. We use a regular Mkdir and
+				// not MkdirAll because the parent should already exist.
+				return os.Mkdir(path, mode)
+			}
+
+			if err = osutil.InWritableDir(mkdir, realName); err == nil {
+				p.model.updateLocal(p.repo, file)
+			} else {
+				l.Infof("Puller (repo %q, file %q): %v", p.repo, file.Name, err)
+			}
+			return
+		}
+
+		// Weird error when stat()'ing the dir. Probably won't work to do
+		// anything else with it if we can't even stat() it.
+		l.Infof("Puller (repo %q, file %q): %v", p.repo, file.Name, err)
+		return
 	} else if !info.IsDir() {
 		l.Infof("Puller (repo %q, file %q): should be dir, but is not", p.repo, file.Name)
 		return
-	} else {
-		err = os.Chmod(realName, mode)
 	}
 
-	if err == nil {
+	// The directory already exists, so we just correct the mode bits. (We
+	// don't handle modification times on directories, because that sucks...)
+	// It's OK to change mode bits on stuff within non-writable directories.
+
+	if err := os.Chmod(realName, mode); err == nil {
 		p.model.updateLocal(p.repo, file)
+	} else {
+		l.Infof("Puller (repo %q, file %q): %v", p.repo, file.Name, err)
 	}
 }
 
 // deleteDir attempts to delete the given directory
 func (p *Puller) deleteDir(file protocol.FileInfo) {
 	realName := filepath.Join(p.dir, file.Name)
-	err := os.Remove(realName)
+	err := osutil.InWritableDir(os.Remove, realName)
 	if err == nil || os.IsNotExist(err) {
 		p.model.updateLocal(p.repo, file)
 	}
@@ -299,27 +324,12 @@ func (p *Puller) deleteDir(file protocol.FileInfo) {
 // deleteFile attempts to delete the given file
 func (p *Puller) deleteFile(file protocol.FileInfo) {
 	realName := filepath.Join(p.dir, file.Name)
-	realDir := filepath.Dir(realName)
-	if info, err := os.Stat(realDir); err == nil && info.IsDir() && info.Mode()&04 == 0 {
-		// A non-writeable directory (for this user; we assume that's the
-		// relevant part). Temporarily change the mode so we can delete the
-		// file inside it.
-		err = os.Chmod(realDir, 0755)
-		if err == nil {
-			defer func() {
-				err = os.Chmod(realDir, info.Mode())
-				if err != nil {
-					panic(err)
-				}
-			}()
-		}
-	}
 
 	var err error
 	if p.versioner != nil {
-		err = p.versioner.Archive(realName)
+		err = osutil.InWritableDir(p.versioner.Archive, realName)
 	} else {
-		err = os.Remove(realName)
+		err = osutil.InWritableDir(os.Remove, realName)
 	}
 
 	if err != nil {

internal/model/sharedpullerstate.go

@@ -5,7 +5,6 @@
 package model
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"sync"
@@ -47,21 +46,13 @@ func (s *sharedPullerState) tempFile() (*os.File, error) {
 		return s.fd, nil
 	}
 
-	// Ensure that the parent directory exists or can be created
+	// Ensure that the parent directory is writable. This is
+	// osutil.InWritableDir except we need to do more stuff so we duplicate it
+	// here.
 	dir := filepath.Dir(s.tempName)
-	if info, err := os.Stat(dir); err != nil && os.IsNotExist(err) {
-		err = os.MkdirAll(dir, 0755)
-		if err != nil {
-			s.earlyCloseLocked("dst mkdir", err)
-			return nil, err
-		}
-	} else if err != nil {
+	if info, err := os.Stat(dir); err != nil {
 		s.earlyCloseLocked("dst stat dir", err)
 		return nil, err
-	} else if !info.IsDir() {
-		err = fmt.Errorf("%q: not a directory", dir)
-		s.earlyCloseLocked("dst mkdir", err)
-		return nil, err
 	} else if info.Mode()&04 == 0 {
 		err := os.Chmod(dir, 0755)
 		if err == nil {

internal/osutil/osutil.go

@@ -45,3 +45,28 @@ func Rename(from, to string) error {
 	defer os.Remove(from)
 	return os.Rename(from, to)
 }
+
+// InWritableDir calls fn(path), while making sure that the directory
+// containing `path` is writable for the duration of the call.
+func InWritableDir(fn func(string) error, path string) error {
+	dir := filepath.Dir(path)
+	if info, err := os.Stat(dir); err == nil && info.IsDir() && info.Mode()&04 == 0 {
+		// A non-writeable directory (for this user; we assume that's the
+		// relevant part). Temporarily change the mode so we can delete the
+		// file or directory inside it.
+		err = os.Chmod(dir, 0755)
+		if err == nil {
+			defer func() {
+				err = os.Chmod(dir, info.Mode())
+				if err != nil {
+					// We managed to change the permission bits like a
+					// millisecond ago, so it'd be bizarre if we couldn't
+					// change it back.
+					panic(err)
+				}
+			}()
+		}
+	}
+
+	return fn(path)
+}