diff options
author | Jacob Vosmaer <jacob@gitlab.com> | 2019-03-06 17:04:48 +0300 |
---|---|---|
committer | Jacob Vosmaer <jacob@gitlab.com> | 2019-03-06 17:04:48 +0300 |
commit | b8cb00077a3b84a18c62212c11cd6653800f7ba5 (patch) | |
tree | 1a57189ddb859886eb01f785422cfdb5c28da586 | |
parent | e029f87ba0da4c4e6376f06453ca94a1e11a131a (diff) | |
parent | 7c8e949441e82d18f51990e24202a538e51b1081 (diff) |
Merge branch 'remote-add-remove-go' into 'master'
Rewrite remove remote in Go
See merge request gitlab-org/gitaly!1051
-rw-r--r-- | changelogs/unreleased/remote-go.yml | 5 | ||||
-rw-r--r-- | internal/git/objectpool/link.go | 6 | ||||
-rw-r--r-- | internal/git/objectpool/pool.go | 3 | ||||
-rw-r--r-- | internal/git/objectpool/remote.go | 37 | ||||
-rw-r--r-- | internal/git/objectpool/remote_test.go | 50 | ||||
-rw-r--r-- | internal/git/remote/remote.go | 39 | ||||
-rw-r--r-- | internal/git/remote/remote_test.go | 68 | ||||
-rw-r--r-- | internal/service/remote/remotes.go | 16 | ||||
-rw-r--r-- | ruby/lib/gitaly_server/remote_service.rb | 1 |
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) |