From cf6020b8f01b829c5dfa8865f30013f2359cd112 Mon Sep 17 00:00:00 2001 From: Dominik Schulz Date: Mon, 8 Mar 2021 22:43:55 +0100 Subject: [PATCH] Do not shadow directories (#1817) Fixes #1813 RELEASE_NOTES=[BUGFIX] Do not shadow directories Signed-off-by: Dominik Schulz --- internal/store/root/move.go | 18 +++-- internal/store/root/move_test.go | 132 +++++++++++++++++++++---------- 2 files changed, 103 insertions(+), 47 deletions(-) diff --git a/internal/store/root/move.go b/internal/store/root/move.go index 341fab9d6e..4643455f12 100644 --- a/internal/store/root/move.go +++ b/internal/store/root/move.go @@ -39,7 +39,7 @@ func (r *Store) move(ctx context.Context, from, to string, delete bool) error { return fmt.Errorf("destination is a file") } - if err := r.moveFromTo(ctx, subFrom, from, to, fromPrefix, srcIsDir, delete); err != nil { + if err := r.moveFromTo(ctx, subFrom, from, to, fromPrefix, srcIsDir, dstIsDir, delete); err != nil { return err } if err := subFrom.Storage().Commit(ctx, fmt.Sprintf("Move from %s to %s", from, to)); delete && err != nil { @@ -63,7 +63,7 @@ func (r *Store) move(ctx context.Context, from, to string, delete bool) error { if err := subFrom.Storage().Push(ctx, "", ""); err != nil { if errors.Is(err, store.ErrGitNotInit) { - msg := "Warning: git is not initialized for this.storage. Ignoring auto-push option\n" + + msg := "Warning: git is not initialized for this storage. Ignoring auto-push option\n" + "Run: gopass git init" debug.Log(msg) return nil @@ -79,7 +79,7 @@ func (r *Store) move(ctx context.Context, from, to string, delete bool) error { if !subFrom.Equals(subTo) { if err := subTo.Storage().Push(ctx, "", ""); err != nil { if errors.Is(err, store.ErrGitNotInit) { - msg := "Warning: git is not initialized for this.storage. Ignoring auto-push option\n" + + msg := "Warning: git is not initialized for this storage. Ignoring auto-push option\n" + "Run: gopass git init" debug.Log(msg) return nil @@ -96,10 +96,12 @@ func (r *Store) move(ctx context.Context, from, to string, delete bool) error { return nil } -func (r *Store) moveFromTo(ctx context.Context, subFrom *leaf.Store, from, to, fromPrefix string, srcIsDir, delete bool) error { +func (r *Store) moveFromTo(ctx context.Context, subFrom *leaf.Store, from, to, fromPrefix string, srcIsDir, dstIsDir, delete bool) error { ctx = ctxutil.WithGitCommit(ctx, false) entries := []string{from} + // if the source is a directory we enumerate all it's children + // and move them one by one. if r.IsDir(ctx, from) { var err error entries, err = subFrom.List(ctx, fromPrefix) @@ -111,6 +113,8 @@ func (r *Store) moveFromTo(ctx context.Context, subFrom *leaf.Store, from, to, f return fmt.Errorf("no entries") } + debug.Log("Moving %q to %q (entries: %+v)", from, to, entries) + for _, src := range entries { dst := to if srcIsDir { @@ -120,8 +124,12 @@ func (r *Store) moveFromTo(ctx context.Context, subFrom *leaf.Store, from, to, f } else { dst = path.Join(to, path.Base(from), strings.TrimPrefix(src, from)) } + } else { + if dstIsDir || strings.HasSuffix(to, "/") { + dst = path.Join(to, path.Base(src)) + } } - debug.Log("Moving %s (%s) => %s (%s) (sid:%t, delete:%t)\n", from, src, to, dst, srcIsDir, delete) + debug.Log("Moving %q (%q) => %q (%q) (sid:%t, did:%t, delete:%t)\n", from, src, to, dst, srcIsDir, dstIsDir, delete) content, err := r.Get(ctx, src) if err != nil { diff --git a/internal/store/root/move_test.go b/internal/store/root/move_test.go index 4d8573a99a..405b478614 100644 --- a/internal/store/root/move_test.go +++ b/internal/store/root/move_test.go @@ -13,6 +13,43 @@ import ( "github.com/stretchr/testify/require" ) +func TestMoveShadow(t *testing.T) { + u := gptest.NewUnitTester(t) + u.Entries = []string{ + "old/www/foo", + "old/www/bar", + } + + require.NoError(t, u.InitStore("")) + defer u.Remove() + + ctx := context.Background() + ctx = ctxutil.WithAlwaysYes(ctx, true) + ctx = ctxutil.WithHidden(ctx, true) + + rs, err := createRootStore(ctx, u) + require.NoError(t, err) + assert.NoError(t, rs.Delete(ctx, "foo")) + + // -> move old/www/foo www/dir/foo => OK + assert.NoError(t, rs.Move(ctx, "old/www/foo", "www/dir/foo")) + entries, err := rs.List(ctx, tree.INF) + require.NoError(t, err) + require.Equal(t, []string{ + "old/www/bar", + "www/dir/foo", + }, entries) + + // -> move old/www/bar www/ => OK + assert.NoError(t, rs.Move(ctx, "old/www/bar", "www/")) + entries, err = rs.List(ctx, tree.INF) + require.NoError(t, err) + require.Equal(t, []string{ + "www/bar", + "www/dir/foo", + }, entries) +} + func TestMove(t *testing.T) { u := gptest.NewUnitTester(t) u.Entries = []string{ @@ -39,8 +76,10 @@ func TestMove(t *testing.T) { "foo/baz", "misc/zab", }, entries) + // -> move foo/ misc/zab => ERROR: misc/zab is a file assert.Error(t, rs.Move(ctx, "foo/", "misc/zab")) + // -> move foo misc/zab => ERROR: misc/zab is a file assert.Error(t, rs.Move(ctx, "foo", "misc/zab")) @@ -125,56 +164,65 @@ func TestCopy(t *testing.T) { assert.NoError(t, rs.Delete(ctx, "foo")) // Initial state: - entries, err := rs.List(ctx, tree.INF) - require.NoError(t, err) - assert.Equal(t, []string{ - "foo/bar", - "foo/baz", - "misc/zab", - }, entries) + t.Run("initial state", func(t *testing.T) { + entries, err := rs.List(ctx, tree.INF) + require.NoError(t, err) + assert.Equal(t, []string{ + "foo/bar", + "foo/baz", + "misc/zab", + }, entries) + }) + // -> copy foo/ misc/zab => ERROR: misc/zab is a file assert.Error(t, rs.Copy(ctx, "foo/", "misc/zab")) // -> copy foo misc/zab => ERROR: misc/zab is a file assert.Error(t, rs.Copy(ctx, "foo", "misc/zab")) // -> copy foo/ misc => OK - assert.NoError(t, rs.Copy(ctx, "foo", "misc")) - entries, err = rs.List(ctx, tree.INF) - require.NoError(t, err) - assert.Equal(t, []string{ - "foo/bar", - "foo/baz", - "misc/foo/bar", - "misc/foo/baz", - "misc/zab", - }, entries) + t.Run("copy foo misc", func(t *testing.T) { + assert.NoError(t, rs.Copy(ctx, "foo", "misc")) + entries, err := rs.List(ctx, tree.INF) + require.NoError(t, err) + assert.Equal(t, []string{ + "foo/bar", + "foo/baz", + "misc/foo/bar", + "misc/foo/baz", + "misc/zab", + }, entries) + }) // -> copy misc/foo/ bar/ => OK - assert.NoError(t, rs.Copy(ctx, "misc/foo/", "bar/")) - entries, err = rs.List(ctx, tree.INF) - require.NoError(t, err) - assert.Equal(t, []string{ - "bar/bar", - "bar/baz", - "foo/bar", - "foo/baz", - "misc/foo/bar", - "misc/foo/baz", - "misc/zab", - }, entries) + t.Run("copy misc/foo/ bar/", func(t *testing.T) { + assert.NoError(t, rs.Copy(ctx, "misc/foo/", "bar/")) + entries, err := rs.List(ctx, tree.INF) + require.NoError(t, err) + assert.Equal(t, []string{ + "bar/bar", + "bar/baz", + "foo/bar", + "foo/baz", + "misc/foo/bar", + "misc/foo/baz", + "misc/zab", + }, entries) + }) // -> copy misc/zab bar/foo/zab => OK - assert.NoError(t, rs.Copy(ctx, "misc/zab", "bar/foo/zab")) - entries, err = rs.List(ctx, tree.INF) - require.NoError(t, err) - assert.Equal(t, []string{ - "bar/bar", - "bar/baz", - "bar/foo/zab", - "foo/bar", - "foo/baz", - "misc/foo/bar", - "misc/foo/baz", - "misc/zab", - }, entries) + t.Run("copy misc/zab bar/foo/zab", func(t *testing.T) { + assert.NoError(t, rs.Copy(ctx, "misc/zab", "bar/foo/zab")) + entries, err := rs.List(ctx, tree.INF) + require.NoError(t, err) + assert.Equal(t, []string{ + "bar/bar", + "bar/baz", + "bar/foo/zab", + "foo/bar", + "foo/baz", + "misc/foo/bar", + "misc/foo/baz", + "misc/zab", + }, entries) + }) }