Merge branch 'git2go_revert' into 'master'

gitaly-git2go revert command

See merge request gitlab-org/gitaly!2625

8 files changed, 555 insertions, 5 deletions

diff --git a/cmd/gitaly-git2go/main.go b/cmd/gitaly-git2go/main.go
index 2d2d1626a..c9a46a13f 100644
--- a/cmd/gitaly-git2go/main.go
+++ b/cmd/gitaly-git2go/main.go
@@ -16,6 +16,7 @@ type subcmd interface {
 var subcommands = map[string]subcmd{
 	"conflicts": &conflictsSubcommand{},
 	"merge":     &mergeSubcommand{},
+	"revert":    &revertSubcommand{},
 }
 
 const programName = "gitaly-git2go"
diff --git a/cmd/gitaly-git2go/merge.go b/cmd/gitaly-git2go/merge.go
index 97ea6f49a..3abf34e83 100644
--- a/cmd/gitaly-git2go/merge.go
+++ b/cmd/gitaly-git2go/merge.go
@@ -49,6 +49,7 @@ func (cmd *mergeSubcommand) Run() error {
 	if err != nil {
 		return fmt.Errorf("could not open repository: %w", err)
 	}
+	defer repo.Free()
 
 	ours, err := lookupCommit(repo, request.Ours)
 	if err != nil {
@@ -64,6 +65,7 @@ func (cmd *mergeSubcommand) Run() error {
 	if err != nil {
 		return fmt.Errorf("could not merge commits: %w", err)
 	}
+	defer index.Free()
 
 	if index.HasConflicts() {
 		return errors.New("could not auto-merge due to conflicts")
diff --git a/cmd/gitaly-git2go/merge_test.go b/cmd/gitaly-git2go/merge_test.go
index c5ae2a5d9..c201560a0 100644
--- a/cmd/gitaly-git2go/merge_test.go
+++ b/cmd/gitaly-git2go/merge_test.go
@@ -82,9 +82,10 @@ func TestMergeFailsWithInvalidRepositoryPath(t *testing.T) {
 	require.Contains(t, err.Error(), "merge: could not open repository")
 }
 
-func buildCommit(t *testing.T, repoPath string, parent *git.Oid, fileContents map[string]string) *git.Oid {
+func buildCommit(t testing.TB, repoPath string, parent *git.Oid, fileContents map[string]string) *git.Oid {
 	repo, err := git.OpenRepository(repoPath)
 	require.NoError(t, err)
+	defer repo.Free()
 
 	odb, err := repo.Odb()
 	require.NoError(t, err)
@@ -230,16 +231,17 @@ func TestMergeTrees(t *testing.T) {
 			}.Run(ctx, config.Config)
 
 			if tc.expectedStderr != "" {
-				assert.Error(t, err)
-				assert.Contains(t, err.Error(), tc.expectedStderr)
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.expectedStderr)
 				return
 			}
 
-			assert.NoError(t, err)
+			require.NoError(t, err)
 			assert.Equal(t, tc.expectedResponse, response)
 
 			repo, err := git.OpenRepository(repoPath)
 			require.NoError(t, err)
+			defer repo.Free()
 
 			commitOid, err := git.NewOid(response.CommitID)
 			require.NoError(t, err)
@@ -249,7 +251,7 @@ func TestMergeTrees(t *testing.T) {
 
 			tree, err := commit.Tree()
 			require.NoError(t, err)
-			require.Equal(t, uint64(len(tc.expected)), tree.EntryCount())
+			require.EqualValues(t, len(tc.expected), tree.EntryCount())
 
 			for name, contents := range tc.expected {
 				entry := tree.EntryByName(name)
diff --git a/cmd/gitaly-git2go/revert.go b/cmd/gitaly-git2go/revert.go
new file mode 100644
index 000000000..c1a4d4920
--- /dev/null
+++ b/cmd/gitaly-git2go/revert.go
@@ -0,0 +1,78 @@
+// +build static,system_libgit2
+
+package main
+
+import (
+	"errors"
+	"flag"
+	"fmt"
+	"os"
+
+	git "github.com/libgit2/git2go/v30"
+	"gitlab.com/gitlab-org/gitaly/internal/git2go"
+)
+
+type revertSubcommand struct {
+	request string
+}
+
+func (cmd *revertSubcommand) Flags() *flag.FlagSet {
+	flags := flag.NewFlagSet("revert", flag.ExitOnError)
+	flags.StringVar(&cmd.request, "request", "", "git2go.RevertCommand")
+	return flags
+}
+
+func (cmd *revertSubcommand) Run() error {
+	request, err := git2go.RevertCommandFromSerialized(cmd.request)
+	if err != nil {
+		return err
+	}
+
+	repo, err := git.OpenRepository(request.Repository)
+	if err != nil {
+		return fmt.Errorf("open repository: %w", err)
+	}
+	defer repo.Free()
+
+	ours, err := lookupCommit(repo, request.Ours)
+	if err != nil {
+		return fmt.Errorf("ours commit lookup: %w", err)
+	}
+
+	revert, err := lookupCommit(repo, request.Revert)
+	if err != nil {
+		return fmt.Errorf("revert commit lookup: %w", err)
+	}
+
+	index, err := repo.RevertCommit(revert, ours, request.Mainline, nil)
+	if err != nil {
+		return fmt.Errorf("revert: %w", err)
+	}
+	defer index.Free()
+
+	if index.HasConflicts() {
+		return errors.New("could not revert due to conflicts")
+	}
+
+	tree, err := index.WriteTreeTo(repo)
+	if err != nil {
+		return fmt.Errorf("write tree: %w", err)
+	}
+
+	committer := git.Signature{
+		Name:  sanitizeSignatureInfo(request.AuthorName),
+		Email: sanitizeSignatureInfo(request.AuthorMail),
+		When:  request.AuthorDate,
+	}
+
+	commit, err := repo.CreateCommitFromIds("", &committer, &committer, request.Message, tree, ours.Id())
+	if err != nil {
+		return fmt.Errorf("create revert commit: %w", err)
+	}
+
+	response := git2go.RevertResult{
+		CommitID: commit.String(),
+	}
+
+	return response.SerializeTo(os.Stdout)
+}
diff --git a/cmd/gitaly-git2go/revert_test.go b/cmd/gitaly-git2go/revert_test.go
new file mode 100644
index 000000000..d624f5784
--- /dev/null
+++ b/cmd/gitaly-git2go/revert_test.go
@@ -0,0 +1,206 @@
+// +build static,system_libgit2
+
+package main
+
+import (
+	"testing"
+	"time"
+
+	git "github.com/libgit2/git2go/v30"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/git2go"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+)
+
+func TestRevert_validation(t *testing.T) {
+	_, repoPath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	testcases := []struct {
+		desc        string
+		request     git2go.RevertCommand
+		expectedErr string
+	}{
+		{
+			desc:        "no arguments",
+			expectedErr: "revert: invalid parameters: missing repository",
+		},
+		{
+			desc:        "missing repository",
+			request:     git2go.RevertCommand{AuthorName: "Foo", AuthorMail: "foo@example.com", Message: "Foo", Ours: "HEAD", Revert: "HEAD"},
+			expectedErr: "revert: invalid parameters: missing repository",
+		},
+		{
+			desc:        "missing author name",
+			request:     git2go.RevertCommand{Repository: repoPath, AuthorMail: "foo@example.com", Message: "Foo", Ours: "HEAD", Revert: "HEAD"},
+			expectedErr: "revert: invalid parameters: missing author name",
+		},
+		{
+			desc:        "missing author mail",
+			request:     git2go.RevertCommand{Repository: repoPath, AuthorName: "Foo", Message: "Foo", Ours: "HEAD", Revert: "HEAD"},
+			expectedErr: "revert: invalid parameters: missing author mail",
+		},
+		{
+			desc:        "missing message",
+			request:     git2go.RevertCommand{Repository: repoPath, AuthorName: "Foo", AuthorMail: "foo@example.com", Ours: "HEAD", Revert: "HEAD"},
+			expectedErr: "revert: invalid parameters: missing message",
+		},
+		{
+			desc:        "missing ours",
+			request:     git2go.RevertCommand{Repository: repoPath, AuthorName: "Foo", AuthorMail: "foo@example.com", Message: "Foo", Revert: "HEAD"},
+			expectedErr: "revert: invalid parameters: missing ours",
+		},
+		{
+			desc:        "missing revert",
+			request:     git2go.RevertCommand{Repository: repoPath, AuthorName: "Foo", AuthorMail: "foo@example.com", Message: "Foo", Ours: "HEAD"},
+			expectedErr: "revert: invalid parameters: missing revert",
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			_, err := tc.request.Run(ctx, config.Config)
+			require.Error(t, err)
+			require.Equal(t, tc.expectedErr, err.Error())
+		})
+	}
+}
+
+func TestRevert_trees(t *testing.T) {
+	testcases := []struct {
+		desc             string
+		setupRepo        func(t testing.TB, repoPath string) (ours, revert string)
+		expected         map[string]string
+		expectedResponse git2go.RevertResult
+		expectedStderr   string
+	}{
+		{
+			desc: "trivial revert succeeds",
+			setupRepo: func(t testing.TB, repoPath string) (ours, revert string) {
+				baseOid := buildCommit(t, repoPath, nil, map[string]string{
+					"a": "apple",
+					"b": "banana",
+				})
+				revertOid := buildCommit(t, repoPath, baseOid, map[string]string{
+					"a": "apple",
+					"b": "pineapple",
+				})
+				oursOid := buildCommit(t, repoPath, revertOid, map[string]string{
+					"a": "apple",
+					"b": "pineapple",
+					"c": "carrot",
+				})
+
+				return oursOid.String(), revertOid.String()
+			},
+			expected: map[string]string{
+				"a": "apple",
+				"b": "banana",
+				"c": "carrot",
+			},
+			expectedResponse: git2go.RevertResult{
+				CommitID: "0be709b57f18ad3f592a8cb7c57f920861392573",
+			},
+		},
+		{
+			desc: "conflicting revert fails",
+			setupRepo: func(t testing.TB, repoPath string) (ours, revert string) {
+				baseOid := buildCommit(t, repoPath, nil, map[string]string{
+					"a": "apple",
+				})
+				revertOid := buildCommit(t, repoPath, baseOid, map[string]string{
+					"a": "pineapple",
+				})
+				oursOid := buildCommit(t, repoPath, revertOid, map[string]string{
+					"a": "carrot",
+				})
+
+				return oursOid.String(), revertOid.String()
+			},
+			expectedStderr: "revert: could not revert due to conflicts\n",
+		},
+		{
+			desc: "nonexistent ours fails",
+			setupRepo: func(t testing.TB, repoPath string) (ours, revert string) {
+				revertOid := buildCommit(t, repoPath, nil, map[string]string{
+					"a": "apple",
+				})
+
+				return "nonexistent", revertOid.String()
+			},
+			expectedStderr: "revert: ours commit lookup: could not lookup reference: revspec 'nonexistent' not found\n",
+		},
+		{
+			desc: "nonexistent revert fails",
+			setupRepo: func(t testing.TB, repoPath string) (ours, revert string) {
+				oursOid := buildCommit(t, repoPath, nil, map[string]string{
+					"a": "apple",
+				})
+
+				return oursOid.String(), "nonexistent"
+			},
+			expectedStderr: "revert: revert commit lookup: could not lookup reference: revspec 'nonexistent' not found\n",
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.desc, func(t *testing.T) {
+			_, repoPath, cleanup := testhelper.NewTestRepo(t)
+			defer cleanup()
+
+			ours, revert := tc.setupRepo(t, repoPath)
+
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			authorDate := time.Date(2020, 7, 30, 7, 45, 50, 0, time.FixedZone("UTC+2", +2*60*60))
+
+			request := git2go.RevertCommand{
+				Repository: repoPath,
+				AuthorName: "Foo",
+				AuthorMail: "foo@example.com",
+				AuthorDate: authorDate,
+				Message:    "Foo",
+				Ours:       ours,
+				Revert:     revert,
+			}
+
+			response, err := request.Run(ctx, config.Config)
+
+			if tc.expectedStderr != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.expectedStderr)
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tc.expectedResponse, response)
+
+			repo, err := git.OpenRepository(repoPath)
+			require.NoError(t, err)
+			defer repo.Free()
+
+			commitOid, err := git.NewOid(response.CommitID)
+			require.NoError(t, err)
+
+			commit, err := repo.LookupCommit(commitOid)
+			require.NoError(t, err)
+
+			tree, err := commit.Tree()
+			require.NoError(t, err)
+			require.EqualValues(t, len(tc.expected), tree.EntryCount())
+
+			for name, contents := range tc.expected {
+				entry := tree.EntryByName(name)
+				require.NotNil(t, entry)
+
+				blob, err := repo.LookupBlob(entry.Id)
+				require.NoError(t, err)
+				require.Equal(t, []byte(contents), blob.Contents())
+			}
+		})
+	}
+}
diff --git a/internal/git2go/merge_test.go b/internal/git2go/merge_test.go
index 5f0bc60c4..227475471 100644
--- a/internal/git2go/merge_test.go
+++ b/internal/git2go/merge_test.go
@@ -109,6 +109,7 @@ func TestGit2Go_MergeCommandSerialization(t *testing.T) {
 
 func TestGit2Go_MergeResultSerialization(t *testing.T) {
 	serializeResult := func(t *testing.T, result MergeResult) string {
+		t.Helper()
 		var buf bytes.Buffer
 		err := result.SerializeTo(&buf)
 		require.NoError(t, err)
diff --git a/internal/git2go/revert.go b/internal/git2go/revert.go
new file mode 100644
index 000000000..ca11169bf
--- /dev/null
+++ b/internal/git2go/revert.go
@@ -0,0 +1,100 @@
+package git2go
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"io"
+	"time"
+
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
+)
+
+// RevertResult contains results from a revert.
+type RevertResult struct {
+	// CommitID is the object ID of the generated revert commit.
+	CommitID string `json:"commit_id"`
+}
+
+// SerializeTo serializes the revert result and writes it into the writer.
+func (r RevertResult) SerializeTo(w io.Writer) error {
+	return serializeTo(w, r)
+}
+
+type RevertCommand struct {
+	// Repository is the path to execute the revert in.
+	Repository string `json:"repository"`
+	// AuthorName is the author name of revert commit.
+	AuthorName string `json:"author_name"`
+	// AuthorMail is the author mail of revert commit.
+	AuthorMail string `json:"author_mail"`
+	// AuthorDate is the author date of revert commit.
+	AuthorDate time.Time `json:"author_date"`
+	// Message is the message to be used for the revert commit.
+	Message string `json:"message"`
+	// Ours is the commit that the revert is applied to.
+	Ours string `json:"ours"`
+	// Revert is the commit to be reverted.
+	Revert string `json:"revert"`
+	// Mainline is the parent to be considered the mainline
+	Mainline uint `json:"mainline"`
+}
+
+func (r RevertCommand) Run(ctx context.Context, cfg config.Cfg) (RevertResult, error) {
+	if err := r.verify(); err != nil {
+		return RevertResult{}, fmt.Errorf("revert: %w: %s", ErrInvalidArgument, err.Error())
+	}
+
+	serialized, err := serialize(r)
+	if err != nil {
+		return RevertResult{}, err
+	}
+
+	stdout, err := run(ctx, cfg, "revert", serialized)
+	if err != nil {
+		return RevertResult{}, err
+	}
+
+	var response RevertResult
+	if err := deserialize(stdout, &response); err != nil {
+		return RevertResult{}, err
+	}
+
+	return response, nil
+}
+
+func (r RevertCommand) verify() error {
+	if r.Repository == "" {
+		return errors.New("missing repository")
+	}
+	if r.AuthorName == "" {
+		return errors.New("missing author name")
+	}
+	if r.AuthorMail == "" {
+		return errors.New("missing author mail")
+	}
+	if r.Message == "" {
+		return errors.New("missing message")
+	}
+	if r.Ours == "" {
+		return errors.New("missing ours")
+	}
+	if r.Revert == "" {
+		return errors.New("missing revert")
+	}
+	return nil
+}
+
+// RevertCommandFromSerialized deserializes the revert request from its JSON representation encoded with base64.
+func RevertCommandFromSerialized(serialized string) (RevertCommand, error) {
+	var request RevertCommand
+	if err := deserialize(serialized, &request); err != nil {
+		return RevertCommand{}, err
+	}
+
+	if err := request.verify(); err != nil {
+		return RevertCommand{}, fmt.Errorf("revert: %w: %s", ErrInvalidArgument, err.Error())
+	}
+
+	return request, nil
+}
diff --git a/internal/git2go/revert_test.go b/internal/git2go/revert_test.go
new file mode 100644
index 000000000..c56e4da4e
--- /dev/null
+++ b/internal/git2go/revert_test.go
@@ -0,0 +1,160 @@
+package git2go
+
+import (
+	"bytes"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestGit2Go_RevertCommandSerialization(t *testing.T) {
+	testcases := []struct {
+		desc string
+		cmd  RevertCommand
+		err  string
+	}{
+		{
+			desc: "missing repository",
+			cmd:  RevertCommand{},
+			err:  "missing repository",
+		},
+		{
+			desc: "missing author name",
+			cmd: RevertCommand{
+				Repository: "foo",
+			},
+			err: "missing author name",
+		},
+		{
+			desc: "missing author mail",
+			cmd: RevertCommand{
+				Repository: "foo",
+				AuthorName: "Au Thor",
+			},
+			err: "missing author mail",
+		},
+		{
+			desc: "missing author message",
+			cmd: RevertCommand{
+				Repository: "foo",
+				AuthorName: "Au Thor",
+				AuthorMail: "au@thor.com",
+			},
+			err: "missing message",
+		},
+		{
+			desc: "missing author ours",
+			cmd: RevertCommand{
+				Repository: "foo",
+				AuthorName: "Au Thor",
+				AuthorMail: "au@thor.com",
+				Message:    "Message",
+			},
+			err: "missing ours",
+		},
+		{
+			desc: "missing revert",
+			cmd: RevertCommand{
+				Repository: "foo",
+				AuthorName: "Au Thor",
+				AuthorMail: "au@thor.com",
+				Message:    "Message",
+				Ours:       "refs/heads/master",
+			},
+			err: "missing revert",
+		},
+		{
+			desc: "valid command",
+			cmd: RevertCommand{
+				Repository: "foo",
+				AuthorName: "Au Thor",
+				AuthorMail: "au@thor.com",
+				Message:    "Message",
+				Ours:       "refs/heads/master",
+				Revert:     "refs/heads/master",
+			},
+		},
+		{
+			desc: "valid command with date",
+			cmd: RevertCommand{
+				Repository: "foo",
+				AuthorName: "Au Thor",
+				AuthorMail: "au@thor.com",
+				AuthorDate: time.Now().UTC(),
+				Message:    "Message",
+				Ours:       "refs/heads/master",
+				Revert:     "refs/heads/master",
+			},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.desc, func(t *testing.T) {
+			serialized, err := serialize(tc.cmd)
+			require.NoError(t, err)
+
+			deserialized, err := RevertCommandFromSerialized(serialized)
+
+			if tc.err != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tc.cmd, deserialized)
+			}
+		})
+	}
+}
+
+func TestGit2Go_RevertResultSerialization(t *testing.T) {
+	serializeResult := func(t *testing.T, result RevertResult) string {
+		t.Helper()
+		var buf bytes.Buffer
+		err := result.SerializeTo(&buf)
+		require.NoError(t, err)
+		return buf.String()
+	}
+
+	testcases := []struct {
+		desc       string
+		serialized string
+		expected   RevertResult
+		err        string
+	}{
+		{
+			desc:       "empty revert result",
+			serialized: serializeResult(t, RevertResult{}),
+			expected:   RevertResult{},
+		},
+		{
+			desc: "revert result with commit",
+			serialized: serializeResult(t, RevertResult{
+				CommitID: "1234",
+			}),
+			expected: RevertResult{
+				CommitID: "1234",
+			},
+		},
+		{
+			desc:       "invalid serialized representation",
+			serialized: "xvlc",
+			err:        "invalid character",
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.desc, func(t *testing.T) {
+			var deserialized RevertResult
+			err := deserialize(tc.serialized, &deserialized)
+
+			if tc.err != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.err)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tc.expected, deserialized)
+			}
+		})
+	}
+}