Merge branch 'migrate-writeref' into 'master'

Migrate writeref from using the ruby implementation to go

Closes #1424

See merge request gitlab-org/gitaly!1008

5 files changed, 59 insertions, 12 deletions

diff --git a/changelogs/unreleased/migrate-writeref.yml b/changelogs/unreleased/migrate-writeref.yml
new file mode 100644
index 000000000..adee7d3e1
--- /dev/null
+++ b/changelogs/unreleased/migrate-writeref.yml
@@ -0,0 +1,5 @@
+---
+title: Migrate writeref from using the ruby implementation to go
+merge_request: 1008
+author: johncai
+type: other
diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go
index c61cc7e62..01376be52 100644
--- a/internal/git/updateref/updateref.go
+++ b/internal/git/updateref/updateref.go
@@ -40,8 +40,8 @@ func (u *Updater) Create(ref, value string) error {
 
 // Update commands the reference to be updated to point at the sha specified in
 // newvalue
-func (u *Updater) Update(ref, newvalue string) error {
-	_, err := fmt.Fprintf(u.cmd, "update %s\x00%s\x00\x00", ref, newvalue)
+func (u *Updater) Update(ref, newvalue, oldvalue string) error {
+	_, err := fmt.Fprintf(u.cmd, "update %s\x00%s\x00%s\x00", ref, newvalue, oldvalue)
 	return err
 }
 
diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go
index 1fb9aefb8..3de350b25 100644
--- a/internal/git/updateref/updateref_test.go
+++ b/internal/git/updateref/updateref_test.go
@@ -66,7 +66,7 @@ func TestUpdate(t *testing.T) {
 	require.NotNil(t, commit, "%s does not exist in the test repository", ref)
 	require.NotEqual(t, commit.Id, sha, "%s points to HEAD: %s in the test repository", ref, sha)
 
-	require.NoError(t, updater.Update(ref, sha))
+	require.NoError(t, updater.Update(ref, sha, ""))
 	require.NoError(t, updater.Wait())
 
 	// check the ref was updated
@@ -74,6 +74,17 @@ func TestUpdate(t *testing.T) {
 	require.NoError(t, logErr)
 	require.NotNil(t, commit)
 	require.Equal(t, commit.Id, sha, "reference was not updated")
+
+	// since ref has been updated to HEAD, we know that it does not point to HEAD^. So, HEAD^ is an invalid "old value" for updating ref
+	parentCommit, err := log.GetCommit(ctx, testRepo, "HEAD^")
+	require.NoError(t, err)
+	require.Error(t, updater.Update(ref, parentCommit.Id, parentCommit.Id))
+
+	// check the ref was not updated
+	commit, logErr = log.GetCommit(ctx, testRepo, ref)
+	require.NoError(t, logErr)
+	require.NotNil(t, commit)
+	require.NotEqual(t, commit.Id, parentCommit.Id, "reference was updated when it shouldn't have been")
 }
 
 func TestDelete(t *testing.T) {
diff --git a/internal/helper/error.go b/internal/helper/error.go
index 4966b788d..738974760 100644
--- a/internal/helper/error.go
+++ b/internal/helper/error.go
@@ -17,6 +17,12 @@ func DecorateError(code codes.Code, err error) error {
 	return err
 }
 
+// ErrInternal wrappes err with codes.Internal, unless err is already a grpc error
+func ErrInternal(err error) error { return DecorateError(codes.Internal, err) }
+
+// ErrInvalidArgument wrappes err with codes.InvalidArgument, unless err is already a grpc error
+func ErrInvalidArgument(err error) error { return DecorateError(codes.InvalidArgument, err) }
+
 // GrpcCode emulates the old grpc.Code function: it translates errors into codes.Code values.
 func GrpcCode(err error) codes.Code {
 	if err == nil {
diff --git a/internal/service/repository/write_ref.go b/internal/service/repository/write_ref.go
index a70c15490..f9b193bac 100644
--- a/internal/service/repository/write_ref.go
+++ b/internal/service/repository/write_ref.go
@@ -6,28 +6,53 @@ import (
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
-	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
+	"gitlab.com/gitlab-org/gitaly/internal/git/updateref"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"golang.org/x/net/context"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func (s *server) WriteRef(ctx context.Context, req *gitalypb.WriteRefRequest) (*gitalypb.WriteRefResponse, error) {
 	if err := validateWriteRefRequest(req); err != nil {
-		return nil, status.Errorf(codes.InvalidArgument, "WriteRef: %v", err)
+		return nil, helper.ErrInvalidArgument(err)
 	}
+	if err := writeRef(ctx, req); err != nil {
+		return nil, helper.ErrInternal(err)
+	}
+
+	return &gitalypb.WriteRefResponse{}, nil
+}
+
+func writeRef(ctx context.Context, req *gitalypb.WriteRefRequest) error {
+	if string(req.Ref) == "HEAD" {
+		return updateSymbolicRef(ctx, req)
+	}
+	return updateRef(ctx, req)
+}
 
-	client, err := s.RepositoryServiceClient(ctx)
+func updateSymbolicRef(ctx context.Context, req *gitalypb.WriteRefRequest) error {
+	cmd, err := git.Command(ctx, req.GetRepository(), "symbolic-ref", string(req.GetRef()), string(req.GetRevision()))
 	if err != nil {
-		return nil, err
+		return fmt.Errorf("error when creating symbolic-ref command: %v", err)
 	}
+	if err = cmd.Wait(); err != nil {
+		return fmt.Errorf("error when running symbolic-ref command: %v", err)
+	}
+	return nil
+}
 
-	clientCtx, err := rubyserver.SetHeaders(ctx, req.GetRepository())
+func updateRef(ctx context.Context, req *gitalypb.WriteRefRequest) error {
+	u, err := updateref.New(ctx, req.GetRepository())
 	if err != nil {
-		return nil, err
+		return fmt.Errorf("error when running creating new updater: %v", err)
 	}
+	if err = u.Update(string(req.GetRef()), string(req.GetRevision()), string(req.GetOldRevision())); err != nil {
+		return fmt.Errorf("error when creating update-ref command: %v", err)
+	}
+	if err = u.Wait(); err != nil {
+		return fmt.Errorf("error when running update-ref command: %v", err)
+	}
+	return nil
 
-	return client.WriteRef(clientCtx, req)
 }
 
 func validateWriteRefRequest(req *gitalypb.WriteRefRequest) error {