From 502fc3281c6e1c2f97e4a88bef2d17070bf45e0b Mon Sep 17 00:00:00 2001 From: Aneesh Agrawal Date: Sun, 23 Jan 2022 23:31:10 -0500 Subject: [PATCH 1/3] Add CONTRIBUTING.md docs to not edit man pages Document this code review feedback I got for other contributors. --- CONTRIBUTING.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2f6134e6a..9c8fedf1b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -123,7 +123,10 @@ down to the following steps: writing, ask yourself: If I were the user, what would I need to be aware of with this change? - 8. Once your code looks good and passes all the tests, we'll merge it. Thanks + 8. Do not edit the man pages under `doc/man` or `doc/manual_rest.rst` - + these are autogenerated before new releases. + + 9. Once your code looks good and passes all the tests, we'll merge it. Thanks a lot for your contribution! Please provide the patches for each bug or feature in a separate branch and From 058dfc20da780dd48d0eee165740bc164ca36629 Mon Sep 17 00:00:00 2001 From: Aneesh Agrawal Date: Tue, 4 Jan 2022 01:03:53 -0500 Subject: [PATCH 2/3] Avoid choosing parent snapshot newer than time of current snapshot Currently, `restic backup` (if a `--parent` is not provided) will choose the most recent matching snapshot as the parent snapshot. This makes sense in the usual case, where we tag the snapshot-being-created with the current time. However, this doesn't make sense if the user has passed `--time` and is currently creating a snapshot older than the latest snapshot. Instead, choose the most recent snapshot which is not newer than the snapshot-being-created's timestamp, to avoid any time travel. Impetus for this change: I'm using restic for the first time! I have a number of existing BTRFS snapshots I am backing up via restic to serve as my initial set of backups. I initially `restic backup`'d the most recent snapshot to test, then started backing up each of the other snapshots. I noticed in `restic cat snapshot ` output that all the remaining snapshots have the most recent as the parent. --- changelog/unreleased/pull-3619 | 12 +++++++ cmd/restic/cmd_backup.go | 8 ++--- cmd/restic/cmd_dump.go | 2 +- cmd/restic/cmd_restore.go | 2 +- cmd/restic/find.go | 2 +- internal/restic/snapshot_find.go | 8 +++-- internal/restic/snapshot_find_test.go | 47 +++++++++++++++++++++++++++ 7 files changed, 72 insertions(+), 9 deletions(-) create mode 100644 changelog/unreleased/pull-3619 create mode 100644 internal/restic/snapshot_find_test.go diff --git a/changelog/unreleased/pull-3619 b/changelog/unreleased/pull-3619 new file mode 100644 index 000000000..1806d56b1 --- /dev/null +++ b/changelog/unreleased/pull-3619 @@ -0,0 +1,12 @@ +Bugfix: Avoid choosing parent snapshots newer than time of current snapshot + +`restic backup` (if a `--parent` is not provided) +previously chose the most recent matching snapshot as the parent snapshot. +However, this didn't make sense when the user passed `--time` +to create a snapshot older than the most recent snapshot. + +Instead, `restic backup` now chooses the most recent snapshot +which is not newer than the snapshot-being-created's timestamp, +to avoid any time travel. + +https://github.com/restic/restic/pull/3619 diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index ab9ee41f0..5332284b2 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -103,7 +103,7 @@ func init() { cmdRoot.AddCommand(cmdBackup) f := cmdBackup.Flags() - f.StringVar(&backupOptions.Parent, "parent", "", "use this parent `snapshot` (default: last snapshot in the repo that has the same target files/directories)") + f.StringVar(&backupOptions.Parent, "parent", "", "use this parent `snapshot` (default: last snapshot in the repo that has the same target files/directories, and is not newer than the snapshot time)") f.BoolVarP(&backupOptions.Force, "force", "f", false, `force re-reading the target files/directories (overrides the "parent" flag)`) f.StringArrayVarP(&backupOptions.Excludes, "exclude", "e", nil, "exclude a `pattern` (can be specified multiple times)") f.StringArrayVar(&backupOptions.InsensitiveExcludes, "iexclude", nil, "same as --exclude `pattern` but ignores the casing of filenames") @@ -472,7 +472,7 @@ func collectTargets(opts BackupOptions, args []string) (targets []string, err er // parent returns the ID of the parent snapshot. If there is none, nil is // returned. -func findParentSnapshot(ctx context.Context, repo restic.Repository, opts BackupOptions, targets []string) (parentID *restic.ID, err error) { +func findParentSnapshot(ctx context.Context, repo restic.Repository, opts BackupOptions, targets []string, timeStamp time.Time) (parentID *restic.ID, err error) { // Force using a parent if !opts.Force && opts.Parent != "" { id, err := restic.FindSnapshot(ctx, repo, opts.Parent) @@ -485,7 +485,7 @@ func findParentSnapshot(ctx context.Context, repo restic.Repository, opts Backup // Find last snapshot to set it as parent, if not already set if !opts.Force && parentID == nil { - id, err := restic.FindLatestSnapshot(ctx, repo, targets, []restic.TagList{}, []string{opts.Host}) + id, err := restic.FindLatestSnapshot(ctx, repo, targets, []restic.TagList{}, []string{opts.Host}, &timeStamp) if err == nil { parentID = &id } else if err != restic.ErrNoSnapshotFound { @@ -579,7 +579,7 @@ func runBackup(opts BackupOptions, gopts GlobalOptions, term *termstatus.Termina return err } - parentSnapshotID, err := findParentSnapshot(gopts.ctx, repo, opts, targets) + parentSnapshotID, err := findParentSnapshot(gopts.ctx, repo, opts, targets, timeStamp) if err != nil { return err } diff --git a/cmd/restic/cmd_dump.go b/cmd/restic/cmd_dump.go index 4cb96053c..4449af5ce 100644 --- a/cmd/restic/cmd_dump.go +++ b/cmd/restic/cmd_dump.go @@ -152,7 +152,7 @@ func runDump(opts DumpOptions, gopts GlobalOptions, args []string) error { var id restic.ID if snapshotIDString == "latest" { - id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts) + id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts, nil) if err != nil { Exitf(1, "latest snapshot for criteria not found: %v Paths:%v Hosts:%v", err, opts.Paths, opts.Hosts) } diff --git a/cmd/restic/cmd_restore.go b/cmd/restic/cmd_restore.go index 0ece13e5f..4d5818593 100644 --- a/cmd/restic/cmd_restore.go +++ b/cmd/restic/cmd_restore.go @@ -118,7 +118,7 @@ func runRestore(opts RestoreOptions, gopts GlobalOptions, args []string) error { var id restic.ID if snapshotIDString == "latest" { - id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts) + id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts, nil) if err != nil { Exitf(1, "latest snapshot for criteria not found: %v Paths:%v Hosts:%v", err, opts.Paths, opts.Hosts) } diff --git a/cmd/restic/find.go b/cmd/restic/find.go index 1297420f9..792f1f6a2 100644 --- a/cmd/restic/find.go +++ b/cmd/restic/find.go @@ -23,7 +23,7 @@ func FindFilteredSnapshots(ctx context.Context, repo *repository.Repository, hos for _, s := range snapshotIDs { if s == "latest" { usedFilter = true - id, err = restic.FindLatestSnapshot(ctx, repo, paths, tags, hosts) + id, err = restic.FindLatestSnapshot(ctx, repo, paths, tags, hosts, nil) if err != nil { Warnf("Ignoring %q, no snapshot matched given filter (Paths:%v Tags:%v Hosts:%v)\n", s, paths, tags, hosts) continue diff --git a/internal/restic/snapshot_find.go b/internal/restic/snapshot_find.go index fe9b943de..f85a2020d 100644 --- a/internal/restic/snapshot_find.go +++ b/internal/restic/snapshot_find.go @@ -13,8 +13,8 @@ import ( // ErrNoSnapshotFound is returned when no snapshot for the given criteria could be found. var ErrNoSnapshotFound = errors.New("no snapshot found") -// FindLatestSnapshot finds latest snapshot with optional target/directory, tags and hostname filters. -func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, tagLists []TagList, hostnames []string) (ID, error) { +// FindLatestSnapshot finds latest snapshot with optional target/directory, tags, hostname, and timestamp filters. +func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, tagLists []TagList, hostnames []string, timeStamp *time.Time) (ID, error) { var err error absTargets := make([]string, 0, len(targets)) for _, target := range targets { @@ -38,6 +38,10 @@ func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, return errors.Errorf("Error loading snapshot %v: %v", id.Str(), err) } + if timeStamp != nil && snapshot.Time.After(*timeStamp) { + return nil + } + if snapshot.Time.Before(latest) { return nil } diff --git a/internal/restic/snapshot_find_test.go b/internal/restic/snapshot_find_test.go new file mode 100644 index 000000000..d4fedcc9d --- /dev/null +++ b/internal/restic/snapshot_find_test.go @@ -0,0 +1,47 @@ +package restic_test + +import ( + "context" + "testing" + + "github.com/restic/restic/internal/repository" + "github.com/restic/restic/internal/restic" +) + +func TestFindLatestSnapshot(t *testing.T) { + repo, cleanup := repository.TestRepository(t) + defer cleanup() + + restic.TestCreateSnapshot(t, repo, parseTimeUTC("2015-05-05 05:05:05"), 1, 0) + restic.TestCreateSnapshot(t, repo, parseTimeUTC("2017-07-07 07:07:07"), 1, 0) + latestSnapshot := restic.TestCreateSnapshot(t, repo, parseTimeUTC("2019-09-09 09:09:09"), 1, 0) + + id, err := restic.FindLatestSnapshot(context.TODO(), repo, []string{}, []restic.TagList{}, []string{"foo"}, nil) + if err != nil { + t.Fatalf("FindLatestSnapshot returned error: %v", err) + } + + if id != *latestSnapshot.ID() { + t.Errorf("FindLatestSnapshot returned wrong snapshot ID: %v", id) + } +} + +func TestFindLatestSnapshotWithMaxTimestamp(t *testing.T) { + repo, cleanup := repository.TestRepository(t) + defer cleanup() + + restic.TestCreateSnapshot(t, repo, parseTimeUTC("2015-05-05 05:05:05"), 1, 0) + desiredSnapshot := restic.TestCreateSnapshot(t, repo, parseTimeUTC("2017-07-07 07:07:07"), 1, 0) + restic.TestCreateSnapshot(t, repo, parseTimeUTC("2019-09-09 09:09:09"), 1, 0) + + maxTimestamp := parseTimeUTC("2018-08-08 08:08:08") + + id, err := restic.FindLatestSnapshot(context.TODO(), repo, []string{}, []restic.TagList{}, []string{"foo"}, &maxTimestamp) + if err != nil { + t.Fatalf("FindLatestSnapshot returned error: %v", err) + } + + if id != *desiredSnapshot.ID() { + t.Errorf("FindLatestSnapshot returned wrong snapshot ID: %v", id) + } +} From 8ae4d86a846bc125fec57fead2445e2f1a0ff151 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 5 Feb 2022 22:42:38 +0100 Subject: [PATCH 3/3] rename snapshot timestamp filter variable --- cmd/restic/cmd_backup.go | 4 ++-- internal/restic/snapshot_find.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index 5332284b2..fe7da3333 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -472,7 +472,7 @@ func collectTargets(opts BackupOptions, args []string) (targets []string, err er // parent returns the ID of the parent snapshot. If there is none, nil is // returned. -func findParentSnapshot(ctx context.Context, repo restic.Repository, opts BackupOptions, targets []string, timeStamp time.Time) (parentID *restic.ID, err error) { +func findParentSnapshot(ctx context.Context, repo restic.Repository, opts BackupOptions, targets []string, timeStampLimit time.Time) (parentID *restic.ID, err error) { // Force using a parent if !opts.Force && opts.Parent != "" { id, err := restic.FindSnapshot(ctx, repo, opts.Parent) @@ -485,7 +485,7 @@ func findParentSnapshot(ctx context.Context, repo restic.Repository, opts Backup // Find last snapshot to set it as parent, if not already set if !opts.Force && parentID == nil { - id, err := restic.FindLatestSnapshot(ctx, repo, targets, []restic.TagList{}, []string{opts.Host}, &timeStamp) + id, err := restic.FindLatestSnapshot(ctx, repo, targets, []restic.TagList{}, []string{opts.Host}, &timeStampLimit) if err == nil { parentID = &id } else if err != restic.ErrNoSnapshotFound { diff --git a/internal/restic/snapshot_find.go b/internal/restic/snapshot_find.go index f85a2020d..1f309f0bd 100644 --- a/internal/restic/snapshot_find.go +++ b/internal/restic/snapshot_find.go @@ -14,7 +14,7 @@ import ( var ErrNoSnapshotFound = errors.New("no snapshot found") // FindLatestSnapshot finds latest snapshot with optional target/directory, tags, hostname, and timestamp filters. -func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, tagLists []TagList, hostnames []string, timeStamp *time.Time) (ID, error) { +func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, tagLists []TagList, hostnames []string, timeStampLimit *time.Time) (ID, error) { var err error absTargets := make([]string, 0, len(targets)) for _, target := range targets { @@ -38,7 +38,7 @@ func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, return errors.Errorf("Error loading snapshot %v: %v", id.Str(), err) } - if timeStamp != nil && snapshot.Time.After(*timeStamp) { + if timeStampLimit != nil && snapshot.Time.After(*timeStampLimit) { return nil }