gitaly-lfs-smudge: Simplify error handling and logging

The way `smudge()` handles errors is a bit convoluted right now: in many
paths it will both log the error and return it to the caller, where the
caller should then use it as an indicator to decide whether to exit with
an error code or whether it should exit successfully.

Refactor the code so that we only log any such errors in our `main()`
function to simplify this logic and make it more Go-like. Also, now that
logging is mostly done in the `main()` function, move the initialization
of the logging system close to it.

2 files changed, 28 insertions, 28 deletions

diff --git a/cmd/gitaly-lfs-smudge/lfs_smudge.go b/cmd/gitaly-lfs-smudge/lfs_smudge.go
index 7d956aea7..75166b001 100644
--- a/cmd/gitaly-lfs-smudge/lfs_smudge.go
+++ b/cmd/gitaly-lfs-smudge/lfs_smudge.go
@@ -7,13 +7,11 @@ import (
 	"fmt"
 	"io"
 	"net/url"
-	"path/filepath"
 
 	"github.com/git-lfs/git-lfs/lfs"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config/prometheus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitlab"
-	gitalylog "gitlab.com/gitlab-org/gitaly/v15/internal/log"
 	"gitlab.com/gitlab-org/labkit/log"
 	"gitlab.com/gitlab-org/labkit/tracing"
 )
@@ -22,22 +20,7 @@ type configProvider interface {
 	Get(key string) string
 }
 
-func initLogging(p configProvider) (io.Closer, error) {
-	path := p.Get(gitalylog.GitalyLogDirEnvKey)
-	if path == "" {
-		return log.Initialize(log.WithWriter(io.Discard))
-	}
-
-	filepath := filepath.Join(path, "gitaly_lfs_smudge.log")
-
-	return log.Initialize(
-		log.WithFormatter("json"),
-		log.WithLogLevel("info"),
-		log.WithOutputName(filepath),
-	)
-}
-
-func smudge(to io.Writer, from io.Reader, cfgProvider configProvider) error {
+func smudge(to io.Writer, from io.Reader, cfgProvider configProvider) (returnedErr error) {
 	// Since the environment is sanitized at the moment, we're only
 	// using this to extract the correlation ID. The finished() call
 	// to clean up the tracing will be a NOP here.
@@ -46,19 +29,16 @@ func smudge(to io.Writer, from io.Reader, cfgProvider configProvider) error {
 
 	output, err := handleSmudge(ctx, to, from, cfgProvider)
 	if err != nil {
-		log.WithError(err).Error(err)
-		return err
+		return fmt.Errorf("smudging contents: %w", err)
 	}
 	defer func() {
-		if err := output.Close(); err != nil {
-			log.ContextLogger(ctx).WithError(err).Error("closing LFS object: %w", err)
+		if err := output.Close(); err != nil && returnedErr == nil {
+			returnedErr = fmt.Errorf("closing LFS object: %w", err)
 		}
 	}()
 
-	_, copyErr := io.Copy(to, output)
-	if copyErr != nil {
-		log.WithError(err).Error(copyErr)
-		return copyErr
+	if _, err := io.Copy(to, output); err != nil {
+		return fmt.Errorf("writing smudged contents: %w", err)
 	}
 
 	return nil
diff --git a/cmd/gitaly-lfs-smudge/main.go b/cmd/gitaly-lfs-smudge/main.go
index 314105b50..2d39f2619 100644
--- a/cmd/gitaly-lfs-smudge/main.go
+++ b/cmd/gitaly-lfs-smudge/main.go
@@ -2,7 +2,12 @@ package main
 
 import (
 	"fmt"
+	"io"
 	"os"
+	"path/filepath"
+
+	gitalylog "gitlab.com/gitlab-org/gitaly/v15/internal/log"
+	"gitlab.com/gitlab-org/labkit/log"
 )
 
 type envConfig struct{}
@@ -36,8 +41,23 @@ func main() {
 	}
 	defer closer.Close()
 
-	err = smudge(os.Stdout, os.Stdin, &envConfig{})
-	if err != nil {
+	if err := smudge(os.Stdout, os.Stdin, &envConfig{}); err != nil {
+		log.WithError(err).Error(err)
 		os.Exit(1)
 	}
 }
+
+func initLogging(p configProvider) (io.Closer, error) {
+	path := p.Get(gitalylog.GitalyLogDirEnvKey)
+	if path == "" {
+		return log.Initialize(log.WithWriter(io.Discard))
+	}
+
+	filepath := filepath.Join(path, "gitaly_lfs_smudge.log")
+
+	return log.Initialize(
+		log.WithFormatter("json"),
+		log.WithLogLevel("info"),
+		log.WithOutputName(filepath),
+	)
+}