Fix pre-existing review findings

Amolith created 1 month ago

Address issues found by CodeRabbit in pre-existing code:

- Fix FormatSnapshotLine doc comment to note that both paths and
tags are omitted when empty, not just tags
- Fix misleading "resolveFixtureTOML-style" comment in snapshot
tests — the fixtures are JSON, not TOML
- Remove redundant tt := tt loop variable captures in exec_test.go
and list_snapshots_test.go (unnecessary since Go 1.22)
- Reject whitespace-only input in notEmpty validator by trimming
before checking; update the test expectation to match

Change summary

internal/form/form.go                  | 5 +++--
internal/form/form_test.go             | 2 +-
internal/restic/exec_test.go           | 2 --
internal/restic/list_snapshots_test.go | 1 -
internal/restic/snapshots.go           | 3 ++-
internal/restic/snapshots_test.go      | 4 ++--
6 files changed, 8 insertions(+), 9 deletions(-)

Detailed changes

internal/form/form.go

@@ -89,10 +89,11 @@ func BackupPaths() ([]string, error) {
 	return splitFields(raw), nil
 }
 
-// notEmpty returns a validation function that rejects blank input.
+// notEmpty returns a validation function that rejects blank or
+// whitespace-only input.
 func notEmpty(fieldName string) func(string) error {
 	return func(s string) error {
-		if len(s) == 0 {
+		if strings.TrimSpace(s) == "" {
 			return fmt.Errorf("%s is required", fieldName)
 		}
 		return nil

internal/form/form_test.go

@@ -16,7 +16,7 @@ func TestNotEmpty(t *testing.T) {
 	}{
 		{name: "empty string", input: "", wantErr: true},
 		{name: "non-empty string", input: "hello", wantErr: false},
-		{name: "whitespace only", input: "  ", wantErr: false},
+		{name: "whitespace only", input: "  ", wantErr: true},
 	}
 
 	for _, tt := range tests {

internal/restic/exec_test.go

@@ -44,7 +44,6 @@ func TestBuildArgv(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 
@@ -234,7 +233,6 @@ func TestResolveEnvironCommands(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 

internal/restic/list_snapshots_test.go

@@ -134,7 +134,6 @@ func TestBuildSnapshotCmd(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 

internal/restic/snapshots.go

@@ -33,7 +33,8 @@ func ParseSnapshots(data []byte) ([]Snapshot, error) {
 //
 //	<short_id>  <date> <time>  <hostname>  <paths>  [<tags>]
 //
-// Tags are omitted when empty. Hostname shows "(unknown)" when blank.
+// Paths and tags are omitted when empty. Hostname shows "(unknown)"
+// when blank.
 func FormatSnapshotLine(s Snapshot) string {
 	var b strings.Builder
 

internal/restic/snapshots_test.go

@@ -5,8 +5,8 @@ import (
 	"time"
 )
 
-// resolveFixtureTOML-style: embed test JSON as string constants so tests
-// are self-contained and don't depend on external files.
+// Embed test JSON as string constants so tests are self-contained and
+// don't depend on external files.
 
 const snapshotFixtureNormal = `[
   {