From 901cd5edef48b7a00b67d0e5c174e67e0235b669 Mon Sep 17 00:00:00 2001 From: Fabian Wickborn Date: Mon, 27 Nov 2017 17:23:08 +0100 Subject: [PATCH 1/3] cmd/restic: Add test for rejectIfPresent bug All RejectFuncs returned by rejectIfPresent share the same rejection cache and hence might cancel each other out. --- cmd/restic/exclude_test.go | 55 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/cmd/restic/exclude_test.go b/cmd/restic/exclude_test.go index 5cadf6e9b..7932080fd 100644 --- a/cmd/restic/exclude_test.go +++ b/cmd/restic/exclude_test.go @@ -2,6 +2,7 @@ package main import ( "io/ioutil" + "os" "path/filepath" "testing" @@ -82,3 +83,57 @@ func TestIsExcludedByFile(t *testing.T) { }) } } + +func TestMultipleIsExcludedByFile(t *testing.T) { + tempDir, cleanup := test.TempDir(t) + defer cleanup() + files := []struct { + path string + incl bool + }{ + {"42", true}, + + {"foodir/NOFOO", true}, + {"foodir/foo", false}, + {"foodir/foosub/underfoo", false}, + + {"bardir/NOBAR", true}, + {"bardir/bar", false}, + {"bardir/barsub/underbar", false}, + + {"bazdir/baz", true}, + {"bazdir/bazsub/underbaz", true}, + } + var errs []error + for _, f := range files { + p := filepath.Join(tempDir, filepath.FromSlash(f.path)) + errs = append(errs, os.MkdirAll(filepath.Dir(p), 0700)) + errs = append(errs, ioutil.WriteFile(p, []byte(f.path), 0600)) + } + test.OKs(t, errs) + rc := &rejectionCache{} + fooExclude, _ := rejectIfPresent("NOFOO", rc) + barExclude, _ := rejectIfPresent("NOBAR", rc) + m := make(map[string]bool) + walk := func(p string, fi os.FileInfo, err error) error { + if err != nil { + return err + } + excludedByFoo := fooExclude(p, fi) + excludedByBar := barExclude(p, fi) + excluded := excludedByFoo || excludedByBar + t.Logf("%q: %v || %v = %v", p, excludedByFoo, excludedByBar, excluded) + m[p] = !excluded + if excluded { + return filepath.SkipDir + } + return nil + } + test.OK(t, filepath.Walk(tempDir, walk)) + for _, f := range files { + p := filepath.Join(tempDir, filepath.FromSlash(f.path)) + if m[p] != f.incl { + t.Errorf("inclusion status of %s is wrong: want %v, got %v", f.path, f.incl, m[p]) + } + } +} From 1ea518d5ef3ccf315aedbfa42dccd8e7a1185c91 Mon Sep 17 00:00:00 2001 From: Fabian Wickborn Date: Mon, 27 Nov 2017 17:30:53 +0100 Subject: [PATCH 2/3] cmd/restic: Use a dedicated cache for each rejectIfPresent --- cmd/restic/cmd_backup.go | 3 +-- cmd/restic/exclude.go | 3 ++- cmd/restic/exclude_test.go | 5 ++--- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index eb04b6c4c..af6e1ece1 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -372,9 +372,8 @@ func runBackup(opts BackupOptions, gopts GlobalOptions, args []string) error { opts.ExcludeIfPresent = append(opts.ExcludeIfPresent, "CACHEDIR.TAG:Signature: 8a477f597d28d172789f06886806bc55") } - rc := &rejectionCache{} for _, spec := range opts.ExcludeIfPresent { - f, err := rejectIfPresent(spec, rc) + f, err := rejectIfPresent(spec) if err != nil { return err } diff --git a/cmd/restic/exclude.go b/cmd/restic/exclude.go index 0a6d8bcec..e4a934d9b 100644 --- a/cmd/restic/exclude.go +++ b/cmd/restic/exclude.go @@ -90,7 +90,7 @@ func rejectByPattern(patterns []string) RejectFunc { // non-nil if the filename component of excludeFileSpec is empty. If rc is // non-nil, it is going to be used in the RejectFunc to expedite the evaluation // of a directory based on previous visits. -func rejectIfPresent(excludeFileSpec string, rc *rejectionCache) (RejectFunc, error) { +func rejectIfPresent(excludeFileSpec string) (RejectFunc, error) { if excludeFileSpec == "" { return nil, errors.New("name for exclusion tagfile is empty") } @@ -106,6 +106,7 @@ func rejectIfPresent(excludeFileSpec string, rc *rejectionCache) (RejectFunc, er tf = excludeFileSpec } debug.Log("using %q as exclusion tagfile", tf) + rc := &rejectionCache{} fn := func(filename string, _ os.FileInfo) bool { return isExcludedByFile(filename, tf, tc, rc) } diff --git a/cmd/restic/exclude_test.go b/cmd/restic/exclude_test.go index 7932080fd..6cdf990ce 100644 --- a/cmd/restic/exclude_test.go +++ b/cmd/restic/exclude_test.go @@ -111,9 +111,8 @@ func TestMultipleIsExcludedByFile(t *testing.T) { errs = append(errs, ioutil.WriteFile(p, []byte(f.path), 0600)) } test.OKs(t, errs) - rc := &rejectionCache{} - fooExclude, _ := rejectIfPresent("NOFOO", rc) - barExclude, _ := rejectIfPresent("NOBAR", rc) + fooExclude, _ := rejectIfPresent("NOFOO") + barExclude, _ := rejectIfPresent("NOBAR") m := make(map[string]bool) walk := func(p string, fi os.FileInfo, err error) error { if err != nil { From 27fadd2c6ee18ec9c1651f29b94e10c86bce29a8 Mon Sep 17 00:00:00 2001 From: Fabian Wickborn Date: Mon, 27 Nov 2017 21:38:15 +0100 Subject: [PATCH 3/3] Document approach for multiple reject-if-present test --- cmd/restic/exclude_test.go | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/cmd/restic/exclude_test.go b/cmd/restic/exclude_test.go index 6cdf990ce..eeba76e74 100644 --- a/cmd/restic/exclude_test.go +++ b/cmd/restic/exclude_test.go @@ -84,35 +84,57 @@ func TestIsExcludedByFile(t *testing.T) { } } +// TestMultipleIsExcludedByFile is for testing that multiple instances of +// the --exclude-if-present parameter (or the shortcut --exclude-caches do not +// cancel each other out. It was initially written to demonstrate a bug in +// rejectIfPresent. func TestMultipleIsExcludedByFile(t *testing.T) { tempDir, cleanup := test.TempDir(t) defer cleanup() + + // Create some files in a temporary directory. + // Files in UPPERCASE will be used as exclusion triggers later on. + // We will test the inclusion later, so we add the expected value as + // a bool. files := []struct { path string incl bool }{ {"42", true}, + // everything in foodir except the NOFOO tagfile + // should not be included. {"foodir/NOFOO", true}, {"foodir/foo", false}, {"foodir/foosub/underfoo", false}, + // everything in bardir except the NOBAR tagfile + // should not be included. {"bardir/NOBAR", true}, {"bardir/bar", false}, {"bardir/barsub/underbar", false}, + // everything in bazdir should be included. {"bazdir/baz", true}, {"bazdir/bazsub/underbaz", true}, } var errs []error for _, f := range files { + // create directories first, then the file p := filepath.Join(tempDir, filepath.FromSlash(f.path)) errs = append(errs, os.MkdirAll(filepath.Dir(p), 0700)) errs = append(errs, ioutil.WriteFile(p, []byte(f.path), 0600)) } - test.OKs(t, errs) + test.OKs(t, errs) // see if anything went wrong during the creation + + // create two rejection functions, one that tests for the NOFOO file + // and one for the NOBAR file fooExclude, _ := rejectIfPresent("NOFOO") barExclude, _ := rejectIfPresent("NOBAR") + + // To mock the archiver scanning walk, we create filepath.WalkFn + // that tests against the two rejection functions and stores + // the result in a map against we can test later. m := make(map[string]bool) walk := func(p string, fi os.FileInfo, err error) error { if err != nil { @@ -121,6 +143,7 @@ func TestMultipleIsExcludedByFile(t *testing.T) { excludedByFoo := fooExclude(p, fi) excludedByBar := barExclude(p, fi) excluded := excludedByFoo || excludedByBar + // the log message helps debugging in case the test fails t.Logf("%q: %v || %v = %v", p, excludedByFoo, excludedByBar, excluded) m[p] = !excluded if excluded { @@ -128,7 +151,10 @@ func TestMultipleIsExcludedByFile(t *testing.T) { } return nil } + // walk through the temporary file and check the error test.OK(t, filepath.Walk(tempDir, walk)) + + // compare whether the walk gave the expected values for the test cases for _, f := range files { p := filepath.Join(tempDir, filepath.FromSlash(f.path)) if m[p] != f.incl {