From da419be43c751d7f2ff52c5f9ef316b25e3b6d65 Mon Sep 17 00:00:00 2001
From: greatroar <61184462+greatroar@users.noreply.github.com>
Date: Sat, 27 May 2023 10:30:31 +0200
Subject: [PATCH] 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      | 50 ++++++++++++++++--------------------
 internal/cache/cache_test.go | 46 +++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 28 deletions(-)
 create mode 100644 internal/cache/cache_test.go

diff --git a/internal/cache/cache.go b/internal/cache/cache.go
index 075f7f6a1..5b3601741 100644
--- a/internal/cache/cache.go
+++ b/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 {
diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go
new file mode 100644
index 000000000..b9d0b905d
--- /dev/null
+++ b/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(), "")
+		}
+	}
+}