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,31 +19,34 @@ 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
@@ -51,7 +54,9 @@ gopass
 ├── 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 {

internal/tree/root.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	"github.com/fatih/color"
+	"github.com/gopasspw/gopass/pkg/debug"
 )
 
 const (
@@ -21,6 +22,7 @@ var (
 	colMount    = color.New(color.FgCyan, color.Bold).SprintfFunc()
 	colDir      = color.New(color.FgBlue, color.Bold).SprintfFunc()
 	colTpl      = color.New(color.FgGreen, color.Bold).SprintfFunc()
+	colShadow   = color.New(color.FgRed, color.Bold).SprintfFunc()
 	// sep is intentionally NOT platform-agnostic. This is used for the CLI output
 	// and should always be a regular slash.
 	sep = "/"
@@ -59,18 +61,18 @@ func (r *Root) AddTemplate(path string) error {
 func (r *Root) insert(path string, template bool, mountPath string) error {
 	t := r.Subtree
 
+	debug.Log("adding: %s [tpl: %t, mp: %q]", path, template, mountPath)
 	// split the path into its components, iterate over them and create
 	// the tree structure. Everything but the last element is a folder.
 	p := strings.Split(path, "/")
 	for i, e := range p {
 		n := &Node{
 			Name:    e,
-			Type:    "dir",
 			Subtree: NewTree(),
 		}
 		// this is the final element (a leaf)
 		if i == len(p)-1 {
-			n.Type = "file"
+			n.Leaf = true
 			n.Subtree = nil
 			n.Template = template
 
@@ -80,11 +82,14 @@ func (r *Root) insert(path string, template bool, mountPath string) error {
 			}
 		}
 
-		node, _ := t.Insert(n)
+		debug.Log("[%d] %s -> Node: %+v", i, e, n)
+
+		node := t.Insert(n)
+		debug.Log("node after insert: %+v", node)
+
 		// do we need to extend an existing subtree?
 		if i < len(p)-1 && node.Subtree == nil {
 			node.Subtree = NewTree()
-			node.Type = "dir"
 		}
 
 		// re-root t to the new subtree
@@ -147,8 +152,8 @@ func (r *Root) FindFolder(path string) (*Root, error) {
 	prefix := ""
 
 	for _, e := range p {
-		_, node := t.find(e)
-		if node == nil || node.Type == "file" || node.Subtree == nil {
+		_, node := t.findPositionFor(e)
+		if node == nil || node.Subtree == nil {
 			return nil, ErrNotFound
 		}
 

internal/tree/root_test.go

@@ -21,13 +21,13 @@ func TestRoot(t *testing.T) {
 	assert.NoError(t, r.AddFile("mnt/m1/foo/bar", ""))
 	t.Logf("%+#v", r)
 	assert.Equal(t, `gopass
-├── foo/ (template)
+├── foo/ (template) (shadowed)
 │   └── bar/
 │       ├── baz
 │       └── zab
 └── mnt/
     └── m1 (/tmp/m1)
-        └── foo/
+        └── foo/ (shadowed)
             └── bar
 `, r.Format(INF))
 
@@ -47,7 +47,7 @@ func TestRoot(t *testing.T) {
 	f, err := r.FindFolder("mnt/m1")
 	assert.NoError(t, err)
 	assert.Equal(t, `gopass
-└── foo/
+└── foo/ (shadowed)
     └── bar
 `, f.Format(INF))
 }

internal/tree/tree.go

@@ -44,28 +44,25 @@ func (t *Tree) Equals(other *Tree) bool {
 }
 
 // Insert adds a new node at the right position.
-func (t *Tree) Insert(node *Node) (*Node, error) {
-	pos, found := t.find(node.Name)
-	if found != nil {
-		if node.Mount {
-			t.Nodes[pos] = node
+func (t *Tree) Insert(other *Node) *Node {
+	pos, node := t.findPositionFor(other.Name)
+	if node != nil {
+		m := node.Merge(*other)
+		t.Nodes[pos] = m
 
-			return node, nil
-		}
-
-		return t.Nodes[pos], fmt.Errorf("error at %q: %w", node.Name, ErrNodePresent)
+		return m
 	}
 
 	// insert at the right position, see
 	// https://code.google.com/p/go-wiki/wiki/SliceTricks
 	t.Nodes = append(t.Nodes, &Node{})
 	copy(t.Nodes[pos+1:], t.Nodes[pos:])
-	t.Nodes[pos] = node
+	t.Nodes[pos] = other
 
-	return node, nil
+	return other
 }
 
-func (t *Tree) find(name string) (int, *Node) {
+func (t *Tree) findPositionFor(name string) (int, *Node) {
 	pos := sort.Search(len(t.Nodes), func(i int) bool {
 		return t.Nodes[i].Name >= name
 	})

internal/tree/tree_test.go

@@ -14,11 +14,9 @@ func TestTree(t *testing.T) {
 
 	assert.True(t, t1.Equals(t2))
 
-	_, err := t1.Insert(&Node{Name: "foo"})
-	assert.NoError(t, err)
+	_ = t1.Insert(&Node{Name: "foo"})
 	assert.False(t, t1.Equals(t2))
 
-	_, err = t2.Insert(&Node{Name: "foo"})
-	assert.NoError(t, err)
+	_ = t2.Insert(&Node{Name: "foo"})
 	assert.True(t, t1.Equals(t2))
 }