Merge branch 'smh-fix-update-remote-mirror-bugs' into 'master'

Fix various UpdateRemoteMirror bugs

Closes #3508, #3505, #3503, #3504, and #3502

See merge request gitlab-org/gitaly!3699

6 files changed, 148 insertions, 92 deletions

diff --git a/internal/git/localrepo/refs.go b/internal/git/localrepo/refs.go
index 12f4c7ef7..ac9c69fd1 100644
--- a/internal/git/localrepo/refs.go
+++ b/internal/git/localrepo/refs.go
@@ -192,8 +192,7 @@ func WithConfig(config ...git.ConfigPair) GetRemoteReferencesOption {
 	}
 }
 
-// GetRemoteReferences lists references of the remote. Symbolic references are dereferenced. Peeled
-// tags are not returned.
+// GetRemoteReferences lists references of the remote. Peeled tags are not returned.
 func (repo *Repo) GetRemoteReferences(ctx context.Context, remote string, opts ...GetRemoteReferencesOption) ([]git.Reference, error) {
 	var cfg getRemoteReferenceConfig
 	for _, opt := range opts {
@@ -212,6 +211,7 @@ func (repo *Repo) GetRemoteReferences(ctx context.Context, remote string, opts .
 			Name: "ls-remote",
 			Flags: []git.Option{
 				git.Flag{Name: "--refs"},
+				git.Flag{Name: "--symref"},
 			},
 			Args: append([]string{remote}, cfg.patterns...),
 		},
@@ -226,13 +226,43 @@ func (repo *Repo) GetRemoteReferences(ctx context.Context, remote string, opts .
 	var refs []git.Reference
 	scanner := bufio.NewScanner(stdout)
 	for scanner.Scan() {
-		split := strings.SplitN(string(scanner.Bytes()), "\t", 2)
+		split := strings.SplitN(scanner.Text(), "\t", 2)
 		if len(split) != 2 {
-			return nil, fmt.Errorf("invalid ls-remote output line: %q", scanner.Bytes())
+			return nil, fmt.Errorf("invalid ls-remote output line: %q", scanner.Text())
+		}
+
+		// Symbolic references are outputted as:
+		//	ref: refs/heads/master	refs/heads/symbolic-ref
+		//	0c9cf732b5774fa948348bbd6f273009bd66e04c	refs/heads/symbolic-ref
+		if strings.HasPrefix(split[0], "ref: ") {
+			symRef := split[1]
+			if !scanner.Scan() {
+				if err := scanner.Err(); err != nil {
+					return nil, fmt.Errorf("scan dereferenced symbolic ref: %w", err)
+				}
+
+				return nil, fmt.Errorf("missing dereferenced symbolic ref line for %q", symRef)
+			}
+
+			split = strings.SplitN(scanner.Text(), "\t", 2)
+			if len(split) != 2 {
+				return nil, fmt.Errorf("invalid dereferenced symbolic ref line: %q", scanner.Text())
+			}
+
+			if split[1] != symRef {
+				return nil, fmt.Errorf("expected dereferenced symbolic ref %q but got reference %q", symRef, split[1])
+			}
+
+			refs = append(refs, git.NewSymbolicReference(git.ReferenceName(symRef), split[0]))
+			continue
 		}
 
 		refs = append(refs, git.NewReference(git.ReferenceName(split[1]), split[0]))
 	}
 
+	if err := scanner.Err(); err != nil {
+		return nil, fmt.Errorf("scan: %w", err)
+	}
+
 	return refs, nil
 }
diff --git a/internal/git/localrepo/refs_test.go b/internal/git/localrepo/refs_test.go
index b4a7c449b..4e9e911ba 100644
--- a/internal/git/localrepo/refs_test.go
+++ b/internal/git/localrepo/refs_test.go
@@ -268,7 +268,7 @@ func TestRepo_GetRemoteReferences(t *testing.T) {
 			remote: repoPath,
 			expected: []git.Reference{
 				{Name: "refs/heads/master", Target: commit},
-				{Name: "refs/heads/symbolic", Target: commit},
+				{Name: "refs/heads/symbolic", Target: commit, IsSymbolic: true},
 				{Name: "refs/remote/remote-name/remote-branch", Target: commit},
 				{Name: "refs/tags/annotated-tag", Target: annotatedTagOID},
 				{Name: "refs/tags/lightweight-tag", Target: commit},
@@ -282,7 +282,7 @@ func TestRepo_GetRemoteReferences(t *testing.T) {
 			},
 			expected: []git.Reference{
 				{Name: "refs/heads/master", Target: commit},
-				{Name: "refs/heads/symbolic", Target: commit},
+				{Name: "refs/heads/symbolic", Target: commit, IsSymbolic: true},
 				{Name: "refs/tags/annotated-tag", Target: annotatedTagOID},
 				{Name: "refs/tags/lightweight-tag", Target: commit},
 			},
diff --git a/internal/git/localrepo/remote.go b/internal/git/localrepo/remote.go
index bb659f6d2..be87ab495 100644
--- a/internal/git/localrepo/remote.go
+++ b/internal/git/localrepo/remote.go
@@ -283,6 +283,8 @@ type PushOptions struct {
 	// SSHCommand is the command line to use for git's SSH invocation. The command line is used
 	// as is and must be verified by the caller to be safe.
 	SSHCommand string
+	// Force decides whether to force push all of the refspecs.
+	Force bool
 	// Config is the Git configuration which gets passed to the git-push(1) invocation.
 	// Configuration is set up via `WithConfigEnv()`, so potential credentials won't be leaked
 	// via the command line.
@@ -300,11 +302,16 @@ func (repo *Repo) Push(ctx context.Context, remote string, refspecs []string, op
 		env = append(env, envGitSSHCommand(options.SSHCommand))
 	}
 
+	var flags []git.Option
+	if options.Force {
+		flags = append(flags, git.Flag{Name: "--force"})
+	}
+
 	stderr := &bytes.Buffer{}
 	if err := repo.ExecAndWait(ctx,
 		git.SubCmd{
 			Name:  "push",
-			Flags: []git.Option{git.Flag{Name: "--force"}},
+			Flags: flags,
 			Args:  append([]string{remote}, refspecs...),
 		},
 		git.WithStderr(stderr),
diff --git a/internal/git/localrepo/remote_test.go b/internal/git/localrepo/remote_test.go
index aa1d30e35..d653716d4 100644
--- a/internal/git/localrepo/remote_test.go
+++ b/internal/git/localrepo/remote_test.go
@@ -477,11 +477,33 @@ func TestRepo_Push(t *testing.T) {
 		return NewTestRepo(t, cfg, repoProto), repopath, nil
 	}
 
+	setupDivergedRepo := func(t testing.TB) (*Repo, string, []git.ConfigPair) {
+		repoProto, repoPath, _ := gittest.InitBareRepoAt(t, cfg, cfg.Storages[0])
+		repo := NewTestRepo(t, cfg, repoProto)
+
+		// set up master as a divergin ref in push repo
+		sourceMaster, err := sourceRepo.GetReference(ctx, "refs/heads/master")
+		require.NoError(t, err)
+
+		require.NoError(t, sourceRepo.Push(ctx, repoPath, []string{"refs/*"}, PushOptions{}))
+		divergedMaster := gittest.WriteCommit(t, cfg, repoPath,
+			gittest.WithBranch("master"),
+			gittest.WithParents(git.ObjectID(sourceMaster.Target)),
+		)
+
+		master, err := repo.GetReference(ctx, "refs/heads/master")
+		require.NoError(t, err)
+		require.Equal(t, master.Target, divergedMaster.String())
+
+		return repo, repoPath, nil
+	}
+
 	for _, tc := range []struct {
 		desc           string
 		setupPushRepo  func(testing.TB) (*Repo, string, []git.ConfigPair)
 		config         []git.ConfigPair
 		sshCommand     string
+		force          bool
 		refspecs       []string
 		errorMessage   string
 		expectedFilter []string
@@ -504,28 +526,16 @@ func TestRepo_Push(t *testing.T) {
 			expectedFilter: []string{"refs/heads/master"},
 		},
 		{
-			desc:     "force pushes over diverged refs",
-			refspecs: []string{"refs/heads/master"},
-			setupPushRepo: func(t testing.TB) (*Repo, string, []git.ConfigPair) {
-				repoProto, repoPath, _ := gittest.InitBareRepoAt(t, cfg, cfg.Storages[0])
-				repo := NewTestRepo(t, cfg, repoProto)
-
-				// set up master as a divergin ref in push repo
-				sourceMaster, err := sourceRepo.GetReference(ctx, "refs/heads/master")
-				require.NoError(t, err)
-
-				require.NoError(t, sourceRepo.Push(ctx, repoPath, []string{"refs/*"}, PushOptions{}))
-				divergedMaster := gittest.WriteCommit(t, cfg, repoPath,
-					gittest.WithBranch("master"),
-					gittest.WithParents(git.ObjectID(sourceMaster.Target)),
-				)
-
-				master, err := repo.GetReference(ctx, "refs/heads/master")
-				require.NoError(t, err)
-				require.Equal(t, master.Target, divergedMaster.String())
-
-				return repo, repoPath, nil
-			},
+			desc:          "doesn't force push over diverged refs with Force unset",
+			refspecs:      []string{"refs/heads/master"},
+			setupPushRepo: setupDivergedRepo,
+			errorMessage:  "Updates were rejected because the remote contains work that you do",
+		},
+		{
+			desc:          "force pushes over diverged refs with Force set",
+			refspecs:      []string{"refs/heads/master"},
+			force:         true,
+			setupPushRepo: setupDivergedRepo,
 		},
 		{
 			desc:          "push all refs",
@@ -564,10 +574,11 @@ func TestRepo_Push(t *testing.T) {
 
 			err := sourceRepo.Push(ctx, remote, tc.refspecs, PushOptions{
 				SSHCommand: tc.sshCommand,
+				Force:      tc.force,
 				Config:     remoteConfig,
 			})
 			if tc.errorMessage != "" {
-				require.EqualError(t, err, tc.errorMessage)
+				require.Contains(t, err.Error(), tc.errorMessage)
 				return
 			}
 			require.NoError(t, err)
diff --git a/internal/gitaly/service/remote/update_remote_mirror.go b/internal/gitaly/service/remote/update_remote_mirror.go
index e209c6001..bc7ed1dd2 100644
--- a/internal/gitaly/service/remote/update_remote_mirror.go
+++ b/internal/gitaly/service/remote/update_remote_mirror.go
@@ -106,11 +106,6 @@ func (s *server) updateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMi
 		return fmt.Errorf("get local references: %w", err)
 	}
 
-	if len(localRefs) == 0 {
-		// https://gitlab.com/gitlab-org/gitaly/-/issues/3503
-		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)
@@ -118,12 +113,22 @@ func (s *server) updateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMi
 
 	remoteRefs := make(map[git.ReferenceName]string, len(remoteRefsSlice))
 	for _, ref := range remoteRefsSlice {
+		if ref.IsSymbolic {
+			// There should be no symbolic refs in refs/heads/ or refs/tags, so we'll just ignore
+			// them if something has placed one there.
+			continue
+		}
+
 		remoteRefs[ref.Name] = ref.Target
 	}
 
 	var divergentRefs [][]byte
 	toUpdate := map[git.ReferenceName]string{}
 	for _, localRef := range localRefs {
+		if localRef.IsSymbolic {
+			continue
+		}
+
 		remoteTarget, ok := remoteRefs[localRef.Name]
 		if !ok {
 			// ref does not exist on the mirror, it should be created
@@ -158,11 +163,6 @@ func (s *server) updateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMi
 			}
 		}
 
-		if localRef.Name == "refs/heads/tag" {
-			// https://gitlab.com/gitlab-org/gitaly/-/issues/3502
-			return errors.New("close stream to gitaly-ruby: rpc error: code = Unknown desc = Gitlab::Git::CommandError: fatal: tag shorthand without <tag>")
-		}
-
 		// the mirror's ref does not match ours, we should update it.
 		toUpdate[localRef.Name] = localRef.Target
 		delete(remoteRefs, localRef.Name)
@@ -173,7 +173,6 @@ func (s *server) updateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMi
 		toDelete = map[git.ReferenceName]string{}
 	}
 
-	seen := map[string]struct{}{}
 	var refspecs []string
 	for prefix, references := range map[string]map[git.ReferenceName]string{
 		"": toUpdate, ":": toDelete,
@@ -191,18 +190,6 @@ func (s *server) updateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMi
 				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/")
-			if strings.HasPrefix(reference.String(), "refs/tags/") {
-				name = strings.TrimPrefix(reference.String(), "refs/tags/")
-			}
-
-			if _, ok := seen[name]; ok {
-				return fmt.Errorf("close stream to gitaly-ruby: rpc error: code = Unknown desc = Gitlab::Git::CommandError: error: src refspec %v matches more than one", reference)
-			}
-
-			seen[name] = struct{}{}
 		}
 	}
 
@@ -214,12 +201,9 @@ func (s *server) updateRemoteMirror(stream gitalypb.RemoteService_UpdateRemoteMi
 
 		refspecs = refspecs[len(batch):]
 
-		// The refs could have been modified on the mirror during after we fetched them.
-		// This could cause divergent refs to be force pushed over even with keep_divergent_refs set.
-		// This could be addressed by force pushing only if the current ref still matches what
-		// we received in the original fetch. https://gitlab.com/gitlab-org/gitaly/-/issues/3505
 		if err := repo.Push(ctx, remoteName, batch, localrepo.PushOptions{
 			SSHCommand: sshCommand,
+			Force:      !firstRequest.KeepDivergentRefs,
 			Config:     remoteConfig,
 		}); err != nil {
 			return fmt.Errorf("push to mirror: %w", err)
diff --git a/internal/gitaly/service/remote/update_remote_mirror_test.go b/internal/gitaly/service/remote/update_remote_mirror_test.go
index 9b192c57f..9a37134ee 100644
--- a/internal/gitaly/service/remote/update_remote_mirror_test.go
+++ b/internal/gitaly/service/remote/update_remote_mirror_test.go
@@ -59,12 +59,12 @@ func TestUpdateRemoteMirror(t *testing.T) {
 		expectedMirrorRefs   map[string]string
 	}{
 		{
-			// https://gitlab.com/gitlab-org/gitaly/-/issues/3503
-			desc: "empty mirror source fails",
+			desc: "empty mirror source works",
 			mirrorRefs: refs{
 				"refs/heads/tags": {"commit 1"},
 			},
-			errorContains: "rpc error: code = Internal desc = close stream to gitaly-ruby: rpc error: code = Unknown desc = NoMethodError: undefined method `id' for nil:NilClass",
+			response:           &gitalypb.UpdateRemoteMirrorResponse{},
+			expectedMirrorRefs: map[string]string{},
 		},
 		{
 			desc:     "mirror is up to date",
@@ -180,25 +180,29 @@ func TestUpdateRemoteMirror(t *testing.T) {
 			},
 		},
 		{
-			// https://gitlab.com/gitlab-org/gitaly/-/issues/3502
-			desc: "updating branch called tag fails",
+			desc: "updating branch called tag works",
 			sourceRefs: refs{
 				"refs/heads/tag": {"commit 1", "commit 2"},
 			},
 			mirrorRefs: refs{
 				"refs/heads/tag": {"commit 1"},
 			},
-			errorContains: "rpc error: code = Internal desc = close stream to gitaly-ruby: rpc error: code = Unknown desc = Gitlab::Git::CommandError: fatal: tag shorthand without <tag>",
+			response: &gitalypb.UpdateRemoteMirrorResponse{},
+			expectedMirrorRefs: map[string]string{
+				"refs/heads/tag": "commit 2",
+			},
 		},
 		{
-			// https://gitlab.com/gitlab-org/gitaly/-/issues/3504
-			// Truncate error as either refs/heads/master or refs/tags/master may be returned
-			desc: "fails if tag and branch named the same",
+			desc: "works if tag and branch named the same",
 			sourceRefs: refs{
 				"refs/heads/master": {"commit 1"},
 				"refs/tags/master":  {"commit 1"},
 			},
-			errorContains: "rpc error: code = Internal desc = close stream to gitaly-ruby: rpc error: code = Unknown desc = Gitlab::Git::CommandError: error: src refspec refs/",
+			response: &gitalypb.UpdateRemoteMirrorResponse{},
+			expectedMirrorRefs: map[string]string{
+				"refs/heads/master": "commit 1",
+				"refs/tags/master":  "commit 1",
+			},
 		},
 		{
 			desc: "only local branches are considered",
@@ -296,25 +300,43 @@ func TestUpdateRemoteMirror(t *testing.T) {
 			},
 		},
 		{
-			// https://gitlab.com/gitlab-org/gitaly/-/issues/3508
-			desc: "mirror is up to date with symbolic reference",
+			desc: "doesn't force push over refs that diverged after they were checked with KeepDivergentRefs",
 			sourceRefs: refs{
-				"refs/heads/master": {"commit 1"},
-			},
-			sourceSymRefs: map[string]string{
-				"refs/heads/symbolic-reference": "refs/heads/master",
+				"refs/heads/diverging":     {"commit 1", "commit 2"},
+				"refs/heads/non-diverging": {"commit-3"},
 			},
 			mirrorRefs: refs{
-				"refs/heads/master": {"commit 1"},
+				"refs/heads/diverging":     {"commit 1"},
+				"refs/heads/non-diverging": {"commit-3"},
 			},
-			response: &gitalypb.UpdateRemoteMirrorResponse{},
-			expectedMirrorRefs: map[string]string{
-				"refs/heads/master": "commit 1",
+			keepDivergentRefs: true,
+			wrapCommandFactory: func(t testing.TB, original git.CommandFactory) git.CommandFactory {
+				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" {
+							// Make the branch diverge on the remote before actually performing the pushes the RPC
+							// is attempting to perform to simulate a ref diverging after the RPC has performed
+							// its checks.
+							cmd, err := original.New(ctx, repo, git.SubCmd{
+								Name:  "push",
+								Flags: []git.Option{git.Flag{Name: "--force"}},
+								Args:  []string{"mirror", "refs/heads/non-diverging:refs/heads/diverging"},
+							})
+							if !assert.NoError(t, err) {
+								return nil, err
+							}
+							assert.NoError(t, cmd.Wait())
+						}
+
+						return original.New(ctx, repo, sc, opts...)
+					},
+				}
 			},
+			errorContains: "Updates were rejected because a pushed branch tip is behind its remote",
 		},
 		{
-			// https://gitlab.com/gitlab-org/gitaly/-/issues/3508
-			desc: "updates branch pointed to by symbolic reference",
+			desc: "ignores symbolic references in source repo",
 			sourceRefs: refs{
 				"refs/heads/master": {"commit 1"},
 			},
@@ -323,16 +345,10 @@ func TestUpdateRemoteMirror(t *testing.T) {
 			},
 			onlyBranchesMatching: []string{"symbolic-reference"},
 			response:             &gitalypb.UpdateRemoteMirrorResponse{},
-			expectedMirrorRefs: map[string]string{
-				"refs/heads/master": "commit 1",
-			},
+			expectedMirrorRefs:   map[string]string{},
 		},
 		{
-			// https://gitlab.com/gitlab-org/gitaly/-/issues/3508
-			//
-			// refs/heads/master gets removed but and a broken sym ref is left in
-			// refs/heads/symbolic-reference
-			desc: "removes symbolic ref target from mirror if not symbolic ref is not present locally",
+			desc: "ignores symbolic refs on the mirror",
 			sourceRefs: refs{
 				"refs/heads/master": {"commit 1"},
 			},
@@ -342,19 +358,27 @@ func TestUpdateRemoteMirror(t *testing.T) {
 			mirrorSymRefs: map[string]string{
 				"refs/heads/symbolic-reference": "refs/heads/master",
 			},
-			response:           &gitalypb.UpdateRemoteMirrorResponse{},
-			expectedMirrorRefs: map[string]string{},
+			response: &gitalypb.UpdateRemoteMirrorResponse{},
+			expectedMirrorRefs: map[string]string{
+				// If the symbolic reference was not ignored, master would get deleted
+				// as it's the branch pointed to by a symbolic ref not present in the source
+				// repo.
+				"refs/heads/master":             "commit 1",
+				"refs/heads/symbolic-reference": "commit 1",
+			},
 		},
 		{
-			// https://gitlab.com/gitlab-org/gitaly/-/issues/3508
-			desc: "fails with symbolic reference and target in the same push",
+			desc: "ignores symbolic refs and pushes the branch successfully",
 			sourceRefs: refs{
 				"refs/heads/master": {"commit 1"},
 			},
 			sourceSymRefs: map[string]string{
 				"refs/heads/symbolic-reference": "refs/heads/master",
 			},
-			errorContains: "remote: error: cannot lock ref 'refs/heads/master': reference already exists",
+			response: &gitalypb.UpdateRemoteMirrorResponse{},
+			expectedMirrorRefs: map[string]string{
+				"refs/heads/master": "commit 1",
+			},
 		},
 		{
 			desc: "push batching works",