Merge branch 'smh-update-remote-mirror-fixes' into 'master'

Align UpdateRemoteMirror with Ruby implementation of the RPC

See merge request gitlab-org/gitaly!3506

2 files changed, 125 insertions, 8 deletions

diff --git a/internal/gitaly/service/remote/update_remote_mirror.go b/internal/gitaly/service/remote/update_remote_mirror.go
index 05e5a782c..337c65130 100644
--- a/internal/gitaly/service/remote/update_remote_mirror.go
+++ b/internal/gitaly/service/remote/update_remote_mirror.go
@@ -10,13 +10,19 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
+	"gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
-// PushBatchSize is the maximum number of branches to push in a single push call.
-const PushBatchSize = 10
+const (
+	// pushBatchSize is the maximum number of branches to push in a single push call.
+	pushBatchSize = 10
+	// maxDivergentRefs is the maximum number of divergent refs to return in UpdateRemoteMirror's
+	// response.
+	maxDivergentRefs = 100
+)
 
 func (s *server) UpdateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMirrorServer) error {
 	firstRequest, err := stream.Recv()
@@ -129,6 +135,11 @@ func (s *server) goUpdateRemoteMirror(stream gitalypb.RemoteService_UpdateRemote
 		return errors.New("close stream to gitaly-ruby: rpc error: code = Unknown desc = NoMethodError: undefined method `id' for nil:NilClass")
 	}
 
+	defaultBranch, err := ref.DefaultBranchName(ctx, repo)
+	if err != nil {
+		return fmt.Errorf("get default branch: %w", err)
+	}
+
 	remoteRefs := make(map[git.ReferenceName]string, len(remoteRefsSlice))
 	for _, ref := range remoteRefsSlice {
 		remoteRefs[ref.Name] = ref.Target
@@ -160,7 +171,7 @@ func (s *server) goUpdateRemoteMirror(stream gitalypb.RemoteService_UpdateRemote
 			if !isAncestor {
 				// The mirror's reference has diverged from the local ref, or the mirror contains a commit
 				// which is not present in the local repository.
-				if referenceMatcher.MatchString(localRef.Name.String()) {
+				if referenceMatcher.MatchString(localRef.Name.String()) && len(divergentRefs) < maxDivergentRefs {
 					// diverged branches on the mirror are only included in the response if they match
 					// one of the branches in the selector
 					divergentRefs = append(divergentRefs, []byte(localRef.Name))
@@ -197,6 +208,13 @@ func (s *server) goUpdateRemoteMirror(stream gitalypb.RemoteService_UpdateRemote
 			}
 
 			refspecs = append(refspecs, prefix+reference.String())
+			if reference == git.ReferenceName(defaultBranch) {
+				// The default branch needs to be pushed in the first batch of refspecs as some features
+				// depend on it existing in the repository. The default branch may not exist in the repo
+				// yet if this is the first mirroring push.
+				last := len(refspecs) - 1
+				refspecs[0], refspecs[last] = refspecs[last], refspecs[0]
+			}
 
 			// https://gitlab.com/gitlab-org/gitaly/-/issues/3504
 			name := strings.TrimPrefix(reference.String(), "refs/heads/")
@@ -221,8 +239,8 @@ func (s *server) goUpdateRemoteMirror(stream gitalypb.RemoteService_UpdateRemote
 
 		for len(refspecs) > 0 {
 			batch := refspecs
-			if len(refspecs) > PushBatchSize {
-				batch = refspecs[:PushBatchSize]
+			if len(refspecs) > pushBatchSize {
+				batch = refspecs[:pushBatchSize]
 			}
 
 			refspecs = refspecs[len(batch):]
diff --git a/internal/gitaly/service/remote/update_remote_mirror_test.go b/internal/gitaly/service/remote/update_remote_mirror_test.go
index 705ea747f..c3c7f526f 100644
--- a/internal/gitaly/service/remote/update_remote_mirror_test.go
+++ b/internal/gitaly/service/remote/update_remote_mirror_test.go
@@ -8,9 +8,12 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
@@ -32,8 +35,16 @@ func testUpdateRemoteMirror(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Se
 	})
 }
 
+type commandFactoryWrapper struct {
+	git.CommandFactory
+	newFunc func(context.Context, repository.GitRepo, git.Cmd, ...git.CmdOpt) (*command.Command, error)
+}
+
+func (w commandFactoryWrapper) New(ctx context.Context, repo repository.GitRepo, sc git.Cmd, opts ...git.CmdOpt) (*command.Command, error) {
+	return w.newFunc(ctx, repo, sc, opts...)
+}
+
 func testUpdateRemoteMirrorFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	cfg, _, _, client := setupRemoteServiceWithRuby(t, cfg, rubySrv)
 	testhelper.ConfigureGitalyGit2GoBin(t, cfg)
 
 	type refs map[string][]string
@@ -46,6 +57,7 @@ func testUpdateRemoteMirrorFeatured(t *testing.T, ctx context.Context, cfg confi
 		mirrorSymRefs        map[string]string
 		keepDivergentRefs    bool
 		onlyBranchesMatching []string
+		wrapCommandFactory   func(testing.TB, git.CommandFactory) git.CommandFactory
 		requests             []*gitalypb.UpdateRemoteMirrorRequest
 		errorContains        string
 		response             *gitalypb.UpdateRemoteMirrorResponse
@@ -352,7 +364,7 @@ func testUpdateRemoteMirrorFeatured(t *testing.T, ctx context.Context, cfg confi
 			desc: "push batching works",
 			sourceRefs: func() refs {
 				out := refs{}
-				for i := 0; i < 2*PushBatchSize+1; i++ {
+				for i := 0; i < 2*pushBatchSize+1; i++ {
 					out[fmt.Sprintf("refs/heads/branch-%d", i)] = []string{"commit 1"}
 				}
 				return out
@@ -360,12 +372,80 @@ func testUpdateRemoteMirrorFeatured(t *testing.T, ctx context.Context, cfg confi
 			response: &gitalypb.UpdateRemoteMirrorResponse{},
 			expectedMirrorRefs: func() map[string]string {
 				out := map[string]string{}
-				for i := 0; i < 2*PushBatchSize+1; i++ {
+				for i := 0; i < 2*pushBatchSize+1; i++ {
 					out[fmt.Sprintf("refs/heads/branch-%d", i)] = "commit 1"
 				}
 				return out
 			}(),
 		},
+		{
+			desc: "pushes default branch in the first batch",
+			wrapCommandFactory: func(t testing.TB, original git.CommandFactory) git.CommandFactory {
+				firstPush := true
+				return commandFactoryWrapper{
+					CommandFactory: original,
+					newFunc: func(ctx context.Context, repo repository.GitRepo, sc git.Cmd, opts ...git.CmdOpt) (*command.Command, error) {
+						if sc.Subcommand() == "push" && firstPush {
+							firstPush = false
+							args, err := sc.CommandArgs()
+							assert.NoError(t, err)
+							assert.Contains(t, args, "refs/heads/master", "first push should contain the default branch")
+						}
+
+						return original.New(ctx, repo, sc, opts...)
+					},
+				}
+			},
+			sourceRefs: func() refs {
+				out := refs{"refs/heads/master": []string{"commit 1"}}
+				for i := 0; i < 2*pushBatchSize; i++ {
+					out[fmt.Sprintf("refs/heads/branch-%d", i)] = []string{"commit 1"}
+				}
+				return out
+			}(),
+			response: &gitalypb.UpdateRemoteMirrorResponse{},
+			expectedMirrorRefs: func() map[string]string {
+				out := map[string]string{"refs/heads/master": "commit 1"}
+				for i := 0; i < 2*pushBatchSize; i++ {
+					out[fmt.Sprintf("refs/heads/branch-%d", i)] = "commit 1"
+				}
+				return out
+			}(),
+		},
+		{
+			desc: "limits the number of divergent refs returned",
+			sourceRefs: func() refs {
+				out := refs{}
+				for i := 0; i < maxDivergentRefs+1; i++ {
+					out[fmt.Sprintf("refs/heads/branch-%03d", i)] = []string{"commit 1"}
+				}
+				return out
+			}(),
+			mirrorRefs: func() refs {
+				out := refs{}
+				for i := 0; i < maxDivergentRefs+1; i++ {
+					out[fmt.Sprintf("refs/heads/branch-%03d", i)] = []string{"commit 2"}
+				}
+				return out
+			}(),
+			keepDivergentRefs: true,
+			response: &gitalypb.UpdateRemoteMirrorResponse{
+				DivergentRefs: func() [][]byte {
+					out := make([][]byte, maxDivergentRefs)
+					for i := range out {
+						out[i] = []byte(fmt.Sprintf("refs/heads/branch-%03d", i))
+					}
+					return out
+				}(),
+			},
+			expectedMirrorRefs: func() map[string]string {
+				out := map[string]string{}
+				for i := 0; i < maxDivergentRefs+1; i++ {
+					out[fmt.Sprintf("refs/heads/branch-%03d", i)] = "commit 2"
+				}
+				return out
+			}(),
+		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			_, mirrorRepoPath, cleanMirrorRepo := gittest.InitBareRepoAt(t, cfg, cfg.Storages[0])
@@ -413,6 +493,25 @@ func testUpdateRemoteMirrorFeatured(t *testing.T, ctx context.Context, cfg confi
 				}
 			}
 
+			addr := testserver.RunGitalyServer(t, cfg, rubySrv, func(srv *grpc.Server, deps *service.Dependencies) {
+				cmdFactory := deps.GetGitCmdFactory()
+				if tc.wrapCommandFactory != nil {
+					cmdFactory = tc.wrapCommandFactory(t, deps.GetGitCmdFactory())
+				}
+
+				gitalypb.RegisterRemoteServiceServer(srv, NewServer(
+					deps.GetCfg(),
+					deps.GetRubyServer(),
+					deps.GetLocator(),
+					cmdFactory,
+					deps.GetCatfileCache(),
+					deps.GetTxManager(),
+				))
+			})
+
+			client, conn := newRemoteClient(t, addr)
+			defer conn.Close()
+
 			stream, err := client.UpdateRemoteMirror(ctx)
 			require.NoError(t, err)