Delete empty parent directories on gopass rm -r (gopasspw#1725)

This adds house keeping around empty parent directories. They would
have been left over in some cases. This adds more proetections to avoid
that, i.e. removing empty parents on rm -r (not only rm) as well as
fsck checks.

Fixes gopasspw#1723

RELEASE_NOTES=[BUGFIX] Remove empty parents on gopass rm -r

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>

internal/action/delete.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	"github.com/gopasspw/gopass/pkg/ctxutil"
+	"github.com/gopasspw/gopass/pkg/debug"
 	"github.com/gopasspw/gopass/pkg/termio"
 
 	"github.com/urfave/cli/v2"
@@ -39,17 +40,21 @@ func (s *Action) Delete(c *cli.Context) error {
 	}
 
 	if recursive {
+		debug.Log("pruning %q", name)
 		if err := s.Store.Prune(ctx, name); err != nil {
 			return ExitError(ExitUnknown, err, "failed to prune '%s': %s", name, err)
 		}
+		debug.Log("pruned %q", name)
 		return nil
 	}
 
 	// deletes a single key from a YAML doc
 	if key != "" {
+		debug.Log("removing key %q from %q", key, name)
 		return s.deleteKeyFromYAML(ctx, name, key)
 	}
 
+	debug.Log("removing entry %q", name)
 	if err := s.Store.Delete(ctx, name); err != nil {
 		return ExitError(ExitIO, err, "Can not delete '%s': %s", name, err)
 	}

internal/backend/storage/fs/fsck.go

@@ -2,8 +2,11 @@ package fs
 
 import (
 	"context"
+	"io/ioutil"
 	"os"
 	"path/filepath"
+	"sort"
+	"strings"
 	"syscall"
 
 	"github.com/gopasspw/gopass/internal/out"
@@ -23,7 +26,7 @@ func (s *Store) Fsck(ctx context.Context) error {
 	dirs := make(map[string]struct{}, len(entries))
 	for _, entry := range entries {
 		pcb()
-		debug.Log("Checking %s", entry)
+		debug.Log("checking entry %q", entry)
 
 		filename := filepath.Join(s.path, entry)
 		dirs[filepath.Dir(filename)] = struct{}{}
@@ -34,11 +37,18 @@ func (s *Store) Fsck(ctx context.Context) error {
 	}
 
 	for dir := range dirs {
+		debug.Log("checking dir %q", dir)
 		if err := s.fsckCheckDir(ctx, dir); err != nil {
 			return err
 		}
 	}
-	return nil
+
+	if err := s.fsckCheckEmptyDirs(); err != nil {
+		return err
+	}
+
+	debug.Log("checking root dir %q", s.path)
+	return s.fsckCheckDir(ctx, s.path)
 }
 
 func (s *Store) fsckCheckFile(ctx context.Context, filename string) error {
@@ -90,3 +100,54 @@ func (s *Store) fsckCheckDir(ctx context.Context, dirname string) error {
 	}
 	return nil
 }
+
+func (s *Store) fsckCheckEmptyDirs() error {
+	v := []string{}
+	if err := filepath.Walk(s.path, func(fp string, fi os.FileInfo, ferr error) error {
+		if ferr != nil {
+			return ferr
+		}
+		if !fi.IsDir() {
+			return nil
+		}
+		if strings.HasPrefix(fi.Name(), ".") {
+			return filepath.SkipDir
+		}
+		if fp == s.path {
+			return nil
+		}
+
+		// add candidate
+		debug.Log("adding candidate %q", fp)
+		v = append(v, fp)
+		return nil
+	}); err != nil {
+		return err
+	}
+
+	// start with longest path (deepest dir)
+	sort.Slice(v, func(i, j int) bool {
+		return len(v[i]) > len(v[j])
+	})
+
+	for _, d := range v {
+		if err := fsckRemoveEmptyDir(d); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func fsckRemoveEmptyDir(fp string) error {
+	ls, err := ioutil.ReadDir(fp)
+	if err != nil {
+		return err
+	}
+	if len(ls) > 0 {
+		debug.Log("dir %q is not empty (%d)", fp, len(ls))
+		return nil
+	}
+
+	debug.Log("removing %q ...", fp)
+	return os.Remove(fp)
+}

internal/backend/storage/fs/store.go

@@ -88,6 +88,7 @@ func (s *Store) removeEmptyParentDirectories(path string) error {
 		return nil
 	}
 
+	debug.Log("removing empty parent dir: %q", parent)
 	err := os.Remove(parent)
 	switch {
 	case err == nil:
@@ -162,7 +163,12 @@ func (s *Store) IsDir(ctx context.Context, name string) bool {
 func (s *Store) Prune(ctx context.Context, prefix string) error {
 	path := filepath.Join(s.path, filepath.Clean(prefix))
 	debug.Log("Purning %s from %s", prefix, path)
-	return os.RemoveAll(path)
+
+	if err := os.RemoveAll(path); err != nil {
+		return err
+	}
+
+	return s.removeEmptyParentDirectories(path)
 }
 
 // Name returns the name of this backend

internal/store/leaf/move.go

@@ -76,6 +76,8 @@ func (s *Store) delete(ctx context.Context, name string, recurse bool) error {
 		}
 	}
 	if err := s.deleteSingle(ctx, path); err != nil {
+		// might fail if we deleted the root of a tree which isn't a secret
+		// itself
 		if !recurse {
 			return err
 		}
@@ -115,6 +117,7 @@ func (s *Store) deleteRecurse(ctx context.Context, name, path string) error {
 
 	debug.Log("Pruning %s", name)
 	if err := s.storage.Prune(ctx, name); err != nil {
+		debug.Log("storage.Prune(%v) failed", name)
 		return err
 	}
 
@@ -124,6 +127,7 @@ func (s *Store) deleteRecurse(ctx context.Context, name, path string) error {
 		}
 		return errors.Wrapf(err, "failed to add '%s' to git", path)
 	}
+	debug.Log("pruned")
 	return nil
 }
 

internal/store/leaf/move_test.go

@@ -280,8 +280,7 @@ func TestPrune(t *testing.T) {
 					assert.Error(t, err)
 
 					// delete empty folder
-					assert.NoError(t, s.Prune(ctx, "foo/"))
-					assert.Error(t, s.Prune(ctx, "foo/"))
+					assert.Error(t, s.Prune(ctx, "foo/"), "delete non-existing entry")
 				}
 			},
 		},