Merge branch 'jf_stop_using_stderr' into 'master'

gitaly-git2go: Stop using stderr

See merge request gitlab-org/gitaly!4343

16 files changed, 54 insertions, 85 deletions

diff --git a/cmd/gitaly-git2go/apply.go b/cmd/gitaly-git2go/apply.go
index 876bd314b..0c863d82c 100644
--- a/cmd/gitaly-git2go/apply.go
+++ b/cmd/gitaly-git2go/apply.go
@@ -53,9 +53,7 @@ func (cmd *applySubcommand) Flags() *flag.FlagSet {
 }
 
 // Run runs the subcommand.
-func (cmd *applySubcommand) Run(ctx context.Context, stdin io.Reader, stdout io.Writer) error {
-	decoder := gob.NewDecoder(stdin)
-
+func (cmd *applySubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var params git2go.ApplyParams
 	if err := decoder.Decode(&params); err != nil {
 		return fmt.Errorf("decode params: %w", err)
@@ -63,9 +61,8 @@ func (cmd *applySubcommand) Run(ctx context.Context, stdin io.Reader, stdout io.
 
 	params.Patches = &patchIterator{decoder: decoder}
 	commitID, err := cmd.apply(ctx, params)
-	return gob.NewEncoder(stdout).Encode(git2go.Result{
+	return encoder.Encode(git2go.Result{
 		CommitID: commitID,
-		Error:    git2go.SerializableError(err), // Set both fields for backwards compatibility.
 		Err:      git2go.SerializableError(err),
 	})
 }
diff --git a/cmd/gitaly-git2go/cherry_pick.go b/cmd/gitaly-git2go/cherry_pick.go
index 7180b10b3..0125e73e0 100644
--- a/cmd/gitaly-git2go/cherry_pick.go
+++ b/cmd/gitaly-git2go/cherry_pick.go
@@ -9,7 +9,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"io"
 
 	git "github.com/libgit2/git2go/v33"
 	"gitlab.com/gitlab-org/gitaly/v14/cmd/gitaly-git2go/git2goutil"
@@ -22,16 +21,15 @@ func (cmd *cherryPickSubcommand) Flags() *flag.FlagSet {
 	return flag.NewFlagSet("cherry-pick", flag.ExitOnError)
 }
 
-func (cmd *cherryPickSubcommand) Run(ctx context.Context, r io.Reader, w io.Writer) error {
+func (cmd *cherryPickSubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var request git2go.CherryPickCommand
-	if err := gob.NewDecoder(r).Decode(&request); err != nil {
+	if err := decoder.Decode(&request); err != nil {
 		return err
 	}
 
 	commitID, err := cmd.cherryPick(ctx, &request)
-	return gob.NewEncoder(w).Encode(git2go.Result{
+	return encoder.Encode(git2go.Result{
 		CommitID: commitID,
-		Error:    git2go.SerializableError(err), // Set both fields for backwards compatibility.
 		Err:      git2go.SerializableError(err),
 	})
 }
diff --git a/cmd/gitaly-git2go/commit.go b/cmd/gitaly-git2go/commit.go
index 33a8ff9a2..0f6b69cdb 100644
--- a/cmd/gitaly-git2go/commit.go
+++ b/cmd/gitaly-git2go/commit.go
@@ -5,8 +5,8 @@ package main
 
 import (
 	"context"
+	"encoding/gob"
 	"flag"
-	"io"
 
 	"gitlab.com/gitlab-org/gitaly/v14/cmd/gitaly-git2go/commit"
 )
@@ -15,6 +15,6 @@ type commitSubcommand struct{}
 
 func (commitSubcommand) Flags() *flag.FlagSet { return flag.NewFlagSet("commit", flag.ExitOnError) }
 
-func (commitSubcommand) Run(ctx context.Context, stdin io.Reader, stdout io.Writer) error {
-	return commit.Run(ctx, stdin, stdout)
+func (commitSubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
+	return commit.Run(ctx, decoder, encoder)
 }
diff --git a/cmd/gitaly-git2go/commit/commit.go b/cmd/gitaly-git2go/commit/commit.go
index 58bb1a51f..0e62eee7c 100644
--- a/cmd/gitaly-git2go/commit/commit.go
+++ b/cmd/gitaly-git2go/commit/commit.go
@@ -8,7 +8,6 @@ import (
 	"encoding/gob"
 	"errors"
 	"fmt"
-	"io"
 
 	git "github.com/libgit2/git2go/v33"
 	"gitlab.com/gitlab-org/gitaly/v14/cmd/gitaly-git2go/git2goutil"
@@ -16,16 +15,15 @@ import (
 )
 
 // Run runs the commit subcommand.
-func Run(ctx context.Context, stdin io.Reader, stdout io.Writer) error {
+func Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var params git2go.CommitParams
-	if err := gob.NewDecoder(stdin).Decode(&params); err != nil {
+	if err := decoder.Decode(&params); err != nil {
 		return err
 	}
 
 	commitID, err := commit(ctx, params)
-	return gob.NewEncoder(stdout).Encode(git2go.Result{
+	return encoder.Encode(git2go.Result{
 		CommitID: commitID,
-		Error:    git2go.SerializableError(err), // Set both fields for backwards compatibility.
 		Err:      git2go.SerializableError(err),
 	})
 }
diff --git a/cmd/gitaly-git2go/conflicts.go b/cmd/gitaly-git2go/conflicts.go
index 0ce3cd623..7ac05fa57 100644
--- a/cmd/gitaly-git2go/conflicts.go
+++ b/cmd/gitaly-git2go/conflicts.go
@@ -9,7 +9,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"io"
 
 	git "github.com/libgit2/git2go/v33"
 	"gitlab.com/gitlab-org/gitaly/v14/cmd/gitaly-git2go/git2goutil"
@@ -25,13 +24,13 @@ func (cmd *conflictsSubcommand) Flags() *flag.FlagSet {
 	return flag.NewFlagSet("conflicts", flag.ExitOnError)
 }
 
-func (cmd *conflictsSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) error {
+func (cmd *conflictsSubcommand) Run(_ context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var request git2go.ConflictsCommand
-	if err := gob.NewDecoder(r).Decode(&request); err != nil {
+	if err := decoder.Decode(&request); err != nil {
 		return err
 	}
 	res := cmd.conflicts(request)
-	return gob.NewEncoder(w).Encode(res)
+	return encoder.Encode(res)
 }
 
 func (conflictsSubcommand) conflicts(request git2go.ConflictsCommand) git2go.ConflictsResult {
@@ -160,8 +159,7 @@ func conflictError(code codes.Code, message string) git2go.ConflictsResult {
 		Message: message,
 	}
 	return git2go.ConflictsResult{
-		Error: err, // Set both fields for backwards compatibility.
-		Err:   err,
+		Err: err,
 	}
 }
 
diff --git a/cmd/gitaly-git2go/main.go b/cmd/gitaly-git2go/main.go
index 08652080f..1bbdf6e20 100644
--- a/cmd/gitaly-git2go/main.go
+++ b/cmd/gitaly-git2go/main.go
@@ -5,9 +5,9 @@ package main
 
 import (
 	"context"
+	"encoding/gob"
 	"flag"
 	"fmt"
-	"io"
 	"os"
 
 	git "github.com/libgit2/git2go/v33"
@@ -16,7 +16,7 @@ import (
 
 type subcmd interface {
 	Flags() *flag.FlagSet
-	Run(ctx context.Context, stdin io.Reader, stdout io.Writer) error
+	Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error
 }
 
 var subcommands = map[string]subcmd{
@@ -31,41 +31,51 @@ var subcommands = map[string]subcmd{
 	"submodule":   &submoduleSubcommand{},
 }
 
-func fatalf(format string, args ...interface{}) {
-	fmt.Fprintf(os.Stderr, format+"\n", args...)
-	os.Exit(1)
+func fatalf(encoder *gob.Encoder, format string, args ...interface{}) {
+	// Once logging has been implemented these encoding errors can be logged.
+	// Until then these will be ignored as we can no longer use stderr.
+	// https://gitlab.com/gitlab-org/gitaly/-/issues/3229
+	_ = encoder.Encode(git2go.Result{
+		Err: git2go.SerializableError(fmt.Errorf(format, args...)),
+	})
+	// An exit code of 1 would indicate an error over stderr. Since our errors
+	// are encoded over gob, we need to exit cleanly
+	os.Exit(0)
 }
 
 func main() {
-	flags := flag.NewFlagSet(git2go.BinaryName, flag.ExitOnError)
+	decoder := gob.NewDecoder(os.Stdin)
+	encoder := gob.NewEncoder(os.Stdout)
+
+	flags := flag.NewFlagSet(git2go.BinaryName, flag.ContinueOnError)
 
 	if err := flags.Parse(os.Args); err != nil {
-		fatalf("parsing flags: %s", err)
+		fatalf(encoder, "parsing flags: %s", err)
 	}
 
 	if flags.NArg() < 2 {
-		fatalf("missing subcommand")
+		fatalf(encoder, "missing subcommand")
 	}
 
 	subcmd, ok := subcommands[flags.Arg(1)]
 	if !ok {
-		fatalf("unknown subcommand: %q", flags.Arg(1))
+		fatalf(encoder, "unknown subcommand: %q", flags.Arg(0))
 	}
 
 	subcmdFlags := subcmd.Flags()
 	if err := subcmdFlags.Parse(flags.Args()[2:]); err != nil {
-		fatalf("parsing flags of %q: %s", subcmdFlags.Name(), err)
+		fatalf(encoder, "parsing flags of %q: %s", subcmdFlags.Name(), err)
 	}
 
 	if subcmdFlags.NArg() != 0 {
-		fatalf("%s: trailing arguments", subcmdFlags.Name())
+		fatalf(encoder, "%s: trailing arguments", subcmdFlags.Name())
 	}
 
 	if err := git.EnableFsyncGitDir(true); err != nil {
-		fatalf("enable fsync: %s", err)
+		fatalf(encoder, "enable fsync: %s", err)
 	}
 
-	if err := subcmd.Run(context.Background(), os.Stdin, os.Stdout); err != nil {
-		fatalf("%s: %s", subcmdFlags.Name(), err)
+	if err := subcmd.Run(context.Background(), decoder, encoder); err != nil {
+		fatalf(encoder, "%s: %s", subcmdFlags.Name(), err)
 	}
 }
diff --git a/cmd/gitaly-git2go/merge.go b/cmd/gitaly-git2go/merge.go
index 6a0b66607..541cecf84 100644
--- a/cmd/gitaly-git2go/merge.go
+++ b/cmd/gitaly-git2go/merge.go
@@ -9,7 +9,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"io"
 	"time"
 
 	git "github.com/libgit2/git2go/v33"
@@ -24,9 +23,9 @@ func (cmd *mergeSubcommand) Flags() *flag.FlagSet {
 	return flags
 }
 
-func (cmd *mergeSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) error {
+func (cmd *mergeSubcommand) Run(_ context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var request git2go.MergeCommand
-	if err := gob.NewDecoder(r).Decode(&request); err != nil {
+	if err := decoder.Decode(&request); err != nil {
 		return err
 	}
 
@@ -36,9 +35,8 @@ func (cmd *mergeSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) err
 
 	commitID, err := merge(request)
 
-	return gob.NewEncoder(w).Encode(git2go.Result{
+	return encoder.Encode(git2go.Result{
 		CommitID: commitID,
-		Error:    git2go.SerializableError(err), // Set both fields for backwards compatibility.
 		Err:      git2go.SerializableError(err),
 	})
 }
diff --git a/cmd/gitaly-git2go/rebase.go b/cmd/gitaly-git2go/rebase.go
index 133ec204f..8d6d44010 100644
--- a/cmd/gitaly-git2go/rebase.go
+++ b/cmd/gitaly-git2go/rebase.go
@@ -9,7 +9,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"io"
 
 	git "github.com/libgit2/git2go/v33"
 	"gitlab.com/gitlab-org/gitaly/v14/cmd/gitaly-git2go/git2goutil"
@@ -22,16 +21,15 @@ func (cmd *rebaseSubcommand) Flags() *flag.FlagSet {
 	return flag.NewFlagSet("rebase", flag.ExitOnError)
 }
 
-func (cmd *rebaseSubcommand) Run(ctx context.Context, r io.Reader, w io.Writer) error {
+func (cmd *rebaseSubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var request git2go.RebaseCommand
-	if err := gob.NewDecoder(r).Decode(&request); err != nil {
+	if err := decoder.Decode(&request); err != nil {
 		return err
 	}
 
 	commitID, err := cmd.rebase(ctx, &request)
-	return gob.NewEncoder(w).Encode(git2go.Result{
+	return encoder.Encode(git2go.Result{
 		CommitID: commitID,
-		Error:    git2go.SerializableError(err), // Set both fields for backwards compatibility.
 		Err:      git2go.SerializableError(err),
 	})
 }
diff --git a/cmd/gitaly-git2go/resolve_conflicts.go b/cmd/gitaly-git2go/resolve_conflicts.go
index eaf53eec3..6314e6a87 100644
--- a/cmd/gitaly-git2go/resolve_conflicts.go
+++ b/cmd/gitaly-git2go/resolve_conflicts.go
@@ -10,7 +10,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"io"
 	"strings"
 	"time"
 
@@ -26,9 +25,9 @@ func (cmd *resolveSubcommand) Flags() *flag.FlagSet {
 	return flag.NewFlagSet("resolve", flag.ExitOnError)
 }
 
-func (cmd resolveSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) error {
+func (cmd resolveSubcommand) Run(_ context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var request git2go.ResolveCommand
-	if err := gob.NewDecoder(r).Decode(&request); err != nil {
+	if err := decoder.Decode(&request); err != nil {
 		return err
 	}
 
@@ -205,7 +204,7 @@ func (cmd resolveSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) er
 		},
 	}
 
-	return gob.NewEncoder(w).Encode(response)
+	return encoder.Encode(response)
 }
 
 func readConflictEntries(odb *git.Odb, c git.IndexConflict) (*conflict.Entry, *conflict.Entry, *conflict.Entry, error) {
diff --git a/cmd/gitaly-git2go/revert.go b/cmd/gitaly-git2go/revert.go
index 134eed9c7..fdefa1fd2 100644
--- a/cmd/gitaly-git2go/revert.go
+++ b/cmd/gitaly-git2go/revert.go
@@ -9,7 +9,6 @@ import (
 	"errors"
 	"flag"
 	"fmt"
-	"io"
 
 	git "github.com/libgit2/git2go/v33"
 	"gitlab.com/gitlab-org/gitaly/v14/cmd/gitaly-git2go/git2goutil"
@@ -22,16 +21,15 @@ func (cmd *revertSubcommand) Flags() *flag.FlagSet {
 	return flag.NewFlagSet("revert", flag.ExitOnError)
 }
 
-func (cmd *revertSubcommand) Run(ctx context.Context, r io.Reader, w io.Writer) error {
+func (cmd *revertSubcommand) Run(ctx context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var request git2go.RevertCommand
-	if err := gob.NewDecoder(r).Decode(&request); err != nil {
+	if err := decoder.Decode(&request); err != nil {
 		return err
 	}
 
 	commitID, err := cmd.revert(ctx, &request)
-	return gob.NewEncoder(w).Encode(git2go.Result{
+	return encoder.Encode(git2go.Result{
 		CommitID: commitID,
-		Error:    git2go.SerializableError(err), // Set both fields for backwards compatibility.
 		Err:      git2go.SerializableError(err),
 	})
 }
diff --git a/cmd/gitaly-git2go/submodule.go b/cmd/gitaly-git2go/submodule.go
index dd0963cfb..ef993fd21 100644
--- a/cmd/gitaly-git2go/submodule.go
+++ b/cmd/gitaly-git2go/submodule.go
@@ -8,7 +8,6 @@ import (
 	"encoding/gob"
 	"flag"
 	"fmt"
-	"io"
 	"time"
 
 	git "github.com/libgit2/git2go/v33"
@@ -22,10 +21,10 @@ func (cmd *submoduleSubcommand) Flags() *flag.FlagSet {
 	return flag.NewFlagSet("submodule", flag.ExitOnError)
 }
 
-func (cmd *submoduleSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) error {
+func (cmd *submoduleSubcommand) Run(_ context.Context, decoder *gob.Decoder, encoder *gob.Encoder) error {
 	var request git2go.SubmoduleCommand
 
-	if err := gob.NewDecoder(r).Decode(&request); err != nil {
+	if err := decoder.Decode(&request); err != nil {
 		return fmt.Errorf("deserializing submodule command request: %w", err)
 	}
 
@@ -34,7 +33,7 @@ func (cmd *submoduleSubcommand) Run(_ context.Context, r io.Reader, w io.Writer)
 		return err
 	}
 
-	return gob.NewEncoder(w).Encode(res)
+	return encoder.Encode(res)
 }
 
 func (cmd *submoduleSubcommand) run(request git2go.SubmoduleCommand) (*git2go.SubmoduleResult, error) {
diff --git a/internal/git2go/apply.go b/internal/git2go/apply.go
index a2c29afbe..d14062deb 100644
--- a/internal/git2go/apply.go
+++ b/internal/git2go/apply.go
@@ -118,10 +118,6 @@ func (b *Executor) Apply(ctx context.Context, repo repository.GitRepo, params Ap
 		return "", result.Err
 	}
 
-	if result.Error != nil {
-		return "", result.Error
-	}
-
 	commitID, err := git.NewObjectIDFromHex(result.CommitID)
 	if err != nil {
 		return "", fmt.Errorf("could not parse commit ID: %w", err)
diff --git a/internal/git2go/commit.go b/internal/git2go/commit.go
index 2bed2dad9..b8aeda71c 100644
--- a/internal/git2go/commit.go
+++ b/internal/git2go/commit.go
@@ -80,10 +80,6 @@ func (b *Executor) Commit(ctx context.Context, repo repository.GitRepo, params C
 		return "", result.Err
 	}
 
-	if result.Error != nil {
-		return "", result.Error
-	}
-
 	commitID, err := git.NewObjectIDFromHex(result.CommitID)
 	if err != nil {
 		return "", fmt.Errorf("could not parse commit ID: %w", err)
diff --git a/internal/git2go/conflicts.go b/internal/git2go/conflicts.go
index 994cfc0dc..650057c59 100644
--- a/internal/git2go/conflicts.go
+++ b/internal/git2go/conflicts.go
@@ -58,10 +58,6 @@ func (e ConflictError) Error() string {
 type ConflictsResult struct {
 	// Conflicts
 	Conflicts []Conflict
-	// Error is an optional conflict error
-	//
-	// Deprecated: Use Err instead. Error clashes with the Result error field.
-	Error ConflictError
 	// Err is set if an error occurred. Err must exist on all gob serialized
 	// results so that any error can be returned.
 	//
@@ -100,9 +96,6 @@ func (b *Executor) Conflicts(ctx context.Context, repo repository.GitRepo, c Con
 		return ConflictsResult{}, result.Err
 	}
 
-	if result.Error.Code != codes.OK {
-		return ConflictsResult{}, status.Error(result.Error.Code, result.Error.Message)
-	}
 	return result, nil
 }
 
diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go
index 7ede19b35..35b62b682 100644
--- a/internal/git2go/executor.go
+++ b/internal/git2go/executor.go
@@ -90,10 +90,6 @@ func (b *Executor) runWithGob(ctx context.Context, repo repository.GitRepo, cmd
 		return "", fmt.Errorf("%s: %w", cmd, result.Err)
 	}
 
-	if result.Error != nil {
-		return "", fmt.Errorf("%s: %w", cmd, result.Error)
-	}
-
 	commitID, err := git.NewObjectIDFromHex(result.CommitID)
 	if err != nil {
 		return "", fmt.Errorf("could not parse commit ID: %w", err)
diff --git a/internal/git2go/serialization.go b/internal/git2go/serialization.go
index 6c0b2f0b0..b4967ca25 100644
--- a/internal/git2go/serialization.go
+++ b/internal/git2go/serialization.go
@@ -35,11 +35,6 @@ var registeredTypes = map[reflect.Type]struct{}{
 type Result struct {
 	// CommitID is the result of the call.
 	CommitID string
-	// Error is set if the call errord.
-	//
-	// Deprecated: Use Err instead. Error clashes with other serialized types
-	// where Error is not of type error
-	Error error
 	// Err is set if an error occurred. Err must exist on all gob serialized
 	// results so that all errors can be returned.
 	Err error