Do not omit shadowed entries in ls output (#2341)

* Do not omit shadowed entries in ls output

Fixes #2338

RELEASE_NOTES=[BUGFIX] Do not shadow entries behind folders.

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

* Add shadow marker

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

* Adjust tests to match the new shadow behaviour

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

* Update list docs wrt. shadowing

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

* Do not mark mounts as shadowed. That's already implicit.

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

* Add more comments and some other cleanup

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

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

docs/commands/list.md

@@ -5,8 +5,8 @@ The `list` command is used to list all the entries in the password store or at a
 ## Synopsis
 
 ```bash
-$ gopass ls
-$ gopass ls path/to/entries
+gopass ls
+gopass ls path/to/entries
 ```
 
 - List all the entries in the password store including the one in mounted stores: `gopass list`
@@ -19,39 +19,44 @@ Note: `list` will not change anything, nor encrypt or decrypt anything.
 Flag | Aliases | Description
 ---- | ------- | -----------
 `--limit value` | `-l value`| Max tree depth (default: -1)
-` --flat `      |` -f`      | Print a flat list of secrets (default: false)
-` --folders`    | `-d`    |  Print a flat list of folders (default: false)
-` --strip-prefix` | `-s`    |  Strip prefix from filtered entries (default: false)
+`--flat`      |`-f`      | Print a flat list of secrets (default: false)
+`--folders`    | `-d`    |  Print a flat list of folders (default: false)
+`--strip-prefix` | `-s`    |  Strip prefix from filtered entries (default: false)
 
-The `--flat` and `--folders` flags provide a plaintext list of the entries located at 
-the given prefix (default prefix being the root `/`). They are notably used to produce the 
-completion results. 
+The `--flat` and `--folders` flags provide a plaintext list of the entries located at
+the given prefix (default prefix being the root `/`). They are notably used to produce the
+completion results.
 The `--flat` one will list all entries, one per line, using its full path.
-The `--folders` one will display all the folders, one per line, recursively per level. 
+The `--folders` one will display all the folders, one per line, recursively per level.
 For instance an entry `folder/sub/entry` would cause it to list both:
+
 ```bash
 $ gopass list --folders
 folder
 folder/sub
 ```
+
 whereas `gopass list --flat` would have just displayed one line: `folder/sub/entry`.
 
 The `--strip-prefix` flag is meant to be used along with `--flat` or `--folders`.
-It will list the relative path from the current prefix, removing the said prefix, 
+It will list the relative path from the current prefix, removing the said prefix,
 instead of listing the relative paths from the root.
 For instance on entry `folder/sub/entry`, running `gopass ls -f -s folder` would display
  only `sub/entry` instead of `folder/sub/entry`.
 
-The `--limit` flag starts counting its depth from the root store, which means that 
+The `--limit` flag starts counting its depth from the root store, which means that
 a depth of 0 only lists the items in the root gopass store:
+
 ```bash
 $ gopass list -l 0
 gopass
 ├── bar/
 ├── foo/
 └── test (/home/user/.local/share/gopass/stores/substore1)
 ```
+
 A value of 1 would list all the items in the root, plus their sub-items but no more:
+
 ```bash
 $ gopass list -l 1
 gopass
@@ -63,7 +68,9 @@ gopass
 └── test (/home/user/.local/share/gopass/stores/substore1)
     └── foo
 ```
-A negative value lists all the items without any depth limit. 
+
+A negative value lists all the items without any depth limit.
+
 ```bash
 $ gopass list -l -1
 gopass
@@ -80,6 +87,7 @@ gopass
 ```
 
 The flags can be used together: `gopass -l 1 -d` will list only the folders up to a depth of 1:
+
 ```bash
 $ gopass list -l 1 -d
 bar/
@@ -90,18 +98,20 @@ test/foo/
 ```
 
 ## Shadowing
+
 It is possible to have a path that is both an entry and a folder. In that case the list command
-will always display the folder and the entry is "shadowed", but it can still be accessed using 
+will display the folder with a marker of `(shadowed)`, it can still be accessed using
 `gopass show path/to/it`, while the content of the folder can be listed using `gopass list path/to/it`.
 
 It should also be noted that the `mount` command can completely "shadow" an entry in a password store,
-simply by having the same name and this entry and its subentries will not be visible 
+simply by having the same name and this entry and its subentries will not be visible
 using `ls` anymore until the substore is unmounted.
 The entries shadowed by a mount will not show up in a search and cannot be accessed at all without unmounting.
 
-For instance in our example above, maybe there is an entry test/zaz in the root store, 
-but since the substore is mounted as `test/`, it only displays the content of the substore. 
+For instance in our example above, maybe there is an entry test/zaz in the root store,
+but since the substore is mounted as `test/`, it only displays the content of the substore.
 Unmounting it reveals its shadowed entries:
+
 ```bash
 $ gopass list test
 test/ 

internal/action/list_test.go

@@ -123,53 +123,65 @@ func TestListLimit(t *testing.T) { //nolint:paralleltest
 	assert.Equal(t, want, buf.String())
 	buf.Reset()
 
-	assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"folders": "true", "limit": "0"})))
-	want = `foo/
+	t.Run("folders-limit-0", func(t *testing.T) { //nolint:paralleltest
+		assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"folders": "true", "limit": "0"})))
+		want = `foo/
 foo2/
 `
