Repository: restic/restic (go)

Issue:
Expose additional compression levels
Output of `restic version`
--------------------------
restic 0.16.4 compiled with go1.21.6 on linux/amd64


What should restic do differently? Which functionality do you think we should add?
----------------------------------------------------------------------------------
It would be great if restic exposed the [additional compression levels](https://pkg.go.dev/github.com/klauspost/compress/zstd#EncoderLevel) offered by the [klauspost/compress](https://github.com/klauspost/compress) library.  


What are you trying to do? What problem would this solve?
---------------------------------------------------------
The current 'auto' and 'max' levels map to the `SpeedDefault` and `SpeedBestCompression` levels, respectively, in the klauspost/compress library, and while these are great to have, the 'max' setting occupies a fairly extreme point on the compression:speed tradeoff curve.  It's useful for remote repos when upstream bandwidth is limited (<10Mbps), but can significantly slow down transfers over faster connections.  The klauspost/compress library offers an intermediate mode (`SpeedBetterCompression`) that might be more useful to people who want a bit more compression than `SpeedDefault` but who don't want to go all the way to the orders-of-magnitude-slower `SpeedBestCompression`.   Additionally, `SpeedFastest` might be a desirable configuration for people who are doing local disk backups and care most about speed but still want some compression.

I've benchmarked the various zstd compression levels (1-22) on my (dinky) [Celeron N5095](https://www.cpu-monkey.com/en/cpu-intel_celeron_n5095) and noticed:
* Level 1 (`SpeedFastest`) runs at around 500-2000 MB/s.
* Level 3 (`SpeedDefault` / 'auto') runs at around 300-1500 MB/s and achieves approximately the same compression ratios as Level 1.
* Level 7-8 (`SpeedBetterCompression`) runs at around 50-100 MB/s and achieves 10-30% better compression ratios than Level 1.
* Level 19 (I think this is equivalent to `SpeedBestCompression` / 'max') runs at around 0.8-3 MB/s and achieves 20-70% better compression ratios on compressible files than Level 1. 

These are all pretty significant differences, especially with respect to speed, and since people use restic with a variety of backends on a variety of hardware, it seems like more control over the compression levels would be useful. 

I tested the two extra modes ("better" and "fastest") inside restic by adding the appropriate enums and switch cases in internal/repository/repository.go and they seem to produce backup speeds and repo sizes proportional to what would be expected given my zstd benchmarks above. 

Would exposing these additional compression levels be useful to the restic community?  Does doing so jive with the general philosophy of the project?  I'm sure this was considered at some point when compression was initially added, but I had trouble finding relevant discussions.

This is the first time I've touched golang or restic code.  If there's interest, I'm happy to take a stab at a proper PR, but I wanted to get a sense for whether this was worth pursuing first.  I imagine this is something an experienced restic contributor could knock out in 20 minutes (which is why I think there might be philosophical issues with the proposed changes or something else I'm not considering). 


Did restic help you today? Did it make you happy in any way?
------------------------------------------------------------
I'm pretty new to restic, but I'm currently pretty pumped about it and I especially like using it with resticprofile to automate and organize all of my backups.  It also did my taxes for me today and I even saw it help an old lady cross the street. 


Pull request:
Add support for additional compression levels `fastest` and `better`
What does this PR change? What problem does it solve?
-----------------------------------------------------
Added support for `fastest` and `better` compression levels exposed by klaupost/compress library.

Was the change previously discussed in an issue or on the forum?
----------------------------------------------------------------
Closes #4728 

Checklist
---------

- [ ] I have added tests for all code changes.
- [x] 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'm done! This pull request is ready for review.


