Merge pull request #3243 from restic/fix-scanner-overlap

backup: Fix total size for overlapping targets

changelog/unreleased/issue-3232

@@ -0,0 +1,11 @@
+Bugfix: Show correct statistics for overlapping targets
+
+A user reported that restic's statistics and progress information during backup
+is not correctly calculated when the backup targets (files/dirs to save)
+overlap. For example, consider a directory `foo` which contains (among others)
+a file `foo/bar`. When `restic backup foo foo/bar` is run, restic counted the
+size of the file `foo/bar` twice, so the completeness percentage as well as the
+number of files was wrong. This is now corrected.
+
+https://github.com/restic/restic/issues/3232
+https://github.com/restic/restic/pull/3243

internal/archiver/archiver.go

@@ -555,13 +555,7 @@ func (arch *Archiver) SaveTree(ctx context.Context, snPath string, atree *Tree,
 	futureNodes := make(map[string]FutureNode)
 
 	// iterate over the nodes of atree in lexicographic (=deterministic) order
-	names := make([]string, 0, len(atree.Nodes))
-	for name := range atree.Nodes {
-		names = append(names, name)
-	}
-	sort.Strings(names)
-
-	for _, name := range names {
+	for _, name := range atree.NodeNames() {
 		subatree := atree.Nodes[name]
 
 		// test if context has been cancelled
@@ -570,7 +564,7 @@ func (arch *Archiver) SaveTree(ctx context.Context, snPath string, atree *Tree,
 		}
 
 		// this is a leaf node
-		if subatree.Path != "" {
+		if subatree.Leaf() {
 			fn, excluded, err := arch.Save(ctx, join(snPath, name), subatree.Path, previous.Find(name))
 
 			if err != nil {

internal/archiver/scanner.go

@@ -6,6 +6,7 @@ import (
 	"path/filepath"
 	"sort"
 
+	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/fs"
 )
 
@@ -37,27 +38,63 @@ type ScanStats struct {
 	Bytes               uint64
 }
 
-// Scan traverses the targets. The function Result is called for each new item
-// found, the complete result is also returned by Scan.
-func (s *Scanner) Scan(ctx context.Context, targets []string) error {
-	var stats ScanStats
-	for _, target := range targets {
-		abstarget, err := s.FS.Abs(target)
+func (s *Scanner) scanTree(ctx context.Context, stats ScanStats, tree Tree) (ScanStats, error) {
+	// traverse the path in the file system for all leaf nodes
+	if tree.Leaf() {
+		abstarget, err := s.FS.Abs(tree.Path)
 		if err != nil {
-			return err
+			return ScanStats{}, err
 		}
 
 		stats, err = s.scan(ctx, stats, abstarget)
 		if err != nil {
-			return err
+			return ScanStats{}, err
+		}
+
+		return stats, nil
+	}
+
+	// otherwise recurse into the nodes in a deterministic order
+	for _, name := range tree.NodeNames() {
+		var err error
+		stats, err = s.scanTree(ctx, stats, tree.Nodes[name])
+		if err != nil {
+			return ScanStats{}, err
 		}
 
 		if ctx.Err() != nil {
-			return nil
+			return stats, nil
 		}
 	}
 
+	return stats, nil
+}
+
+// Scan traverses the targets. The function Result is called for each new item
+// found, the complete result is also returned by Scan.
+func (s *Scanner) Scan(ctx context.Context, targets []string) error {
+	debug.Log("start scan for %v", targets)
+
+	cleanTargets, err := resolveRelativeTargets(s.FS, targets)
+	if err != nil {
+		return err
+	}
+
+	debug.Log("clean targets %v", cleanTargets)
+
+	// we're using the same tree representation as the archiver does
+	tree, err := NewTree(s.FS, cleanTargets)
+	if err != nil {
+		return err
+	}
+
+	stats, err := s.scanTree(ctx, ScanStats{}, *tree)
+	if err != nil {
+		return err
+	}
+
 	s.Result("", stats)
+	debug.Log("result: %+v", stats)
 	return nil
 }
 

internal/archiver/scanner_test.go

@@ -40,8 +40,7 @@ func TestScanner(t *testing.T) {
 				filepath.FromSlash("work/subdir/other"):   {Files: 5, Bytes: 60},
 				filepath.FromSlash("work/subdir"):         {Files: 5, Dirs: 1, Bytes: 60},
 				filepath.FromSlash("work"):                {Files: 5, Dirs: 2, Bytes: 60},
-				filepath.FromSlash("."):                   {Files: 5, Dirs: 3, Bytes: 60},
-				filepath.FromSlash(""):                    {Files: 5, Dirs: 3, Bytes: 60},
+				filepath.FromSlash(""):                    {Files: 5, Dirs: 2, Bytes: 60},
 			},
 		},
 		{
@@ -72,8 +71,7 @@ func TestScanner(t *testing.T) {
 				filepath.FromSlash("work/subdir/bar.txt"): {Files: 2, Bytes: 30},
 				filepath.FromSlash("work/subdir"):         {Files: 2, Dirs: 1, Bytes: 30},
 				filepath.FromSlash("work"):                {Files: 2, Dirs: 2, Bytes: 30},
-				filepath.FromSlash("."):                   {Files: 2, Dirs: 3, Bytes: 30},
-				filepath.FromSlash(""):                    {Files: 2, Dirs: 3, Bytes: 30},
+				filepath.FromSlash(""):                    {Files: 2, Dirs: 2, Bytes: 30},
 			},
 		},
 	}
@@ -152,7 +150,7 @@ func TestScannerError(t *testing.T) {
 					},
 				},
 			},
-			result: ScanStats{Files: 5, Dirs: 3, Bytes: 60},
+			result: ScanStats{Files: 5, Dirs: 2, Bytes: 60},
 		},
 		{
 			name: "unreadable-dir",
@@ -168,7 +166,7 @@ func TestScannerError(t *testing.T) {
 					},
 				},
 			},
-			result: ScanStats{Files: 3, Dirs: 2, Bytes: 28},
+			result: ScanStats{Files: 3, Dirs: 1, Bytes: 28},
 			prepare: func(t testing.TB) {
 				err := os.Chmod(filepath.Join("work", "subdir"), 0000)
 				if err != nil {
@@ -191,7 +189,7 @@ func TestScannerError(t *testing.T) {
 				"foo":   TestFile{Content: "foo"},
 				"other": TestFile{Content: "other"},
 			},
-			result: ScanStats{Files: 3, Dirs: 1, Bytes: 11},
+			result: ScanStats{Files: 3, Dirs: 0, Bytes: 11},
 			resFn: func(t testing.TB, item string, s ScanStats) {
 				if item == "bar" {
 					err := os.Remove("foo")
@@ -289,7 +287,7 @@ func TestScannerCancel(t *testing.T) {
 		"other": TestFile{Content: "other"},
 	}
 
-	result := ScanStats{Files: 2, Dirs: 1, Bytes: 6}
+	result := ScanStats{Files: 2, Dirs: 0, Bytes: 6}
 
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()

internal/archiver/tree.go

@@ -2,6 +2,7 @@ package archiver
 
 import (
 	"fmt"
+	"sort"
 
 	"github.com/restic/restic/internal/debug"
 	"github.com/restic/restic/internal/errors"
@@ -199,6 +200,24 @@ func (t Tree) String() string {
 	return formatTree(t, "")
 }
 
+// Leaf returns true if this is a leaf node, which means Path is set to a
+// non-empty string and the contents of Path should be inserted at this point
+// in the tree.
+func (t Tree) Leaf() bool {
+	return t.Path != ""
+}
+
+// NodeNames returns the sorted list of subtree names.
+func (t Tree) NodeNames() []string {
+	// iterate over the nodes of atree in lexicographic (=deterministic) order
+	names := make([]string, 0, len(t.Nodes))
+	for name := range t.Nodes {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	return names
+}
+
 // formatTree returns a text representation of the tree t.
 func formatTree(t Tree, indent string) (s string) {
 	for name, node := range t.Nodes {