Merge pull request #4343 from greatroar/cache

cache: Restructure New to remove redundant operations

internal/cache/cache.go

@@ -26,10 +26,6 @@ const fileMode = 0644
 
 func readVersion(dir string) (v uint, err error) {
 	buf, err := os.ReadFile(filepath.Join(dir, "version"))
-	if errors.Is(err, os.ErrNotExist) {
-		return 0, nil
-	}
-
 	if err != nil {
 		return 0, errors.Wrap(err, "readVersion")
 	}
@@ -53,10 +49,6 @@ var cacheLayoutPaths = map[restic.FileType]string{
 const cachedirTagSignature = "Signature: 8a477f597d28d172789f06886806bc55\n"
 
 func writeCachedirTag(dir string) error {
-	if err := fs.MkdirAll(dir, dirMode); err != nil {
-		return errors.WithStack(err)
-	}
-
 	tagfile := filepath.Join(dir, "CACHEDIR.TAG")
 	f, err := fs.OpenFile(tagfile, os.O_CREATE|os.O_EXCL|os.O_WRONLY, fileMode)
 	if err != nil {
@@ -89,7 +81,7 @@ func New(id string, basedir string) (c *Cache, err error) {
 		}
 	}
 
-	err = fs.MkdirAll(basedir, 0700)
+	err = fs.MkdirAll(basedir, dirMode)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
@@ -102,30 +94,32 @@ func New(id string, basedir string) (c *Cache, err error) {
 	cachedir := filepath.Join(basedir, id)
 	debug.Log("using cache dir %v", cachedir)
 
+	created := false
 	v, err := readVersion(cachedir)
-	if err != nil {
-		return nil, err
-	}
-
-	if v > cacheVersion {
-		return nil, errors.New("cache version is newer")
-	}
-
-	// create the repo cache dir if it does not exist yet
-	var created bool
-	_, err = fs.Lstat(cachedir)
-	if errors.Is(err, os.ErrNotExist) {
-		err = fs.MkdirAll(cachedir, dirMode)
+	switch {
+	case err == nil:
+		if v > cacheVersion {
+			return nil, errors.New("cache version is newer")
+		}
+		// Update the timestamp so that we can detect old cache dirs.
+		err = updateTimestamp(cachedir)
 		if err != nil {
+			return nil, err
+		}
+
+	case errors.Is(err, os.ErrNotExist):
+		// Create the repo cache dir. The parent exists, so Mkdir suffices.
+		err := fs.Mkdir(cachedir, dirMode)
+		switch {
+		case err == nil:
+			created = true
+		case errors.Is(err, os.ErrExist):
+		default:
 			return nil, errors.WithStack(err)
 		}
-		created = true
-	}
 
-	// update the timestamp so that we can detect old cache dirs
-	err = updateTimestamp(cachedir)
-	if err != nil {
-		return nil, err
+	default:
+		return nil, errors.Wrap(err, "readVersion")
 	}
 
 	if v < cacheVersion {

internal/cache/cache_test.go

@@ -0,0 +1,46 @@
+package cache
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/restic/restic/internal/restic"
+	rtest "github.com/restic/restic/internal/test"
+)
+
+func TestNew(t *testing.T) {
+	parent := rtest.TempDir(t)
+	basedir := filepath.Join(parent, "cache")
+	id := restic.NewRandomID().String()
+	tagFile := filepath.Join(basedir, "CACHEDIR.TAG")
+	versionFile := filepath.Join(basedir, id, "version")
+
+	const (
+		stepCreate = iota
+		stepComplete
+		stepRmTag
+		stepRmVersion
+		stepEnd
+	)
+
+	for step := stepCreate; step < stepEnd; step++ {
+		switch step {
+		case stepRmTag:
+			rtest.OK(t, os.Remove(tagFile))
+		case stepRmVersion:
+			rtest.OK(t, os.Remove(versionFile))
+		}
+
+		c, err := New(id, basedir)
+		rtest.OK(t, err)
+		rtest.Equals(t, basedir, c.Base)
+		rtest.Equals(t, step == stepCreate, c.Created)
+
+		for _, name := range []string{tagFile, versionFile} {
+			info, err := os.Lstat(name)
+			rtest.OK(t, err)
+			rtest.Assert(t, info.Mode().IsRegular(), "")
+		}
+	}
+}