Repository: kopia/kopia
Pull request #4666 (2025-06-14 06:01:11)

refactor(general): small misc. cleanups

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kopia/kopia/pull/4666?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) Report
Attention: Patch coverage is `83.33333%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 76.39%. Comparing base [(`cb455c6`)](https://app.codecov.io/gh/kopia/kopia/commit/cb455c699a04e54cb47e25a676d7d65063ce883e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) to head [(`29a9e33`)](https://app.codecov.io/gh/kopia/kopia/commit/29a9e33fb8dea29b46a73481b5b24467e91726c4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).
> Report is 572 commits behind head on master.

| [Files with missing lines](https://app.codecov.io/gh/kopia/kopia/pull/4666?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) | Patch % | Lines |
|---|---|---|
| [internal/cache/content\_cache.go](https://app.codecov.io/gh/kopia/kopia/pull/4666?src=pr&el=tree&filepath=internal%2Fcache%2Fcontent_cache.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-aW50ZXJuYWwvY2FjaGUvY29udGVudF9jYWNoZS5nbw==) | 80.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/4666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |
| [internal/cache/persistent\_lru\_cache.go](https://app.codecov.io/gh/kopia/kopia/pull/4666?src=pr&el=tree&filepath=internal%2Fcache%2Fpersistent_lru_cache.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-aW50ZXJuYWwvY2FjaGUvcGVyc2lzdGVudF9scnVfY2FjaGUuZ28=) | 83.33% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/4666?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4666      +/-   ##
==========================================
+ Coverage   75.86%   76.39%   +0.52%     
==========================================
  Files         470      529      +59     
  Lines       37301    40157    +2856     
==========================================
+ Hits        28299    30677    +2378     
- Misses       7071     7461     +390     
- Partials     1931     2019      +88     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/kopia/kopia/pull/4666?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).

<details><summary> :rocket: New features to boost your workflow: </summary>

- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>

Patch:
diff --git a/internal/cache/content_cache.go b/internal/cache/content_cache.go
index 38c07b45..992d8af8 100644
--- a/internal/cache/content_cache.go
+++ b/internal/cache/content_cache.go
@@ -66,7 +66,7 @@ func (c *contentCacheImpl) getContentFromFullBlob(ctx context.Context, blobID bl
 	defer c.pc.exclusiveUnlock(string(blobID))
 
 	// check again to see if we perhaps lost the race and the data is now in cache.
-	if c.pc.GetPartial(ctx, BlobIDCacheKey(blobID), offset, length, output) {
+	if c.pc.getPartial(ctx, BlobIDCacheKey(blobID), offset, length, output) {
 		return nil
 	}
 
@@ -116,7 +116,7 @@ func (c *contentCacheImpl) getContentFromFullOrPartialBlob(ctx context.Context,
 	defer c.pc.sharedUnlock(string(blobID))
 
 	// see if we have the full blob cached by extracting a partial range.
-	if c.pc.GetPartial(ctx, BlobIDCacheKey(blobID), offset, length, output) {
+	if c.pc.getPartial(ctx, BlobIDCacheKey(blobID), offset, length, output) {
 		return nil
 	}
 
@@ -126,7 +126,7 @@ func (c *contentCacheImpl) getContentFromFullOrPartialBlob(ctx context.Context,
 
 	output.Reset()
 
-	if c.pc.GetFull(ctx, ContentIDCacheKey(contentID), output) {
+	if c.pc.getFull(ctx, ContentIDCacheKey(contentID), output) {
 		return nil
 	}
 
@@ -152,7 +152,7 @@ func (c *contentCacheImpl) PrefetchBlob(ctx context.Context, blobID blob.ID) err
 	defer blobData.Close()
 
 	// see if it's already cached before taking a lock
-	if c.pc.GetPartial(ctx, BlobIDCacheKey(blobID), 0, 1, &blobData) {
+	if c.pc.getPartial(ctx, BlobIDCacheKey(blobID), 0, 1, &blobData) {
 		return nil
 	}
 
@@ -160,7 +160,7 @@ func (c *contentCacheImpl) PrefetchBlob(ctx context.Context, blobID blob.ID) err
 	c.pc.exclusiveLock(string(blobID))
 	defer c.pc.exclusiveUnlock(string(blobID))
 
-	if c.pc.GetPartial(ctx, BlobIDCacheKey(blobID), 0, 1, &blobData) {
+	if c.pc.getPartial(ctx, BlobIDCacheKey(blobID), 0, 1, &blobData) {
 		return nil
 	}
 
diff --git a/internal/cache/persistent_lru_cache.go b/internal/cache/persistent_lru_cache.go
index 18e038d6..bebb35a4 100644
--- a/internal/cache/persistent_lru_cache.go
+++ b/internal/cache/persistent_lru_cache.go
@@ -63,7 +63,7 @@ func (c *PersistentCache) GetOrLoad(ctx context.Context, key string, fetch func(
 		return fetch(output)
 	}
 
-	if c.GetFull(ctx, key, output) {
+	if c.getFull(ctx, key, output) {
 		return nil
 	}
 
@@ -73,7 +73,7 @@ func (c *PersistentCache) GetOrLoad(ctx context.Context, key string, fetch func(
 	defer c.exclusiveUnlock(key)
 
 	// check again while holding the mutex
-	if c.GetFull(ctx, key, output) {
+	if c.getFull(ctx, key, output) {
 		return nil
 	}
 
@@ -90,9 +90,9 @@ func (c *PersistentCache) GetOrLoad(ctx context.Context, key string, fetch func(
 	return nil
 }
 
-// GetFull fetches the contents of a full blob. Returns false if not found.
-func (c *PersistentCache) GetFull(ctx context.Context, key string, output *gather.WriteBuffer) bool {
-	return c.GetPartial(ctx, key, 0, -1, output)
+// getFull fetches the contents of a full blob. Returns false if not found.
+func (c *PersistentCache) getFull(ctx context.Context, key string, output *gather.WriteBuffer) bool {
+	return c.getPartial(ctx, key, 0, -1, output)
 }
 
 func (c *PersistentCache) getPartialCacheHit(ctx context.Context, key string, length int64, output *gather.WriteBuffer) {
@@ -126,9 +126,9 @@ func (c *PersistentCache) deleteInvalidBlob(ctx context.Context, key string) {
 	}
 }
 
-// GetPartial fetches the contents of a cached blob when (length < 0) or a subset of it (when length >= 0).
-// returns false if not found.
-func (c *PersistentCache) GetPartial(ctx context.Context, key string, offset, length int64, output *gather.WriteBuffer) bool {
+// getPartial fetches the contents of a cached blob when (length < 0) or a subset
+// of it (when length >= 0) and returns true if it is found.
+func (c *PersistentCache) getPartial(ctx context.Context, key string, offset, length int64, output *gather.WriteBuffer) bool {
 	if c == nil {
 		return false
 	}
@@ -155,12 +155,7 @@ func (c *PersistentCache) GetPartial(ctx context.Context, key string, offset, le
 	}
 
 	// cache miss
-	l := length
-	if l < 0 {
-		l = 0
-	}
-
-	c.reportMissBytes(l)
+	c.reportMissBytes(max(length, 0))
 
 	return false
 }
diff --git a/internal/stat/stat_unix.go b/internal/stat/stat_unix.go
index 8f6ea65f..a0555cc9 100644
--- a/internal/stat/stat_unix.go
+++ b/internal/stat/stat_unix.go
@@ -1,8 +1,7 @@
 //go:build linux || freebsd || darwin
 // +build linux freebsd darwin
 
-// Package stat provides a cross-platform abstraction for
-// common stat commands.
+// Package stat provides a cross-platform abstraction for common stat commands.
 package stat
 
 import (
diff --git a/snapshot/policy/scheduling_policy.go b/snapshot/policy/scheduling_policy.go
index 18e2c8b1..66eb8315 100644
--- a/snapshot/policy/scheduling_policy.go
+++ b/snapshot/policy/scheduling_policy.go
@@ -58,7 +58,7 @@ func SortAndDedupeTimesOfDay(tod []TimeOfDay) []TimeOfDay {
 	})
 
 	// Remove subsequent duplicates
-	return slices.Compact[[]TimeOfDay, TimeOfDay](tod)
+	return slices.Compact(tod)
 }
 
 // SchedulingPolicy describes policy for scheduling snapshots.


Test patch:
diff --git a/internal/cache/export_test.go b/internal/cache/export_test.go
new file mode 100644
index 00000000..aa08d679
--- /dev/null
+++ b/internal/cache/export_test.go
@@ -0,0 +1,12 @@
+package cache
+
+import (
+	"context"
+
+	"github.com/kopia/kopia/internal/gather"
+)
+
+// TestingGetFull fetches the contents of a full blob. Returns false if not found.
+func (c *PersistentCache) TestingGetFull(ctx context.Context, key string, output *gather.WriteBuffer) bool {
+	return c.getFull(ctx, key, output)
+}
diff --git a/internal/cache/persistent_lru_cache_test.go b/internal/cache/persistent_lru_cache_test.go
index 4acb83d6..e8d241d1 100644
--- a/internal/cache/persistent_lru_cache_test.go
+++ b/internal/cache/persistent_lru_cache_test.go
@@ -33,12 +33,7 @@ func TestPersistentLRUCache(t *testing.T) {
 	}, nil, clock.Now)
 	require.NoError(t, err)
 
-	var tmp gather.WriteBuffer
-	defer tmp.Close()
-
-	if got := pc.GetFull(ctx, "key", &tmp); got {
-		t.Fatalf("unexpected cache hit on empty cache")
-	}
+	verifyNotCached(ctx, t, pc, "key") // no hits on an empty cache
 
 	someData := bytes.Repeat([]byte{1}, 300)
 
@@ -52,11 +47,7 @@ func TestPersistentLRUCache(t *testing.T) {
 	pc.Put(ctx, "key4", gather.FromSlice(someData))
 	verifyBlobExists(ctx, t, cs, "key4")
 
-	require.True(t, pc.GetFull(ctx, "key2", &tmp))
-
-	if got, want := tmp.ToByteSlice(), someData; !bytes.Equal(got, want) {
-		t.Fatalf("invalid data retrieved from cache: %x", got)
-	}
+	verifyCached(ctx, t, pc, "key2", someData)
 
 	// final sweep is performed on close at which time key1 becomes candidate
 	// for expulsion from cache because it's the oldest and we have 1200 bytes in the cache
@@ -74,7 +65,7 @@ func TestPersistentLRUCache(t *testing.T) {
 	}, nil, clock.Now)
 	require.NoError(t, err)
 
-	verifyCached(ctx, t, pc, "key1", nil)
+	verifyNotCached(ctx, t, pc, "key1")
 	verifyCached(ctx, t, pc, "key2", someData)
 	verifyCached(ctx, t, pc, "key3", someData)
 	verifyCached(ctx, t, pc, "key4", someData)
@@ -106,7 +97,7 @@ func TestPersistentLRUCache(t *testing.T) {
 
 	// at this point 'cs' was updated with a different checksum, so attempting to read it using
 	// 'pc' will return cache miss.
-	verifyCached(ctx, t, pc, "key2", nil)
+	verifyNotCached(ctx, t, pc, "key2")
 
 	require.ErrorIs(t, pc2.GetOrLoad(ctx, "key9", func(output *gather.WriteBuffer) error {
 		return someError
@@ -158,17 +149,16 @@ func TestPersistentLRUCache_GetDeletesInvalidBlob(t *testing.T) {
 	require.NoError(t, err)
 
 	pc.Put(ctx, "key", gather.FromSlice([]byte{1, 2, 3}))
+	verifyCached(ctx, t, pc, "key", []byte{1, 2, 3})
 
 	// corrupt cached data
 	data["key"][0] ^= 1
 
-	var tmp gather.WriteBuffer
-	defer tmp.Close()
-
 	// simulate failure when trying to delete.
 	fs.AddFault(blobtesting.MethodDeleteBlob).ErrorInstead(someError)
 
-	pc.GetFull(ctx, "key", &tmp)
+	// retrieving should not return anything after the data is corrupted
+	verifyNotCached(ctx, t, pc, "key")
 }
 
 func TestPersistentLRUCache_PutIgnoresStorageFailure(t *testing.T) {
@@ -190,11 +180,7 @@ func TestPersistentLRUCache_PutIgnoresStorageFailure(t *testing.T) {
 	fs.AddFault(blobtesting.MethodPutBlob).ErrorInstead(someError)
 
 	pc.Put(ctx, "key", gather.FromSlice([]byte{1, 2, 3}))
-
-	var tmp gather.WriteBuffer
-	defer tmp.Close()
-
-	require.False(t, pc.GetFull(ctx, "key", &tmp))
+	verifyNotCached(ctx, t, pc, "key")
 
 	require.Equal(t, 1, fs.NumCalls(blobtesting.MethodPutBlob))
 }
@@ -298,10 +284,10 @@ func TestPersistentLRUCacheNil(t *testing.T) {
 	// no-op
 	pc.Close(ctx)
 	pc.Put(ctx, "key", gather.FromSlice([]byte{1, 2, 3}))
+	verifyNotCached(ctx, t, pc, "key")
 
 	var tmp gather.WriteBuffer
-
-	require.False(t, pc.GetFull(ctx, "key", &tmp))
+	defer tmp.Close()
 
 	called := false
 
@@ -332,12 +318,7 @@ func TestPersistentLRUCache_Defaults(t *testing.T) {
 	defer pc.Close(ctx)
 
 	pc.Put(ctx, "key1", gather.FromSlice([]byte{1, 2, 3}))
-
-	var tmp gather.WriteBuffer
-	defer tmp.Close()
-
-	cs.GetBlob(ctx, "key1", 0, -1, &tmp)
-	require.Len(t, tmp.ToByteSlice(), 3)
+	verifyCached(ctx, t, pc, "key1", []byte{1, 2, 3})
 }
 
 func verifyCached(ctx context.Context, t *testing.T, pc *cache.PersistentCache, key string, want []byte) {
@@ -347,28 +328,29 @@ func verifyCached(ctx context.Context, t *testing.T, pc *cache.PersistentCache,
 	defer tmp.Close()
 
 	if want == nil {
-		require.False(t, pc.GetFull(ctx, key, &tmp))
+		require.False(t, pc.TestingGetFull(ctx, key, &tmp))
 	} else {
-		require.True(t, pc.GetFull(ctx, key, &tmp))
-
-		if got := tmp.ToByteSlice(); !bytes.Equal(got, want) {
-			t.Fatalf("invalid cached result for %v: %x, want %x", key, got, want)
-		}
+		require.True(t, pc.TestingGetFull(ctx, key, &tmp))
+		require.Equalf(t, want, tmp.ToByteSlice(), "invalid cached result for '%s'", key)
 	}
 }
 
+func verifyNotCached(ctx context.Context, t *testing.T, pc *cache.PersistentCache, key string) {
+	t.Helper()
+
+	verifyCached(ctx, t, pc, key, nil)
+}
+
 func verifyBlobExists(ctx context.Context, t *testing.T, st cache.Storage, blobID blob.ID) {
 	t.Helper()
 
-	if _, err := st.GetMetadata(ctx, blobID); err != nil {
-		t.Fatalf("blob %v error: %v", blobID, err)
-	}
+	_, err := st.GetMetadata(ctx, blobID)
+	require.NoErrorf(t, err, "blob '%s'", blobID)
 }
 
 func verifyBlobDoesNotExist(ctx context.Context, t *testing.T, st cache.Storage, blobID blob.ID) {
 	t.Helper()
 
-	if _, err := st.GetMetadata(ctx, blobID); !errors.Is(err, blob.ErrBlobNotFound) {
-		t.Fatalf("unexpected blob %v error: %v", blobID, err)
-	}
+	_, err := st.GetMetadata(ctx, blobID)
+	require.ErrorIsf(t, err, blob.ErrBlobNotFound, "expected blob not found for '%s'", blobID)
 }
diff --git a/internal/stat/stat_test.go b/internal/stat/stat_test.go
index 538de678..14392076 100644
--- a/internal/stat/stat_test.go
+++ b/internal/stat/stat_test.go
@@ -4,7 +4,6 @@
 package stat
 
 import (
-	"bytes"
 	"os"
 	"path/filepath"
 	"testing"
@@ -13,30 +12,31 @@
 )
 
 func TestGetBlockSize(t *testing.T) {
-	s, err := GetBlockSize(os.DevNull)
+	size, err := GetBlockSize(os.DevNull)
 	require.NoError(t, err)
 
-	if s <= 0 {
-		t.Fatalf("invalid disk block size: %d, must be greater than 0", s)
-	}
+	require.Positive(t, size)
+}
+
+func TestGetBlockSizeFromCurrentFS(t *testing.T) {
+	size, err := GetBlockSize(".")
+
+	require.NoError(t, err)
+	require.Positive(t, size)
 }
 
 func TestGetFileAllocSize(t *testing.T) {
-	const size = 4096
+	const expectedMinAllocSize = 512
 
 	d := t.TempDir()
 	f := filepath.Join(d, "test")
-	data := bytes.Repeat([]byte{1}, size)
 
-	err := os.WriteFile(f, data, os.ModePerm)
+	err := os.WriteFile(f, []byte{1}, os.ModePerm)
 	require.NoError(t, err)
 
 	s, err := GetFileAllocSize(f)
-	if err != nil {
-		t.Fatalf("error getting file alloc size for %s: %v", f, err)
-	}
 
-	if s < size {
-		t.Fatalf("invalid allocated file size %d, expected at least %d", s, size)
-	}
+	require.NoError(t, err, "error getting file alloc size for %s: %v", f, err)
+	t.Log("file alloc size:", s)
+	require.GreaterOrEqual(t, s, uint64(expectedMinAllocSize), "invalid allocated file size %d, expected at least %d", s, expectedMinAllocSize)
 }
diff --git a/tests/end_to_end_test/snapshot_actions_test.go b/tests/end_to_end_test/snapshot_actions_test.go
index 1cfc7d8f..2b69a891 100644
--- a/tests/end_to_end_test/snapshot_actions_test.go
+++ b/tests/end_to_end_test/snapshot_actions_test.go
@@ -265,7 +265,7 @@ func TestSnapshotActionsEmbeddedScript(t *testing.T) {
 		badRedirectScript  = tmpfileWithContents(t, "echo KOPIA_SNAPSHOT_PATH=/no/such/directory")
 	)
 
-	if runtime.GOOS == "windows" {
+	if runtime.GOOS == windowsOSName {
 		failingScript = tmpfileWithContents(t, "exit /b 1")
 		successScript2 = tmpfileWithContents(t, "echo Hello world!")
 	} else {
diff --git a/tests/end_to_end_test/snapshot_create_test.go b/tests/end_to_end_test/snapshot_create_test.go
index 7493be35..3530bcad 100644
--- a/tests/end_to_end_test/snapshot_create_test.go
+++ b/tests/end_to_end_test/snapshot_create_test.go
@@ -777,7 +777,7 @@ func TestSnapshotCreateAllSnapshotPath(t *testing.T) {
 	require.Equal(t, "foo", si[2].User)
 	require.Equal(t, "foo", si[2].Host)
 
-	if runtime.GOOS == "windows" {
+	if runtime.GOOS == windowsOSName {
 		require.Regexp(t, `[A-Z]:\\foo\\bar`, si[2].Path)
 	} else {
 		require.Equal(t, "/foo/bar", si[2].Path)