Rewrite remove remote in Go

Part of #1465

Signed-off-by: Mateusz Nowotyński <maxmati4@gmail.com>

9 files changed, 129 insertions, 96 deletions

diff --git a/changelogs/unreleased/remote-go.yml b/changelogs/unreleased/remote-go.yml
new file mode 100644
index 000000000..7bd81dd98
--- /dev/null
+++ b/changelogs/unreleased/remote-go.yml
@@ -0,0 +1,5 @@
+---
+title: Rewrite remove remote in go
+merge_request: 1051
+author: maxmati
+type: performance
diff --git a/internal/git/objectpool/link.go b/internal/git/objectpool/link.go
index 7645f7dd9..ae3055c07 100644
--- a/internal/git/objectpool/link.go
+++ b/internal/git/objectpool/link.go
@@ -7,6 +7,8 @@ import (
 	"os"
 	"path/filepath"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git/remote"
+
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -55,8 +57,8 @@ func (o *ObjectPool) Unlink(ctx context.Context, repo *gitalypb.Repository) erro
 	// We need to use removeRemote, and can't leverage `git config --remove-section`
 	// as the latter doesn't clean up refs
 	remoteName := repo.GetGlRepository()
-	if err := o.removeRemote(ctx, remoteName); err != nil {
-		if present, err2 := o.hasRemote(ctx, remoteName); err2 != nil || present {
+	if err := remote.Remove(ctx, o, remoteName); err != nil {
+		if present, err2 := remote.Exists(ctx, o, remoteName); err2 != nil || present {
 			return err
 		}
 	}
diff --git a/internal/git/objectpool/pool.go b/internal/git/objectpool/pool.go
index 824eb94ae..f018fe6f8 100644
--- a/internal/git/objectpool/pool.go
+++ b/internal/git/objectpool/pool.go
@@ -6,6 +6,7 @@ import (
 	"path"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git/remote"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
@@ -77,7 +78,7 @@ func (o *ObjectPool) Create(ctx context.Context, repo *gitalypb.Repository) (err
 		return err
 	}
 
-	if err := o.removeRemote(ctx, "origin"); err != nil {
+	if err := remote.Remove(ctx, o, "origin"); err != nil {
 		return err
 	}
 
diff --git a/internal/git/objectpool/remote.go b/internal/git/objectpool/remote.go
deleted file mode 100644
index bed46c55f..000000000
--- a/internal/git/objectpool/remote.go
+++ /dev/null
@@ -1,37 +0,0 @@
-package objectpool
-
-import (
-	"bufio"
-	"context"
-
-	"gitlab.com/gitlab-org/gitaly/internal/git"
-)
-
-func (o *ObjectPool) removeRemote(ctx context.Context, name string) error {
-	cmd, err := git.Command(ctx, o, "remote", "remove", name)
-	if err != nil {
-		return err
-	}
-
-	return cmd.Wait()
-}
-
-// hasRemote will always return a boolean value, but should only be depended on
-// when the error value is nil
-func (o *ObjectPool) hasRemote(ctx context.Context, name string) (bool, error) {
-	cmd, err := git.Command(ctx, o, "remote")
-	if err != nil {
-		return false, err
-	}
-
-	found := false
-	scanner := bufio.NewScanner(cmd)
-	for scanner.Scan() {
-		if scanner.Text() == name {
-			found = true
-			break
-		}
-	}
-
-	return found, cmd.Wait()
-}
diff --git a/internal/git/objectpool/remote_test.go b/internal/git/objectpool/remote_test.go
deleted file mode 100644
index ce9ec85f6..000000000
--- a/internal/git/objectpool/remote_test.go
+++ /dev/null
@@ -1,50 +0,0 @@
-package objectpool
-
-import (
-	"testing"
-
-	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
-)
-
-func TestRemoveRemote(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
-	defer cleanupFn()
-
-	pool, err := NewObjectPool(testRepo.GetStorageName(), t.Name())
-	require.NoError(t, err)
-
-	require.NoError(t, pool.clone(ctx, testRepo))
-	defer pool.Remove(ctx)
-
-	require.NoError(t, pool.removeRemote(ctx, "origin"))
-
-	out := testhelper.MustRunCommand(t, nil, "git", "-C", pool.FullPath(), "remote")
-	require.Len(t, out, 0)
-}
-
-func TestHasRemote(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
-	defer cleanupFn()
-
-	pool, err := NewObjectPool(testRepo.GetStorageName(), t.Name())
-	require.NoError(t, err)
-
-	// This creates a remote to the repository, "origin"
-	require.NoError(t, pool.clone(ctx, testRepo))
-	defer pool.Remove(ctx)
-
-	found, err := pool.hasRemote(ctx, "origin")
-	require.NoError(t, err)
-	require.True(t, found)
-
-	found, err = pool.hasRemote(ctx, "can-not-be-found")
-	require.NoError(t, err)
-	require.False(t, found)
-}
diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go
new file mode 100644
index 000000000..660af4711
--- /dev/null
+++ b/internal/git/remote/remote.go
@@ -0,0 +1,39 @@
+package remote
+
+import (
+	"bufio"
+	"context"
+
+	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
+)
+
+//Remove removes the remote from repository
+func Remove(ctx context.Context, repo repository.GitRepo, name string) error {
+	cmd, err := git.Command(ctx, repo, "remote", "remove", name)
+	if err != nil {
+		return err
+	}
+
+	return cmd.Wait()
+}
+
+// Exists will always return a boolean value, but should only be depended on
+// when the error value is nil
+func Exists(ctx context.Context, repo repository.GitRepo, name string) (bool, error) {
+	cmd, err := git.Command(ctx, repo, "remote")
+	if err != nil {
+		return false, err
+	}
+
+	found := false
+	scanner := bufio.NewScanner(cmd)
+	for scanner.Scan() {
+		if scanner.Text() == name {
+			found = true
+			break
+		}
+	}
+
+	return found, cmd.Wait()
+}
diff --git a/internal/git/remote/remote_test.go b/internal/git/remote/remote_test.go
new file mode 100644
index 000000000..ec4809d5e
--- /dev/null
+++ b/internal/git/remote/remote_test.go
@@ -0,0 +1,68 @@
+package remote
+
+import (
+	"testing"
+
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+)
+
+func TestRemoveRemote(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	require.NoError(t, Remove(ctx, testRepo, "origin"))
+
+	repoPath, err := helper.GetRepoPath(testRepo)
+	require.NoError(t, err)
+
+	out := testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "remote")
+	require.Len(t, out, 0)
+}
+
+func TestRemoveRemoteDontRemoveLocalBranches(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	repoPath, err := helper.GetRepoPath(testRepo)
+	require.NoError(t, err)
+
+	//configure remote as fetch mirror
+	testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "config", "remote.origin.fetch", "+refs/*:refs/*")
+	testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "fetch")
+
+	masterBeforeRemove := testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "show-ref", "refs/heads/master")
+
+	require.NoError(t, Remove(ctx, testRepo, "origin"))
+
+	out := testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "remote")
+	require.Len(t, out, 0)
+
+	out = testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "show-ref", "refs/heads/master")
+	require.Equal(t, masterBeforeRemove, out)
+
+}
+
+func TestRemoteExists(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	found, err := Exists(ctx, testRepo, "origin")
+	require.NoError(t, err)
+	require.True(t, found)
+
+	found, err = Exists(ctx, testRepo, "can-not-be-found")
+	require.NoError(t, err)
+	require.False(t, found)
+}
diff --git a/internal/service/remote/remotes.go b/internal/service/remote/remotes.go
index c075750d6..ea77c2030 100644
--- a/internal/service/remote/remotes.go
+++ b/internal/service/remote/remotes.go
@@ -7,9 +7,11 @@ import (
 	"io/ioutil"
 	"strings"
 
+	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
+
 	"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/remote"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -47,20 +49,22 @@ func validateAddRemoteRequest(req *gitalypb.AddRemoteRequest) error {
 // RemoveRemote removes the given remote
 func (s *server) RemoveRemote(ctx context.Context, req *gitalypb.RemoveRemoteRequest) (*gitalypb.RemoveRemoteResponse, error) {
 	if err := validateRemoveRemoteRequest(req); err != nil {
-		return nil, status.Errorf(codes.InvalidArgument, "AddRemote: %v", err)
+		return nil, status.Errorf(codes.InvalidArgument, "RemoveRemote: %v", err)
 	}
 
-	client, err := s.RemoteServiceClient(ctx)
+	hasRemote, err := remote.Exists(ctx, req.GetRepository(), req.Name)
 	if err != nil {
 		return nil, err
 	}
+	if !hasRemote {
+		return &gitalypb.RemoveRemoteResponse{Result: false}, nil
+	}
 
-	clientCtx, err := rubyserver.SetHeaders(ctx, req.GetRepository())
-	if err != nil {
+	if err := remote.Remove(ctx, req.GetRepository(), req.Name); err != nil {
 		return nil, err
 	}
 
-	return client.RemoveRemote(clientCtx, req)
+	return &gitalypb.RemoveRemoteResponse{Result: true}, nil
 }
 
 func (s *server) FindRemoteRepository(ctx context.Context, req *gitalypb.FindRemoteRepositoryRequest) (*gitalypb.FindRemoteRepositoryResponse, error) {
diff --git a/ruby/lib/gitaly_server/remote_service.rb b/ruby/lib/gitaly_server/remote_service.rb
index 959fc3ab4..510cac93f 100644
--- a/ruby/lib/gitaly_server/remote_service.rb
+++ b/ruby/lib/gitaly_server/remote_service.rb
@@ -12,6 +12,7 @@ module GitalyServer
       Gitaly::AddRemoteResponse.new
     end
 
+    # TODO: remove in gitlab 11.9, this is implemented in Go now
     def remove_remote(request, call)
       repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)