Merge branch 'ps-remove-custom-errors' into 'master'

Custom error replacement

See merge request gitlab-org/gitaly!2082

9 files changed, 50 insertions, 62 deletions

diff --git a/internal/cache/walker.go b/internal/cache/walker.go
index d69f29fa0..bd7c06457 100644
--- a/internal/cache/walker.go
+++ b/internal/cache/walker.go
@@ -139,6 +139,18 @@ func moveAndClear(storagePath string) error {
 		return err
 	}
 
+	defer func() {
+		dontpanic.Go(func() {
+			start := time.Now()
+			if err := os.RemoveAll(tmpDir); err != nil {
+				logger.Errorf("unable to remove disk cache objects: %q", err)
+				return
+			}
+
+			logger.Infof("cleared all cache object files in %s after %s", tmpDir, time.Since(start))
+		})
+	}()
+
 	logger.Infof("moving disk cache object folder to %s", tmpDir)
 	cachePath := tempdir.AppendCacheDir(storagePath)
 	if err := os.Rename(cachePath, filepath.Join(tmpDir, "moved")); err != nil {
@@ -150,15 +162,6 @@ func moveAndClear(storagePath string) error {
 		return err
 	}
 
-	dontpanic.Go(func() {
-		start := time.Now()
-		if err := os.RemoveAll(tmpDir); err != nil {
-			logger.Errorf("unable to remove disk cache objects: %q", err)
-		}
-
-		logger.Infof("cleared all cache object files in %s after %s", tmpDir, time.Since(start))
-	})
-
 	return nil
 }
 
diff --git a/internal/cache/walker_test.go b/internal/cache/walker_test.go
index ef7ba95f9..dd95f945f 100644
--- a/internal/cache/walker_test.go
+++ b/internal/cache/walker_test.go
@@ -128,7 +128,6 @@ func pollCountersUntil(t testing.TB, expectRemovals int) {
 		if cache.ExportMockRemovalCounter.Count() == expectRemovals {
 			break
 		}
-		t.Log(cache.ExportMockRemovalCounter.Count())
 		time.Sleep(time.Millisecond)
 	}
 }
diff --git a/internal/command/command.go b/internal/command/command.go
index 3477c33ea..0baa29e42 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -142,9 +142,7 @@ func WaitAllDone() {
 	wg.Wait()
 }
 
-type spawnTimeoutError struct{ error }
 type contextWithoutDonePanic string
-type nullInArgvError struct{ error }
 
 // noopWriteCloser has a noop Close(). The reason for this is so we can close any WriteClosers that get
 // passed into writeLines. We need this for WriteClosers such as the Logrus writer, which has a
