Address code review feedback

- Cache isGNUTar() result using sync.Once to avoid repeated subprocess calls
- Align internal naming: SkipExisting → IgnoreCollisions
- Add assertion for restored file content in test

Co-Authored-By: Claude <noreply@anthropic.com>

Author: cchristous

cache-cli/cmd/restore.go

@@ -42,7 +42,7 @@ func RunRestore(cmd *cobra.Command, args []string) {
 	utils.Check(err)
 
 	archiver := archive.NewArchiverWithOptions(metricsManager, archive.ArchiverOptions{
-		SkipExisting: ignoreCollisions,
+		IgnoreCollisions: ignoreCollisions,
 	})
 
 	if len(args) == 0 {

cache-cli/pkg/archive/archiver.go

@@ -12,7 +12,7 @@ type Archiver interface {
 }
 
 type ArchiverOptions struct {
-	SkipExisting bool
+	IgnoreCollisions bool
 }
 
 func NewArchiver(metricsManager metrics.MetricsManager) Archiver {

cache-cli/pkg/archive/archiver_test.go

@@ -298,9 +298,9 @@ func Test__Compress(t *testing.T) {
 			// Delete only tempFile2, keep tempFile1 to simulate existing file
 			assert.NoError(t, os.Remove(tempFile2.Name()))
 
-			// Create an archiver with SkipExisting enabled for decompression
+			// Create an archiver with IgnoreCollisions enabled for decompression
 			metricsManager := metrics.NewNoOpMetricsManager()
-			opts := ArchiverOptions{SkipExisting: true}
+			opts := ArchiverOptions{IgnoreCollisions: true}
 			var skipArchiver Archiver
 			switch archiverType {
 			case "shell-out":
@@ -321,9 +321,10 @@ func Test__Compress(t *testing.T) {
 			assert.NoError(t, err)
 			assert.Equal(t, originalContent, content1)
 
-			// Verify tempFile2 was restored
-			_, err = os.Stat(tempFile2.Name())
+			// Verify tempFile2 was restored with correct content
+			content2, err := ioutil.ReadFile(tempFile2.Name())
 			assert.NoError(t, err)
+			assert.Equal(t, cachedContent, content2)
 
 			assert.NoError(t, os.RemoveAll(tempDirBase))
 			assert.NoError(t, os.Remove(compressedFileName))

cache-cli/pkg/archive/native_archiver.go

@@ -17,9 +17,9 @@ import (
 )
 
 type NativeArchiver struct {
-	MetricsManager metrics.MetricsManager
-	UseParallelism bool
-	SkipExisting   bool
+	MetricsManager   metrics.MetricsManager
+	UseParallelism   bool
+	IgnoreCollisions bool
 }
 
 func NewNativeArchiver(metricsManager metrics.MetricsManager, useParallelism bool) *NativeArchiver {
@@ -31,9 +31,9 @@ func NewNativeArchiver(metricsManager metrics.MetricsManager, useParallelism boo
 
 func NewNativeArchiverWithOptions(metricsManager metrics.MetricsManager, useParallelism bool, opts ArchiverOptions) *NativeArchiver {
 	return &NativeArchiver{
-		MetricsManager: metricsManager,
-		UseParallelism: useParallelism,
-		SkipExisting:   opts.SkipExisting,
+		MetricsManager:   metricsManager,
+		UseParallelism:   useParallelism,
+		IgnoreCollisions: opts.IgnoreCollisions,
 	}
 }
 
@@ -215,7 +215,7 @@ func (a *NativeArchiver) Decompress(src string) (string, error) {
 				continue
 			}
 
-			// nil outFile means the file should be skipped (e.g., SkipExisting is enabled)
+			// nil outFile means the file should be skipped (e.g., IgnoreCollisions is enabled)
 			if outFile == nil {
 				// Discard the file contents from the tar reader
 				if _, err := io.Copy(io.Discard, tarReader); err != nil {
@@ -263,7 +263,7 @@ func (a *NativeArchiver) Decompress(src string) (string, error) {
 }
 
 // openFile opens a file for writing during decompression.
-// Returns (nil, nil) if the file should be skipped (e.g., when SkipExisting is true and file exists).
+// Returns (nil, nil) if the file should be skipped (e.g., when IgnoreCollisions is true and file exists).
 func (a *NativeArchiver) openFile(header *tar.Header, tarReader *tar.Reader) (*os.File, error) {
 	outFile, err := os.OpenFile(header.Name, os.O_RDWR|os.O_CREATE|os.O_EXCL, header.FileInfo().Mode())
 
@@ -274,8 +274,8 @@ func (a *NativeArchiver) openFile(header *tar.Header, tarReader *tar.Reader) (*o
 
 	// Since we are using O_EXCL, this error could mean that the file already exists.
 	if errors.Is(err, os.ErrExist) {
-		// If SkipExisting is enabled, skip this file silently.
-		if a.SkipExisting {
+		// If IgnoreCollisions is enabled, skip this file silently.
+		if a.IgnoreCollisions {
 			return nil, nil
 		}
 		// Otherwise, attempt to remove it before opening again.

cache-cli/pkg/archive/shell_out_archiver.go

@@ -9,14 +9,15 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strings"
+	"sync"
 
 	"github.com/semaphoreci/toolbox/cache-cli/pkg/metrics"
 	log "github.com/sirupsen/logrus"
 )
 
 type ShellOutArchiver struct {
-	metricsManager metrics.MetricsManager
-	skipExisting   bool
+	metricsManager   metrics.MetricsManager
+	ignoreCollisions bool
 }
 
 func NewShellOutArchiver(metricsManager metrics.MetricsManager) *ShellOutArchiver {
@@ -25,8 +26,8 @@ func NewShellOutArchiver(metricsManager metrics.MetricsManager) *ShellOutArchive
 
 func NewShellOutArchiverWithOptions(metricsManager metrics.MetricsManager, opts ArchiverOptions) *ShellOutArchiver {
 	return &ShellOutArchiver{
-		metricsManager: metricsManager,
-		skipExisting:   opts.SkipExisting,
+		metricsManager:   metricsManager,
+		ignoreCollisions: opts.IgnoreCollisions,
 	}
 }
 
@@ -84,10 +85,10 @@ func (a *ShellOutArchiver) decompressionCmd(dst, tempFile string) *exec.Cmd {
 		args = append(args, "xzf", tempFile, "-C", ".")
 	}
 
-	// When skipExisting is enabled, skip existing files without overwriting.
+	// When ignoreCollisions is enabled, skip existing files without overwriting.
 	// GNU tar uses --skip-old-files (silent, exit 0).
 	// BSD tar uses -k (silent, exit 0).
-	if a.skipExisting {
+	if a.ignoreCollisions {
 		if isGNUTar() {
 			args = append(args, "--skip-old-files")
 		} else {
@@ -98,15 +99,25 @@ func (a *ShellOutArchiver) decompressionCmd(dst, tempFile string) *exec.Cmd {
 	return exec.Command("tar", args...)
 }
 
+var (
+	gnuTarOnce   sync.Once
+	gnuTarCached bool
+)
+
 // isGNUTar returns true if the system tar is GNU tar.
 // GNU tar includes "GNU tar" in its --version output.
+// The result is cached to avoid repeated subprocess calls.
 func isGNUTar() bool {
-	cmd := exec.Command("tar", "--version")
-	output, err := cmd.Output()
-	if err != nil {
-		return false
-	}
-	return strings.Contains(string(output), "GNU tar")
+	gnuTarOnce.Do(func() {
+		cmd := exec.Command("tar", "--version")
+		output, err := cmd.Output()
+		if err != nil {
+			gnuTarCached = false
+			return
+		}
+		gnuTarCached = strings.Contains(string(output), "GNU tar")
+	})
+	return gnuTarCached
 }
 
 func (a *ShellOutArchiver) findRestorationPath(src string) (string, error) {