Fix directory permission walker for Go 1.10

2 files changed, 31 insertions, 19 deletions

diff --git a/internal/helper/housekeeping/housekeeping.go b/internal/helper/housekeeping/housekeeping.go
index 3dbd119da..e4e3b841f 100644
--- a/internal/helper/housekeeping/housekeeping.go
+++ b/internal/helper/housekeeping/housekeeping.go
@@ -20,20 +20,19 @@ func Perform(ctx context.Context, repoPath string) error {
 	filesMarkedForRemoval := 0
 	unremovableFiles := 0
 
-	err := filepath.Walk(repoPath, func(path string, info os.FileInfo, err error) error {
+	err := filepath.Walk(repoPath, func(path string, info os.FileInfo, _ error) error {
 		if repoPath == path {
 			// Never consider the root path
 			return nil
 		}
 
-		if info == nil || !shouldRemove(path, info.ModTime(), info.Mode(), err) {
+		if info == nil || !shouldRemove(path, info.ModTime(), info.Mode()) {
 			return nil
 		}
 
 		filesMarkedForRemoval++
 
-		err = forceRemove(path)
-		if err != nil {
+		if err := forceRemove(path); err != nil {
 			unremovableFiles++
 			logger.WithError(err).WithField("path", path).Warn("unable to remove stray file")
 		}
@@ -56,10 +55,29 @@ func Perform(ctx context.Context, repoPath string) error {
 	return err
 }
 
-func fixPermissions(path string) {
-	filepath.Walk(path, func(path string, info os.FileInfo, err error) error {
-		if info.IsDir() {
-			os.Chmod(path, 0700)
+// FixDirectoryPermissions does a recursive directory walk to look for
+// directories that cannot be accessed by the current user, and tries to
+// fix those with chmod. The motivating problem is that directories with mode
+// 0 break os.RemoveAll.
+func FixDirectoryPermissions(path string) error {
+	return fixDirectoryPermissions(path, make(map[string]struct{}))
+}
+
+const minimumDirPerm = 0700
+
+func fixDirectoryPermissions(path string, retriedPaths map[string]struct{}) error {
+	return filepath.Walk(path, func(path string, info os.FileInfo, errIncoming error) error {
+		if !info.IsDir() || info.Mode()&minimumDirPerm == minimumDirPerm {
+			return nil
+		}
+
+		if err := os.Chmod(path, info.Mode()|minimumDirPerm); err != nil {
+			return err
+		}
+
+		if _, retried := retriedPaths[path]; !retried && os.IsPermission(errIncoming) {
+			retriedPaths[path] = struct{}{}
+			return fixDirectoryPermissions(path, retriedPaths)
 		}
 
 		return nil
@@ -74,12 +92,14 @@ func forceRemove(path string) error {
 	}
 
 	// Delete failed. Try again after chmod'ing directories recursively
-	fixPermissions(path)
+	if err := FixDirectoryPermissions(path); err != nil {
+		return err
+	}
 
 	return os.RemoveAll(path)
 }
 
-func shouldRemove(path string, modTime time.Time, mode os.FileMode, err error) bool {
+func shouldRemove(path string, modTime time.Time, mode os.FileMode) bool {
 	base := filepath.Base(path)
 
 	// Only delete entries starting with `tmp_` and older than a week
diff --git a/internal/helper/housekeeping/housekeeping_test.go b/internal/helper/housekeeping/housekeeping_test.go
index e8dd38c86..8f6defa73 100644
--- a/internal/helper/housekeeping/housekeeping_test.go
+++ b/internal/helper/housekeeping/housekeeping_test.go
@@ -1,7 +1,6 @@
 package housekeeping
 
 import (
-	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -201,7 +200,6 @@ func TestShouldUnlink(t *testing.T) {
 		path    string
 		modTime time.Time
 		mode    os.FileMode
-		err     error
 	}
 	tests := []struct {
 		name string
@@ -214,7 +212,6 @@ func TestShouldUnlink(t *testing.T) {
 				path:    "/tmp/objects",
 				modTime: time.Now().Add(-1 * time.Hour),
 				mode:    0700,
-				err:     nil,
 			},
 			want: false,
 		},
@@ -224,7 +221,6 @@ func TestShouldUnlink(t *testing.T) {
 				path:    "/tmp/",
 				modTime: time.Now().Add(-1 * time.Hour),
 				mode:    0770,
-				err:     nil,
 			},
 			want: false,
 		},
@@ -234,7 +230,6 @@ func TestShouldUnlink(t *testing.T) {
 				path:    "/tmp/tmp_DELETEME",
 				modTime: time.Now().Add(-1 * time.Hour),
 				mode:    0600,
-				err:     nil,
 			},
 			want: false,
 		},
@@ -244,7 +239,6 @@ func TestShouldUnlink(t *testing.T) {
 				path:    "/tmp/tmp_DELETEME",
 				modTime: time.Now().Add(-8 * 24 * time.Hour),
 				mode:    0600,
-				err:     nil,
 			},
 			want: true,
 		},
@@ -254,7 +248,6 @@ func TestShouldUnlink(t *testing.T) {
 				path:    "/tmp/tmp_DELETEME",
 				modTime: time.Now().Add(-1 * time.Hour),
 				mode:    0000,
-				err:     nil,
 			},
 			want: false,
 		},
@@ -264,7 +257,6 @@ func TestShouldUnlink(t *testing.T) {
 				path:    "/tmp/tmp_DELETEME",
 				modTime: time.Now().Add(-1 * time.Hour),
 				mode:    0000,
-				err:     fmt.Errorf("file inaccessible"),
 			},
 			want: false,
 		},
@@ -272,7 +264,7 @@ func TestShouldUnlink(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := shouldRemove(tt.args.path, tt.args.modTime, tt.args.mode, tt.args.err); got != tt.want {
+			if got := shouldRemove(tt.args.path, tt.args.modTime, tt.args.mode); got != tt.want {
 				t.Errorf("shouldUnlink() = %v, want %v", got, tt.want)
 			}
 		})