git2go: Support driving merge command via gob

The gitaly-git2go merge subcommand has learned to use gob for
serialization of parameters and results. This commit now teaches the
internal package to drive the subcommand with gob.

This change is implemented behind a feature flag because of backwards
compatibility.

1 files changed, 50 insertions, 25 deletions

diff --git a/cmd/gitaly-git2go/merge_test.go b/cmd/gitaly-git2go/merge_test.go
index 270e3dae9..ef1c2100b 100644
--- a/cmd/gitaly-git2go/merge_test.go
+++ b/cmd/gitaly-git2go/merge_test.go
@@ -3,6 +3,8 @@
 package main
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"testing"
 	"time"
@@ -15,11 +17,18 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 )
 
 func TestMergeFailsWithMissingArguments(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.Git2GoMergeGob,
+	}).Run(t, testMergeFailsWithMissingArguments)
+}
+
+func testMergeFailsWithMissingArguments(t *testing.T, ctx context.Context) {
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 	executor := git2go.NewExecutor(cfg, config.NewLocator(cfg))
 
@@ -66,9 +75,6 @@ func TestMergeFailsWithMissingArguments(t *testing.T) {
 
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-
 			_, err := executor.Merge(ctx, repo, tc.request)
 			require.Error(t, err)
 			require.Equal(t, tc.expectedErr, err.Error())
@@ -77,13 +83,16 @@ func TestMergeFailsWithMissingArguments(t *testing.T) {
 }
 
 func TestMergeFailsWithInvalidRepositoryPath(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.Git2GoMergeGob,
+	}).Run(t, testMergeFailsWithInvalidRepositoryPath)
+}
+
+func testMergeFailsWithInvalidRepositoryPath(t *testing.T, ctx context.Context) {
 	cfg, repo, _ := testcfg.BuildWithRepo(t)
 	testhelper.BuildGitalyGit2Go(t, cfg)
 	executor := git2go.NewExecutor(cfg, config.NewLocator(cfg))
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	_, err := executor.Merge(ctx, repo, git2go.MergeCommand{
 		Repository: "/does/not/exist", AuthorName: "Foo", AuthorMail: "foo@example.com", Message: "Foo", Ours: "HEAD", Theirs: "HEAD",
 	})
@@ -92,14 +101,21 @@ func TestMergeFailsWithInvalidRepositoryPath(t *testing.T) {
 }
 
 func TestMergeTrees(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.Git2GoMergeGob,
+	}).Run(t, testMergeTrees)
+}
+
+func testMergeTrees(t *testing.T, ctx context.Context) {
 	testcases := []struct {
-		desc             string
-		base             map[string]string
-		ours             map[string]string
-		theirs           map[string]string
-		expected         map[string]string
-		expectedResponse git2go.MergeResult
-		expectedStderr   string
+		desc                  string
+		base                  map[string]string
+		ours                  map[string]string
+		theirs                map[string]string
+		expected              map[string]string
+		expectedResponse      git2go.MergeResult
+		expectedErr           error
+		expectedErrWithoutGob error
 	}{
 		{
 			desc: "trivial merge succeeds",
@@ -174,7 +190,9 @@ func TestMergeTrees(t *testing.T) {
 			theirs: map[string]string{
 				"1": "qux",
 			},
-			expectedStderr: "merge: could not auto-merge due to conflicts\n",
+			expectedErr: fmt.Errorf("merge: %w", git2go.SerializableError(errors.New("could not auto-merge due to conflicts"))),
+			//nolint:revive
+			expectedErrWithoutGob: errors.New("merge: could not auto-merge due to conflicts\n"),
 		},
 	}
 
@@ -190,9 +208,6 @@ func TestMergeTrees(t *testing.T) {
 		authorDate := time.Date(2020, 7, 30, 7, 45, 50, 0, time.FixedZone("UTC+2", +2*60*60))
 
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-
 			response, err := executor.Merge(ctx, repoProto, git2go.MergeCommand{
 				Repository: repoPath,
 				AuthorName: "John Doe",
@@ -203,9 +218,12 @@ func TestMergeTrees(t *testing.T) {
 				Theirs:     theirs.String(),
 			})
 
-			if tc.expectedStderr != "" {
-				require.Error(t, err)
-				require.Contains(t, err.Error(), tc.expectedStderr)
+			if tc.expectedErr != nil {
+				if featureflag.Git2GoMergeGob.IsEnabled(ctx) {
+					require.Equal(t, tc.expectedErr, err)
+				} else {
+					require.Equal(t, tc.expectedErrWithoutGob, err)
+				}
 				return
 			}
 
@@ -239,6 +257,12 @@ func TestMergeTrees(t *testing.T) {
 }
 
 func TestMerge_recursive(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.Git2GoMergeGob,
+	}).Run(t, testMergeRecursive)
+}
+
+func testMergeRecursive(t *testing.T, ctx context.Context) {
 	cfg := testcfg.Build(t)
 	testhelper.BuildGitalyGit2Go(t, cfg)
 	executor := git2go.NewExecutor(cfg, config.NewLocator(cfg))
@@ -278,9 +302,6 @@ func TestMerge_recursive(t *testing.T) {
 
 	authorDate := time.Date(2020, 7, 30, 7, 45, 50, 0, time.FixedZone("UTC+2", +2*60*60))
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	// When creating the criss-cross merges, we have been doing evil merges
 	// as each merge has applied changes from the other side while at the
 	// same time incrementing the own file contents. As we exceed the merge
@@ -308,8 +329,12 @@ func TestMerge_recursive(t *testing.T) {
 		Ours:       ours[len(ours)-1].String(),
 		Theirs:     theirs[len(theirs)-1].String(),
 	})
-	require.Error(t, err)
-	require.Equal(t, err.Error(), "merge: could not auto-merge due to conflicts\n")
+	if featureflag.Git2GoMergeGob.IsEnabled(ctx) {
+		require.Equal(t, fmt.Errorf("merge: %w", git2go.SerializableError(errors.New("could not auto-merge due to conflicts"))), err)
+	} else {
+		//nolint:revive
+		require.Equal(t, errors.New("merge: could not auto-merge due to conflicts\n"), err)
+	}
 
 	// Otherwise, if we're merging an earlier criss-cross merge which has
 	// half of the limit many criss-cross patterns, we exactly hit the