-	assert.Equal(t, want, buf.String())
-	buf.Reset()
+		assert.Equal(t, want, buf.String())
+		buf.Reset()
+	})
 
-	assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"folders": "true", "limit": "1"})))
-	want = `foo/
+	t.Run("folders-limit-1", func(t *testing.T) { //nolint:paralleltest
+		assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"folders": "true", "limit": "1"})))
+		want = `foo/
 foo/zen/
 foo2/
 `
-	assert.Equal(t, want, buf.String())
-	buf.Reset()
+		assert.Equal(t, want, buf.String())
+		buf.Reset()
+	})
 
-	assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"folders": "true", "limit": "-1"})))
-	want = `foo/
+	t.Run("folders-limit--1", func(t *testing.T) { //nolint:paralleltest
+		assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"folders": "true", "limit": "-1"})))
+		want = `foo/
 foo/zen/
 foo/zen/baz/
 foo2/
 `
-	assert.Equal(t, want, buf.String())
-	buf.Reset()
+		assert.Equal(t, want, buf.String())
+		buf.Reset()
+	})
 
-	assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"flat": "true", "limit": "-1"})))
-	want = `foo/bar
+	t.Run("flat-limit--1", func(t *testing.T) { //nolint:paralleltest
+		assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"flat": "true", "limit": "-1"})))
+		want = `foo/bar
 foo/zen/baz/bar
 foo2/bar2
 `
-	assert.Equal(t, want, buf.String())
-	buf.Reset()
+		assert.Equal(t, want, buf.String())
+		buf.Reset()
+	})
 
-	assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"flat": "true", "limit": "0"})))
-	want = `foo/
+	t.Run("folders-limit-0", func(t *testing.T) { //nolint:paralleltest
+		assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"flat": "true", "limit": "0"})))
+		want = `foo/
 foo2/
 `
-	assert.Equal(t, want, buf.String())
-	buf.Reset()
+		assert.Equal(t, want, buf.String())
+		buf.Reset()
+	})
 
