trees: count a nested dupe tree once in the reclaimable total (closes #168) (#188)

internal/sfdupes/trees.go

@@ -108,7 +108,7 @@ func runTrees(ctx context.Context, args []string, stdout io.Writer) error {
 		label: treesPhase("write"), unit: "groups", total: int64(len(dupes)),
 		stalledOn: "out",
 	})
-	dupeTrees, reclaimable, err := writeTreeGroups(stdout, dupes, prog)
+	dupeTrees, err := writeTreeGroups(stdout, dupes, prog)
 
 	prog.finish()
 
@@ -119,7 +119,7 @@ func runTrees(ctx context.Context, args []string, stdout io.Writer) error {
 	fmt.Fprintf(os.Stderr,
 		"trees: %d records read, %d duplicate tree groups, %d dupe trees, "+
 			"%s reclaimable\n",
-		records, len(dupes), dupeTrees, humanBytes(reclaimable))
+		records, len(dupes), dupeTrees, humanBytes(reclaimableBytes(dupes)))
 
 	return nil
 }
@@ -177,22 +177,19 @@ func readTree(ctx context.Context, db *sql.DB, operands []string,
 // a row for every member of each group but its first, counting each
 // group on prog once its rows are written. The disk_size column is the
 // dupe's own. stdout may be the terminal prog is drawn on, so the lines
-// go through prog's writer. It returns the number of rows (dupe trees)
-// and the bytes they could reclaim.
+// go through prog's writer. It returns the number of rows (dupe trees).
 func writeTreeGroups(stdout io.Writer, dupes [][]*treeNode,
 	prog *progress,
-) (int, int64, error) {
+) (int, error) {
 	out := prog.writer(stdout)
 
 	_, err := fmt.Fprintln(out, "first\tdupe\tfiles\tsize\tdisk_size")
 	if err != nil {
-		return 0, 0, fmt.Errorf("write stdout: %w", err)
+		return 0, fmt.Errorf("write stdout: %w", err)
 	}
 
 	dupeTrees := 0
 
-	var reclaimable int64
-
 	for _, g := range dupes {
 		first := g[0]
 		for _, n := range g[1:] {
@@ -200,11 +197,10 @@ func writeTreeGroups(stdout io.Writer, dupes [][]*treeNode,
 				escapePath(first.path), escapePath(n.path),
 				first.fileCount, first.totalSize, n.diskSize)
 			if err != nil {
-				return 0, 0, fmt.Errorf("write stdout: %w", err)
+				return 0, fmt.Errorf("write stdout: %w", err)
 			}
 
 			dupeTrees++
-			reclaimable += first.totalSize
 		}
 
 		prog.increment()
@@ -212,10 +208,44 @@ func writeTreeGroups(stdout io.Writer, dupes [][]*treeNode,
 
 	err = out.Flush()
 	if err != nil {
-		return 0, 0, fmt.Errorf("write stdout: %w", err)
+		return 0, fmt.Errorf("write stdout: %w", err)
 	}
 
-	return dupeTrees, reclaimable, nil
+	return dupeTrees, nil
+}
+
+// reclaimableBytes returns the sum of the size column over the rows
+// that writeTreeGroups writes for dupes, leaving out a row whose dupe
+// lies inside another row's dupe.
+func reclaimableBytes(dupes [][]*treeNode) int64 {
+	isDupe := make(map[*treeNode]bool)
+
+	for _, g := range dupes {
+		for _, n := range g[1:] {
+			isDupe[n] = true
+		}
+	}
+
+	var total int64
+
+	for n := range isDupe {
+		if !insideDupe(n, isDupe) {
+			total += n.totalSize
+		}
+	}
+
+	return total
+}
+
+// insideDupe reports whether a directory above n is in isDupe.
+func insideDupe(n *treeNode, isDupe map[*treeNode]bool) bool {
+	for p := n.parent; p != nil; p = p.parent {
+		if isDupe[p] {
+			return true
+		}
+	}
+
+	return false
 }
 
 // treeBuilder reconstructs the directory hierarchy from records added

internal/sfdupes/trees_test.go

@@ -364,7 +364,7 @@ func TestWriteTreeGroupsOnTerminal(t *testing.T) {
 		label: treesPhase("write"), unit: "groups", total: int64(len(dupes)),
 	}, terminalWidth, time.Hour, time.Now)
 
-	_, _, err := writeTreeGroups(os.Stderr, dupes, p)
+	_, err := writeTreeGroups(os.Stderr, dupes, p)
 
 	p.finish()
 
@@ -638,3 +638,64 @@ func TestCollectTreeGroupsKeepsGroupLeftWithTwoCopies(t *testing.T) {
 		t.Errorf("groups = %v, want %v", got, want)
 	}
 }
+
+// TestReclaimableBytesCountsNestedDupeOnce checks that a dupe tree
+// inside another dupe tree adds nothing to the reclaimable bytes. /B/x
+// lies in /B, so the total adds the sizes of /B and /C/x, 1500 + 1000.
+func TestReclaimableBytesCountsNestedDupeOnce(t *testing.T) {
+	t.Parallel()
+
+	fh, ft, fc := testHash("fh"), testHash("ft"), testHash("fc")
+	gh, gt, gc := testHash("gh"), testHash("gt"), testHash("gc")
+	recs := []scanRec{
+		{size: 1000, head: fh, tail: ft, content: fc, path: "/A/x/f"},
+		{size: 500, head: gh, tail: gt, content: gc, path: "/A/y/g"},
+		{size: 1000, head: fh, tail: ft, content: fc, path: "/B/x/f"},
+		{size: 500, head: gh, tail: gt, content: gc, path: "/B/y/g"},
+		{size: 1000, head: fh, tail: ft, content: fc, path: "/C/x/f"},
+	}
+
+	super, dirs := treeOf(t, recs)
+	dupes := collectTreeGroups(dirs, super, everyPathExists, nil)
+
+	wantGroups := [][]string{{"/A", "/B"}, {"/A/x", "/B/x", "/C/x"}}
+	if got := groupPaths(dupes); !slices.EqualFunc(got, wantGroups,
+		slices.Equal) {
+		t.Fatalf("groups = %v, want %v", got, wantGroups)
+	}
+
+	if got := reclaimableBytes(dupes); got != 2500 {
+		t.Errorf("reclaimableBytes = %d, want 2500", got)
+	}
+}
+
+// TestReclaimableBytesCountsDupeInsideGrandparentDupeOnce checks that a
+// dupe tree adds nothing when a dupe tree above its parent holds it.
+// /B/m/x lies in /B, and /B/m is no dupe, since the group of /A and /B
+// implies that of /A/m and /B/m. The total adds the sizes of /B and
+// /C/x, 1300 + 1000.
+func TestReclaimableBytesCountsDupeInsideGrandparentDupeOnce(t *testing.T) {
+	t.Parallel()
+
+	f, w := testHash("f"), testHash("w")
+	recs := []scanRec{
+		{size: 1000, head: f, tail: f, content: f, path: "/A/m/x/f"},
+		{size: 300, head: w, tail: w, content: w, path: "/A/m/w"},
+		{size: 1000, head: f, tail: f, content: f, path: "/B/m/x/f"},
+		{size: 300, head: w, tail: w, content: w, path: "/B/m/w"},
+		{size: 1000, head: f, tail: f, content: f, path: "/C/x/f"},
+	}
+
+	super, dirs := treeOf(t, recs)
+	dupes := collectTreeGroups(dirs, super, everyPathExists, nil)
+
+	wantGroups := [][]string{{"/A", "/B"}, {"/A/m/x", "/B/m/x", "/C/x"}}
+	if got := groupPaths(dupes); !slices.EqualFunc(got, wantGroups,
+		slices.Equal) {
+		t.Fatalf("groups = %v, want %v", got, wantGroups)
+	}
+
+	if got := reclaimableBytes(dupes); got != 2300 {
+		t.Errorf("reclaimableBytes = %d, want 2300", got)
+	}
+}