Merge pull request #4234 from thndrbrrr/forget-opts-neg1-means-forever-issue-2565

restic forget --keep-* options will interpret -1 as "forever"

changelog/unreleased/issue-2565

@@ -0,0 +1,8 @@
+Bugfix: Restic forget --keep-* options now interpret "-1" as "forever"
+
+Restic would forget snapshots that should have been kept when "-1" was
+used as a value for --keep-* options. It now interprets "-1" as forever,
+e.g. an option like --keep-monthly -1 will keep all monthly snapshots.
+
+https://github.com/restic/restic/issues/2565
+https://github.com/restic/restic/pull/4234

cmd/restic/cmd_forget.go

@@ -67,12 +67,12 @@ func init() {
 	cmdRoot.AddCommand(cmdForget)
 
 	f := cmdForget.Flags()
-	f.IntVarP(&forgetOptions.Last, "keep-last", "l", 0, "keep the last `n` snapshots")
+	f.IntVarP(&forgetOptions.Last, "keep-last", "l", 0, "keep the last `n` snapshots (use '-1' to keep all snapshots)")
-	f.IntVarP(&forgetOptions.Hourly, "keep-hourly", "H", 0, "keep the last `n` hourly snapshots")
+	f.IntVarP(&forgetOptions.Hourly, "keep-hourly", "H", 0, "keep the last `n` hourly snapshots (use '-1' to keep all hourly snapshots)")
-	f.IntVarP(&forgetOptions.Daily, "keep-daily", "d", 0, "keep the last `n` daily snapshots")
+	f.IntVarP(&forgetOptions.Daily, "keep-daily", "d", 0, "keep the last `n` daily snapshots (use '-1' to keep all daily snapshots)")
-	f.IntVarP(&forgetOptions.Weekly, "keep-weekly", "w", 0, "keep the last `n` weekly snapshots")
+	f.IntVarP(&forgetOptions.Weekly, "keep-weekly", "w", 0, "keep the last `n` weekly snapshots (use '-1' to keep all weekly snapshots)")
-	f.IntVarP(&forgetOptions.Monthly, "keep-monthly", "m", 0, "keep the last `n` monthly snapshots")
+	f.IntVarP(&forgetOptions.Monthly, "keep-monthly", "m", 0, "keep the last `n` monthly snapshots (use '-1' to keep all monthly snapshots)")
-	f.IntVarP(&forgetOptions.Yearly, "keep-yearly", "y", 0, "keep the last `n` yearly snapshots")
+	f.IntVarP(&forgetOptions.Yearly, "keep-yearly", "y", 0, "keep the last `n` yearly snapshots (use '-1' to keep all yearly snapshots)")
 	f.VarP(&forgetOptions.Within, "keep-within", "", "keep snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot")
 	f.VarP(&forgetOptions.WithinHourly, "keep-within-hourly", "", "keep hourly snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot")
 	f.VarP(&forgetOptions.WithinDaily, "keep-within-daily", "", "keep daily snapshots that are newer than `duration` (eg. 1y5m7d2h) relative to the latest snapshot")
@@ -99,8 +99,29 @@ func init() {
 	addPruneOptions(cmdForget)
 }
 
+func verifyForgetOptions(opts *ForgetOptions) error {
+	if opts.Last < -1 || opts.Hourly < -1 || opts.Daily < -1 || opts.Weekly < -1 ||
+		opts.Monthly < -1 || opts.Yearly < -1 {
+		return errors.Fatal("negative values other than -1 are not allowed for --keep-*")
+	}
+
+	for _, d := range []restic.Duration{opts.Within, opts.WithinHourly, opts.WithinDaily,
+		opts.WithinMonthly, opts.WithinWeekly, opts.WithinYearly} {
+		if d.Hours < 0 || d.Days < 0 || d.Months < 0 || d.Years < 0 {
+			return errors.Fatal("durations containing negative values are not allowed for --keep-within*")
+		}
+	}
+
+	return nil
+}
+
 func runForget(ctx context.Context, opts ForgetOptions, gopts GlobalOptions, args []string) error {
-	err := verifyPruneOptions(&pruneOptions)
+	err := verifyForgetOptions(&opts)
+	if err != nil {
+		return err
+	}
+
+	err = verifyPruneOptions(&pruneOptions)
 	if err != nil {
 		return err
 	}

cmd/restic/cmd_forget_test.go

@@ -0,0 +1,65 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/restic/restic/internal/restic"
+	rtest "github.com/restic/restic/internal/test"
+)
+
+func TestForgetOptionValues(t *testing.T) {
+	const negValErrorMsg = "Fatal: negative values other than -1 are not allowed for --keep-*"
+	const negDurationValErrorMsg = "Fatal: durations containing negative values are not allowed for --keep-within*"
+	testCases := []struct {
+		input        ForgetOptions
+		expectsError bool
+		errorMsg     string
+	}{
+		{ForgetOptions{Last: 1}, false, ""},
+		{ForgetOptions{Hourly: 1}, false, ""},
+		{ForgetOptions{Daily: 1}, false, ""},
+		{ForgetOptions{Weekly: 1}, false, ""},
+		{ForgetOptions{Monthly: 1}, false, ""},
+		{ForgetOptions{Yearly: 1}, false, ""},
+		{ForgetOptions{Last: 0}, false, ""},
+		{ForgetOptions{Hourly: 0}, false, ""},
+		{ForgetOptions{Daily: 0}, false, ""},
+		{ForgetOptions{Weekly: 0}, false, ""},
+		{ForgetOptions{Monthly: 0}, false, ""},
+		{ForgetOptions{Yearly: 0}, false, ""},
+		{ForgetOptions{Last: -1}, false, ""},
+		{ForgetOptions{Hourly: -1}, false, ""},
+		{ForgetOptions{Daily: -1}, false, ""},
+		{ForgetOptions{Weekly: -1}, false, ""},
+		{ForgetOptions{Monthly: -1}, false, ""},
+		{ForgetOptions{Yearly: -1}, false, ""},
+		{ForgetOptions{Last: -2}, true, negValErrorMsg},
+		{ForgetOptions{Hourly: -2}, true, negValErrorMsg},
+		{ForgetOptions{Daily: -2}, true, negValErrorMsg},
+		{ForgetOptions{Weekly: -2}, true, negValErrorMsg},
+		{ForgetOptions{Monthly: -2}, true, negValErrorMsg},
+		{ForgetOptions{Yearly: -2}, true, negValErrorMsg},
+		{ForgetOptions{Within: restic.ParseDurationOrPanic("1y2m3d3h")}, false, ""},
+		{ForgetOptions{WithinHourly: restic.ParseDurationOrPanic("1y2m3d3h")}, false, ""},
+		{ForgetOptions{WithinDaily: restic.ParseDurationOrPanic("1y2m3d3h")}, false, ""},
+		{ForgetOptions{WithinWeekly: restic.ParseDurationOrPanic("1y2m3d3h")}, false, ""},
+		{ForgetOptions{WithinMonthly: restic.ParseDurationOrPanic("2y4m6d8h")}, false, ""},
+		{ForgetOptions{WithinYearly: restic.ParseDurationOrPanic("2y4m6d8h")}, false, ""},
+		{ForgetOptions{Within: restic.ParseDurationOrPanic("-1y2m3d3h")}, true, negDurationValErrorMsg},
+		{ForgetOptions{WithinHourly: restic.ParseDurationOrPanic("1y-2m3d3h")}, true, negDurationValErrorMsg},
+		{ForgetOptions{WithinDaily: restic.ParseDurationOrPanic("1y2m-3d3h")}, true, negDurationValErrorMsg},
+		{ForgetOptions{WithinWeekly: restic.ParseDurationOrPanic("1y2m3d-3h")}, true, negDurationValErrorMsg},
+		{ForgetOptions{WithinMonthly: restic.ParseDurationOrPanic("-2y4m6d8h")}, true, negDurationValErrorMsg},
+		{ForgetOptions{WithinYearly: restic.ParseDurationOrPanic("2y-4m6d8h")}, true, negDurationValErrorMsg},
+	}
+
+	for _, testCase := range testCases {
+		err := verifyForgetOptions(&testCase.input)
+		if testCase.expectsError {
+			rtest.Assert(t, err != nil, "should have returned error for input %+v", testCase.input)
+			rtest.Equals(t, testCase.errorMsg, err.Error())
+		} else {
+			rtest.Assert(t, err == nil, "expected no error for input %+v", testCase.input)
+		}
+	}
+}

doc/060_forget.rst

@@ -205,6 +205,7 @@ The ``forget`` command accepts the following policy options:
     natural time boundaries and *not* relative to when you run ``forget``. Weeks
     are Monday 00:00 to Sunday 23:59, days 00:00 to 23:59, hours :00 to :59, etc.
     They also only count hours/days/weeks/etc which have one or more snapshots.
+    A value of ``-1`` will be interpreted as "forever", i.e. "keep all".
 
 .. note:: All duration related options (``--keep-{within,-*}``) ignore snapshots
     with a timestamp in the future (relative to when the ``forget`` command is

internal/restic/snapshot_group.go

@@ -68,7 +68,7 @@ type SnapshotGroupKey struct {
 }
 
 // GroupSnapshots takes a list of snapshots and a grouping criteria and creates
-// a group list of snapshots.
+// a grouped list of snapshots.
 func GroupSnapshots(snapshots Snapshots, groupBy SnapshotGroupByOptions) (map[string]Snapshots, bool, error) {
 	// group by hostname and dirs
 	snapshotGroups := make(map[string]Snapshots)

internal/restic/snapshot_policy.go

@@ -31,23 +31,22 @@ func (e ExpirePolicy) String() (s string) {
 	var keeps []string
 	var keepw []string
 
-	if e.Last > 0 {
+	for _, opt := range []struct {
-		keeps = append(keeps, fmt.Sprintf("%d latest", e.Last))
+		count int
-	}
+		descr string
-	if e.Hourly > 0 {
+	}{
-		keeps = append(keeps, fmt.Sprintf("%d hourly", e.Hourly))
+		{e.Last, "latest"},
-	}
+		{e.Hourly, "hourly"},
-	if e.Daily > 0 {
+		{e.Daily, "daily"},
-		keeps = append(keeps, fmt.Sprintf("%d daily", e.Daily))
+		{e.Weekly, "weekly"},
-	}
+		{e.Monthly, "monthly"},
-	if e.Weekly > 0 {
+		{e.Yearly, "yearly"},
-		keeps = append(keeps, fmt.Sprintf("%d weekly", e.Weekly))
+	} {
-	}
+		if opt.count > 0 {
-	if e.Monthly > 0 {
+			keeps = append(keeps, fmt.Sprintf("%d %s", opt.count, opt.descr))
-		keeps = append(keeps, fmt.Sprintf("%d monthly", e.Monthly))
+		} else if opt.count == -1 {
-	}
+			keeps = append(keeps, fmt.Sprintf("all %s", opt.descr))
-	if e.Yearly > 0 {
+		}
-		keeps = append(keeps, fmt.Sprintf("%d yearly", e.Yearly))
 	}
 
 	if !e.WithinHourly.Zero() {
@@ -100,13 +99,7 @@ func (e ExpirePolicy) String() (s string) {
 	return s
 }
 
-// Sum returns the maximum number of snapshots to be kept according to this
+// Empty returns true if no policy has been configured (all values zero).
-// 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).
 func (e ExpirePolicy) Empty() bool {
 	if len(e.Tags) != 0 {
 		return false
@@ -260,13 +253,16 @@ 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 > 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 > 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 {
+// Returns the maximum number of snapshots to be kept according to this policy.
-	d, err := restic.ParseDuration(s)
+// If any of the counts is -1 it will return 0.
-	if err != nil {
+func policySum(e *restic.ExpirePolicy) int {
-		panic(err)
+	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,30 @@ func TestApplyPolicy(t *testing.T) {
 		{Tags: []restic.TagList{{"foo"}}},
 		{Tags: []restic.TagList{{"foo", "bar"}}},
 		{Tags: []restic.TagList{{"foo"}, {"bar"}}},
-		{Within: parseDuration("1d")},
+		{Within: restic.ParseDurationOrPanic("1d")},
-		{Within: parseDuration("2d")},
+		{Within: restic.ParseDurationOrPanic("2d")},
-		{Within: parseDuration("7d")},
+		{Within: restic.ParseDurationOrPanic("7d")},
-		{Within: parseDuration("1m")},
+		{Within: restic.ParseDurationOrPanic("1m")},
-		{Within: parseDuration("1m14d")},
+		{Within: restic.ParseDurationOrPanic("1m14d")},
-		{Within: parseDuration("1y1d1m")},
+		{Within: restic.ParseDurationOrPanic("1y1d1m")},
-		{Within: parseDuration("13d23h")},
+		{Within: restic.ParseDurationOrPanic("13d23h")},
-		{Within: parseDuration("2m2h")},
+		{Within: restic.ParseDurationOrPanic("2m2h")},
-		{Within: parseDuration("1y2m3d3h")},
+		{Within: restic.ParseDurationOrPanic("1y2m3d3h")},
-		{WithinHourly: parseDuration("1y2m3d3h")},
+		{WithinHourly: restic.ParseDurationOrPanic("1y2m3d3h")},
-		{WithinDaily: parseDuration("1y2m3d3h")},
+		{WithinDaily: restic.ParseDurationOrPanic("1y2m3d3h")},
-		{WithinWeekly: parseDuration("1y2m3d3h")},
+		{WithinWeekly: restic.ParseDurationOrPanic("1y2m3d3h")},
-		{WithinMonthly: parseDuration("1y2m3d3h")},
+		{WithinMonthly: restic.ParseDurationOrPanic("1y2m3d3h")},
-		{WithinYearly: parseDuration("1y2m3d3h")},
+		{WithinYearly: restic.ParseDurationOrPanic("1y2m3d3h")},
-		{Within: parseDuration("1h"),
+		{Within: restic.ParseDurationOrPanic("1h"),
-			WithinHourly:  parseDuration("1d"),
+			WithinHourly:  restic.ParseDurationOrPanic("1d"),
-			WithinDaily:   parseDuration("7d"),
+			WithinDaily:   restic.ParseDurationOrPanic("7d"),
-			WithinWeekly:  parseDuration("1m"),
+			WithinWeekly:  restic.ParseDurationOrPanic("1m"),
-			WithinMonthly: parseDuration("1y"),
+			WithinMonthly: restic.ParseDurationOrPanic("1y"),
-			WithinYearly:  parseDuration("9999y")},
+			WithinYearly:  restic.ParseDurationOrPanic("9999y")},
+		{Last: -1},             // keep all
+		{Last: -1, Hourly: -1}, // keep all (Last overrides Hourly)
+		{Hourly: -1},           // keep all hourlies
+		{Daily: 3, Weekly: 2, Monthly: -1, Yearly: -1},
 	}
 
 	for i, p := range tests {
@@ -251,9 +256,9 @@ func TestApplyPolicy(t *testing.T) {
 					len(keep)+len(remove), len(testExpireSnapshots))
 			}
 
-			if p.Sum() > 0 && len(keep) > p.Sum() {
+			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",
-					p.Sum(), len(keep))
+					policySum(&p), len(keep))
 			}
 
 			if len(keep) != len(reasons) {

internal/restic/testdata/policy_keep_snapshots_39

@@ -0,0 +1,194 @@
+{
+  "keep": [
+    {
+      "time": "2016-01-18T12:02:03Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2016-01-12T21:08:03Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2016-01-09T21:02:03Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2015-11-22T10:20:30Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2015-10-22T10:20:30Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2015-09-22T10:20:30Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2015-08-22T10:20:30Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2014-11-22T10:20:30Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2014-10-22T10:20:30Z",
+      "tree": null,
+      "paths": null,
+      "tags": [
+        "foo"
+      ]
+    },
+    {
+      "time": "2014-09-22T10:20:30Z",
+      "tree": null,
+      "paths": null
+    },
+    {
+      "time": "2014-08-22T10:20:30Z",
+      "tree": null,
+      "paths": null
+    }
+  ],
+  "reasons": [
+    {
+      "snapshot": {
+        "time": "2016-01-18T12:02:03Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "daily snapshot",
+        "weekly snapshot",
+  			"monthly snapshot",
+  			"yearly snapshot"
+      ],
+      "counters": {"Daily": 2, "Weekly": 1, "Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2016-01-12T21:08:03Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "daily snapshot",
+        "weekly snapshot"
+      ],
+      "counters": {"Daily": 1, "Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2016-01-09T21:02:03Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "daily snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2015-11-22T10:20:30Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "monthly snapshot",
+        "yearly snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2015-10-22T10:20:30Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "monthly snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2015-09-22T10:20:30Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "monthly snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2015-08-22T10:20:30Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "monthly snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2014-11-22T10:20:30Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "monthly snapshot",
+        "yearly snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2014-10-22T10:20:30Z",
+        "tree": null,
+        "paths": null,
+        "tags": [
+          "foo"
+        ]
+      },
+      "matches": [
+        "monthly snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2014-09-22T10:20:30Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "monthly snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    },
+    {
+      "snapshot": {
+        "time": "2014-08-22T10:20:30Z",
+        "tree": null,
+        "paths": null
+      },
+      "matches": [
+        "monthly snapshot"
+      ],
+      "counters": {"Monthly": -1, "Yearly": -1}
+    }
+  ]
+}

internal/restic/testing.go

@@ -212,3 +212,14 @@ func TestParseHandle(s string, t BlobType) BlobHandle {
 func TestSetSnapshotID(t testing.TB, sn *Snapshot, id ID) {
 	sn.id = &id
 }
+
+// ParseDurationOrPanic parses a duration from a string or panics if string is invalid.
+// The format is `6y5m234d37h`.
+func ParseDurationOrPanic(s string) Duration {
+	d, err := ParseDuration(s)
+	if err != nil {
+		panic(err)
+	}
+
+	return d
+}