git2go: Convert commands to return OIDs

This commit converts commands in our `internal/git2go` wrappers to
return properly typed `git.ObjectID`s instead of plain strings.

7 files changed, 53 insertions, 23 deletions

diff --git a/internal/git2go/apply.go b/internal/git2go/apply.go
index 447e92530..93c74925b 100644
--- a/internal/git2go/apply.go
+++ b/internal/git2go/apply.go
@@ -5,6 +5,8 @@ import (
 	"encoding/gob"
 	"fmt"
 	"io"
+
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 )
 
 // ErrMergeConflict is returned when there is a merge conflict.
@@ -70,7 +72,7 @@ func (iter *slicePatchIterator) Err() error { return nil }
 
 // Apply applies the provided patches and returns the OID of the commit with the patches
 // applied.
-func (b Executor) Apply(ctx context.Context, params ApplyParams) (string, error) {
+func (b Executor) Apply(ctx context.Context, params ApplyParams) (git.ObjectID, error) {
 	reader, writer := io.Pipe()
 	defer writer.Close()
 
@@ -108,5 +110,14 @@ func (b Executor) Apply(ctx context.Context, params ApplyParams) (string, error)
 		return "", fmt.Errorf("decode: %w", err)
 	}
 
-	return result.CommitID, result.Error
+	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)
+	}
+
+	return commitID, nil
 }
diff --git a/internal/git2go/apply_test.go b/internal/git2go/apply_test.go
index 6f303257c..ca140f395 100644
--- a/internal/git2go/apply_test.go
+++ b/internal/git2go/apply_test.go
@@ -60,7 +60,7 @@ func TestExecutor_Apply(t *testing.T) {
 		Author:     author,
 		Committer:  committer,
 		Message:    "commit with a",
-		Parent:     parentCommitSHA,
+		Parent:     parentCommitSHA.String(),
 		Actions:    []Action{UpdateFile{Path: "file", OID: oidA.String()}},
 	})
 	require.NoError(t, err)
@@ -70,7 +70,7 @@ func TestExecutor_Apply(t *testing.T) {
 		Author:     author,
 		Committer:  committer,
 		Message:    "commit to b",
-		Parent:     parentCommitSHA,
+		Parent:     parentCommitSHA.String(),
 		Actions:    []Action{UpdateFile{Path: "file", OID: oidB.String()}},
 	})
 	require.NoError(t, err)
@@ -80,7 +80,7 @@ func TestExecutor_Apply(t *testing.T) {
 		Author:     author,
 		Committer:  committer,
 		Message:    "commit a -> b",
-		Parent:     updateToA,
+		Parent:     updateToA.String(),
 		Actions:    []Action{UpdateFile{Path: "file", OID: oidB.String()}},
 	})
 	require.NoError(t, err)
@@ -94,16 +94,16 @@ func TestExecutor_Apply(t *testing.T) {
 	})
 	require.NoError(t, err)
 
-	diffBetween := func(t testing.TB, fromCommit, toCommit string) []byte {
+	diffBetween := func(t testing.TB, fromCommit, toCommit git.ObjectID) []byte {
 		t.Helper()
 		return testhelper.MustRunCommand(t, nil,
-			"git", "-C", repoPath, "format-patch", "--stdout", fromCommit+".."+toCommit)
+			"git", "-C", repoPath, "format-patch", "--stdout", fromCommit.String()+".."+toCommit.String())
 	}
 
 	for _, tc := range []struct {
 		desc         string
 		patches      []Patch
-		parentCommit string
+		parentCommit git.ObjectID
 		tree         []gittest.TreeEntry
 		error        error
 	}{
@@ -198,7 +198,7 @@ func TestExecutor_Apply(t *testing.T) {
 			commitID, err := executor.Apply(ctx, ApplyParams{
 				Repository:   repoPath,
 				Committer:    committer,
-				ParentCommit: parentCommitSHA,
+				ParentCommit: parentCommitSHA.String(),
 				Patches:      NewSlicePatchIterator(tc.patches),
 			})
 			if tc.error != nil {
@@ -212,7 +212,7 @@ func TestExecutor_Apply(t *testing.T) {
 				Committer: committer,
 				Message:   tc.patches[len(tc.patches)-1].Message,
 			}, getCommit(t, ctx, repo, commitID))
-			gittest.RequireTree(t, config.Config.Git.BinPath, repoPath, commitID, tc.tree)
+			gittest.RequireTree(t, config.Config.Git.BinPath, repoPath, commitID.String(), tc.tree)
 		})
 	}
 }
diff --git a/internal/git2go/cherry_pick.go b/internal/git2go/cherry_pick.go
index 71c7c6f06..fb5b9de4f 100644
--- a/internal/git2go/cherry_pick.go
+++ b/internal/git2go/cherry_pick.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"time"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 )
 
@@ -28,6 +29,6 @@ type CherryPickCommand struct {
 }
 
 // Run performs a cherry pick via gitaly-git2go.
