Improve convert output (#2171)

RELEASE_NOTES=[BUGFIX] Improve convert output

Fixes #2170

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

internal/action/commands.go

@@ -175,9 +175,8 @@ func (s *Action) GetCommands() []*cli.Command {
 			Hidden:      true,
 			Flags: []cli.Flag{
 				&cli.StringFlag{
-					Name:     "store",
-					Usage:    "Specify which store to convert",
-					Required: true,
+					Name:  "store",
+					Usage: "Specify which store to convert",
 				},
 				&cli.BoolFlag{
 					Name:  "move",

internal/action/convert.go

@@ -1,26 +1,91 @@
 package action
 
 import (
+	"fmt"
+
 	"github.com/gopasspw/gopass/internal/backend"
+	"github.com/gopasspw/gopass/internal/backend/crypto/age"
+	"github.com/gopasspw/gopass/internal/out"
 	"github.com/gopasspw/gopass/pkg/ctxutil"
+	"github.com/gopasspw/gopass/pkg/termio"
 	"github.com/urfave/cli/v2"
 )
 
 // Convert converts a store to a different set of backends.
 func (s *Action) Convert(c *cli.Context) error {
 	ctx := ctxutil.WithGlobalFlags(c)
+	ctx = age.WithOnlyNative(ctx, true)
 
 	store := c.String("store")
 	move := c.Bool("move")
-	storage, err := backend.StorageRegistry.Backend(c.String("storage"))
+
+	sub, err := s.Store.GetSubStore(store)
 	if err != nil {
-		return err
+		return fmt.Errorf("mount %q not found: %w", store, err)
+	}
+
+	oldStorage := sub.Storage().Name()
+
+	storage, err := backend.StorageRegistry.Backend(oldStorage)
+	if err != nil {
+		return fmt.Errorf("unknown storage backend %q: %w", oldStorage, err)
+	}
+	if sv := c.String("storage"); sv != "" {
+		var err error
+		storage, err = backend.StorageRegistry.Backend(sv)
+		if err != nil {
+			return fmt.Errorf("unknown storage backend %q: %w", sv, err)
+		}
+	}
+
+	oldCrypto := sub.Crypto().Name()
+
+	crypto, err := backend.CryptoRegistry.Backend(oldCrypto)
+	if err != nil {
+		return fmt.Errorf("unknown crypto backend %q: %w", oldCrypto, err)
+	}
+	if sv := c.String("crypto"); sv != "" {
+		var err error
+		crypto, err = backend.CryptoRegistry.Backend(sv)
+		if err != nil {
+			return fmt.Errorf("unknown crypto backend %q: %w", sv, err)
+		}
+	}
+
+	if oldCrypto == crypto.String() && oldStorage == storage.String() {
+		out.Notice(ctx, "No conversion needed")
+
+		return nil
+	}
+
+	if oldCrypto != crypto.String() {
+		cbe, err := backend.NewCrypto(ctx, crypto)
+		if err != nil {
+			return err
+		}
+
+		if err := s.initCheckPrivateKeys(ctx, cbe); err != nil {
+			return err
+		}
+		out.Printf(ctx, "Crypto %q has private keys", crypto.String())
 	}
 
-	crypto, err := backend.CryptoRegistry.Backend(c.String("crypto"))
+	out.Noticef(ctx, "Converting %q. Crypto: %q -> %q, Storage: %q -> %q", store, oldCrypto, crypto, oldStorage, storage)
+	ok, err := termio.AskForBool(ctx, "Continue?", false)
 	if err != nil {
 		return err
 	}
+	if !ok {
+		out.Notice(ctx, "Aborted")
+
+		return nil
+	}
+
+	if err := s.Store.Convert(ctx, store, crypto, storage, move); err != nil {
+		return fmt.Errorf("failed to convert %q: %w", store, err)
+	}
+
+	out.OKf(ctx, "Successfully converted %q", store)
 
-	return s.Store.Convert(ctx, store, crypto, storage, move)
+	return nil
 }

internal/action/setup.go

@@ -73,6 +73,24 @@ func (s *Action) Setup(c *cli.Context) error {
 	}
 	debug.Log("Crypto Backend initialized as: %s", crypto.Name())
 
+	if err := s.initCheckPrivateKeys(ctx, crypto); err != nil {
+		return fmt.Errorf("failed to check private keys: %w", err)
+	}
+
+	// if a git remote and a team name are given attempt unattended team setup.
+	if remote != "" && team != "" {
+		if create {
+			return s.initCreateTeam(ctx, team, remote)
+		}
+
+		return s.initJoinTeam(ctx, team, remote)
+	}
+
+	// assume local setup by default, remotes can be added easily later.
+	return s.initLocal(ctx)
+}
+
+func (s *Action) initCheckPrivateKeys(ctx context.Context, crypto backend.Crypto) error {
 	// check for existing GPG/Age keypairs (private/secret keys). We need at least
 	// one useable key pair. If none exists try to create one.
 	if !s.initHasUseablePrivateKeys(ctx, crypto) {
@@ -88,17 +106,7 @@ func (s *Action) Setup(c *cli.Context) error {
 
 	debug.Log("We have useable private keys")
 
-	// if a git remote and a team name are given attempt unattended team setup.
-	if remote != "" && team != "" {
-		if create {
-			return s.initCreateTeam(ctx, team, remote)
-		}
-
-		return s.initJoinTeam(ctx, team, remote)
-	}
-
-	// assume local setup by default, remotes can be added easily later.
-	return s.initLocal(ctx)
+	return nil
 }
 
 func (s *Action) initGenerateIdentity(ctx context.Context, crypto backend.Crypto, name, email string) error {

internal/backend/storage/gitfs/git.go

@@ -154,7 +154,7 @@ func (g *Git) Cmd(ctx context.Context, name string, args ...string) error {
 
 // Name returns git.
 func (g *Git) Name() string {
-	return "git"
+	return name
 }
 
 // Version returns the git version as major, minor and patch level.

internal/backend/storage/gitfs/git_test.go

@@ -58,7 +58,7 @@ func TestGit(t *testing.T) { //nolint:paralleltest
 		git, err := New(gitdir)
 		require.NoError(t, err)
 		require.NotNil(t, git)
-		assert.Equal(t, "git", git.Name())
+		assert.Equal(t, "gitfs", git.Name())
 		assert.NoError(t, git.AddRemote(ctx, "foo", "file:///tmp/foo"))
 		assert.NoError(t, git.RemoveRemote(ctx, "foo"))
 		assert.Error(t, git.RemoveRemote(ctx, "foo"))
@@ -68,7 +68,7 @@ func TestGit(t *testing.T) { //nolint:paralleltest
 		git, err := Clone(ctx, gitdir, gitdir2, "", "")
 		require.NoError(t, err)
 		require.NotNil(t, git)
-		assert.Equal(t, "git", git.Name())
+		assert.Equal(t, "gitfs", git.Name())
 
 		tf := filepath.Join(gitdir2, "some-other-file")
 		require.NoError(t, os.WriteFile(tf, []byte("foobar"), 0o644))

internal/store/leaf/convert.go

@@ -12,15 +12,28 @@ import (
 	"github.com/gopasspw/gopass/internal/backend"
 	"github.com/gopasspw/gopass/internal/cui"
 	"github.com/gopasspw/gopass/internal/out"
+	"github.com/gopasspw/gopass/internal/queue"
 	"github.com/gopasspw/gopass/pkg/ctxutil"
 	"github.com/gopasspw/gopass/pkg/debug"
+	"github.com/gopasspw/gopass/pkg/fsutil"
 	"github.com/gopasspw/gopass/pkg/termio"
 )
 
 // Convert will convert an existing store to a new store with possibly
 // different set of crypto and storage backends. Please note that it
 // will happily convert to the same set of backends if requested.
 func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, storageBe backend.StorageBackend, move bool) error {
+	// use a temp queue so we can flush it before removing the old store
+	q := queue.New(ctx)
+	ctx = queue.WithQueue(ctx, q)
+
+	// remove any previous attempts
+	if pDir := filepath.Join(filepath.Dir(s.path), filepath.Base(s.path)+"-autoconvert"); fsutil.IsDir(pDir) {
+		if err := os.RemoveAll(pDir); err != nil {
+			return fmt.Errorf("failed to remove previous attempt %q: %w", pDir, err)
+		}
+	}
+
 	// create temp path
 	tmpPath := s.path + "-autoconvert"
 	if err := os.MkdirAll(tmpPath, 0o700); err != nil {
@@ -44,8 +57,6 @@ func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, sto
 
 	debug.Log("initialized Crypto %s", crypto)
 
-	// TODO(gh-2170) need to initialize recipients
-
 	tmpStore := &Store{
 		alias:   s.alias,
 		path:    tmpPath,
@@ -86,20 +97,6 @@ func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, sto
 		if err != nil {
 			return err
 		}
-		if len(revs) < 2 {
-			debug.Log("entry %s has no revisions. convering latest", e)
-			sec, err := s.Get(ctx, e)
-			if err != nil {
-				return err
-			}
-
-			if err := tmpStore.Set(ctx, e, sec); err != nil {
-				return err
-			}
-
-			continue
-		}
-
 		sort.Sort(sort.Reverse(backend.Revisions(revs)))
 
 		for _, r := range revs {
@@ -127,14 +124,26 @@ func (s *Store) Convert(ctx context.Context, cryptoBe backend.CryptoBackend, sto
 	}
 	bar.Done()
 
+	// flush queue
+	_ = q.Close(ctx)
+
 	if !move {
 		return nil
 	}
 
+	// remove any previous backups
+	bDir := filepath.Join(filepath.Dir(s.path), filepath.Base(s.path)+"-backup")
+	if fsutil.IsDir(bDir) {
+		if err := os.RemoveAll(bDir); err != nil {
+			debug.Log("failed to remove previous backup %q: %s", bDir, err)
+		}
+	}
+
 	// rename old to backup
-	if err := os.Rename(s.path, filepath.Join(filepath.Dir(s.path), filepath.Base(s.path)+"-backup")); err != nil {
+	if err := os.Rename(s.path, bDir); err != nil {
 		return err
 	}
+
 	// rename temp to old
 	return os.Rename(tmpPath, s.path)
 }

internal/store/leaf/fsck.go

@@ -178,5 +178,8 @@ func compareStringSlices(want, have []string) ([]string, []string) {
 		}
 	}
 
+	sort.Strings(missing)
+	sort.Strings(extra)
+
 	return missing, extra
 }

internal/store/root/convert.go

@@ -2,6 +2,7 @@ package root
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/gopasspw/gopass/internal/backend"
 	"github.com/gopasspw/gopass/pkg/debug"
@@ -12,13 +13,13 @@ import (
 func (r *Store) Convert(ctx context.Context, name string, cryptoBe backend.CryptoBackend, storageBe backend.StorageBackend, move bool) error {
 	sub, err := r.GetSubStore(name)
 	if err != nil {
-		return err
+		return fmt.Errorf("mount not found: %w", err)
 	}
 
 	debug.Log("converting %s to crypto: %s, rcs: %s, storage: %s", name, cryptoBe, storageBe)
 
 	if err := sub.Convert(ctx, cryptoBe, storageBe, move); err != nil {
-		return err
+		return fmt.Errorf("failed to convert %q: %w", name, err)
 	}
 
 	if name == "" {

main_test.go

@@ -62,7 +62,6 @@ var commandsWithError = set.Map([]string{
 	".audit",
 	".cat",
 	".clone",
-	".convert",
 	".copy",
 	".create",
 	".delete",