From 79c41966afc77b920e8493f457e3f85317943666 Mon Sep 17 00:00:00 2001 From: Srigovind Nayak <5201843+konidev20@users.noreply.github.com> Date: Sat, 19 Apr 2025 14:13:50 +0530 Subject: [PATCH 1/4] errors: enhance fatalError type to include underlying errors --- internal/errors/fatal.go | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/internal/errors/fatal.go b/internal/errors/fatal.go index 9370a68d7..12b310aca 100644 --- a/internal/errors/fatal.go +++ b/internal/errors/fatal.go @@ -7,25 +7,48 @@ import ( // fatalError is an error that should be printed to the user, then the program // should exit with an error code. -type fatalError string +type fatalError struct { + msg string + err error // Underlying error +} -func (e fatalError) Error() string { - return string(e) +func (e *fatalError) Error() string { + return e.msg +} + +func (e *fatalError) Unwrap() error { + return e.err } // IsFatal returns true if err is a fatal message that should be printed to the // user. Then, the program should exit. func IsFatal(err error) bool { - var fatal fatalError + var fatal *fatalError return errors.As(err, &fatal) } // Fatal returns an error that is marked fatal. func Fatal(s string) error { - return Wrap(fatalError(s), "Fatal") + return Wrap(&fatalError{msg: s}, "Fatal") } -// Fatalf returns an error that is marked fatal. +// Fatalf returns an error that is marked fatal, preserving an underlying error if passed. func Fatalf(s string, data ...interface{}) error { - return Wrap(fatalError(fmt.Sprintf(s, data...)), "Fatal") + // Use the last error found. + var underlyingErr error + for i := len(data) - 1; i >= 0; i-- { + if err, ok := data[i].(error); ok { + underlyingErr = err + break + } + } + + msg := fmt.Sprintf(s, data...) + + fatal := &fatalError{ + msg: msg, + err: underlyingErr, + } + + return Wrap(fatal, "Fatal") } From 18b8f8870f2e74f03d63f087bc62755ab6ab2e9c Mon Sep 17 00:00:00 2001 From: Srigovind Nayak <5201843+konidev20@users.noreply.github.com> Date: Sat, 19 Apr 2025 14:20:52 +0530 Subject: [PATCH 2/4] tests: add tests for preserving underlying errors --- internal/errors/fatal_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/errors/fatal_test.go b/internal/errors/fatal_test.go index 41da8dee7..da966bbaa 100644 --- a/internal/errors/fatal_test.go +++ b/internal/errors/fatal_test.go @@ -20,3 +20,23 @@ func TestFatal(t *testing.T) { } } } + +func TestFatalErrorWrapping(t *testing.T) { + underlying := errors.New("underlying error") + fatal := errors.Fatalf("fatal error: %v", underlying) + + // Test that the fatal error message is preserved + if fatal.Error() != "Fatal: fatal error: underlying error" { + t.Errorf("unexpected error message: %v", fatal.Error()) + } + + // Test that we can unwrap to get the underlying error + if !errors.Is(fatal, underlying) { + t.Error("fatal error should wrap the underlying error") + } + + // Test that the error is marked as fatal + if !errors.IsFatal(fatal) { + t.Error("error should be marked as fatal") + } +} From 576d35b37b1b42edc40f5ec9f48ad355cf61664b Mon Sep 17 00:00:00 2001 From: Srigovind Nayak <5201843+konidev20@users.noreply.github.com> Date: Sat, 26 Apr 2025 23:11:18 +0530 Subject: [PATCH 3/4] changelog: add bugfix changelog for issue-5258 --- changelog/unreleased/issue-5258 | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog/unreleased/issue-5258 diff --git a/changelog/unreleased/issue-5258 b/changelog/unreleased/issue-5258 new file mode 100644 index 000000000..1c8ffc75b --- /dev/null +++ b/changelog/unreleased/issue-5258 @@ -0,0 +1,7 @@ +Bugfix: Exit with correct code on SIGINT + +Restic previously returned exit code 1 on SIGINT, which is incorrect. +Restic now returns 130 on SIGINT. + +https://github.com/restic/restic/issues/5258 +https://github.com/restic/restic/pull/5363 \ No newline at end of file From ce089f7e2d45c6d468aea036e25e2cf10c3aa4e9 Mon Sep 17 00:00:00 2001 From: Srigovind Nayak <5201843+konidev20@users.noreply.github.com> Date: Tue, 3 Jun 2025 14:34:12 +0530 Subject: [PATCH 4/4] errors: standardize error wrapping for Fatal errors * replace all occurences of `errors.Fatal(err.Error())` with `errors.Fatalf("%s", err)` so that the error wrapping is correct across the codebase * updated the review comments --- cmd/restic/cmd_copy.go | 2 +- cmd/restic/cmd_diff.go | 2 +- cmd/restic/cmd_init.go | 2 +- cmd/restic/global.go | 4 ++-- internal/repository/prune.go | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/restic/cmd_copy.go b/cmd/restic/cmd_copy.go index 2ad5a464c..02bc027c8 100644 --- a/cmd/restic/cmd_copy.go +++ b/cmd/restic/cmd_copy.go @@ -250,7 +250,7 @@ func copyTree(ctx context.Context, srcRepo restic.Repository, dstRepo restic.Rep ) bar.Done() if err != nil { - return errors.Fatal(err.Error()) + return errors.Fatalf("%s", err) } return nil } diff --git a/cmd/restic/cmd_diff.go b/cmd/restic/cmd_diff.go index e065ba4b6..5d321e7f3 100644 --- a/cmd/restic/cmd_diff.go +++ b/cmd/restic/cmd_diff.go @@ -72,7 +72,7 @@ func (opts *DiffOptions) AddFlags(f *pflag.FlagSet) { func loadSnapshot(ctx context.Context, be restic.Lister, repo restic.LoaderUnpacked, desc string) (*restic.Snapshot, string, error) { sn, subfolder, err := restic.FindSnapshot(ctx, be, repo, desc) if err != nil { - return nil, "", errors.Fatal(err.Error()) + return nil, "", errors.Fatalf("%s", err) } return sn, subfolder, err } diff --git a/cmd/restic/cmd_init.go b/cmd/restic/cmd_init.go index d66163af1..c31cb1cbd 100644 --- a/cmd/restic/cmd_init.go +++ b/cmd/restic/cmd_init.go @@ -101,7 +101,7 @@ func runInit(ctx context.Context, opts InitOptions, gopts GlobalOptions, args [] PackSize: gopts.PackSize * 1024 * 1024, }) if err != nil { - return errors.Fatal(err.Error()) + return errors.Fatalf("%s", err) } err = s.Init(ctx, version, gopts.password, chunkerPolynomial) diff --git a/cmd/restic/global.go b/cmd/restic/global.go index 6e58a0d73..c48960559 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -486,7 +486,7 @@ func OpenRepository(ctx context.Context, opts GlobalOptions) (*repository.Reposi NoExtraVerify: opts.NoExtraVerify, }) if err != nil { - return nil, errors.Fatal(err.Error()) + return nil, errors.Fatalf("%s", err) } passwordTriesLeft := 1 @@ -613,7 +613,7 @@ func innerOpen(ctx context.Context, s string, gopts GlobalOptions, opts options. rt, err := backend.Transport(globalOptions.TransportOptions) if err != nil { - return nil, errors.Fatal(err.Error()) + return nil, errors.Fatalf("%s", err) } // wrap the transport so that the throughput via HTTP is limited diff --git a/internal/repository/prune.go b/internal/repository/prune.go index 9726a6032..30152e208 100644 --- a/internal/repository/prune.go +++ b/internal/repository/prune.go @@ -567,7 +567,7 @@ func (plan *PrunePlan) Execute(ctx context.Context, printer progress.Printer) er _, err := Repack(ctx, repo, repo, plan.repackPacks, plan.keepBlobs, bar, printer.P) bar.Done() if err != nil { - return errors.Fatal(err.Error()) + return errors.Fatalf("%s", err) } // Also remove repacked packs