Repository: restic/restic
Pull request #4426 (2023-07-28 17:07:13)

forget: replace `--keep-* -1` with `--keep-* unlimited`

<!--
Thank you very much for contributing code or documentation to restic! Please
fill out the following questions to make it easier for us to review your
changes.
-->

What does this PR change? What problem does it solve?
-----------------------------------------------------
This ensures consistency with the `prune --max-unused unlimited` option.

Now all negative values for `forget --keep-* n` are forbidden. To keep all snapshots the special value `unlimited` can be used.

<!--
Describe the changes and their purpose here, as detailed as needed.
-->

Was the change previously discussed in an issue or on the forum?
----------------------------------------------------------------
Follow-up to https://github.com/restic/restic/pull/4234 
<!--
Link issues and relevant forum posts here.

If this PR resolves an issue on GitHub, use "Closes #1234" so that the issue
is closed automatically when this PR is merged.
-->

Checklist
---------

<!--
You do not need to check all the boxes below all at once. Feel free to take
your time and add more commits. If you're done and ready for review, please
check the last box. Enable a checkbox by replacing [ ] with [x].
-->

- [x] I have read the [contribution guidelines](https://github.com/restic/restic/blob/master/CONTRIBUTING.md#providing-patches).
- [x] I have [enabled maintainer edits](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork).
- [x] I have added tests for all code changes.
- [ ] I have added documentation for relevant changes (in the manual).
- [x] There's a new file in `changelog/unreleased/` that describes the changes for our users (see [template](https://github.com/restic/restic/blob/master/changelog/TEMPLATE)).
- [x] I have run `gofmt` on the code in all commits.
- [x] All commit messages are formatted in the same style as [the other commits in the repo](https://github.com/restic/restic/blob/master/CONTRIBUTING.md#git-commits).
- [ ] I'm done! This pull request is ready for review.



Patch:
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,


Test patch:
diff --git a/cmd/restic/cmd_forget_test.go b/cmd/restic/cmd_forget_test.go
index 9fd5c7bb0..ddeef028a 100644
--- a/cmd/restic/cmd_forget_test.go
+++ b/cmd/restic/cmd_forget_test.go
@@ -7,55 +7,84 @@ 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*"
 	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 {