-func (m CherryPickCommand) Run(ctx context.Context, cfg config.Cfg) (string, error) {
+func (m CherryPickCommand) Run(ctx context.Context, cfg config.Cfg) (git.ObjectID, error) {
 	return runWithGob(ctx, cfg, "cherry-pick", m)
 }
diff --git a/internal/git2go/commit.go b/internal/git2go/commit.go
index 558c82bd9..8f0f03256 100644
--- a/internal/git2go/commit.go
+++ b/internal/git2go/commit.go
@@ -5,6 +5,8 @@ import (
 	"context"
 	"encoding/gob"
 	"fmt"
+
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 )
 
 // IndexError is an error that was produced by performing an invalid operation on the index.
@@ -57,7 +59,7 @@ type CommitParams struct {
 
 // Commit builds a commit from the actions, writes it to the object database and
 // returns its object id.
-func (b Executor) Commit(ctx context.Context, params CommitParams) (string, error) {
+func (b Executor) Commit(ctx context.Context, params CommitParams) (git.ObjectID, error) {
 	input := &bytes.Buffer{}
 	if err := gob.NewEncoder(input).Encode(params); err != nil {
 		return "", err
@@ -73,5 +75,14 @@ func (b Executor) Commit(ctx context.Context, params CommitParams) (string, erro
 		return "", err
 	}
 
-	return result.CommitID, result.Error
+	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)
+	}
+
+	return commitID, nil
 }
diff --git a/internal/git2go/commit_test.go b/internal/git2go/commit_test.go
index c4c8427f7..ea6ecc5c2 100644
--- a/internal/git2go/commit_test.go
+++ b/internal/git2go/commit_test.go
@@ -33,7 +33,7 @@ func testMain(m *testing.M) int {
 }
 
 type commit struct {
-	Parent    string
+	Parent    git.ObjectID
 	Author    Signature
 	Committer Signature
 	Message   string
@@ -464,7 +464,7 @@ func TestExecutor_Commit(t *testing.T) {
 		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())
-			var parentCommit string
+			var parentCommit git.ObjectID
 			for i, step := range tc.steps {
 				message := fmt.Sprintf("commit %d", i+1)
 				commitID, err := executor.Commit(ctx, CommitParams{
@@ -472,7 +472,7 @@ func TestExecutor_Commit(t *testing.T) {
 					Author:     author,
 					Committer:  committer,
 					Message:    message,
-					Parent:     parentCommit,
+					Parent:     parentCommit.String(),
 					Actions:    step.actions,
 				})
 
@@ -490,17 +490,17 @@ func TestExecutor_Commit(t *testing.T) {
 					Message:   message,
 				}, getCommit(t, ctx, repo, commitID))
 
-				gittest.RequireTree(t, config.Config.Git.BinPath, repoPath, commitID, step.treeEntries)
+				gittest.RequireTree(t, config.Config.Git.BinPath, repoPath, commitID.String(), step.treeEntries)
 				parentCommit = commitID
 			}
 		})
 	}
 }
 
-func getCommit(t testing.TB, ctx context.Context, repo *localrepo.Repo, oid string) commit {
+func getCommit(t testing.TB, ctx context.Context, repo *localrepo.Repo, oid git.ObjectID) commit {
 	t.Helper()
 
-	data, err := repo.ReadObject(ctx, git.ObjectID(oid))
+	data, err := repo.ReadObject(ctx, oid)
 	require.NoError(t, err)
 
 	var commit commit
@@ -518,7 +518,7 @@ func getCommit(t testing.TB, ctx context.Context, repo *localrepo.Repo, oid stri
 		switch field {
 		case "parent":
 			require.Empty(t, commit.Parent, "multi parent parsing not implemented")
-			commit.Parent = value
+			commit.Parent = git.ObjectID(value)
 		case "author":
 			require.Empty(t, commit.Author, "commit contained multiple authors")
 			commit.Author = unmarshalSignature(t, value)
diff --git a/internal/git2go/gob.go b/internal/git2go/gob.go
index 4e760b437..7c4ef0554 100644
--- a/internal/git2go/gob.go
+++ b/internal/git2go/gob.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"reflect"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 )
 
@@ -86,7 +87,7 @@ func SerializableError(err error) error {
 
 // runWithGob runs the specified gitaly-git2go cmd with the request gob-encoded
 // as input and returns the commit ID as string or an error.
-func runWithGob(ctx context.Context, cfg config.Cfg, cmd string, request interface{}) (string, error) {
+func runWithGob(ctx context.Context, cfg config.Cfg, cmd string, request interface{}) (git.ObjectID, error) {
 	input := &bytes.Buffer{}
 	if err := gob.NewEncoder(input).Encode(request); err != nil {
 		return "", fmt.Errorf("%s: %w", cmd, err)
@@ -106,5 +107,10 @@ func runWithGob(ctx context.Context, cfg config.Cfg, cmd string, request interfa
 		return "", fmt.Errorf("%s: %w", cmd, result.Error)
 	}
 
-	return result.CommitID, nil
+	commitID, err := git.NewObjectIDFromHex(result.CommitID)
+	if err != nil {
+		return "", fmt.Errorf("could not parse commit ID: %w", err)
+	}
+
+	return commitID, nil
 }
diff --git a/internal/git2go/revert.go b/internal/git2go/revert.go
index 8e7af30ba..7dd1d7420 100644
--- a/internal/git2go/revert.go
+++ b/internal/git2go/revert.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"time"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 )
 
@@ -26,6 +27,6 @@ type RevertCommand struct {
 	Mainline uint `json:"mainline"`
 }
 
-func (r RevertCommand) Run(ctx context.Context, cfg config.Cfg) (string, error) {
+func (r RevertCommand) Run(ctx context.Context, cfg config.Cfg) (git.ObjectID, error) {
 	return runWithGob(ctx, cfg, "revert", r)
 }