Patch:
diff --git a/changelog/unreleased/issue-4728 b/changelog/unreleased/issue-4728
new file mode 100644
index 000000000..e0149e675
--- /dev/null
+++ b/changelog/unreleased/issue-4728
@@ -0,0 +1,7 @@
+Enhancement: Added support for zstd compression levels `fastest` and `better`
+
+Restic now supports the zstd compression modes `fastest` and `better`. Set the
+environment variable `RESTIC_COMPRESSION` to `fastest` or `better` to use these
+compression levels. This can also be set with the `--compression` flag.
+
+https://github.com/restic/restic/issues/4728
diff --git a/changelog/unreleased/issue-5233 b/changelog/unreleased/issue-5233
new file mode 100644
index 000000000..14ff2b249
--- /dev/null
+++ b/changelog/unreleased/issue-5233
@@ -0,0 +1,8 @@
+Bugfix: forget command returns exit code 3 on partial removal of snapshots
+
+The `forget` command now returns exit code 3 when it fails to remove one or
+more snapshots. Previously, it returned exit code 0, which could lead to
+confusion if the command was used in a script.
+
+https://github.com/restic/restic/issues/5233
+https://github.com/restic/restic/pull/5322
diff --git a/changelog/unreleased/issue-5325 b/changelog/unreleased/issue-5325
new file mode 100644
index 000000000..883391a0a
--- /dev/null
+++ b/changelog/unreleased/issue-5325
@@ -0,0 +1,7 @@
+Bugfix: Correctly handle `RESTIC_HOST` in `forget` command
+
+The `forget` command did not use the host name from the `RESTIC_HOST`
+environment variable. This has been fixed.
+
+https://github.com/restic/restic/issues/5325
+https://github.com/restic/restic/pull/5327
diff --git a/cmd/restic/cmd_forget.go b/cmd/restic/cmd_forget.go
index 2bfacc4de..bef00ffa1 100644
--- a/cmd/restic/cmd_forget.go
+++ b/cmd/restic/cmd_forget.go
@@ -41,6 +41,7 @@ EXIT STATUS
 
 Exit status is 0 if the command was successful.
 Exit status is 1 if there was any error.
+Exit status is 3 if there was an error removing one or more snapshots.
 Exit status is 10 if the repository does not exist.
 Exit status is 11 if the repository is already locked.
 Exit status is 12 if the password is incorrect.
@@ -62,6 +63,7 @@ Exit status is 12 if the password is incorrect.
 type ForgetPolicyCount int
 
 var ErrNegativePolicyCount = errors.New("negative values not allowed, use 'unlimited' instead")
