From f92ecf13c96f614f96ebd99960cbe26df8536dc6 Mon Sep 17 00:00:00 2001 From: greatroar <61184462+greatroar@users.noreply.github.com> Date: Mon, 13 Jun 2022 20:35:37 +0200 Subject: [PATCH] all: Move away from pkg/errors, easy cases github.com/pkg/errors is no longer getting updates, because Go 1.13 went with the more flexible errors.{As,Is} function. Use those instead: errors from pkg/errors already support the Unwrap interface used by 1.13 error handling. Also: * check for io.EOF with a straight ==. That value should not be wrapped, and the chunker (whose error is checked in the cases changed) does not wrap it. * Give custom Error methods pointer receivers, so there's no ambiguity when type-switching since the value type will no longer implement error. * Make restic.ErrAlreadyLocked private, and rename it to alreadyLockedError to match the stdlib convention that error type names end in Error. * Same with rest.ErrIsNotExist => rest.notExistError. * Make s3.Backend.IsAccessDenied a private function. --- cmd/restic/cmd_backup.go | 2 +- cmd/restic/cmd_mount.go | 2 +- cmd/restic/global.go | 4 ++-- cmd/restic/main.go | 2 +- internal/archiver/archiver_test.go | 4 ++-- internal/archiver/file_saver.go | 2 +- internal/backend/layout.go | 2 +- internal/backend/mem/mem_backend.go | 2 +- internal/backend/rclone/backend_test.go | 3 ++- internal/backend/rclone/internal_test.go | 3 ++- internal/backend/rest/rest.go | 17 ++++++++-------- internal/backend/s3/s3.go | 24 ++++++++-------------- internal/backend/sftp/sftp.go | 3 +-- internal/backend/sftp/sftp_test.go | 2 +- internal/backend/swift/swift.go | 7 ++----- internal/backend/test/tests.go | 10 ++++----- internal/checker/checker.go | 15 ++++++-------- internal/checker/checker_test.go | 4 ++-- internal/errors/errors.go | 2 ++ internal/repository/key.go | 2 +- internal/restic/lock.go | 26 +++++++++++------------- internal/restic/testing.go | 4 +--- internal/test/helpers.go | 4 ++-- 23 files changed, 66 insertions(+), 80 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index bf1e17a47..24ee1e19f 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -144,7 +144,7 @@ func init() { func filterExisting(items []string) (result []string, err error) { for _, item := range items { _, err := fs.Lstat(item) - if err != nil && os.IsNotExist(errors.Cause(err)) { + if errors.Is(err, os.ErrNotExist) { Warnf("%v does not exist, skipping\n", item) continue } diff --git a/cmd/restic/cmd_mount.go b/cmd/restic/cmd_mount.go index fbe1cc14e..905cee1d3 100644 --- a/cmd/restic/cmd_mount.go +++ b/cmd/restic/cmd_mount.go @@ -116,7 +116,7 @@ func runMount(opts MountOptions, gopts GlobalOptions, args []string) error { mountpoint := args[0] - if _, err := resticfs.Stat(mountpoint); os.IsNotExist(errors.Cause(err)) { + if _, err := resticfs.Stat(mountpoint); errors.Is(err, os.ErrNotExist) { Verbosef("Mountpoint %s doesn't exist\n", mountpoint) return err } diff --git a/cmd/restic/global.go b/cmd/restic/global.go index 65dbbb6be..87ac854ce 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -297,7 +297,7 @@ func resolvePassword(opts GlobalOptions, envStr string) (string, error) { } if opts.PasswordFile != "" { s, err := textfile.Read(opts.PasswordFile) - if os.IsNotExist(errors.Cause(err)) { + if errors.Is(err, os.ErrNotExist) { return "", errors.Fatalf("%s does not exist", opts.PasswordFile) } return strings.TrimSpace(string(s)), errors.Wrap(err, "Readfile") @@ -398,7 +398,7 @@ func ReadRepo(opts GlobalOptions) (string, error) { } s, err := textfile.Read(opts.RepositoryFile) - if os.IsNotExist(errors.Cause(err)) { + if errors.Is(err, os.ErrNotExist) { return "", errors.Fatalf("%s does not exist", opts.RepositoryFile) } if err != nil { diff --git a/cmd/restic/main.go b/cmd/restic/main.go index 2f962b966..ad3ef89d4 100644 --- a/cmd/restic/main.go +++ b/cmd/restic/main.go @@ -98,7 +98,7 @@ func main() { err := cmdRoot.Execute() switch { - case restic.IsAlreadyLocked(errors.Cause(err)): + case restic.IsAlreadyLocked(err): fmt.Fprintf(os.Stderr, "%v\nthe `unlock` command can be used to remove stale locks\n", err) case err == ErrInvalidSourceData: fmt.Fprintf(os.Stderr, "Warning: %v\n", err) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 0d6295c39..b14fc573d 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -2040,8 +2040,8 @@ func TestArchiverAbortEarlyOnError(t *testing.T) { }) _, _, err := arch.Snapshot(ctx, []string{"."}, SnapshotOptions{Time: time.Now()}) - if errors.Cause(err) != test.err { - t.Errorf("expected error (%v) not found, got %v", test.err, errors.Cause(err)) + if !errors.Is(err, test.err) { + t.Errorf("expected error (%v) not found, got %v", test.err, err) } t.Logf("Snapshot return error: %v", err) diff --git a/internal/archiver/file_saver.go b/internal/archiver/file_saver.go index 2c43aefa1..65f54e523 100644 --- a/internal/archiver/file_saver.go +++ b/internal/archiver/file_saver.go @@ -165,7 +165,7 @@ func (s *FileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat for { buf := s.saveFilePool.Get() chunk, err := chnker.Next(buf.Data) - if errors.Cause(err) == io.EOF { + if err == io.EOF { buf.Release() break } diff --git a/internal/backend/layout.go b/internal/backend/layout.go index e916df705..ebd54e4af 100644 --- a/internal/backend/layout.go +++ b/internal/backend/layout.go @@ -152,7 +152,7 @@ func ParseLayout(ctx context.Context, repo Filesystem, layout, defaultLayout, pa l, err = DetectLayout(ctx, repo, path) // use the default layout if auto detection failed - if errors.Cause(err) == ErrLayoutDetectionFailed && defaultLayout != "" { + if errors.Is(err, ErrLayoutDetectionFailed) && defaultLayout != "" { debug.Log("error: %v, use default layout %v", err, defaultLayout) return ParseLayout(ctx, repo, defaultLayout, "", path) } diff --git a/internal/backend/mem/mem_backend.go b/internal/backend/mem/mem_backend.go index b14149d52..a9b03209d 100644 --- a/internal/backend/mem/mem_backend.go +++ b/internal/backend/mem/mem_backend.go @@ -71,7 +71,7 @@ func (be *MemoryBackend) Test(ctx context.Context, h restic.Handle) (bool, error // IsNotExist returns true if the file does not exist. func (be *MemoryBackend) IsNotExist(err error) bool { - return errors.Cause(err) == errNotFound + return errors.Is(err, errNotFound) } // Save adds new Data to the backend. diff --git a/internal/backend/rclone/backend_test.go b/internal/backend/rclone/backend_test.go index 0a8f91aea..9708c6af2 100644 --- a/internal/backend/rclone/backend_test.go +++ b/internal/backend/rclone/backend_test.go @@ -29,7 +29,8 @@ func newTestSuite(t testing.TB) *test.Suite { t.Logf("Create()") cfg := config.(rclone.Config) be, err := rclone.Create(context.TODO(), cfg) - if e, ok := errors.Cause(err).(*exec.Error); ok && e.Err == exec.ErrNotFound { + var e *exec.Error + if errors.As(err, &e) && e.Err == exec.ErrNotFound { t.Skipf("program %q not found", e.Name) return nil, nil } diff --git a/internal/backend/rclone/internal_test.go b/internal/backend/rclone/internal_test.go index 8bc661ab7..fe9a63d30 100644 --- a/internal/backend/rclone/internal_test.go +++ b/internal/backend/rclone/internal_test.go @@ -18,7 +18,8 @@ func TestRcloneExit(t *testing.T) { cfg := NewConfig() cfg.Remote = dir be, err := Open(cfg, nil) - if e, ok := errors.Cause(err).(*exec.Error); ok && e.Err == exec.ErrNotFound { + var e *exec.Error + if errors.As(err, &e) && e.Err == exec.ErrNotFound { t.Skipf("program %q not found", e.Name) return } diff --git a/internal/backend/rest/rest.go b/internal/backend/rest/rest.go index b9824bb53..dfb701f9d 100644 --- a/internal/backend/rest/rest.go +++ b/internal/backend/rest/rest.go @@ -169,21 +169,20 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea return errors.Wrap(cerr, "Close") } -// ErrIsNotExist is returned whenever the requested file does not exist on the +// notExistError is returned whenever the requested file does not exist on the // server. -type ErrIsNotExist struct { +type notExistError struct { restic.Handle } -func (e ErrIsNotExist) Error() string { +func (e *notExistError) Error() string { return fmt.Sprintf("%v does not exist", e.Handle) } // IsNotExist returns true if the error was caused by a non-existing file. func (b *Backend) IsNotExist(err error) bool { - err = errors.Cause(err) - _, ok := err.(ErrIsNotExist) - return ok + var e *notExistError + return errors.As(err, &e) } // Load runs fn with a reader that yields the contents of the file at h at the @@ -296,7 +295,7 @@ func (b *Backend) openReader(ctx context.Context, h restic.Handle, length int, o if resp.StatusCode == http.StatusNotFound { _ = resp.Body.Close() - return nil, ErrIsNotExist{h} + return nil, ¬ExistError{h} } if resp.StatusCode != 200 && resp.StatusCode != 206 { @@ -341,7 +340,7 @@ func (b *Backend) Stat(ctx context.Context, h restic.Handle) (restic.FileInfo, e if resp.StatusCode == http.StatusNotFound { _ = resp.Body.Close() - return restic.FileInfo{}, ErrIsNotExist{h} + return restic.FileInfo{}, ¬ExistError{h} } if resp.StatusCode != 200 { @@ -392,7 +391,7 @@ func (b *Backend) Remove(ctx context.Context, h restic.Handle) error { if resp.StatusCode == http.StatusNotFound { _ = resp.Body.Close() - return ErrIsNotExist{h} + return ¬ExistError{h} } if resp.StatusCode != 200 { diff --git a/internal/backend/s3/s3.go b/internal/backend/s3/s3.go index ac1a1d5ce..bcee64349 100644 --- a/internal/backend/s3/s3.go +++ b/internal/backend/s3/s3.go @@ -137,7 +137,7 @@ func Create(ctx context.Context, cfg Config, rt http.RoundTripper) (restic.Backe } found, err := be.client.BucketExists(ctx, cfg.Bucket) - if err != nil && be.IsAccessDenied(err) { + if err != nil && isAccessDenied(err) { err = nil found = true } @@ -158,29 +158,23 @@ func Create(ctx context.Context, cfg Config, rt http.RoundTripper) (restic.Backe return be, nil } -// IsAccessDenied returns true if the error is caused by Access Denied. -func (be *Backend) IsAccessDenied(err error) bool { - debug.Log("IsAccessDenied(%T, %#v)", err, err) +// isAccessDenied returns true if the error is caused by Access Denied. +func isAccessDenied(err error) bool { + debug.Log("isAccessDenied(%T, %#v)", err, err) - if e, ok := errors.Cause(err).(minio.ErrorResponse); ok && e.Code == "AccessDenied" { - return true - } - - return false + var e minio.ErrorResponse + return errors.As(err, &e) && e.Code == "Access Denied" } // IsNotExist returns true if the error is caused by a not existing file. func (be *Backend) IsNotExist(err error) bool { debug.Log("IsNotExist(%T, %#v)", err, err) - if os.IsNotExist(errors.Cause(err)) { + if errors.Is(err, os.ErrNotExist) { return true } - if e, ok := errors.Cause(err).(minio.ErrorResponse); ok && e.Code == "NoSuchKey" { - return true - } - - return false + var e minio.ErrorResponse + return errors.As(err, &e) && e.Code == "NoSuchKey" } // Join combines path components with slashes. diff --git a/internal/backend/sftp/sftp.go b/internal/backend/sftp/sftp.go index 99eb2c09a..8d5ce6de6 100644 --- a/internal/backend/sftp/sftp.go +++ b/internal/backend/sftp/sftp.go @@ -183,7 +183,6 @@ func (r *SFTP) ReadDir(ctx context.Context, dir string) ([]os.FileInfo, error) { // IsNotExist returns true if the error is caused by a not existing file. func (r *SFTP) IsNotExist(err error) bool { - err = errors.Cause(err) return errors.Is(err, os.ErrNotExist) } @@ -496,7 +495,7 @@ func (r *SFTP) Test(ctx context.Context, h restic.Handle) (bool, error) { defer r.sem.ReleaseToken() _, err := r.c.Lstat(r.Filename(h)) - if os.IsNotExist(errors.Cause(err)) { + if errors.Is(err, os.ErrNotExist) { return false, nil } diff --git a/internal/backend/sftp/sftp_test.go b/internal/backend/sftp/sftp_test.go index f0573dcb5..e682b343a 100644 --- a/internal/backend/sftp/sftp_test.go +++ b/internal/backend/sftp/sftp_test.go @@ -20,7 +20,7 @@ func findSFTPServerBinary() string { for _, dir := range strings.Split(rtest.TestSFTPPath, ":") { testpath := filepath.Join(dir, "sftp-server") _, err := os.Stat(testpath) - if !os.IsNotExist(errors.Cause(err)) { + if !errors.Is(err, os.ErrNotExist) { return testpath } } diff --git a/internal/backend/swift/swift.go b/internal/backend/swift/swift.go index b127cb832..0bf03a6b8 100644 --- a/internal/backend/swift/swift.go +++ b/internal/backend/swift/swift.go @@ -311,11 +311,8 @@ func (be *beSwift) removeKeys(ctx context.Context, t restic.FileType) error { // IsNotExist returns true if the error is caused by a not existing file. func (be *beSwift) IsNotExist(err error) bool { - if e, ok := errors.Cause(err).(*swift.Error); ok { - return e.StatusCode == http.StatusNotFound - } - - return false + var e *swift.Error + return errors.As(err, &e) && e.StatusCode == http.StatusNotFound } // Delete removes all restic objects in the container. diff --git a/internal/backend/test/tests.go b/internal/backend/test/tests.go index bc89f737d..f05366f6d 100644 --- a/internal/backend/test/tests.go +++ b/internal/backend/test/tests.go @@ -361,8 +361,8 @@ func (s *Suite) TestListCancel(t *testing.T) { return nil }) - if errors.Cause(err) != context.Canceled { - t.Fatalf("expected error not found, want %v, got %v", context.Canceled, errors.Cause(err)) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected error not found, want %v, got %v", context.Canceled, err) } }) @@ -380,7 +380,7 @@ func (s *Suite) TestListCancel(t *testing.T) { return nil }) - if errors.Cause(err) != context.Canceled { + if !errors.Is(err, context.Canceled) { t.Fatalf("expected error not found, want %v, got %v", context.Canceled, err) } @@ -403,7 +403,7 @@ func (s *Suite) TestListCancel(t *testing.T) { return nil }) - if errors.Cause(err) != context.Canceled { + if !errors.Is(err, context.Canceled) { t.Fatalf("expected error not found, want %v, got %v", context.Canceled, err) } @@ -429,7 +429,7 @@ func (s *Suite) TestListCancel(t *testing.T) { return nil }) - if errors.Cause(err) != context.DeadlineExceeded { + if !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("expected error not found, want %#v, got %#v", context.DeadlineExceeded, err) } diff --git a/internal/checker/checker.go b/internal/checker/checker.go index 2ecd1469c..a31235fae 100644 --- a/internal/checker/checker.go +++ b/internal/checker/checker.go @@ -160,18 +160,15 @@ type PackError struct { Err error } -func (e PackError) Error() string { +func (e *PackError) Error() string { return "pack " + e.ID.Str() + ": " + e.Err.Error() } // IsOrphanedPack returns true if the error describes a pack which is not // contained in any index. func IsOrphanedPack(err error) bool { - if e, ok := errors.Cause(err).(PackError); ok && e.Orphaned { - return true - } - - return false + var e *PackError + return errors.As(err, &e) && e.Orphaned } // Packs checks that all packs referenced in the index are still available and @@ -204,7 +201,7 @@ func (c *Checker) Packs(ctx context.Context, errChan chan<- error) { select { case <-ctx.Done(): return - case errChan <- PackError{ID: id, Err: errors.New("does not exist")}: + case errChan <- &PackError{ID: id, Err: errors.New("does not exist")}: } continue } @@ -214,7 +211,7 @@ func (c *Checker) Packs(ctx context.Context, errChan chan<- error) { select { case <-ctx.Done(): return - case errChan <- PackError{ID: id, Err: errors.Errorf("unexpected file size: got %d, expected %d", reposize, size)}: + case errChan <- &PackError{ID: id, Err: errors.Errorf("unexpected file size: got %d, expected %d", reposize, size)}: } } } @@ -224,7 +221,7 @@ func (c *Checker) Packs(ctx context.Context, errChan chan<- error) { select { case <-ctx.Done(): return - case errChan <- PackError{ID: orphanID, Orphaned: true, Err: errors.New("not referenced in any index")}: + case errChan <- &PackError{ID: orphanID, Orphaned: true, Err: errors.New("not referenced in any index")}: } } } diff --git a/internal/checker/checker_test.go b/internal/checker/checker_test.go index f2ee0c732..86fbf0582 100644 --- a/internal/checker/checker_test.go +++ b/internal/checker/checker_test.go @@ -109,7 +109,7 @@ func TestMissingPack(t *testing.T) { test.Assert(t, len(errs) == 1, "expected exactly one error, got %v", len(errs)) - if err, ok := errs[0].(checker.PackError); ok { + if err, ok := errs[0].(*checker.PackError); ok { test.Equals(t, packHandle.Name, err.ID.String()) } else { t.Errorf("expected error returned by checker.Packs() to be PackError, got %v", err) @@ -145,7 +145,7 @@ func TestUnreferencedPack(t *testing.T) { test.Assert(t, len(errs) == 1, "expected exactly one error, got %v", len(errs)) - if err, ok := errs[0].(checker.PackError); ok { + if err, ok := errs[0].(*checker.PackError); ok { test.Equals(t, packID, err.ID.String()) } else { t.Errorf("expected error returned by checker.Packs() to be PackError, got %v", err) diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 09ca656b3..021da72c5 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -52,4 +52,6 @@ func Cause(err error) error { // Go 1.13-style error handling. +func As(err error, tgt interface{}) bool { return errors.As(err, tgt) } + func Is(x, y error) bool { return errors.Is(x, y) } diff --git a/internal/repository/key.go b/internal/repository/key.go index 8c62d7e36..43e0a1989 100644 --- a/internal/repository/key.go +++ b/internal/repository/key.go @@ -153,7 +153,7 @@ func SearchKey(ctx context.Context, s *Repository, password string, maxKeys int, debug.Log("key %v returned error %v", fi.Name, err) // ErrUnauthenticated means the password is wrong, try the next key - if errors.Cause(err) == crypto.ErrUnauthenticated { + if errors.Is(err, crypto.ErrUnauthenticated) { return nil } diff --git a/internal/restic/lock.go b/internal/restic/lock.go index 9a5fd841d..27daa34a6 100644 --- a/internal/restic/lock.go +++ b/internal/restic/lock.go @@ -38,13 +38,13 @@ type Lock struct { lockID *ID } -// ErrAlreadyLocked is returned when NewLock or NewExclusiveLock are unable to +// alreadyLockedError is returned when NewLock or NewExclusiveLock are unable to // acquire the desired lock. -type ErrAlreadyLocked struct { +type alreadyLockedError struct { otherLock *Lock } -func (e ErrAlreadyLocked) Error() string { +func (e *alreadyLockedError) Error() string { s := "" if e.otherLock.Exclusive { s = "exclusively " @@ -52,25 +52,23 @@ func (e ErrAlreadyLocked) Error() string { return fmt.Sprintf("repository is already locked %sby %v", s, e.otherLock) } -// IsAlreadyLocked returns true iff err is an instance of ErrAlreadyLocked. +// IsAlreadyLocked returns true iff err indicates that a repository is +// already locked. func IsAlreadyLocked(err error) bool { - if _, ok := errors.Cause(err).(ErrAlreadyLocked); ok { - return true - } - - return false + var e *alreadyLockedError + return errors.As(err, &e) } // NewLock returns a new, non-exclusive lock for the repository. If an -// exclusive lock is already held by another process, ErrAlreadyLocked is -// returned. +// exclusive lock is already held by another process, it returns an error +// that satisfies IsAlreadyLocked. func NewLock(ctx context.Context, repo Repository) (*Lock, error) { return newLock(ctx, repo, false) } // NewExclusiveLock returns a new, exclusive lock for the repository. If // another lock (normal and exclusive) is already held by another process, -// ErrAlreadyLocked is returned. +// it returns an error that satisfies IsAlreadyLocked. func NewExclusiveLock(ctx context.Context, repo Repository) (*Lock, error) { return newLock(ctx, repo, true) } @@ -147,11 +145,11 @@ func (l *Lock) checkForOtherLocks(ctx context.Context) error { } if l.Exclusive { - return ErrAlreadyLocked{otherLock: lock} + return &alreadyLockedError{otherLock: lock} } if !l.Exclusive && lock.Exclusive { - return ErrAlreadyLocked{otherLock: lock} + return &alreadyLockedError{otherLock: lock} } return nil diff --git a/internal/restic/testing.go b/internal/restic/testing.go index f6965070b..392538506 100644 --- a/internal/restic/testing.go +++ b/internal/restic/testing.go @@ -9,8 +9,6 @@ import ( "testing" "time" - "github.com/restic/restic/internal/errors" - "github.com/restic/chunker" ) @@ -44,7 +42,7 @@ func (fs *fakeFileSystem) saveFile(ctx context.Context, rd io.Reader) (blobs IDs blobs = IDs{} for { chunk, err := fs.chunker.Next(fs.buf) - if errors.Cause(err) == io.EOF { + if err == io.EOF { break } diff --git a/internal/test/helpers.go b/internal/test/helpers.go index 1e127f786..9ace2df5e 100644 --- a/internal/test/helpers.go +++ b/internal/test/helpers.go @@ -175,7 +175,7 @@ func ResetReadOnly(t testing.TB, dir string) { return nil }) - if os.IsNotExist(errors.Cause(err)) { + if errors.Is(err, os.ErrNotExist) { err = nil } OK(t, err) @@ -186,7 +186,7 @@ func ResetReadOnly(t testing.TB, dir string) { func RemoveAll(t testing.TB, path string) { ResetReadOnly(t, path) err := os.RemoveAll(path) - if os.IsNotExist(errors.Cause(err)) { + if errors.Is(err, os.ErrNotExist) { err = nil } OK(t, err)