-	assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"flat": "true", "limit": "2"})))
-	want = `foo/bar
+	t.Run("folders-limit-2", func(t *testing.T) { //nolint:paralleltest
+		assert.NoError(t, act.List(gptest.CliCtxWithFlags(ctx, t, map[string]string{"flat": "true", "limit": "2"})))
+		want = `foo/bar
 foo/zen/baz/
 foo2/bar2
 `
 
-	assert.Equal(t, want, buf.String())
-	buf.Reset()
+		assert.Equal(t, want, buf.String())
+		buf.Reset()
+	})
 }
 
 func TestRedirectPager(t *testing.T) { //nolint:paralleltest

internal/store/root/list.go

@@ -9,6 +9,7 @@ import (
 	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/internal/store"
 	"github.com/gopasspw/gopass/internal/tree"
+	"github.com/gopasspw/gopass/pkg/debug"
 )
 
 // List will return a flattened list of all tree entries.
@@ -61,7 +62,9 @@ func (r *Store) Tree(ctx context.Context) (*tree.Root, error) {
 		return nil, err
 	}
 
+	debug.Log("[root] adding files: %q", sf)
 	addFileFunc(sf...)
+	debug.Log("[root] Tree: %s", root.Format(-1))
 	addTplFunc(r.store.ListTemplates(ctx, "")...)
 
 	mps := r.MountPoints()
@@ -82,6 +85,7 @@ func (r *Store) Tree(ctx context.Context) (*tree.Root, error) {
 			return nil, fmt.Errorf("failed to add file: %w", err)
 		}
 
+		debug.Log("[%s] adding files: %q", alias, sf)
 		addFileFunc(sf...)
 		addTplFunc(substore.ListTemplates(ctx, alias)...)
 	}

internal/tree/node.go

@@ -2,12 +2,14 @@ package tree
 
 import (
 	"bytes"
+
+	"github.com/gopasspw/gopass/pkg/debug"
 )
 
 // Node is a tree node.
 type Node struct {
 	Name     string
-	Type     string
+	Leaf     bool
 	Template bool
 	Mount    bool
 	Path     string
@@ -40,7 +42,7 @@ func (n Node) Equals(other Node) bool {
 		return false
 	}
 
-	if n.Type != other.Type {
+	if n.Leaf != other.Leaf {
 		return false
 	}
 
@@ -59,6 +61,61 @@ func (n Node) Equals(other Node) bool {
 	return true
 }
 
+// Merge will merge two nodes into a new node. Does not change either of the two
+// input nodes. The merged node will be in the returned node. Implements semantics
+// specific to gopass' tree model, i.e. mounts shadow (erase) everything below
+// a mount point, nodes within a tree can be leafs (i.e. contain a secret as well
+// as subdirectories) and any node can also contain a template.
+func (n Node) Merge(other Node) *Node {
+	r := Node{
+		Name:     n.Name,
+		Leaf:     n.Leaf,
+		Template: n.Template,
+		Mount:    n.Mount,
+		Path:     n.Path,
+		Subtree:  n.Subtree,
+	}
+
+	// During a merge we can't change the name.
+
+	// If either of the nodes is a leaf (i.e. contains a secret) the
+	// merged node will be a leaf.
+	if other.Leaf {
+		r.Leaf = true
+	}
+
+	// If either node has a template the merged has a template, too.
+	if other.Template {
+		r.Template = true
+	}
+
+	// Handling of mounts is a bit more tricky. See the comment above.
+	// If we're adding a mount to the tree this shadows (erases) everything
+	// that was on this branch before a replaces it with the mount.
+	// Think of Unix mount semantics here.
+	if other.Mount {
+		r.Mount = true
+		// anything at the mount point, including a secret at the root
+		// of the mount point will become inaccessible.
+		r.Leaf = false
+		r.Path = other.Path
+		// existing templates will become invisible
+		r.Template = false
+		// the subtree from the mount overlays (shadows) the original tree
+		r.Subtree = other.Subtree
+	}
+	// Merging can't change the path (except a mount, see above)
+	// If the other node has a subtree we use that, otherwise
+	// this method shouldn't have been called in the first place.
+	if r.Subtree == nil && other.Subtree != nil {
+		r.Subtree = other.Subtree
+	}
+
+	debug.Log("merged %+v and %+v into %+v", n, other, r)
+
+	return &r
+}
+
 // format returns a pretty printed string of all nodes in and below
 // this node, e.g. `├── baz`.
 func (n *Node) format(prefix string, last bool, maxDepth, curDepth int) string {
@@ -86,7 +143,7 @@ func (n *Node) format(prefix string, last bool, maxDepth, curDepth int) string {
 	switch {
 	case n.Mount:
 		_, _ = out.WriteString(colMount(n.Name + " (" + n.Path + ")"))
-	case n.Type == "dir":
+	case n.Subtree != nil:
 		_, _ = out.WriteString(colDir(n.Name + sep))
 	default:
 		_, _ = out.WriteString(n.Name)
@@ -95,6 +152,10 @@ func (n *Node) format(prefix string, last bool, maxDepth, curDepth int) string {
 	if n.Template {
 		_, _ = out.WriteString(" " + colTpl("(template)"))
 	}
+	// mark shadowed entries
+	if n.Leaf && n.Subtree != nil && !n.Mount {
+		_, _ = out.WriteString(" " + colShadow("(shadowed)"))
+	}
 	// finish this output
 	_, _ = out.WriteString("\n")
 
@@ -113,12 +174,18 @@ func (n *Node) format(prefix string, last bool, maxDepth, curDepth int) string {
 
 // Len returns the length of this subtree.
 func (n *Node) Len() int {
-	if n.Type == "file" {
+	if n.Subtree == nil {
 		return 1
 	}
 
 	var l int
 
+	// this node might point to a secret itself so we must account for that
+	if n.Leaf {
+		l++
+	}
+
+	// and for any secret it's subtree might contain
 	for _, t := range n.Subtree.Nodes {
 		l += t.Len()
 	}
@@ -137,17 +204,17 @@ func (n *Node) list(prefix string, maxDepth, curDepth int, files bool) []string
 
 	prefix += n.Name
 
+	out := make([]string, 0, n.Len())
 	// if it's a file and we are looking for files
-	if n.Type == "file" && files {
+	if n.Subtree == nil && files {
 		// we return the file
-		return []string{prefix}
-	} else if curDepth == maxDepth && n.Type != "file" {
+		out = append(out, prefix)
+	} else if curDepth == maxDepth && n.Subtree != nil {
 		// otherwise if we are "at the bottom" and it's not a file
 		// we return the directory name with a separator at the end
 		return []string{prefix + sep}
 	}
 
-	out := make([]string, 0, n.Len())
 	// if we don't have subitems, then it's a leaf and we return
 	// (notice that this is what ends the recursion when maxDepth is set to -1)
 	if n.Subtree == nil {