+var ErrFailedToRemoveOneOrMoreSnapshots = errors.New("failed to remove one or more snapshots")
 
 func (c *ForgetPolicyCount) Set(s string) error {
 	switch s {
@@ -137,13 +139,14 @@ func (opts *ForgetOptions) AddFlags(f *pflag.FlagSet) {
 	f.Var(&opts.KeepTags, "keep-tag", "keep snapshots with this `taglist` (can be specified multiple times)")
 	f.BoolVar(&opts.UnsafeAllowRemoveAll, "unsafe-allow-remove-all", false, "allow deleting all snapshots of a snapshot group")
 
-	initMultiSnapshotFilter(f, &opts.SnapshotFilter, false)
 	f.StringArrayVar(&opts.Hosts, "hostname", nil, "only consider snapshots with the given `hostname` (can be specified multiple times)")
 	err := f.MarkDeprecated("hostname", "use --host")
 	if err != nil {
 		// MarkDeprecated only returns an error when the flag is not found
 		panic(err)
 	}
+	// must be defined after `--hostname` to not override the default value from the environment
+	initMultiSnapshotFilter(f, &opts.SnapshotFilter, false)
 
 	f.BoolVarP(&opts.Compact, "compact", "c", false, "use compact output format")
 	opts.GroupBy = restic.SnapshotGroupByOptions{Host: true, Path: true}
@@ -304,12 +307,15 @@ func runForget(ctx context.Context, opts ForgetOptions, pruneOptions PruneOption
 		return ctx.Err()
 	}
 
+	// these are the snapshots that failed to be removed
+	failedSnIDs := restic.NewIDSet()
 	if len(removeSnIDs) > 0 {
 		if !opts.DryRun {
 			bar := printer.NewCounter("files deleted")
 			err := restic.ParallelRemove(ctx, repo, removeSnIDs, restic.WriteableSnapshotFile, func(id restic.ID, err error) error {
 				if err != nil {
 					printer.E("unable to remove %v/%v from the repository\n", restic.SnapshotFile, id)
+					failedSnIDs.Insert(id)
 				} else {
 					printer.VV("removed %v/%v\n", restic.SnapshotFile, id)
 				}
@@ -331,6 +337,10 @@ func runForget(ctx context.Context, opts ForgetOptions, pruneOptions PruneOption
 		}
 	}
 
+	if len(failedSnIDs) > 0 {
+		return ErrFailedToRemoveOneOrMoreSnapshots
+	}
+
 	if len(removeSnIDs) > 0 && opts.Prune {
 		if opts.DryRun {
 			printer.P("%d snapshots would be removed, running prune dry run\n", len(removeSnIDs))
diff --git a/cmd/restic/global.go b/cmd/restic/global.go
index 7dafa6f96..6e58a0d73 100644
--- a/cmd/restic/global.go
+++ b/cmd/restic/global.go
@@ -114,7 +114,7 @@ func (opts *GlobalOptions) AddFlags(f *pflag.FlagSet) {
 	f.BoolVar(&opts.InsecureNoPassword, "insecure-no-password", false, "use an empty password for the repository, must be passed to every restic command (insecure)")
 	f.BoolVar(&opts.InsecureTLS, "insecure-tls", false, "skip TLS certificate verification when connecting to the repository (insecure)")
 	f.BoolVar(&opts.CleanupCache, "cleanup-cache", false, "auto remove old cache directories")
-	f.Var(&opts.Compression, "compression", "compression mode (only available for repository format version 2), one of (auto|off|max) (default: $RESTIC_COMPRESSION)")
+	f.Var(&opts.Compression, "compression", "compression mode (only available for repository format version 2), one of (auto|off|fastest|better|max) (default: $RESTIC_COMPRESSION)")
 	f.BoolVar(&opts.NoExtraVerify, "no-extra-verify", false, "skip additional verification of data before upload (see documentation)")
 	f.IntVar(&opts.Limits.UploadKb, "limit-upload", 0, "limits uploads to a maximum `rate` in KiB/s. (default: unlimited)")
 	f.IntVar(&opts.Limits.DownloadKb, "limit-download", 0, "limits downloads to a maximum `rate` in KiB/s. (default: unlimited)")
diff --git a/cmd/restic/main.go b/cmd/restic/main.go
index b04b3b9ab..f4b17aa92 100644
--- a/cmd/restic/main.go
+++ b/cmd/restic/main.go
@@ -206,6 +206,8 @@ func main() {
 		exitCode = 0
 	case err == ErrInvalidSourceData:
 		exitCode = 3
+	case errors.Is(err, ErrFailedToRemoveOneOrMoreSnapshots):
+		exitCode = 3
 	case errors.Is(err, ErrNoRepository):
 		exitCode = 10
 	case restic.IsAlreadyLocked(err):
diff --git a/doc/030_preparing_a_new_repo.rst b/doc/030_preparing_a_new_repo.rst
index 34a0cd8bc..ac9657460 100644
--- a/doc/030_preparing_a_new_repo.rst
+++ b/doc/030_preparing_a_new_repo.rst
@@ -854,6 +854,6 @@ with an empty password, use the following command.
     restic init --insecure-no-password
 
 
-The ``init`` and ``copy`` command also support the option ``--from-insecure-no-password``
+The ``init`` and ``copy`` commands also support the option ``--from-insecure-no-password``
 which applies to the source repository. The ``key add`` and ``key passwd`` commands
-include the ``--new-insecure-no-password`` option to add or set and empty password.
+include the ``--new-insecure-no-password`` option to add or set an empty password.
diff --git a/doc/faq.rst b/doc/faq.rst
index 3b62f641d..0df4aa0ea 100644
--- a/doc/faq.rst
+++ b/doc/faq.rst
@@ -253,7 +253,7 @@ Archive** storage classes is available:
 .. code-block:: console
 
    $ restic backup -o s3.storage-class=GLACIER somedir/
-   $ RESTIC_FEATURES=s3-restore restic restore -o s3.enable-restore=1 -o s3.restore-days=7 -o s3.restore-timeout=1d latest
+   $ RESTIC_FEATURES=s3-restore restic restore -o s3.enable-restore=1 -o s3.restore-days=7 -o s3.restore-timeout=24h latest
 
 **Notes:**
 
diff --git a/internal/backend/s3/config.go b/internal/backend/s3/config.go
index 77f27408e..365b16bf1 100644
--- a/internal/backend/s3/config.go
+++ b/internal/backend/s3/config.go
@@ -26,7 +26,7 @@ type Config struct {
 
 	EnableRestore  bool          `option:"enable-restore" help:"restore objects from GLACIER or DEEP_ARCHIVE storage classes (default: false, requires \"s3-restore\" feature flag)"`
 	RestoreDays    int           `option:"restore-days" help:"lifetime in days of restored object (default: 7)"`
-	RestoreTimeout time.Duration `option:"restore-timeout" help:"maximum time to wait for objects transition (default: 1d)"`
+	RestoreTimeout time.Duration `option:"restore-timeout" help:"maximum time to wait for objects transition (default: 24h)"`
 	RestoreTier    string        `option:"restore-tier" help:"Retrieval tier at which the restore will be processed. (Standard, Bulk or Expedited) (default: Standard)"`
 
 	Connections         uint   `option:"connections" help:"set a limit for the number of concurrent connections (default: 5)"`
diff --git a/internal/repository/repository.go b/internal/repository/repository.go
index 066ba5ed5..fb718e9f9 100644
--- a/internal/repository/repository.go
+++ b/internal/repository/repository.go
@@ -73,7 +73,9 @@ const (
 	CompressionAuto    CompressionMode = 0
 	CompressionOff     CompressionMode = 1
 	CompressionMax     CompressionMode = 2
-	CompressionInvalid CompressionMode = 3
+	CompressionFastest CompressionMode = 3
+	CompressionBetter  CompressionMode = 4
+	CompressionInvalid CompressionMode = 5
 )
 
 // Set implements the method needed for pflag command flag parsing.
@@ -85,9 +87,13 @@ func (c *CompressionMode) Set(s string) error {
 		*c = CompressionOff
 	case "max":
 		*c = CompressionMax
+	case "fastest":
+		*c = CompressionFastest
+	case "better":
+		*c = CompressionBetter
 	default:
 		*c = CompressionInvalid
-		return fmt.Errorf("invalid compression mode %q, must be one of (auto|off|max)", s)
+		return fmt.Errorf("invalid compression mode %q, must be one of (auto|off|fastest|better|max)", s)
 	}
 
 	return nil
@@ -101,6 +107,10 @@ func (c *CompressionMode) String() string {
 		return "off"
 	case CompressionMax:
 		return "max"
+	case CompressionFastest:
+		return "fastest"
+	case CompressionBetter:
+		return "better"
 	default:
 		return "invalid"
 	}
@@ -305,9 +315,17 @@ func (r *Repository) loadBlob(ctx context.Context, blobs []restic.PackedBlob, bu
 
 func (r *Repository) getZstdEncoder() *zstd.Encoder {
 	r.allocEnc.Do(func() {
-		level := zstd.SpeedDefault
-		if r.opts.Compression == CompressionMax {
+
+		var level zstd.EncoderLevel
+		switch r.opts.Compression {
+		case CompressionFastest:
+			level = zstd.SpeedFastest
+		case CompressionBetter:
+			level = zstd.SpeedBetterCompression
+		case CompressionMax:
 			level = zstd.SpeedBestCompression
+		default:
+			level = zstd.SpeedDefault
 		}
 
 		opts := []zstd.EOption{


Test patch:
diff --git a/cmd/restic/cmd_forget_test.go b/cmd/restic/cmd_forget_test.go
index ddeef028a..0d60f7445 100644
--- a/cmd/restic/cmd_forget_test.go
+++ b/cmd/restic/cmd_forget_test.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/restic/restic/internal/restic"
 	rtest "github.com/restic/restic/internal/test"
+	"github.com/spf13/pflag"
 )
 
 func TestForgetPolicyValues(t *testing.T) {
@@ -92,3 +93,10 @@ func TestForgetOptionValues(t *testing.T) {
 		}
 	}
 }
+
+func TestForgetHostnameDefaulting(t *testing.T) {
+	t.Setenv("RESTIC_HOST", "testhost")
+	opts := ForgetOptions{}
+	opts.AddFlags(pflag.NewFlagSet("test", pflag.ContinueOnError))
+	rtest.Equals(t, []string{"testhost"}, opts.Hosts)
+}