From 83c51db90303c8ffae9ccc3754d7dd0b25f9751b Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 20 Jun 2018 22:53:53 +0200 Subject: [PATCH 1/3] fs: Add helper functions ReadDir/ReadDirNames --- internal/fs/fs_helpers.go | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 internal/fs/fs_helpers.go diff --git a/internal/fs/fs_helpers.go b/internal/fs/fs_helpers.go new file mode 100644 index 000000000..6b269f763 --- /dev/null +++ b/internal/fs/fs_helpers.go @@ -0,0 +1,45 @@ +package fs + +import "os" + +// ReadDir reads the directory named by dirname within fs and returns a list of +// directory entries. +func ReadDir(fs FS, dirname string) ([]os.FileInfo, error) { + f, err := fs.Open(dirname) + if err != nil { + return nil, err + } + + entries, err := f.Readdir(-1) + if err != nil { + return nil, err + } + + err = f.Close() + if err != nil { + return nil, err + } + + return entries, nil +} + +// ReadDirNames reads the directory named by dirname within fs and returns a +// list of entry names. +func ReadDirNames(fs FS, dirname string) ([]string, error) { + f, err := fs.Open(dirname) + if err != nil { + return nil, err + } + + entries, err := f.Readdirnames(-1) + if err != nil { + return nil, err + } + + err = f.Close() + if err != nil { + return nil, err + } + + return entries, nil +} From 9ffc26883a352caa0fdcd79773e5bec822d41379 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 20 Jun 2018 22:54:47 +0200 Subject: [PATCH 2/3] archiver: Unroll tree --- internal/archiver/tree.go | 56 +++++++++---- internal/archiver/tree_test.go | 143 ++++++++++++++++++++++++++++++--- 2 files changed, 175 insertions(+), 24 deletions(-) diff --git a/internal/archiver/tree.go b/internal/archiver/tree.go index 8adca2cc3..5835839b2 100644 --- a/internal/archiver/tree.go +++ b/internal/archiver/tree.go @@ -202,30 +202,53 @@ func (t Tree) String() string { // formatTree returns a text representation of the tree t. func formatTree(t Tree, indent string) (s string) { for name, node := range t.Nodes { - if node.Path != "" { - s += fmt.Sprintf("%v/%v, src %q\n", indent, name, node.Path) - continue - } - s += fmt.Sprintf("%v/%v, root %q, meta %q\n", indent, name, node.Root, node.FileInfoPath) + s += fmt.Sprintf("%v/%v, root %q, path %q, meta %q\n", indent, name, node.Root, node.Path, node.FileInfoPath) s += formatTree(node, indent+" ") } return s } -// prune removes sub-trees of leaf nodes. -func prune(t *Tree) { - // if the current tree is a leaf node (Path is set), remove all nodes, - // those are automatically included anyway. +// unrollTree unrolls the tree so that only leaf nodes have Path set. +func unrollTree(f fs.FS, t *Tree) error { + // if the current tree is a leaf node (Path is set) and has additional + // nodes, add the contents of Path to the nodes. if t.Path != "" && len(t.Nodes) > 0 { - t.FileInfoPath = "" - t.Nodes = nil - return + debug.Log("resolve path %v", t.Path) + entries, err := fs.ReadDirNames(f, t.Path) + if err != nil { + return err + } + + for _, entry := range entries { + if node, ok := t.Nodes[entry]; ok { + if node.Path == "" { + node.Path = f.Join(t.Path, entry) + t.Nodes[entry] = node + continue + } + + if node.Path == f.Join(t.Path, entry) { + continue + } + + return errors.Errorf("tree unrollTree: collision on path, node %#v, path %q", node, f.Join(t.Path, entry)) + continue + } + t.Nodes[entry] = Tree{Path: f.Join(t.Path, entry)} + } + t.Path = "" } for i, subtree := range t.Nodes { - prune(&subtree) + err := unrollTree(f, &subtree) + if err != nil { + return err + } + t.Nodes[i] = subtree } + + return nil } // NewTree creates a Tree from the target files/directories. @@ -248,7 +271,12 @@ func NewTree(fs fs.FS, targets []string) (*Tree, error) { } } - prune(tree) + debug.Log("before unroll:\n%v", tree) + err := unrollTree(fs, tree) + if err != nil { + return nil, err + } + debug.Log("result:\n%v", tree) return tree, nil } diff --git a/internal/archiver/tree_test.go b/internal/archiver/tree_test.go index f50bb510f..5d460bb37 100644 --- a/internal/archiver/tree_test.go +++ b/internal/archiver/tree_test.go @@ -7,6 +7,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/restic/restic/internal/fs" + restictest "github.com/restic/restic/internal/test" ) func TestPathComponents(t *testing.T) { @@ -136,6 +137,7 @@ func TestRootDirectory(t *testing.T) { func TestTree(t *testing.T) { var tests = []struct { targets []string + src TestDir want Tree unix bool win bool @@ -227,39 +229,152 @@ func TestTree(t *testing.T) { }}, }, { + src: TestDir{ + "foo": TestDir{ + "file": TestFile{Content: "file content"}, + "work": TestFile{Content: "work file content"}, + }, + }, + targets: []string{"foo", "foo/work"}, + want: Tree{Nodes: map[string]Tree{ + "foo": Tree{ + Root: ".", + FileInfoPath: "foo", + Nodes: map[string]Tree{ + "file": Tree{Path: filepath.FromSlash("foo/file")}, + "work": Tree{Path: filepath.FromSlash("foo/work")}, + }, + }, + }}, + }, + { + src: TestDir{ + "foo": TestDir{ + "file": TestFile{Content: "file content"}, + "work": TestDir{ + "other": TestFile{Content: "other file content"}, + }, + }, + }, + targets: []string{"foo/work", "foo"}, + want: Tree{Nodes: map[string]Tree{ + "foo": Tree{ + Root: ".", + FileInfoPath: "foo", + Nodes: map[string]Tree{ + "file": Tree{Path: filepath.FromSlash("foo/file")}, + "work": Tree{Path: filepath.FromSlash("foo/work")}, + }, + }, + }}, + }, + { + src: TestDir{ + "foo": TestDir{ + "work": TestDir{ + "user1": TestFile{Content: "file content"}, + "user2": TestFile{Content: "other file content"}, + }, + }, + }, targets: []string{"foo/work", "foo/work/user2"}, want: Tree{Nodes: map[string]Tree{ "foo": Tree{Root: ".", FileInfoPath: "foo", Nodes: map[string]Tree{ "work": Tree{ - Path: filepath.FromSlash("foo/work"), + FileInfoPath: filepath.FromSlash("foo/work"), + Nodes: map[string]Tree{ + "user1": Tree{Path: filepath.FromSlash("foo/work/user1")}, + "user2": Tree{Path: filepath.FromSlash("foo/work/user2")}, + }, }, }}, }}, }, { + src: TestDir{ + "foo": TestDir{ + "work": TestDir{ + "user1": TestFile{Content: "file content"}, + "user2": TestFile{Content: "other file content"}, + }, + }, + }, targets: []string{"foo/work/user2", "foo/work"}, want: Tree{Nodes: map[string]Tree{ "foo": Tree{Root: ".", FileInfoPath: "foo", Nodes: map[string]Tree{ - "work": Tree{ - Path: filepath.FromSlash("foo/work"), + "work": Tree{FileInfoPath: filepath.FromSlash("foo/work"), + Nodes: map[string]Tree{ + "user1": Tree{Path: filepath.FromSlash("foo/work/user1")}, + "user2": Tree{Path: filepath.FromSlash("foo/work/user2")}, + }, }, }}, }}, }, { + src: TestDir{ + "foo": TestDir{ + "other": TestFile{Content: "file content"}, + "work": TestDir{ + "user2": TestDir{ + "data": TestDir{ + "secret": TestFile{Content: "secret file content"}, + }, + }, + "user3": TestDir{ + "important.txt": TestFile{Content: "important work"}, + }, + }, + }, + }, targets: []string{"foo/work/user2/data/secret", "foo"}, want: Tree{Nodes: map[string]Tree{ - "foo": Tree{Root: ".", Path: "foo"}, + "foo": Tree{Root: ".", FileInfoPath: "foo", Nodes: map[string]Tree{ + "other": Tree{Path: filepath.FromSlash("foo/other")}, + "work": Tree{FileInfoPath: filepath.FromSlash("foo/work"), Nodes: map[string]Tree{ + "user2": Tree{FileInfoPath: filepath.FromSlash("foo/work/user2"), Nodes: map[string]Tree{ + "data": Tree{FileInfoPath: filepath.FromSlash("foo/work/user2/data"), Nodes: map[string]Tree{ + "secret": Tree{ + Path: filepath.FromSlash("foo/work/user2/data/secret"), + }, + }}, + }}, + "user3": Tree{Path: filepath.FromSlash("foo/work/user3")}, + }}, + }}, }}, }, { - unix: true, - targets: []string{"/mnt/driveA", "/mnt/driveA/work/driveB"}, - want: Tree{Nodes: map[string]Tree{ - "mnt": Tree{Root: "/", FileInfoPath: filepath.FromSlash("/mnt"), Nodes: map[string]Tree{ - "driveA": Tree{ - Path: filepath.FromSlash("/mnt/driveA"), + src: TestDir{ + "mnt": TestDir{ + "driveA": TestDir{ + "work": TestDir{ + "driveB": TestDir{ + "secret": TestFile{Content: "secret file content"}, + }, + "test1": TestDir{ + "important.txt": TestFile{Content: "important work"}, + }, + }, + "test2": TestDir{ + "important.txt": TestFile{Content: "other important work"}, + }, }, + }, + }, + unix: true, + targets: []string{"mnt/driveA", "mnt/driveA/work/driveB"}, + want: Tree{Nodes: map[string]Tree{ + "mnt": Tree{Root: ".", FileInfoPath: filepath.FromSlash("mnt"), Nodes: map[string]Tree{ + "driveA": Tree{FileInfoPath: filepath.FromSlash("mnt/driveA"), Nodes: map[string]Tree{ + "work": Tree{FileInfoPath: filepath.FromSlash("mnt/driveA/work"), Nodes: map[string]Tree{ + "driveB": Tree{ + Path: filepath.FromSlash("mnt/driveA/work/driveB"), + }, + "test1": Tree{Path: filepath.FromSlash("mnt/driveA/work/test1")}, + }}, + "test2": Tree{Path: filepath.FromSlash("mnt/driveA/test2")}, + }}, }}, }}, }, @@ -320,6 +435,14 @@ func TestTree(t *testing.T) { t.Skip("skip test on unix") } + tempdir, cleanup := restictest.TempDir(t) + defer cleanup() + + TestCreateFiles(t, tempdir, test.src) + + back := fs.TestChdir(t, tempdir) + defer back() + tree, err := NewTree(fs.Local{}, test.targets) if test.mustError { if err == nil { From 30cfd133282fc3e8a57fd061c75b1490d4a6a23b Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 20 Jun 2018 23:05:09 +0200 Subject: [PATCH 3/3] Add changelog --- changelog/unreleased/issue-1854 | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 changelog/unreleased/issue-1854 diff --git a/changelog/unreleased/issue-1854 b/changelog/unreleased/issue-1854 new file mode 100644 index 000000000..0168102c3 --- /dev/null +++ b/changelog/unreleased/issue-1854 @@ -0,0 +1,16 @@ +Bugfix: Allow saving files/dirs on different fs with `--one-file-system` + +restic now allows saving files/dirs on a different file system in a subdir +correctly even when `--one-file-system` is specified. + +The first thing the restic archiver code does is to build a tree of the target +files/directories. If it detects that a parent directory is already included +(e.g. `restic backup /foo /foo/bar/baz`), it'll ignore the latter argument. + +Without `--one-file-system`, that's perfectly valid: If `/foo` is to be +archived, it will include `/foo/bar/baz`. But with `--one-file-system`, +`/foo/bar/baz` may reside on a different file system, so it won't be included +with `/foo`. + +https://github.com/restic/restic/issues/1854 +https://github.com/restic/restic/pull/1855