From 50b43fbac090245e852991c063b3e9b4fc4b54d3 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 28 Jul 2023 18:59:57 +0200 Subject: [PATCH 1/2] forget: replace `--keep-* -1` with `--keep-* unlimited` This ensures consistency with the `prune --max-unused unlimited` option. --- changelog/unreleased/issue-2565 | 9 +++-- cmd/restic/cmd_forget.go | 72 ++++++++++++++++++++++++--------- cmd/restic/cmd_forget_test.go | 30 ++++++++++++++ 3 files changed, 89 insertions(+), 22 deletions(-) diff --git a/changelog/unreleased/issue-2565 b/changelog/unreleased/issue-2565 index 4150dcda4..62c8c3ca1 100644 --- a/changelog/unreleased/issue-2565 +++ b/changelog/unreleased/issue-2565 @@ -1,8 +1,9 @@ -Bugfix: Restic forget --keep-* options now interpret "-1" as "forever" +Bugfix: Support "unlimited" in `forget --keep-*` options -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. +Restic would forget snapshots that should have been kept when a negative value +was passed to the `--keep-*` options. Negative values are now forbidden. To +keep all snapshots, the special value `unlimited` is now supported. For +example, `--keep-monthly unlimited` will keep all monthly snapshots. https://github.com/restic/restic/issues/2565 https://github.com/restic/restic/pull/4234 diff --git a/cmd/restic/cmd_forget.go b/cmd/restic/cmd_forget.go index d8e64bc6a..22398b806 100644 --- a/cmd/restic/cmd_forget.go +++ b/cmd/restic/cmd_forget.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "io" + "strconv" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/restic" @@ -36,14 +37,49 @@ Exit status is 0 if the command was successful, and non-zero if there was any er }, } +type ForgetPolicyCount int + +var ErrNegativePolicyCount = errors.New("negative values not allowed, use 'unlimited' instead") + +func (c *ForgetPolicyCount) Set(s string) error { + switch s { + case "unlimited": + *c = -1 + default: + val, err := strconv.ParseInt(s, 10, 0) + if err != nil { + return err + } + if val < 0 { + return ErrNegativePolicyCount + } + *c = ForgetPolicyCount(val) + } + + return nil +} + +func (c *ForgetPolicyCount) String() string { + switch *c { + case -1: + return "unlimited" + default: + return strconv.FormatInt(int64(*c), 10) + } +} + +func (c *ForgetPolicyCount) Type() string { + return "n" +} + // ForgetOptions collects all options for the forget command. type ForgetOptions struct { - Last int - Hourly int - Daily int - Weekly int - Monthly int - Yearly int + Last ForgetPolicyCount + Hourly ForgetPolicyCount + Daily ForgetPolicyCount + Weekly ForgetPolicyCount + Monthly ForgetPolicyCount + Yearly ForgetPolicyCount Within restic.Duration WithinHourly restic.Duration WithinDaily restic.Duration @@ -67,12 +103,12 @@ func init() { cmdRoot.AddCommand(cmdForget) f := cmdForget.Flags() - 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 (use '-1' to keep all hourly 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 (use '-1' to keep all weekly 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 (use '-1' to keep all yearly snapshots)") + f.VarP(&forgetOptions.Last, "keep-last", "l", "keep the last `n` snapshots (use 'unlimited' to keep all snapshots)") + f.VarP(&forgetOptions.Hourly, "keep-hourly", "H", "keep the last `n` hourly snapshots (use 'unlimited' to keep all hourly snapshots)") + f.VarP(&forgetOptions.Daily, "keep-daily", "d", "keep the last `n` daily snapshots (use 'unlimited' to keep all daily snapshots)") + f.VarP(&forgetOptions.Weekly, "keep-weekly", "w", "keep the last `n` weekly snapshots (use 'unlimited' to keep all weekly snapshots)") + f.VarP(&forgetOptions.Monthly, "keep-monthly", "m", "keep the last `n` monthly snapshots (use 'unlimited' to keep all monthly snapshots)") + f.VarP(&forgetOptions.Yearly, "keep-yearly", "y", "keep the last `n` yearly snapshots (use 'unlimited' 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") @@ -165,12 +201,12 @@ func runForget(ctx context.Context, opts ForgetOptions, gopts GlobalOptions, arg } policy := restic.ExpirePolicy{ - Last: opts.Last, - Hourly: opts.Hourly, - Daily: opts.Daily, - Weekly: opts.Weekly, - Monthly: opts.Monthly, - Yearly: opts.Yearly, + Last: int(opts.Last), + Hourly: int(opts.Hourly), + Daily: int(opts.Daily), + Weekly: int(opts.Weekly), + Monthly: int(opts.Monthly), + Yearly: int(opts.Yearly), Within: opts.Within, WithinHourly: opts.WithinHourly, WithinDaily: opts.WithinDaily, diff --git a/cmd/restic/cmd_forget_test.go b/cmd/restic/cmd_forget_test.go index 9fd5c7bb0..d59419d06 100644 --- a/cmd/restic/cmd_forget_test.go +++ b/cmd/restic/cmd_forget_test.go @@ -7,6 +7,36 @@ import ( rtest "github.com/restic/restic/internal/test" ) +func TestForgetPolicyValues(t *testing.T) { + testCases := []struct { + input string + value ForgetPolicyCount + err string + }{ + {"0", ForgetPolicyCount(0), ""}, + {"1", ForgetPolicyCount(1), ""}, + {"unlimited", ForgetPolicyCount(-1), ""}, + {"", ForgetPolicyCount(0), "strconv.ParseInt: parsing \"\": invalid syntax"}, + {"-1", ForgetPolicyCount(0), ErrNegativePolicyCount.Error()}, + {"abc", ForgetPolicyCount(0), "strconv.ParseInt: parsing \"abc\": invalid syntax"}, + } + for _, testCase := range testCases { + t.Run("", func(t *testing.T) { + var count ForgetPolicyCount + err := count.Set(testCase.input) + + if testCase.err != "" { + rtest.Assert(t, err != nil, "should have returned error for input %+v", testCase.input) + rtest.Equals(t, testCase.err, err.Error()) + } else { + rtest.Assert(t, err == nil, "expected no error for input %+v, got %v", testCase.input, err) + rtest.Equals(t, testCase.value, count) + rtest.Equals(t, testCase.input, count.String()) + } + }) + } +} + 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*" From 3f919f2371ea5a10622979bf8ed1374f0ae3944b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 28 Jul 2023 19:20:46 +0200 Subject: [PATCH 2/2] forget: simplify test --- cmd/restic/cmd_forget_test.go | 79 +++++++++++++++++------------------ 1 file changed, 39 insertions(+), 40 deletions(-) diff --git a/cmd/restic/cmd_forget_test.go b/cmd/restic/cmd_forget_test.go index d59419d06..ddeef028a 100644 --- a/cmd/restic/cmd_forget_test.go +++ b/cmd/restic/cmd_forget_test.go @@ -41,51 +41,50 @@ 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 + input ForgetOptions + 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}, + {ForgetOptions{Last: 1}, ""}, + {ForgetOptions{Hourly: 1}, ""}, + {ForgetOptions{Daily: 1}, ""}, + {ForgetOptions{Weekly: 1}, ""}, + {ForgetOptions{Monthly: 1}, ""}, + {ForgetOptions{Yearly: 1}, ""}, + {ForgetOptions{Last: 0}, ""}, + {ForgetOptions{Hourly: 0}, ""}, + {ForgetOptions{Daily: 0}, ""}, + {ForgetOptions{Weekly: 0}, ""}, + {ForgetOptions{Monthly: 0}, ""}, + {ForgetOptions{Yearly: 0}, ""}, + {ForgetOptions{Last: -1}, ""}, + {ForgetOptions{Hourly: -1}, ""}, + {ForgetOptions{Daily: -1}, ""}, + {ForgetOptions{Weekly: -1}, ""}, + {ForgetOptions{Monthly: -1}, ""}, + {ForgetOptions{Yearly: -1}, ""}, + {ForgetOptions{Last: -2}, negValErrorMsg}, + {ForgetOptions{Hourly: -2}, negValErrorMsg}, + {ForgetOptions{Daily: -2}, negValErrorMsg}, + {ForgetOptions{Weekly: -2}, negValErrorMsg}, + {ForgetOptions{Monthly: -2}, negValErrorMsg}, + {ForgetOptions{Yearly: -2}, negValErrorMsg}, + {ForgetOptions{Within: restic.ParseDurationOrPanic("1y2m3d3h")}, ""}, + {ForgetOptions{WithinHourly: restic.ParseDurationOrPanic("1y2m3d3h")}, ""}, + {ForgetOptions{WithinDaily: restic.ParseDurationOrPanic("1y2m3d3h")}, ""}, + {ForgetOptions{WithinWeekly: restic.ParseDurationOrPanic("1y2m3d3h")}, ""}, + {ForgetOptions{WithinMonthly: restic.ParseDurationOrPanic("2y4m6d8h")}, ""}, + {ForgetOptions{WithinYearly: restic.ParseDurationOrPanic("2y4m6d8h")}, ""}, + {ForgetOptions{Within: restic.ParseDurationOrPanic("-1y2m3d3h")}, negDurationValErrorMsg}, + {ForgetOptions{WithinHourly: restic.ParseDurationOrPanic("1y-2m3d3h")}, negDurationValErrorMsg}, + {ForgetOptions{WithinDaily: restic.ParseDurationOrPanic("1y2m-3d3h")}, negDurationValErrorMsg}, + {ForgetOptions{WithinWeekly: restic.ParseDurationOrPanic("1y2m3d-3h")}, negDurationValErrorMsg}, + {ForgetOptions{WithinMonthly: restic.ParseDurationOrPanic("-2y4m6d8h")}, negDurationValErrorMsg}, + {ForgetOptions{WithinYearly: restic.ParseDurationOrPanic("2y-4m6d8h")}, negDurationValErrorMsg}, } for _, testCase := range testCases { err := verifyForgetOptions(&testCase.input) - if testCase.expectsError { + if testCase.errorMsg != "" { rtest.Assert(t, err != nil, "should have returned error for input %+v", testCase.input) rtest.Equals(t, testCase.errorMsg, err.Error()) } else {