Refactor git2go commit and apply tests

- It uses repo.ReadCommit instead of parsing on its own
- Provides interface for testing other signatures (like SSH)

8 files changed, 69 insertions, 160 deletions

diff --git a/Makefile b/Makefile
index cf2d2a8d8..68ff2b3d9 100644
--- a/Makefile
+++ b/Makefile
@@ -253,7 +253,7 @@ find_go_sources              = $(shell find ${SOURCE_DIR} -type d \( -path "${SO
 run_go_tests = PATH='${SOURCE_DIR}/internal/testhelper/testdata/home/bin:${PATH}' \
     TEST_TMP_DIR='${TEST_TMP_DIR}' \
     TEST_LOG_DIR='${TEST_LOG_DIR}' \
-    ${GOTESTSUM} --format ${TEST_FORMAT} --junitfile ${TEST_REPORT} --jsonfile ${TEST_FULL_OUTPUT} -- -ldflags '${GO_LDFLAGS}' -tags '${SERVER_BUILD_TAGS},${GIT2GO_BUILD_TAGS},gitaly_test_signing' ${TEST_OPTIONS} ${TEST_PACKAGES}
+    ${GOTESTSUM} --format ${TEST_FORMAT} --junitfile ${TEST_REPORT} --jsonfile ${TEST_FULL_OUTPUT} -- -ldflags '${GO_LDFLAGS}' -tags '${SERVER_BUILD_TAGS},${GIT2GO_BUILD_TAGS}' ${TEST_OPTIONS} ${TEST_PACKAGES}
 
 ## Test options passed to `dlv test`.
 DEBUG_OPTIONS      ?= $(patsubst -%,-test.%,${TEST_OPTIONS})
diff --git a/cmd/gitaly-git2go/git2goutil/sign.go b/cmd/gitaly-git2go/git2goutil/sign.go
index b8437d9ad..a4c1771b8 100644
--- a/cmd/gitaly-git2go/git2goutil/sign.go
+++ b/cmd/gitaly-git2go/git2goutil/sign.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_signing
-
 package git2goutil
 
 import (
diff --git a/cmd/gitaly-git2go/git2goutil/test_sign.go b/cmd/gitaly-git2go/git2goutil/test_sign.go
deleted file mode 100644
index 990344910..000000000
--- a/cmd/gitaly-git2go/git2goutil/test_sign.go
+++ /dev/null
@@ -1,49 +0,0 @@
-//go:build gitaly_test_signing
-
-package git2goutil
-
-import (
-	"bytes"
-	"fmt"
-	"os"
-	"strings"
-	"time"
-
-	"github.com/ProtonMail/go-crypto/openpgp"
-	"github.com/ProtonMail/go-crypto/openpgp/packet"
-)
-
-// CreateCommitSignature reads the given signing key and produces PKCS#7 detached signature.
-// When the path to the signing key is not present, an empty signature is returned.
-// Test version creates deterministic signature which is the same with the same data every run.
-func CreateCommitSignature(signingKeyPath, contentToSign string) (string, error) {
-	if signingKeyPath == "" {
-		return "", nil
-	}
-
-	file, err := os.Open(signingKeyPath)
-	if err != nil {
-		return "", fmt.Errorf("open file: %w", err)
-	}
-
-	entity, err := openpgp.ReadEntity(packet.NewReader(file))
-	if err != nil {
-		return "", fmt.Errorf("read entity: %w", err)
-	}
-
-	sigBuf := new(bytes.Buffer)
-	if err := openpgp.ArmoredDetachSignText(
-		sigBuf,
-		entity,
-		strings.NewReader(contentToSign),
-		&packet.Config{
-			Time: func() time.Time {
-				return time.Date(2022, 8, 20, 11, 22, 33, 0, time.UTC)
-			},
-		},
-	); err != nil {
-		return "", fmt.Errorf("sign commit: %w", err)
-	}
-
-	return sigBuf.String(), nil
-}
diff --git a/internal/git2go/apply_test.go b/internal/git2go/apply_test.go
index e92f90a6d..b841a3ec0 100644
--- a/internal/git2go/apply_test.go
+++ b/internal/git2go/apply_test.go
@@ -6,7 +6,6 @@ import (
 	"errors"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
@@ -38,8 +37,8 @@ func TestExecutor_Apply(t *testing.T) {
 	oidB, err := repo.WriteBlob(ctx, "file", strings.NewReader("b"))
 	require.NoError(t, err)
 
-	author := NewSignature("Test Author", "test.author@example.com", time.Now())
-	committer := NewSignature("Test Committer", "test.committer@example.com", time.Now())
+	author := defaultCommitAuthorSignature()
+	committer := defaultCommitAuthorSignature()
 
 	parentCommitSHA, err := executor.Commit(ctx, repo, CommitCommand{
 		Repository: repoPath,
@@ -209,12 +208,14 @@ func TestExecutor_Apply(t *testing.T) {
 				return
 			}
 
-			require.Equal(t, commit{
-				Parent:    tc.parentCommit,
-				Author:    author,
-				Committer: committer,
-				Message:   tc.patches[len(tc.patches)-1].Message,
-			}, getCommit(t, ctx, repo, commitID, false))
+			commit, err := repo.ReadCommit(ctx, commitID.Revision())
+			require.NoError(t, err)
+
+			require.Equal(t, []string{string(tc.parentCommit)}, commit.ParentIds)
+			require.Equal(t, gittest.DefaultCommitAuthor, commit.Author)
+			require.Equal(t, gittest.DefaultCommitAuthor, commit.Committer)
+			require.Equal(t, []byte(tc.patches[len(tc.patches)-1].Message), commit.Body)
+
 			gittest.RequireTree(t, cfg, repoPath, commitID.String(), tc.tree)
 		})
 	}
diff --git a/internal/git2go/commit_test.go b/internal/git2go/commit_test.go
index d153425a2..264416ac9 100644
--- a/internal/git2go/commit_test.go
+++ b/internal/git2go/commit_test.go
@@ -8,10 +8,8 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"strconv"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/ProtonMail/go-crypto/openpgp"
 	"github.com/ProtonMail/go-crypto/openpgp/packet"
@@ -28,13 +26,6 @@ func TestMain(m *testing.M) {
 	testhelper.Run(m)
 }
 
-type commit struct {
-	Parent    git.ObjectID
-	Author    Signature
-	Committer Signature
-	Message   string
-}
-
 func TestExecutor_Commit(t *testing.T) {
 	const (
 		DefaultMode    = "100644"
@@ -45,6 +36,7 @@ func TestExecutor_Commit(t *testing.T) {
 		actions     []Action
 		error       error
 		treeEntries []gittest.TreeEntry
+		testCommit  func(testing.TB, git.ObjectID)
 	}
 	ctx := testhelper.Context(t)
 
@@ -66,9 +58,9 @@ func TestExecutor_Commit(t *testing.T) {
 	executor := NewExecutor(cfg, gittest.NewCommandFactory(t, cfg), config.NewLocator(cfg))
 
 	for _, tc := range []struct {
-		desc          string
-		steps         []step
-		signAndVerify bool
+		desc           string
+		steps          []step
+		signingKeyPath string
 	}{
 		{
 			desc: "create directory",
@@ -460,7 +452,7 @@ func TestExecutor_Commit(t *testing.T) {
 			},
 		},
 		{
-			desc: "update created file, sign commit and verify signature",
+			desc: "update created file, sign commit and verify signature using GPG signing key",
 			steps: []step{
 				{
 					actions: []Action{
@@ -470,20 +462,37 @@ func TestExecutor_Commit(t *testing.T) {
 					treeEntries: []gittest.TreeEntry{
 						{Mode: DefaultMode, Path: "file", Content: "updated"},
 					},
+					testCommit: func(tb testing.TB, commitID git.ObjectID) {
+						gpgsig, dataWithoutGpgSig := extractSignature(t, ctx, repo, commitID)
+
+						file, err := os.Open("testdata/signing_gpg_key.pub")
+						require.NoError(tb, err)
+						defer testhelper.MustClose(tb, file)
+
+						keyring, err := openpgp.ReadKeyRing(file)
+						require.NoError(tb, err)
+
+						_, err = openpgp.CheckArmoredDetachedSignature(
+							keyring,
+							strings.NewReader(dataWithoutGpgSig),
+							strings.NewReader(gpgsig),
+							&packet.Config{},
+						)
+						require.NoError(tb, err)
+					},
 				},
 			},
-			signAndVerify: true,
+			signingKeyPath: "testdata/signing_gpg_key",
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			author := NewSignature("Author Name", "author.email@example.com", time.Now())
-			committer := NewSignature("Committer Name", "committer.email@example.com", time.Now())
+			author := defaultCommitAuthorSignature()
+			committer := defaultCommitAuthorSignature()
 
-			if tc.signAndVerify {
-				executor.signingKey = testhelper.SigningKeyPath
-			}
+			executor.signingKey = tc.signingKeyPath
 
 			var parentCommit git.ObjectID
+			var parentIds []string
 			for i, step := range tc.steps {
 				message := fmt.Sprintf("commit %d", i+1)
 				commitID, err := executor.Commit(ctx, repo, CommitCommand{
@@ -502,109 +511,61 @@ func TestExecutor_Commit(t *testing.T) {
 					require.NoError(t, err)
 				}
 
-				require.Equal(t, commit{
-					Parent:    parentCommit,
-					Author:    author,
-					Committer: committer,
-					Message:   message,
-				}, getCommit(t, ctx, repo, commitID, tc.signAndVerify))
+				commit, err := repo.ReadCommit(ctx, commitID.Revision())
+				require.NoError(t, err)
+
+				require.Equal(t, parentIds, commit.ParentIds)
+				require.Equal(t, gittest.DefaultCommitAuthor, commit.Author)
+				require.Equal(t, gittest.DefaultCommitAuthor, commit.Committer)
+				require.Equal(t, []byte(message), commit.Body)
+
+				if step.testCommit != nil {
+					step.testCommit(t, commitID)
+				}
 
 				gittest.RequireTree(t, cfg, repoPath, commitID.String(), step.treeEntries)
 				parentCommit = commitID
+				parentIds = []string{string(commitID)}
 			}
 		})
 	}
 }
 
-func getCommit(tb testing.TB, ctx context.Context, repo *localrepo.Repo, oid git.ObjectID, verifySignature bool) commit {
-	tb.Helper()
-
+func extractSignature(tb testing.TB, ctx context.Context, repo *localrepo.Repo, oid git.ObjectID) (string, string) {
 	data, err := repo.ReadObject(ctx, oid)
 	require.NoError(tb, err)
 
+	const gpgSignaturePrefix = "gpgsig "
 	var (
 		gpgsig, dataWithoutGpgSig string
-		gpgsigStarted             bool
+		inSignature               bool
 	)
 
-	var commit commit
 	lines := strings.Split(string(data), "\n")
+
 	for i, line := range lines {
 		if line == "" {
-			commit.Message = strings.Join(lines[i+1:], "\n")
-			dataWithoutGpgSig += "\n" + commit.Message
+			dataWithoutGpgSig += "\n" + strings.Join(lines[i+1:], "\n")
 			break
 		}
 
-		if gpgsigStarted && strings.HasPrefix(line, " ") {
-			gpgsig += strings.TrimSpace(line) + "\n"
-			continue
-		}
-
-		split := strings.SplitN(line, " ", 2)
-		require.Len(tb, split, 2, "invalid commit: %q", data)
-
-		field, value := split[0], split[1]
-
-		if field != "gpgsig" {
+		if strings.HasPrefix(line, gpgSignaturePrefix) {
+			inSignature = true
+			gpgsig += strings.TrimPrefix(line, gpgSignaturePrefix)
+		} else if inSignature {
+			gpgsig += "\n" + strings.TrimPrefix(line, " ")
+		} else {
 			dataWithoutGpgSig += line + "\n"
 		}
-
-		switch field {
-		case "parent":
-			require.Empty(tb, commit.Parent, "multi parent parsing not implemented")
-			commit.Parent = git.ObjectID(value)
-		case "author":
-			require.Empty(tb, commit.Author, "commit contained multiple authors")
-			commit.Author = unmarshalSignature(tb, value)
-		case "committer":
-			require.Empty(tb, commit.Committer, "commit contained multiple committers")
-			commit.Committer = unmarshalSignature(tb, value)
-		case "gpgsig":
-			gpgsig = value + "\n"
-			gpgsigStarted = true
-		default:
-		}
-	}
-
-	if gpgsig != "" || verifySignature {
-		file, err := os.Open("testdata/publicKey.gpg")
-		require.NoError(tb, err)
-
-		keyring, err := openpgp.ReadKeyRing(file)
-		require.NoError(tb, err)
-
-		_, err = openpgp.CheckArmoredDetachedSignature(
-			keyring,
-			strings.NewReader(dataWithoutGpgSig),
-			strings.NewReader(gpgsig),
-			&packet.Config{},
-		)
-		require.NoError(tb, err)
 	}
 
-	return commit
+	return gpgsig, dataWithoutGpgSig
 }
 
-func unmarshalSignature(tb testing.TB, data string) Signature {
-	tb.Helper()
-
-	// Format: NAME <EMAIL> DATE_UNIX DATE_TIMEZONE
-	split1 := strings.Split(data, " <")
-	require.Len(tb, split1, 2, "invalid signature: %q", data)
-
-	split2 := strings.Split(split1[1], "> ")
-	require.Len(tb, split2, 2, "invalid signature: %q", data)
-
-	split3 := strings.Split(split2[1], " ")
-	require.Len(tb, split3, 2, "invalid signature: %q", data)
-
-	timestamp, err := strconv.ParseInt(split3[0], 10, 64)
-	require.NoError(tb, err)
-
-	return Signature{
-		Name:  split1[0],
-		Email: split2[0],
-		When:  time.Unix(timestamp, 0),
-	}
+func defaultCommitAuthorSignature() Signature {
+	return NewSignature(
+		gittest.DefaultCommitterName,
+		gittest.DefaultCommitterMail,
+		gittest.DefaultCommitTime,
+	)
 }
diff --git a/internal/git2go/testdata/signingKey.gpg b/internal/git2go/testdata/signing_gpg_key
index 841fbc76a..841fbc76a 100644
--- a/internal/git2go/testdata/signingKey.gpg
+++ b/internal/git2go/testdata/signing_gpg_key
diff --git a/internal/git2go/testdata/publicKey.gpg b/internal/git2go/testdata/signing_gpg_key.pub
index 98d02a71e..98d02a71e 100644
--- a/internal/git2go/testdata/publicKey.gpg
+++ b/internal/git2go/testdata/signing_gpg_key.pub
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index db19842c9..aeea988b7 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -40,8 +40,6 @@ const (
 	RepositoryAuthToken = "the-secret-token"
 	// DefaultStorageName is the default name of the Gitaly storage.
 	DefaultStorageName = "default"
-	// SigningKeyPath is the default path to test commit signing.
-	SigningKeyPath = "testdata/signingKey.gpg"
 )
 
 // IsPraefectEnabled returns whether this testing run is done with Praefect in front of the Gitaly.