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

[thndrbrrr]

What does this PR change? What problem does it solve?

restic forget will interpret -1 as "forever" when used with --keep-* options. Using a value less than -1 will result in an error.

Note: This PR does not change the existing behaviour of --keep-within* options until it's decided how durations containing negative values should be handled (see my comment in #2565).

--keep-within* options containing negative values are considered invalid.

Documentation has not been updated (waiting on confirmation for this PR to go ahead).

Was the change previously discussed in an issue or on the forum?

Closes #2565 (if --keep-within-* options could be considered a different issue)

Checklist

• I have read the contribution guidelines.
• I have enabled maintainer edits.
• I have added tests for all code changes.
• I have added documentation for relevant changes (in the manual).
• There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
• I have run gofmt on the code in all commits.
• All commit messages are formatted in the same style as the other commits in the repo.
• I'm done! This pull request is ready for review.

[MichaelEischer]

Thanks for working on this issue! The code looks solid except for the comments below. Regarding --keep-within-*, I think restic should just return an error if any of them contains a negative duration. I don't see a use case in which that would make sense.

[thndrbrrr]

Thanks for the feedback, @MichaelEischer! I'll update the PR with the recommended changes. 👍

[thndrbrrr]

@MichaelEischer Alrighty, made the changes. This resulted in minor refactoring: had to move (and slightly change) a helper function from cmd_snapshots_test.go into testing.go.

[thndrbrrr]

Actually, I'll update this PR further with changes to func (e ExpirePolicy) String() (s string) to make command line output clearer. Right now an option like --keep-daily -1 prints Applying Policy: keep ... which misses an important point ... 😉

[thndrbrrr]

Ok, updated PR. Options such as --keep-daily 7 --keep-yearly -1 --keep-monthly -1 --keep-hourly 24 for example will now print:

Applying Policy: keep 24 hourly, 7 daily, all monthly, all yearly snapshots

[thndrbrrr]

@MichaelEischer Any feedback on the updated PR? Happy to update the docs if that's all that's misisng at this point.

[MichaelEischer]

@thndrbrrr Sorry for the late reply, I've been far too busy during the last few weeks. Thanks for the changes, the code now looks good to me. The only two remaining todos are a short changelog entry (see template ) and a similar note in the documentation.

[thndrbrrr]

@MichaelEischer Docs and changelog updated.

I updated the usage text as well, let me know if you're not happy with the wording:

$ ./restic forget -h
[...]
Usage:
  restic forget [flags] [snapshot ID] [...]

Flags:
  -l, --keep-last n                    keep the last n snapshots (use "-1" for n to keep all snapshots)
  -H, --keep-hourly n                  keep the last n hourly snapshots (use "-1" for n to keep all hourly snapshots)
  -d, --keep-daily n                   keep the last n daily snapshots (use "-1" for n to keep all daily snapshots)
  -w, --keep-weekly n                  keep the last n weekly snapshots (use "-1" for n to keep all weekly snapshots)
  -m, --keep-monthly n                 keep the last n monthly snapshots (use "-1" for n to keep all monthly snapshots)
  -y, --keep-yearly n                  keep the last n yearly snapshots (use "-1" for n to keep all yearly snapshots)

[thndrbrrr]

@thndrbrrr Sorry for the late reply, I've been far too busy during the last few weeks.

No worries, I know how that feels ... 😆

[MichaelEischer]

I only have one small remaining nitpick. Besides that LGTM.