log: add error logging

Add logging for errors returned 'fmt.Errorf()' errors as well as the errors
passed to or created by 'TraceLogger.Fatal()'/'TraceLogger.Fatalf()'.
Replace all use of 'fmt.Errorf()' or direct error returns with
'TraceLogger.Errorf()' or 'TraceLogger.Error()', which return 'error'
instances to stay consistent with what they're replacing.

These errors are logged under the "error" trace2 event. We want to capture
the error as low in the call stack as possible (to more precisely indicate
the cause), but also not log a new "error" event for every 'fmt.Errorf()'
replaced. To do that, have the error wrappers return a custom type
'loggedError', and only log the error event if the 'err' or none of the
'Errorf()' arguments are of the 'loggedError' type.

Signed-off-by: Victoria Dye <[email protected]>

Author: vdye

cmd/git-bundle-server/delete.go

@@ -36,22 +36,22 @@ func (d *deleteCmd) Run(ctx context.Context, args []string) error {
 
 	repo, err := core.CreateRepository(*route)
 	if err != nil {
-		return err
+		return d.logger.Error(ctx, err)
 	}
 
 	err = core.RemoveRoute(*route)
 	if err != nil {
-		return err
+		return d.logger.Error(ctx, err)
 	}
 
 	err = os.RemoveAll(repo.WebDir)
 	if err != nil {
-		return err
+		return d.logger.Error(ctx, err)
 	}
 
 	err = os.RemoveAll(repo.RepoDir)
 	if err != nil {
-		return err
+		return d.logger.Error(ctx, err)
 	}
 
 	return nil

cmd/git-bundle-server/init.go

@@ -40,41 +40,41 @@ func (i *initCmd) Run(ctx context.Context, args []string) error {
 
 	repo, err := core.CreateRepository(*route)
 	if err != nil {
-		return err
+		return i.logger.Error(ctx, err)
 	}
 
 	fmt.Printf("Cloning repository from %s\n", *url)
 	gitErr := git.GitCommand("clone", "--bare", *url, repo.RepoDir)
 
 	if gitErr != nil {
-		return fmt.Errorf("failed to clone repository: %w", gitErr)
+		return i.logger.Errorf(ctx, "failed to clone repository: %w", gitErr)
 	}
 
 	gitErr = git.GitCommand("-C", repo.RepoDir, "config", "remote.origin.fetch", "+refs/heads/*:refs/heads/*")
 	if gitErr != nil {
-		return fmt.Errorf("failed to configure refspec: %w", gitErr)
+		return i.logger.Errorf(ctx, "failed to configure refspec: %w", gitErr)
 	}
 
 	gitErr = git.GitCommand("-C", repo.RepoDir, "fetch", "origin")
 	if gitErr != nil {
-		return fmt.Errorf("failed to fetch latest refs: %w", gitErr)
+		return i.logger.Errorf(ctx, "failed to fetch latest refs: %w", gitErr)
 	}
 
 	bundle := bundles.CreateInitialBundle(repo)
 	fmt.Printf("Constructing base bundle file at %s\n", bundle.Filename)
 
 	written, gitErr := git.CreateBundle(repo.RepoDir, bundle.Filename)
 	if gitErr != nil {
-		return fmt.Errorf("failed to create bundle: %w", gitErr)
+		return i.logger.Errorf(ctx, "failed to create bundle: %w", gitErr)
 	}
 	if !written {
-		return fmt.Errorf("refused to write empty bundle. Is the repo empty?")
+		return i.logger.Errorf(ctx, "refused to write empty bundle. Is the repo empty?")
 	}
 
 	list := bundles.CreateSingletonList(bundle)
 	listErr := bundles.WriteBundleList(list, repo)
 	if listErr != nil {
-		return fmt.Errorf("failed to write bundle list: %w", listErr)
+		return i.logger.Errorf(ctx, "failed to write bundle list: %w", listErr)
 	}
 
 	SetCronSchedule()

cmd/git-bundle-server/start.go

@@ -2,7 +2,6 @@ package main
 
 import (
 	"context"
-	"fmt"
 	"os"
 
 	"github.com/github/git-bundle-server/internal/argparse"
@@ -38,12 +37,12 @@ func (s *startCmd) Run(ctx context.Context, args []string) error {
 	// CreateRepository registers the route.
 	repo, err := core.CreateRepository(*route)
 	if err != nil {
-		return err
+		return s.logger.Error(ctx, err)
 	}
 
 	_, err = os.ReadDir(repo.RepoDir)
 	if err != nil {
-		return fmt.Errorf("route '%s' appears to have been deleted; use 'init' instead", *route)
+		return s.logger.Errorf(ctx, "route '%s' appears to have been deleted; use 'init' instead", *route)
 	}
 
 	// Make sure we have the global schedule running.

cmd/git-bundle-server/stop.go

@@ -33,5 +33,10 @@ func (s *stopCmd) Run(ctx context.Context, args []string) error {
 	route := parser.PositionalString("route", "the route for which bundles should stop being generated")
 	parser.Parse(ctx, args)
 
-	return core.RemoveRoute(*route)
+	err := core.RemoveRoute(*route)
+	if err != nil {
+		s.logger.Error(ctx, err)
+	}
+
+	return nil
 }

cmd/git-bundle-server/update-all.go

@@ -2,7 +2,6 @@ package main
 
 import (
 	"context"
-	"fmt"
 	"os"
 	"os/exec"
 
@@ -34,7 +33,7 @@ For every configured route, run 'git-bundle-server update <options> <route>'.`
 func (u *updateAllCmd) Run(ctx context.Context, args []string) error {
 	user, err := common.NewUserProvider().CurrentUser()
 	if err != nil {
-		return err
+		return u.logger.Error(ctx, err)
 	}
 	fs := common.NewFileSystem()
 
@@ -43,12 +42,12 @@ func (u *updateAllCmd) Run(ctx context.Context, args []string) error {
 
 	exe, err := os.Executable()
 	if err != nil {
-		return fmt.Errorf("failed to get path to execuable: %w", err)
+		return u.logger.Errorf(ctx, "failed to get path to execuable: %w", err)
 	}
 
 	repos, err := core.GetRepositories(user, fs)
 	if err != nil {
-		return err
+		return u.logger.Error(ctx, err)
 	}
 
 	subargs := []string{"update", ""}
@@ -62,12 +61,12 @@ func (u *updateAllCmd) Run(ctx context.Context, args []string) error {
 
 		err := cmd.Start()
 		if err != nil {
-			return fmt.Errorf("git command failed to start: %w", err)
+			return u.logger.Errorf(ctx, "git command failed to start: %w", err)
 		}
 
 		err = cmd.Wait()
 		if err != nil {
-			return fmt.Errorf("git command returned a failure: %w", err)
+			return u.logger.Errorf(ctx, "git command returned a failure: %w", err)
 		}
 	}
 

cmd/git-bundle-server/update.go

@@ -38,18 +38,18 @@ func (u *updateCmd) Run(ctx context.Context, args []string) error {
 
 	repo, err := core.CreateRepository(*route)
 	if err != nil {
-		return err
+		return u.logger.Error(ctx, err)
 	}
 
 	list, err := bundles.GetBundleList(repo)
 	if err != nil {
-		return fmt.Errorf("failed to load bundle list: %w", err)
+		return u.logger.Errorf(ctx, "failed to load bundle list: %w", err)
 	}
 
 	fmt.Printf("Creating new incremental bundle\n")
 	bundle, err := bundles.CreateIncrementalBundle(repo, list)
 	if err != nil {
-		return err
+		return u.logger.Error(ctx, err)
 	}
 
 	// Nothing new!
@@ -62,13 +62,13 @@ func (u *updateCmd) Run(ctx context.Context, args []string) error {
 	fmt.Printf("Collapsing bundle list\n")
 	err = bundles.CollapseList(repo, list)
 	if err != nil {
-		return err
+		return u.logger.Error(ctx, err)
 	}
 
 	fmt.Printf("Writing updated bundle list\n")
 	listErr := bundles.WriteBundleList(list, repo)
 	if listErr != nil {
-		return fmt.Errorf("failed to write bundle list: %w", listErr)
+		return u.logger.Errorf(ctx, "failed to write bundle list: %w", listErr)
 	}
 
 	return nil

cmd/git-bundle-server/web-server.go

@@ -41,7 +41,7 @@ func (webServerCmd) Description() string {
 	return `Manage the web server hosting bundle content`
 }
 
-func (w *webServerCmd) getDaemonConfig() (*daemon.DaemonConfig, error) {
+func (w *webServerCmd) getDaemonConfig(ctx context.Context) (*daemon.DaemonConfig, error) {
 	// Find git-bundle-web-server
 	// First, search for it on the path
 	programPath, err := exec.LookPath("git-bundle-web-server")
@@ -50,25 +50,25 @@ func (w *webServerCmd) getDaemonConfig() (*daemon.DaemonConfig, error) {
 			// Result is a relative path
 			programPath, err = filepath.Abs(programPath)
 			if err != nil {
-				return nil, fmt.Errorf("could not get absolute path to program: %w", err)
+				return nil, w.logger.Errorf(ctx, "could not get absolute path to program: %w", err)
 			}
 		} else {
 			// Fall back on looking for it in the same directory as the currently-running executable
 			exePath, err := os.Executable()
 			if err != nil {
-				return nil, fmt.Errorf("failed to get path to current executable: %w", err)
+				return nil, w.logger.Errorf(ctx, "failed to get path to current executable: %w", err)
 			}
 			exeDir := filepath.Dir(exePath)
 			if err != nil {
-				return nil, fmt.Errorf("failed to get parent dir of current executable: %w", err)
+				return nil, w.logger.Errorf(ctx, "failed to get parent dir of current executable: %w", err)
 			}
 
 			programPath = filepath.Join(exeDir, "git-bundle-web-server")
 			programExists, err := w.fileSystem.FileExists(programPath)
 			if err != nil {
-				return nil, fmt.Errorf("could not determine whether path to 'git-bundle-web-server' exists: %w", err)
+				return nil, w.logger.Errorf(ctx, "could not determine whether path to 'git-bundle-web-server' exists: %w", err)
 			} else if !programExists {
-				return nil, fmt.Errorf("could not find path to 'git-bundle-web-server'")
+				return nil, w.logger.Errorf(ctx, "could not find path to 'git-bundle-web-server'")
 			}
 		}
 	}
@@ -99,12 +99,12 @@ func (w *webServerCmd) startServer(ctx context.Context, args []string) error {
 
 	d, err := daemon.NewDaemonProvider(w.logger, w.user, w.cmdExec, w.fileSystem)
 	if err != nil {
-		return err
+		return w.logger.Error(ctx, err)
 	}
 
-	config, err := w.getDaemonConfig()
+	config, err := w.getDaemonConfig(ctx)
 	if err != nil {
-		return err
+		return w.logger.Error(ctx, err)
 	}
 
 	// Configure flags
@@ -131,17 +131,17 @@ func (w *webServerCmd) startServer(ctx context.Context, args []string) error {
 	})
 	if loopErr != nil {
 		// Error happened in 'Visit'
-		return loopErr
+		return w.logger.Error(ctx, loopErr)
 	}
 
 	err = d.Create(ctx, config, *force)
 	if err != nil {
-		return err
+		return w.logger.Error(ctx, err)
 	}
 
 	err = d.Start(ctx, config.Label)
 	if err != nil {
-		return err
+		return w.logger.Error(ctx, err)
 	}
 
 	return nil
@@ -155,23 +155,23 @@ func (w *webServerCmd) stopServer(ctx context.Context, args []string) error {
 
 	d, err := daemon.NewDaemonProvider(w.logger, w.user, w.cmdExec, w.fileSystem)
 	if err != nil {
-		return err
+		return w.logger.Error(ctx, err)
 	}
 
-	config, err := w.getDaemonConfig()
+	config, err := w.getDaemonConfig(ctx)
 	if err != nil {
-		return err
+		return w.logger.Error(ctx, err)
 	}
 
 	err = d.Stop(ctx, config.Label)
 	if err != nil {
-		return err
+		return w.logger.Error(ctx, err)
 	}
 
 	if *remove {
 		err = d.Remove(ctx, config.Label)
 		if err != nil {
-			return err
+			return w.logger.Error(ctx, err)
 		}
 	}
 

internal/argparse/argparse.go

@@ -145,6 +145,7 @@ func (a *argParser) Parse(ctx context.Context, args []string) {
 	if err != nil {
 		// The error was already printed (via a.FlagSet.Usage()), so we
 		// just need to exit
+		a.logger.Error(ctx, err)
 		a.logger.Exit(ctx, usageExitCode)
 	}
 
@@ -221,5 +222,6 @@ func (a *argParser) Usage(ctx context.Context, errFmt string, args ...any) {
 	fmt.Fprintf(a.FlagSet.Output(), errFmt+"\n", args...)
 	a.FlagSet.Usage()
 
+	a.logger.Errorf(ctx, errFmt, args...)
 	a.logger.Exit(ctx, usageExitCode)
 }