Refactor policy sum calculation & duration parsing

- Convert policy sum calculation to function and move to tests.
- Remove parseDuration(...) and use ParseDurationOrPanic(...) instead.

internal/restic/snapshot_policy.go

@@ -100,13 +100,7 @@ func (e ExpirePolicy) String() (s string) {
 	return s
 }
 
-// Sum returns the maximum number of snapshots to be kept according to this
-// policy.
-func (e ExpirePolicy) Sum() int {
-	return e.Last + e.Hourly + e.Daily + e.Weekly + e.Monthly + e.Yearly
-}
-
-// Empty returns true iff no policy has been configured (all values zero).
+// Empty returns true if no policy has been configured (all values zero).
 func (e ExpirePolicy) Empty() bool {
 	if len(e.Tags) != 0 {
 		return false
@@ -260,15 +254,15 @@ func ApplyPolicy(list Snapshots, p ExpirePolicy) (keep, remove Snapshots, reason
 
 		// Now update the other buckets and see if they have some counts left.
 		for i, b := range buckets {
-			if b.Count <= -1 || b.Count > 0 {
+			// -1 means "keep all"
+			if b.Count > 0 || b.Count == -1 {
 				val := b.bucker(cur.Time, nr)
 				if val != b.Last {
 					debug.Log("keep %v %v, bucker %v, val %v\n", cur.Time, cur.id.Str(), i, val)
 					keepSnap = true
 					buckets[i].Last = val
-					buckets[i].Count--
-					if buckets[i].Count < -1 {
-						buckets[i].Count = -1
+					if buckets[i].Count > 0 {
+						buckets[i].Count--
 					}
 					keepSnapReasons = append(keepSnapReasons, b.reason)
 				}

internal/restic/snapshot_policy_test.go

@@ -22,13 +22,14 @@ func parseTimeUTC(s string) time.Time {
 	return t.UTC()
 }
 
-func parseDuration(s string) restic.Duration {
-	d, err := restic.ParseDuration(s)
-	if err != nil {
-		panic(err)
+// Returns the maximum number of snapshots to be kept according to this policy.
+// If any of the counts is -1 it will return 0.
+func policySum(e *restic.ExpirePolicy) int {
+	if e.Last == -1 || e.Hourly == -1 || e.Daily == -1 || e.Weekly == -1 || e.Monthly == -1 || e.Yearly == -1 {
+		return 0
 	}
 
-	return d
+	return e.Last + e.Hourly + e.Daily + e.Weekly + e.Monthly + e.Yearly
 }
 
 func TestExpireSnapshotOps(t *testing.T) {
@@ -46,7 +47,7 @@ func TestExpireSnapshotOps(t *testing.T) {
 		if isEmpty != d.expectEmpty {
 			t.Errorf("empty test %v: wrong result, want:\n  %#v\ngot:\n  %#v", i, d.expectEmpty, isEmpty)
 		}
-		hasSum := d.p.Sum()
+		hasSum := policySum(d.p)
 		if hasSum != d.expectSum {
 			t.Errorf("sum test %v: wrong result, want:\n  %#v\ngot:\n  %#v", i, d.expectSum, hasSum)
 		}
@@ -219,26 +220,26 @@ func TestApplyPolicy(t *testing.T) {
 		{Tags: []restic.TagList{{"foo"}}},
 		{Tags: []restic.TagList{{"foo", "bar"}}},
 		{Tags: []restic.TagList{{"foo"}, {"bar"}}},
-		{Within: parseDuration("1d")},
-		{Within: parseDuration("2d")},
-		{Within: parseDuration("7d")},
-		{Within: parseDuration("1m")},
-		{Within: parseDuration("1m14d")},
-		{Within: parseDuration("1y1d1m")},
-		{Within: parseDuration("13d23h")},
-		{Within: parseDuration("2m2h")},
-		{Within: parseDuration("1y2m3d3h")},
-		{WithinHourly: parseDuration("1y2m3d3h")},
-		{WithinDaily: parseDuration("1y2m3d3h")},
-		{WithinWeekly: parseDuration("1y2m3d3h")},
-		{WithinMonthly: parseDuration("1y2m3d3h")},
-		{WithinYearly: parseDuration("1y2m3d3h")},
-		{Within: parseDuration("1h"),
-			WithinHourly:  parseDuration("1d"),
-			WithinDaily:   parseDuration("7d"),
-			WithinWeekly:  parseDuration("1m"),
-			WithinMonthly: parseDuration("1y"),
-			WithinYearly:  parseDuration("9999y")},
+		{Within: restic.ParseDurationOrPanic("1d")},
+		{Within: restic.ParseDurationOrPanic("2d")},
+		{Within: restic.ParseDurationOrPanic("7d")},
+		{Within: restic.ParseDurationOrPanic("1m")},
+		{Within: restic.ParseDurationOrPanic("1m14d")},
+		{Within: restic.ParseDurationOrPanic("1y1d1m")},
+		{Within: restic.ParseDurationOrPanic("13d23h")},
+		{Within: restic.ParseDurationOrPanic("2m2h")},
+		{Within: restic.ParseDurationOrPanic("1y2m3d3h")},
+		{WithinHourly: restic.ParseDurationOrPanic("1y2m3d3h")},
+		{WithinDaily: restic.ParseDurationOrPanic("1y2m3d3h")},
+		{WithinWeekly: restic.ParseDurationOrPanic("1y2m3d3h")},
+		{WithinMonthly: restic.ParseDurationOrPanic("1y2m3d3h")},
+		{WithinYearly: restic.ParseDurationOrPanic("1y2m3d3h")},
+		{Within: restic.ParseDurationOrPanic("1h"),
+			WithinHourly:  restic.ParseDurationOrPanic("1d"),
+			WithinDaily:   restic.ParseDurationOrPanic("7d"),
+			WithinWeekly:  restic.ParseDurationOrPanic("1m"),
+			WithinMonthly: restic.ParseDurationOrPanic("1y"),
+			WithinYearly:  restic.ParseDurationOrPanic("9999y")},
 		{Last: -1},             // keep all
 		{Last: -1, Hourly: -1}, // keep all (Last overrides Hourly)
 		{Hourly: -1},           // keep all hourlies
@@ -255,11 +256,9 @@ func TestApplyPolicy(t *testing.T) {
 					len(keep)+len(remove), len(testExpireSnapshots))
 			}
 
-			if p.Last >= 0 && p.Hourly >= 0 && p.Daily >= 0 && p.Weekly >= 0 && p.Monthly >= 0 && p.Yearly >= 0 {
-				if p.Sum() > 0 && len(keep) > p.Sum() {
-					t.Errorf("not enough snapshots removed: policy allows %v snapshots to remain, but ended up with %v",
-						p.Sum(), len(keep))
-				}
+			if policySum(&p) > 0 && len(keep) > policySum(&p) {
+				t.Errorf("not enough snapshots removed: policy allows %v snapshots to remain, but ended up with %v",
+					policySum(&p), len(keep))
 			}
 
 			if len(keep) != len(reasons) {