From b4079f213bcdb48a806e4a44a8d7c90a4b27dbab Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Mon, 21 Oct 2019 16:29:42 -0400 Subject: [PATCH 01/13] Rewrite add_remote in Go --- changelogs/unreleased/wc-go-add-remote.yml | 5 + internal/git/remote/remote.go | 116 ++++++++++++++++++ internal/git/remote/remote_test.go | 73 +++++++++++ internal/metadata/featureflag/featureflags.go | 3 + internal/service/remote/remotes.go | 31 +++++ 5 files changed, 228 insertions(+) create mode 100644 changelogs/unreleased/wc-go-add-remote.yml diff --git a/changelogs/unreleased/wc-go-add-remote.yml b/changelogs/unreleased/wc-go-add-remote.yml new file mode 100644 index 00000000000..aabf322884f --- /dev/null +++ b/changelogs/unreleased/wc-go-add-remote.yml @@ -0,0 +1,5 @@ +--- +title: Rewrite AddRemote in Go +merge_request: 1572 +author: +type: other diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 660af4711b0..bb02cf43b64 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -8,6 +8,54 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/repository" ) +var REFMAPS = map[string]string{ + "allRefs": "+refs/*:refs/*", + "heads": "+refs/heads/*:refs/heads/*", + "tags": "+refs/tags/*:refs/tags/*", +} + +//Add remote to repository +func Add(ctx context.Context, repo repository.GitRepo, name string, url string, refmaps []string) error { + hasRemote, err := Exists(ctx, repo, name) + if err != nil { + return err + } + + if hasRemote { + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "remote", + Flags: []git.Option{ + git.SubSubCmd{"set-url"}, + }, + Args: []string{name, url}, + }) + if err != nil { + return err + } + return cmd.Wait() + } + + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "remote", + Args: []string{"add", name, url}, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + + if len(refmaps) > 0 { + err = setMirror(ctx, repo, name, refmaps) + if err != nil { + return err + } + } + + return nil +} + //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) @@ -37,3 +85,71 @@ func Exists(ctx context.Context, repo repository.GitRepo, name string) (bool, er return found, cmd.Wait() } + +func setMirror(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { + for _, configOption := range []string{"mirror", "prune"} { + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "config", + Flags: []git.Option{ + git.Flag{"--add"}, + git.ConfigPair{"remote." + name + "." + configOption, "true"}, + }, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + } + + return setRefmaps(ctx, repo, name, refmaps) +} + +func setRefmaps(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { + parsedMaps := parseRefmaps(refmaps) + + for i, refmap := range parsedMaps { + var flag git.Flag + if i == 0 { + flag = git.Flag{"--replace-all"} + } else { + flag = git.Flag{"--add"} + } + + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "config", + Flags: []git.Option{ + flag, + git.ConfigPair{"remote." + name + ".fetch", refmap}, + }, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + } + + return nil +} + +func parseRefmaps(refmaps []string) []string { + parsedMaps := make([]string, len(refmaps)) + + for _, refmap := range refmaps { + if len(refmap) == 0 { + continue + } + + expanded, ok := REFMAPS[refmap] + if ok { + parsedMaps = append(parsedMaps, expanded) + } else { + parsedMaps = append(parsedMaps, refmap) + } + } + + return parsedMaps +} diff --git a/internal/git/remote/remote_test.go b/internal/git/remote/remote_test.go index ec4809d5e9c..7fd5ff4d687 100644 --- a/internal/git/remote/remote_test.go +++ b/internal/git/remote/remote_test.go @@ -9,6 +9,79 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/testhelper" ) +func TestAddRemote(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) + + testCases := []struct { + description string + remoteName string + url string + mirrorRefmaps []string + resolvedMirrorRefmaps []string + }{ + { + description: "creates remote with no refmaps", + remoteName: "no-refmap", + url: "https://gitlab.com/gitlab-org/git.git", + }, + { + description: "updates url when remote exists", + remoteName: "no-refmap", + url: "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git", + }, + { + description: "resolves abbreviated refmap", + remoteName: "abbrev-refmap", + url: "https://gitlab.com/gitlab-org/git.git", + mirrorRefmaps: []string{"heads"}, + resolvedMirrorRefmaps: []string{"+refs/heads/*:refs/heads/*"}, + }, + { + description: "adds multiple refmaps", + remoteName: "multiple-refmaps", + url: "https://gitlab.com/gitlab-org/git.git", + mirrorRefmaps: []string{"tags", "+refs/heads/*:refs/remotes/origin/*"}, + resolvedMirrorRefmaps: []string{"+refs/tags/*:refs/tags/*", "+refs/heads/*:refs/remotes/origin/*"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + err := Add(ctx, testRepo, tc.remoteName, tc.url, tc.mirrorRefmaps) + require.NoError(t, err) + + found, err := Exists(ctx, testRepo, tc.remoteName) + require.NoError(t, err) + require.True(t, found) + + url := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "remote", "get-url", tc.remoteName)) + require.Contains(t, url, tc.url) + + mirrorRegex := "remote." + tc.remoteName + mirrorConfig := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "config", "--get-regexp", mirrorRegex)) + if len(tc.resolvedMirrorRefmaps) > 0 { + require.Contains(t, mirrorConfig, "mirror true") + require.Contains(t, mirrorConfig, "prune true") + } else { + require.NotContains(t, mirrorConfig, "mirror true") + } + + mirrorFetch := mirrorRegex + ".fetch" + fetch := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "config", "--get-all", mirrorFetch)) + for _, resolvedMirrorRefmap := range tc.resolvedMirrorRefmaps { + require.Contains(t, fetch, resolvedMirrorRefmap) + } + }) + } +} + func TestRemoveRemote(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/metadata/featureflag/featureflags.go b/internal/metadata/featureflag/featureflags.go index 9860291b97b..4fba997e0e0 100644 --- a/internal/metadata/featureflag/featureflags.go +++ b/internal/metadata/featureflag/featureflags.go @@ -1,6 +1,9 @@ package featureflag const ( + // AddRemoteGo will cause the AddRemote RPC to use the go implementation when set + AddRemoteGo = "add_remote_go" + // GetAllLFSPointersGo will cause the GetAllLFSPointers RPC to use the go implementation when set GetAllLFSPointersGo = "get_all_lfs_pointers_go" diff --git a/internal/service/remote/remotes.go b/internal/service/remote/remotes.go index 8281a799d87..fa7570b471f 100644 --- a/internal/service/remote/remotes.go +++ b/internal/service/remote/remotes.go @@ -11,19 +11,41 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/remote" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" + "github.com/prometheus/client_golang/prometheus" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) +var addRemoteRequests = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "gitaly_add_remote_total", + Help: "Counter of go vs ruby implementation of AddRemote", + }, + []string{"implementation"}, +) + +func init() { + prometheus.MustRegister(addRemoteRequests) +} + // AddRemote adds a remote to the repository func (s *server) AddRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) (*gitalypb.AddRemoteResponse, error) { if err := validateAddRemoteRequest(req); err != nil { return nil, status.Errorf(codes.InvalidArgument, "AddRemote: %v", err) } + if featureflag.IsEnabled(ctx, featureflag.AddRemoteGo) { + addRemoteRequests.WithLabelValues("go").Inc() + + return addRemote(ctx, req) + } + + addRemoteRequests.WithLabelValues("ruby").Inc() + client, err := s.RemoteServiceClient(ctx) if err != nil { return nil, err @@ -37,6 +59,15 @@ func (s *server) AddRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) return client.AddRemote(clientCtx, req) } +func addRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) (*gitalypb.AddRemoteResponse, error) { + err := remote.Add(ctx, req.GetRepository(), req.Name, req.Url, req.GetMirrorRefmaps()) + if err != nil { + return nil, status.Errorf(codes.Internal, "AddRemote: %v", err) + } + + return &gitalypb.AddRemoteResponse{}, nil +} + func validateAddRemoteRequest(req *gitalypb.AddRemoteRequest) error { if strings.TrimSpace(req.GetName()) == "" { return fmt.Errorf("empty remote name") -- GitLab From d46337eb1a0aa7e210fdd324071c1b9d2ef13507 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Mon, 21 Oct 2019 23:33:55 -0400 Subject: [PATCH 02/13] Update MR number in changelog --- changelogs/unreleased/wc-go-add-remote.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/wc-go-add-remote.yml b/changelogs/unreleased/wc-go-add-remote.yml index aabf322884f..89afd203528 100644 --- a/changelogs/unreleased/wc-go-add-remote.yml +++ b/changelogs/unreleased/wc-go-add-remote.yml @@ -1,5 +1,5 @@ --- title: Rewrite AddRemote in Go -merge_request: 1572 +merge_request: 1575 author: type: other -- GitLab From c996d5512638e4fb44b3d0a033589a0dd6c4534a Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Mon, 21 Oct 2019 16:29:42 -0400 Subject: [PATCH 03/13] Rewrite add_remote in Go --- changelogs/unreleased/wc-go-add-remote.yml | 5 + internal/git/remote/remote.go | 116 ++++++++++++++++++ internal/git/remote/remote_test.go | 73 +++++++++++ .../metadata/featureflag/feature_flags.go | 10 +- internal/service/remote/remotes.go | 31 +++++ 5 files changed, 232 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/wc-go-add-remote.yml diff --git a/changelogs/unreleased/wc-go-add-remote.yml b/changelogs/unreleased/wc-go-add-remote.yml new file mode 100644 index 00000000000..aabf322884f --- /dev/null +++ b/changelogs/unreleased/wc-go-add-remote.yml @@ -0,0 +1,5 @@ +--- +title: Rewrite AddRemote in Go +merge_request: 1572 +author: +type: other diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 41e83b18779..37eebe9f1ee 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -8,6 +8,54 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/repository" ) +var REFMAPS = map[string]string{ + "allRefs": "+refs/*:refs/*", + "heads": "+refs/heads/*:refs/heads/*", + "tags": "+refs/tags/*:refs/tags/*", +} + +//Add remote to repository +func Add(ctx context.Context, repo repository.GitRepo, name string, url string, refmaps []string) error { + hasRemote, err := Exists(ctx, repo, name) + if err != nil { + return err + } + + if hasRemote { + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "remote", + Flags: []git.Option{ + git.SubSubCmd{"set-url"}, + }, + Args: []string{name, url}, + }) + if err != nil { + return err + } + return cmd.Wait() + } + + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "remote", + Args: []string{"add", name, url}, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + + if len(refmaps) > 0 { + err = setMirror(ctx, repo, name, refmaps) + if err != nil { + return err + } + } + + return nil +} + //Remove removes the remote from repository func Remove(ctx context.Context, repo repository.GitRepo, name string) error { cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ @@ -41,3 +89,71 @@ func Exists(ctx context.Context, repo repository.GitRepo, name string) (bool, er return found, cmd.Wait() } + +func setMirror(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { + for _, configOption := range []string{"mirror", "prune"} { + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "config", + Flags: []git.Option{ + git.Flag{"--add"}, + git.ConfigPair{"remote." + name + "." + configOption, "true"}, + }, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + } + + return setRefmaps(ctx, repo, name, refmaps) +} + +func setRefmaps(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { + parsedMaps := parseRefmaps(refmaps) + + for i, refmap := range parsedMaps { + var flag git.Flag + if i == 0 { + flag = git.Flag{"--replace-all"} + } else { + flag = git.Flag{"--add"} + } + + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "config", + Flags: []git.Option{ + flag, + git.ConfigPair{"remote." + name + ".fetch", refmap}, + }, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + } + + return nil +} + +func parseRefmaps(refmaps []string) []string { + parsedMaps := make([]string, len(refmaps)) + + for _, refmap := range refmaps { + if len(refmap) == 0 { + continue + } + + expanded, ok := REFMAPS[refmap] + if ok { + parsedMaps = append(parsedMaps, expanded) + } else { + parsedMaps = append(parsedMaps, refmap) + } + } + + return parsedMaps +} diff --git a/internal/git/remote/remote_test.go b/internal/git/remote/remote_test.go index af4c56a9ee6..909491aec73 100644 --- a/internal/git/remote/remote_test.go +++ b/internal/git/remote/remote_test.go @@ -9,6 +9,79 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/testhelper" ) +func TestAddRemote(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) + + testCases := []struct { + description string + remoteName string + url string + mirrorRefmaps []string + resolvedMirrorRefmaps []string + }{ + { + description: "creates remote with no refmaps", + remoteName: "no-refmap", + url: "https://gitlab.com/gitlab-org/git.git", + }, + { + description: "updates url when remote exists", + remoteName: "no-refmap", + url: "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git", + }, + { + description: "resolves abbreviated refmap", + remoteName: "abbrev-refmap", + url: "https://gitlab.com/gitlab-org/git.git", + mirrorRefmaps: []string{"heads"}, + resolvedMirrorRefmaps: []string{"+refs/heads/*:refs/heads/*"}, + }, + { + description: "adds multiple refmaps", + remoteName: "multiple-refmaps", + url: "https://gitlab.com/gitlab-org/git.git", + mirrorRefmaps: []string{"tags", "+refs/heads/*:refs/remotes/origin/*"}, + resolvedMirrorRefmaps: []string{"+refs/tags/*:refs/tags/*", "+refs/heads/*:refs/remotes/origin/*"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + err := Add(ctx, testRepo, tc.remoteName, tc.url, tc.mirrorRefmaps) + require.NoError(t, err) + + found, err := Exists(ctx, testRepo, tc.remoteName) + require.NoError(t, err) + require.True(t, found) + + url := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "remote", "get-url", tc.remoteName)) + require.Contains(t, url, tc.url) + + mirrorRegex := "remote." + tc.remoteName + mirrorConfig := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "config", "--get-regexp", mirrorRegex)) + if len(tc.resolvedMirrorRefmaps) > 0 { + require.Contains(t, mirrorConfig, "mirror true") + require.Contains(t, mirrorConfig, "prune true") + } else { + require.NotContains(t, mirrorConfig, "mirror true") + } + + mirrorFetch := mirrorRegex + ".fetch" + fetch := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "config", "--get-all", mirrorFetch)) + for _, resolvedMirrorRefmap := range tc.resolvedMirrorRefmaps { + require.Contains(t, fetch, resolvedMirrorRefmap) + } + }) + } +} + func TestRemoveRemote(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 7855b633378..32634796e98 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -1,12 +1,16 @@ package featureflag const ( - // UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant - // to upload-pack - UploadPackFilter = "upload_pack_filter" + // AddRemoteGo will cause the AddRemote RPC to use the go implementation when set + AddRemoteGo = "add_remote_go" + // GetAllLFSPointersGo will cause the GetAllLFSPointers RPC to use the go implementation when set GetAllLFSPointersGo = "get_all_lfs_pointers_go" // LinguistFileCountStats will invoke an additional git-linguist command to get the number of files per language LinguistFileCountStats = "linguist_file_count_stats" + + // UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant + // to upload-pack + UploadPackFilter = "upload_pack_filter" ) diff --git a/internal/service/remote/remotes.go b/internal/service/remote/remotes.go index 0cba15dfb5f..b5c69294e4b 100644 --- a/internal/service/remote/remotes.go +++ b/internal/service/remote/remotes.go @@ -11,19 +11,41 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/remote" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" + "github.com/prometheus/client_golang/prometheus" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) +var addRemoteRequests = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "gitaly_add_remote_total", + Help: "Counter of go vs ruby implementation of AddRemote", + }, + []string{"implementation"}, +) + +func init() { + prometheus.MustRegister(addRemoteRequests) +} + // AddRemote adds a remote to the repository func (s *server) AddRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) (*gitalypb.AddRemoteResponse, error) { if err := validateAddRemoteRequest(req); err != nil { return nil, status.Errorf(codes.InvalidArgument, "AddRemote: %v", err) } + if featureflag.IsEnabled(ctx, featureflag.AddRemoteGo) { + addRemoteRequests.WithLabelValues("go").Inc() + + return addRemote(ctx, req) + } + + addRemoteRequests.WithLabelValues("ruby").Inc() + client, err := s.ruby.RemoteServiceClient(ctx) if err != nil { return nil, err @@ -37,6 +59,15 @@ func (s *server) AddRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) return client.AddRemote(clientCtx, req) } +func addRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) (*gitalypb.AddRemoteResponse, error) { + err := remote.Add(ctx, req.GetRepository(), req.Name, req.Url, req.GetMirrorRefmaps()) + if err != nil { + return nil, status.Errorf(codes.Internal, "AddRemote: %v", err) + } + + return &gitalypb.AddRemoteResponse{}, nil +} + func validateAddRemoteRequest(req *gitalypb.AddRemoteRequest) error { if strings.TrimSpace(req.GetName()) == "" { return fmt.Errorf("empty remote name") -- GitLab From d69dc50180e31737da61d4595a5d314058107a63 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Mon, 21 Oct 2019 23:33:55 -0400 Subject: [PATCH 04/13] Update MR number in changelog --- changelogs/unreleased/wc-go-add-remote.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/wc-go-add-remote.yml b/changelogs/unreleased/wc-go-add-remote.yml index aabf322884f..89afd203528 100644 --- a/changelogs/unreleased/wc-go-add-remote.yml +++ b/changelogs/unreleased/wc-go-add-remote.yml @@ -1,5 +1,5 @@ --- title: Rewrite AddRemote in Go -merge_request: 1572 +merge_request: 1575 author: type: other -- GitLab From 517572a42c70a33736c99a1d8eefe1a7256302f7 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Mon, 21 Oct 2019 23:53:13 -0400 Subject: [PATCH 05/13] Rename refmaps dict to resolve lint error --- internal/git/remote/remote.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 37eebe9f1ee..dd60706b125 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -8,7 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/repository" ) -var REFMAPS = map[string]string{ +var stdRefmaps = map[string]string{ "allRefs": "+refs/*:refs/*", "heads": "+refs/heads/*:refs/heads/*", "tags": "+refs/tags/*:refs/tags/*", @@ -147,7 +147,7 @@ func parseRefmaps(refmaps []string) []string { continue } - expanded, ok := REFMAPS[refmap] + expanded, ok := stdRefmaps[refmap] if ok { parsedMaps = append(parsedMaps, expanded) } else { -- GitLab From f1d690a7eb0ec5128216ad81c59dbc4262ffa9d0 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Mon, 21 Oct 2019 16:29:42 -0400 Subject: [PATCH 06/13] Rewrite add_remote in Go --- changelogs/unreleased/wc-go-add-remote.yml | 5 + internal/git/remote/remote.go | 116 ++++++++++++++++++ internal/git/remote/remote_test.go | 73 +++++++++++ .../metadata/featureflag/feature_flags.go | 10 +- internal/service/remote/remotes.go | 31 +++++ 5 files changed, 232 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/wc-go-add-remote.yml diff --git a/changelogs/unreleased/wc-go-add-remote.yml b/changelogs/unreleased/wc-go-add-remote.yml new file mode 100644 index 00000000000..aabf322884f --- /dev/null +++ b/changelogs/unreleased/wc-go-add-remote.yml @@ -0,0 +1,5 @@ +--- +title: Rewrite AddRemote in Go +merge_request: 1572 +author: +type: other diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 41e83b18779..37eebe9f1ee 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -8,6 +8,54 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/repository" ) +var REFMAPS = map[string]string{ + "allRefs": "+refs/*:refs/*", + "heads": "+refs/heads/*:refs/heads/*", + "tags": "+refs/tags/*:refs/tags/*", +} + +//Add remote to repository +func Add(ctx context.Context, repo repository.GitRepo, name string, url string, refmaps []string) error { + hasRemote, err := Exists(ctx, repo, name) + if err != nil { + return err + } + + if hasRemote { + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "remote", + Flags: []git.Option{ + git.SubSubCmd{"set-url"}, + }, + Args: []string{name, url}, + }) + if err != nil { + return err + } + return cmd.Wait() + } + + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "remote", + Args: []string{"add", name, url}, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + + if len(refmaps) > 0 { + err = setMirror(ctx, repo, name, refmaps) + if err != nil { + return err + } + } + + return nil +} + //Remove removes the remote from repository func Remove(ctx context.Context, repo repository.GitRepo, name string) error { cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ @@ -41,3 +89,71 @@ func Exists(ctx context.Context, repo repository.GitRepo, name string) (bool, er return found, cmd.Wait() } + +func setMirror(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { + for _, configOption := range []string{"mirror", "prune"} { + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "config", + Flags: []git.Option{ + git.Flag{"--add"}, + git.ConfigPair{"remote." + name + "." + configOption, "true"}, + }, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + } + + return setRefmaps(ctx, repo, name, refmaps) +} + +func setRefmaps(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { + parsedMaps := parseRefmaps(refmaps) + + for i, refmap := range parsedMaps { + var flag git.Flag + if i == 0 { + flag = git.Flag{"--replace-all"} + } else { + flag = git.Flag{"--add"} + } + + cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "config", + Flags: []git.Option{ + flag, + git.ConfigPair{"remote." + name + ".fetch", refmap}, + }, + }) + if err != nil { + return err + } + if err := cmd.Wait(); err != nil { + return err + } + } + + return nil +} + +func parseRefmaps(refmaps []string) []string { + parsedMaps := make([]string, len(refmaps)) + + for _, refmap := range refmaps { + if len(refmap) == 0 { + continue + } + + expanded, ok := REFMAPS[refmap] + if ok { + parsedMaps = append(parsedMaps, expanded) + } else { + parsedMaps = append(parsedMaps, refmap) + } + } + + return parsedMaps +} diff --git a/internal/git/remote/remote_test.go b/internal/git/remote/remote_test.go index af4c56a9ee6..909491aec73 100644 --- a/internal/git/remote/remote_test.go +++ b/internal/git/remote/remote_test.go @@ -9,6 +9,79 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/testhelper" ) +func TestAddRemote(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) + + testCases := []struct { + description string + remoteName string + url string + mirrorRefmaps []string + resolvedMirrorRefmaps []string + }{ + { + description: "creates remote with no refmaps", + remoteName: "no-refmap", + url: "https://gitlab.com/gitlab-org/git.git", + }, + { + description: "updates url when remote exists", + remoteName: "no-refmap", + url: "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git", + }, + { + description: "resolves abbreviated refmap", + remoteName: "abbrev-refmap", + url: "https://gitlab.com/gitlab-org/git.git", + mirrorRefmaps: []string{"heads"}, + resolvedMirrorRefmaps: []string{"+refs/heads/*:refs/heads/*"}, + }, + { + description: "adds multiple refmaps", + remoteName: "multiple-refmaps", + url: "https://gitlab.com/gitlab-org/git.git", + mirrorRefmaps: []string{"tags", "+refs/heads/*:refs/remotes/origin/*"}, + resolvedMirrorRefmaps: []string{"+refs/tags/*:refs/tags/*", "+refs/heads/*:refs/remotes/origin/*"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + err := Add(ctx, testRepo, tc.remoteName, tc.url, tc.mirrorRefmaps) + require.NoError(t, err) + + found, err := Exists(ctx, testRepo, tc.remoteName) + require.NoError(t, err) + require.True(t, found) + + url := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "remote", "get-url", tc.remoteName)) + require.Contains(t, url, tc.url) + + mirrorRegex := "remote." + tc.remoteName + mirrorConfig := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "config", "--get-regexp", mirrorRegex)) + if len(tc.resolvedMirrorRefmaps) > 0 { + require.Contains(t, mirrorConfig, "mirror true") + require.Contains(t, mirrorConfig, "prune true") + } else { + require.NotContains(t, mirrorConfig, "mirror true") + } + + mirrorFetch := mirrorRegex + ".fetch" + fetch := string(testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "config", "--get-all", mirrorFetch)) + for _, resolvedMirrorRefmap := range tc.resolvedMirrorRefmaps { + require.Contains(t, fetch, resolvedMirrorRefmap) + } + }) + } +} + func TestRemoveRemote(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 7855b633378..32634796e98 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -1,12 +1,16 @@ package featureflag const ( - // UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant - // to upload-pack - UploadPackFilter = "upload_pack_filter" + // AddRemoteGo will cause the AddRemote RPC to use the go implementation when set + AddRemoteGo = "add_remote_go" + // GetAllLFSPointersGo will cause the GetAllLFSPointers RPC to use the go implementation when set GetAllLFSPointersGo = "get_all_lfs_pointers_go" // LinguistFileCountStats will invoke an additional git-linguist command to get the number of files per language LinguistFileCountStats = "linguist_file_count_stats" + + // UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant + // to upload-pack + UploadPackFilter = "upload_pack_filter" ) diff --git a/internal/service/remote/remotes.go b/internal/service/remote/remotes.go index 0cba15dfb5f..b5c69294e4b 100644 --- a/internal/service/remote/remotes.go +++ b/internal/service/remote/remotes.go @@ -11,19 +11,41 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/remote" "gitlab.com/gitlab-org/gitaly/internal/rubyserver" + "github.com/prometheus/client_golang/prometheus" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) +var addRemoteRequests = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "gitaly_add_remote_total", + Help: "Counter of go vs ruby implementation of AddRemote", + }, + []string{"implementation"}, +) + +func init() { + prometheus.MustRegister(addRemoteRequests) +} + // AddRemote adds a remote to the repository func (s *server) AddRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) (*gitalypb.AddRemoteResponse, error) { if err := validateAddRemoteRequest(req); err != nil { return nil, status.Errorf(codes.InvalidArgument, "AddRemote: %v", err) } + if featureflag.IsEnabled(ctx, featureflag.AddRemoteGo) { + addRemoteRequests.WithLabelValues("go").Inc() + + return addRemote(ctx, req) + } + + addRemoteRequests.WithLabelValues("ruby").Inc() + client, err := s.ruby.RemoteServiceClient(ctx) if err != nil { return nil, err @@ -37,6 +59,15 @@ func (s *server) AddRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) return client.AddRemote(clientCtx, req) } +func addRemote(ctx context.Context, req *gitalypb.AddRemoteRequest) (*gitalypb.AddRemoteResponse, error) { + err := remote.Add(ctx, req.GetRepository(), req.Name, req.Url, req.GetMirrorRefmaps()) + if err != nil { + return nil, status.Errorf(codes.Internal, "AddRemote: %v", err) + } + + return &gitalypb.AddRemoteResponse{}, nil +} + func validateAddRemoteRequest(req *gitalypb.AddRemoteRequest) error { if strings.TrimSpace(req.GetName()) == "" { return fmt.Errorf("empty remote name") -- GitLab From 7deca355dda67ddad644c84f2e8ca98501f33220 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Mon, 21 Oct 2019 23:33:55 -0400 Subject: [PATCH 07/13] Update MR number in changelog --- changelogs/unreleased/wc-go-add-remote.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/wc-go-add-remote.yml b/changelogs/unreleased/wc-go-add-remote.yml index aabf322884f..89afd203528 100644 --- a/changelogs/unreleased/wc-go-add-remote.yml +++ b/changelogs/unreleased/wc-go-add-remote.yml @@ -1,5 +1,5 @@ --- title: Rewrite AddRemote in Go -merge_request: 1572 +merge_request: 1575 author: type: other -- GitLab From defaa3df9daaf160102be35150beb78de2bbbca8 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Mon, 21 Oct 2019 23:53:13 -0400 Subject: [PATCH 08/13] Rename refmaps dict to resolve lint error --- internal/git/remote/remote.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 37eebe9f1ee..dd60706b125 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -8,7 +8,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git/repository" ) -var REFMAPS = map[string]string{ +var stdRefmaps = map[string]string{ "allRefs": "+refs/*:refs/*", "heads": "+refs/heads/*:refs/heads/*", "tags": "+refs/tags/*:refs/tags/*", @@ -147,7 +147,7 @@ func parseRefmaps(refmaps []string) []string { continue } - expanded, ok := REFMAPS[refmap] + expanded, ok := stdRefmaps[refmap] if ok { parsedMaps = append(parsedMaps, expanded) } else { -- GitLab From 6962afbf83d3cc74ebc1c9e7a070ad4bfca58059 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 24 Oct 2019 23:01:05 -0400 Subject: [PATCH 09/13] Fix 'all_refs' name and simplify refmap loop --- internal/git/remote/remote.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index dd60706b125..03018301920 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -9,9 +9,9 @@ import ( ) var stdRefmaps = map[string]string{ - "allRefs": "+refs/*:refs/*", - "heads": "+refs/heads/*:refs/heads/*", - "tags": "+refs/tags/*:refs/tags/*", + "all_refs": "+refs/*:refs/*", + "heads": "+refs/heads/*:refs/heads/*", + "tags": "+refs/tags/*:refs/tags/*", } //Add remote to repository @@ -148,11 +148,10 @@ func parseRefmaps(refmaps []string) []string { } expanded, ok := stdRefmaps[refmap] - if ok { - parsedMaps = append(parsedMaps, expanded) - } else { - parsedMaps = append(parsedMaps, refmap) + if !ok { + expanded = refMap } + parsedMaps = append(parsedMaps, expanded) } return parsedMaps -- GitLab From d4c05b75a9b92a2ff4d76fd8fc890cbd6d432815 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 24 Oct 2019 23:11:29 -0400 Subject: [PATCH 10/13] Fix typo in title of 'refmap' --- internal/git/remote/remote.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 03018301920..06082252f1b 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -149,7 +149,7 @@ func parseRefmaps(refmaps []string) []string { expanded, ok := stdRefmaps[refmap] if !ok { - expanded = refMap + expanded = refmap } parsedMaps = append(parsedMaps, expanded) } -- GitLab From 7c5f2377d27e67f1404b72c47fb9d687c999e766 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 24 Oct 2019 23:18:55 -0400 Subject: [PATCH 11/13] Add test cases for all abbreviated ref maps --- internal/git/remote/remote_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/git/remote/remote_test.go b/internal/git/remote/remote_test.go index 909491aec73..e06379173ff 100644 --- a/internal/git/remote/remote_test.go +++ b/internal/git/remote/remote_test.go @@ -40,8 +40,8 @@ func TestAddRemote(t *testing.T) { description: "resolves abbreviated refmap", remoteName: "abbrev-refmap", url: "https://gitlab.com/gitlab-org/git.git", - mirrorRefmaps: []string{"heads"}, - resolvedMirrorRefmaps: []string{"+refs/heads/*:refs/heads/*"}, + mirrorRefmaps: []string{"all_refs", "heads", "tags"}, + resolvedMirrorRefmaps: []string{"+refs/*:refs/*", "+refs/heads/*:refs/heads/*", "+refs/tags/*:refs/tags/*"}, }, { description: "adds multiple refmaps", -- GitLab From 22b569ed572ec2bb70faf411cd47bee75b9a3758 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Thu, 24 Oct 2019 23:44:08 -0400 Subject: [PATCH 12/13] Fix empty string refmaps triggering mirroring --- internal/git/remote/remote.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 06082252f1b..3831432c5ca 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -91,6 +91,12 @@ func Exists(ctx context.Context, repo repository.GitRepo, name string) (bool, er } func setMirror(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { + parsedMaps := parseRefmaps(refmaps) + + if len(parsedMaps) == 0 { + return nil + } + for _, configOption := range []string{"mirror", "prune"} { cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ Name: "config", @@ -107,13 +113,12 @@ func setMirror(ctx context.Context, repo repository.GitRepo, name string, refmap } } - return setRefmaps(ctx, repo, name, refmaps) + return setRefmaps(ctx, repo, name, parsedMaps) } func setRefmaps(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { - parsedMaps := parseRefmaps(refmaps) - for i, refmap := range parsedMaps { + for i, refmap := range refmaps { var flag git.Flag if i == 0 { flag = git.Flag{"--replace-all"} @@ -140,7 +145,7 @@ func setRefmaps(ctx context.Context, repo repository.GitRepo, name string, refma } func parseRefmaps(refmaps []string) []string { - parsedMaps := make([]string, len(refmaps)) + var parsedMaps []string for _, refmap := range refmaps { if len(refmap) == 0 { -- GitLab From 6344b66afc19548f6eeeae1ec0e61008ec91ac36 Mon Sep 17 00:00:00 2001 From: Will Chandler Date: Fri, 25 Oct 2019 00:21:56 -0400 Subject: [PATCH 13/13] Remove leading blank line to resolve formatting lint --- internal/git/remote/remote.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/git/remote/remote.go b/internal/git/remote/remote.go index 3831432c5ca..fe6d2b7024e 100644 --- a/internal/git/remote/remote.go +++ b/internal/git/remote/remote.go @@ -117,7 +117,6 @@ func setMirror(ctx context.Context, repo repository.GitRepo, name string, refmap } func setRefmaps(ctx context.Context, repo repository.GitRepo, name string, refmaps []string) error { - for i, refmap := range refmaps { var flag git.Flag if i == 0 { -- GitLab