@@ -420,7 +418,7 @@ func checkNullArgv(cmd *exec.Cmd) error {
 	for _, arg := range cmd.Args {
 		if strings.IndexByte(arg, 0) > -1 {
 			// Use %q so that the null byte gets printed as \x00
-			return nullInArgvError{fmt.Errorf("detected null byte in command argument %q", arg)}
+			return fmt.Errorf("detected null byte in command argument %q", arg)
 		}
 	}
 
diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index 782635242..e16c7a7cd 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -156,9 +156,7 @@ wait:
 
 	require.True(t, timePassed, "time must have passed")
 	require.Error(t, err)
-
-	_, ok := err.(spawnTimeoutError)
-	require.True(t, ok, "type of error should be spawnTimeoutError")
+	require.Contains(t, err.Error(), "process spawn timed out after")
 }
 
 func TestNewCommandWithSetupStdin(t *testing.T) {
@@ -188,9 +186,7 @@ func TestNewCommandNullInArg(t *testing.T) {
 
 	_, err := New(ctx, exec.Command("sh", "-c", "hello\x00world"), nil, nil, nil)
 	require.Error(t, err)
-
-	_, ok := err.(nullInArgvError)
-	require.True(t, ok, "expected %+v to be nullInArgvError", err)
+	require.EqualError(t, err, `detected null byte in command argument "hello\x00world"`)
 }
 
 func TestCommandStdErr(t *testing.T) {
diff --git a/internal/command/spawntoken.go b/internal/command/spawntoken.go
index 8c5b21783..623d686d8 100644
--- a/internal/command/spawntoken.go
+++ b/internal/command/spawntoken.go
@@ -68,7 +68,7 @@ func getSpawnToken(ctx context.Context) (putToken func(), err error) {
 		logTime(ctx, start, "spawn token timeout")
 		spawnTimeoutCount.Inc()
 
-		return nil, spawnTimeoutError{fmt.Errorf("process spawn timed out after %v", spawnConfig.Timeout)}
+		return nil, fmt.Errorf("process spawn timed out after %v", spawnConfig.Timeout)
 	case <-ctx.Done():
 		return nil, ctx.Err()
 	}
diff --git a/internal/config/config.go b/internal/config/config.go
index b07945472..aa252ceed 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -253,15 +253,20 @@ func validateStorages() error {
 
 	for i, storage := range Config.Storages {
 		if storage.Name == "" {
-			return fmt.Errorf("empty storage name in %v", storage)
+			return fmt.Errorf("empty storage name in %+v", storage)
 		}
 
 		if storage.Path == "" {
-			return fmt.Errorf("empty storage path in %v", storage)
+			return fmt.Errorf("empty storage path in %+v", storage)
 		}
 
-		if fs, err := os.Stat(storage.Path); err != nil || !fs.IsDir() {
-			return fmt.Errorf("storage paths have to exist %v", storage)
+		fs, err := os.Stat(storage.Path)
+		if err != nil {
+			return fmt.Errorf("storage %+v path must exist: %w", storage, err)
+		}
+
+		if !fs.IsDir() {
+			return fmt.Errorf("storage %+v path must be a dir", storage)
 		}
 
 		stPath := filepath.Clean(storage.Path)
diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go
index eaee4f5be..dc1885e48 100644
--- a/internal/git/safecmd.go
+++ b/internal/git/safecmd.go
@@ -2,6 +2,7 @@ package git
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"regexp"
@@ -12,12 +13,17 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 )
 
-var invalidationTotal = prometheus.NewCounterVec(
-	prometheus.CounterOpts{
-		Name: "gitaly_invalid_commands_total",
-		Help: "Total number of invalid arguments tried to execute",
-	},
-	[]string{"command"},
+var (
+	invalidationTotal = prometheus.NewCounterVec(
+		prometheus.CounterOpts{
+			Name: "gitaly_invalid_commands_total",
+			Help: "Total number of invalid arguments tried to execute",
+		},
+		[]string{"command"},
+	)
+
+	// ErrInvalidArg represent family of errors to report about bad argument used to make a call.
+	ErrInvalidArg = errors.New("invalid argument")
 )
 
 func init() {
@@ -52,9 +58,7 @@ func (sc SubCmd) ValidateArgs() ([]string, error) {
 	var safeArgs []string
 
 	if !subCmdNameRegex.MatchString(sc.Name) {
-		return nil, &invalidArgErr{
-			msg: fmt.Sprintf("invalid sub command name %q", sc.Name),
-		}
+		return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Name, ErrInvalidArg)
 	}
 	safeArgs = append(safeArgs, sc.Name)
 
@@ -102,9 +106,7 @@ func (SubSubCmd) IsOption() {}
 // sanitary
 func (sc SubSubCmd) ValidateArgs() ([]string, error) {
 	if !subCmdNameRegex.MatchString(sc.Name) {
-		return nil, &invalidArgErr{
-			msg: fmt.Sprintf("invalid sub-sub command name %q", sc.Name),
-		}
+		return nil, fmt.Errorf("invalid sub-sub command name %q: %w", sc.Name, ErrInvalidArg)
 	}
 	return []string{sc.Name}, nil
 }
@@ -123,9 +125,7 @@ var configKeyRegex = regexp.MustCompile(`^[[:alnum:]]+[-[:alnum:]]*\.(.+\.)*[[:a
 // ValidateArgs validates the config pair args
 func (cp ConfigPair) ValidateArgs() ([]string, error) {
 	if !configKeyRegex.MatchString(cp.Key) {
-		return nil, &invalidArgErr{
-			msg: fmt.Sprintf("config key %q failed regexp validation", cp.Key),
-		}
+		return nil, fmt.Errorf("config key %q failed regexp validation: %w", cp.Key, ErrInvalidArg)
 	}
 	return []string{cp.Key, cp.Value}, nil
 }
@@ -142,9 +142,7 @@ func (Flag) IsOption() {}
 // ValidateArgs returns an error if the flag is not sanitary
 func (f Flag) ValidateArgs() ([]string, error) {
 	if !flagRegex.MatchString(f.Name) {
-		return nil, &invalidArgErr{
-			msg: fmt.Sprintf("flag %q failed regex validation", f.Name),
-		}
+		return nil, fmt.Errorf("flag %q failed regex validation: %w", f.Name, ErrInvalidArg)
 	}
 	return []string{f.Name}, nil
 }
@@ -162,32 +160,21 @@ func (ValueFlag) IsOption() {}
 // ValidateArgs returns an error if the flag is not sanitary
 func (vf ValueFlag) ValidateArgs() ([]string, error) {
 	if !flagRegex.MatchString(vf.Name) {
-		return nil, &invalidArgErr{
-			msg: fmt.Sprintf("value flag %q failed regex validation", vf.Name),
-		}
+		return nil, fmt.Errorf("value flag %q failed regex validation: %w", vf.Name, ErrInvalidArg)
 	}
 	return []string{vf.Name, vf.Value}, nil
 }
 
 var flagRegex = regexp.MustCompile(`^(-|--)[[:alnum:]]`)
 
-type invalidArgErr struct {
-	msg string
-}
-
-func (iae *invalidArgErr) Error() string { return iae.msg }
-
 // IsInvalidArgErr relays if the error is due to an argument validation failure
 func IsInvalidArgErr(err error) bool {
-	_, ok := err.(*invalidArgErr)
-	return ok
+	return errors.Is(err, ErrInvalidArg)
 }
 
 func validatePositionalArg(arg string) error {
 	if strings.HasPrefix(arg, "-") {
-		return &invalidArgErr{
-			msg: fmt.Sprintf("positional arg %q cannot start with dash '-'", arg),
-		}
+		return fmt.Errorf("positional arg %q cannot start with dash '-': %w", arg, ErrInvalidArg)
 	}
 	return nil
 }
diff --git a/internal/git/safecmd_test.go b/internal/git/safecmd_test.go
index 8d9a508ef..6aa8676ef 100644
--- a/internal/git/safecmd_test.go
+++ b/internal/git/safecmd_test.go
@@ -80,28 +80,28 @@ func TestSafeCmdInvalidArg(t *testing.T) {
 	}{
 		{
 			subCmd: git.SubCmd{Name: "--meow"},
-			errMsg: "invalid sub command name \"--meow\"",
+			errMsg: `invalid sub command name "--meow": invalid argument`,
 		},
 		{
 			subCmd: git.SubCmd{
 				Name:  "meow",
 				Flags: []git.Option{git.Flag{"woof"}},
 			},
-			errMsg: "flag \"woof\" failed regex validation",
+			errMsg: `flag "woof" failed regex validation: invalid argument`,
 		},
 		{
 			subCmd: git.SubCmd{
 				Name: "meow",
 				Args: []string{"--tweet"},
 			},
-			errMsg: "positional arg \"--tweet\" cannot start with dash '-'",
+			errMsg: `positional arg "--tweet" cannot start with dash '-': invalid argument`,
 		},
 		{
 			subCmd: git.SubCmd{
 				Name:  "meow",
 				Flags: []git.Option{git.SubSubCmd{"-invalid"}},
 			},
-			errMsg: "invalid sub-sub command name \"-invalid\"",
+			errMsg: `invalid sub-sub command name "-invalid": invalid argument`,
 		},
 	} {
 		_, err := git.SafeCmd(
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index bdc415e1c..03712db58 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -55,6 +55,8 @@ var configureOnce sync.Once
 // terminates the program.
 func Configure() {
 	configureOnce.Do(func() {
+		gitalylog.Configure("json", "info")
+
 		config.Config.Storages = []config.Storage{
 			{Name: "default", Path: GitlabTestStoragePath()},
 		}
@@ -86,8 +88,6 @@ func Configure() {
 				log.Fatalf("error configuring tests: %v", err)
 			}
 		}
-
-		gitalylog.Configure("json", "info")
 	})
 }