cache: Restructure New to remove redundant operations

New and its helpers used to create the cache directories several times
over. They now only do so once. The added test ensures that the cache is
produced in a consistent state when parts are deleted.

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,32 +94,34 @@ 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
-	}
-
+	switch {
+	case err == nil:
 		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)
-		if err != nil {
-			return nil, errors.WithStack(err)
-		}
-		created = true
-	}
-
-	// update the timestamp so that we can detect old cache dirs
+		// 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)
+		}
+
+	default:
+		return nil, errors.Wrap(err, "readVersion")
+	}
+
 	if v < cacheVersion {
 		err = os.WriteFile(filepath.Join(cachedir, "version"), []byte(fmt.Sprintf("%d", cacheVersion)), fileMode)
 		if err != nil {

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(), "")
+		}
+	}
+}