repository: Stop pruning if NoPrune was passed to FetchRemote

The FetchRemote RPC accepts a NoPrune parameter, which if set should
cause us to not prune remote references. We fail to pay attention to
that parameter though and always invoke git-fetch(1) with the "--prune"
parameter and inject "remote.$REMOTE.prune=true".

Let's fix this bug by passing the NoPrune parameter to git-fetch(1). We
can also get rid of the injected config entry as it's essentially
duplicating the "--prune" parameter.

2 files changed, 121 insertions, 7 deletions

diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go
index ad3d72901..bfc351802 100644
--- a/internal/gitaly/service/repository/fetch_remote.go
+++ b/internal/gitaly/service/repository/fetch_remote.go
@@ -29,17 +29,22 @@ func (s *server) FetchRemote(ctx context.Context, req *gitalypb.FetchRemoteReque
 	}
 
 	var stderr bytes.Buffer
-	opts := git.FetchOpts{Stderr: &stderr, Force: req.Force, Prune: true, Tags: git.FetchOptsTagsAll, Verbose: req.GetCheckTagsChanged()}
+	opts := git.FetchOpts{
+		Stderr:  &stderr,
+		Force:   req.Force,
+		Prune:   !req.NoPrune,
+		Tags:    git.FetchOptsTagsAll,
+		Verbose: req.GetCheckTagsChanged(),
+	}
 
 	if req.GetNoTags() {
 		opts.Tags = git.FetchOptsTagsNone
 	}
 
 	repo := git.NewRepository(req.GetRepository(), s.cfg)
-	params := req.GetRemoteParams()
 	remoteName := req.GetRemote()
 
-	if params != nil {
+	if params := req.GetRemoteParams(); params != nil {
 		remoteName = params.GetName()
 		remoteURL := params.GetUrl()
 		refspecs := s.getRefspecs(params.GetMirrorRefmaps())
@@ -66,10 +71,6 @@ func (s *server) FetchRemote(ctx context.Context, req *gitalypb.FetchRemoteReque
 			opts.Global = append(opts.Global, git.ConfigPair{Key: "remote." + remoteName + ".fetch", Value: refspec})
 		}
 
-		opts.Global = append(opts.Global,
-			git.ConfigPair{Key: "remote." + remoteName + ".prune", Value: "true"},
-		)
-
 		if params.GetHttpAuthorizationHeader() != "" {
 			client, err := s.ruby.RepositoryServiceClient(ctx)
 			if err != nil {
diff --git a/internal/gitaly/service/repository/fetch_remote_test.go b/internal/gitaly/service/repository/fetch_remote_test.go
index 7614e9458..84c049c5d 100644
--- a/internal/gitaly/service/repository/fetch_remote_test.go
+++ b/internal/gitaly/service/repository/fetch_remote_test.go
@@ -120,6 +120,119 @@ func TestFetchRemote_withDefaultRefmaps(t *testing.T) {
 	require.Equal(t, sourceRefs, targetRefs)
 }
 
+func TestFetchRemote_prune(t *testing.T) {
+	locator := config.NewLocator(config.Config)
+	serverSocketPath, stop := runRepoServer(t, locator, testhelper.WithInternalSocket(config.Config))
+	defer stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	sourceRepo, sourceRepoPath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	port, stopGitServer := testhelper.GitServer(t, config.Config, sourceRepoPath, nil)
+	defer func() { require.NoError(t, stopGitServer()) }()
+
+	remoteURL := fmt.Sprintf("http://127.0.0.1:%d/%s", port, filepath.Base(sourceRepoPath))
+
+	for _, tc := range []struct {
+		desc        string
+		request     *gitalypb.FetchRemoteRequest
+		ref         git.ReferenceName
+		shouldExist bool
+	}{
+		{
+			desc: "NoPrune=true should not delete reference matching remote's refspec",
+			request: &gitalypb.FetchRemoteRequest{
+				Remote:  "my-remote",
+				NoPrune: true,
+			},
+			ref:         "refs/remotes/my-remote/nonexistent",
+			shouldExist: true,
+		},
+		{
+			desc: "NoPrune=false should delete reference matching remote's refspec",
+			request: &gitalypb.FetchRemoteRequest{
+				Remote:  "my-remote",
+				NoPrune: false,
+			},
+			ref:         "refs/remotes/my-remote/nonexistent",
+			shouldExist: false,
+		},
+		{
+			desc: "NoPrune=false should not delete ref outside of remote's refspec",
+			request: &gitalypb.FetchRemoteRequest{
+				Remote:  "my-remote",
+				NoPrune: false,
+			},
+			ref:         "refs/heads/nonexistent",
+			shouldExist: true,
+		},
+		{
+			desc: "NoPrune=true with explicit Remote should not delete reference",
+			request: &gitalypb.FetchRemoteRequest{
+				RemoteParams: &gitalypb.Remote{
+					Url:  remoteURL,
+					Name: "my-remote",
+				},
+				NoPrune: true,
+			},
+			ref:         "refs/heads/nonexistent",
+			shouldExist: true,
+		},
+		{
+			desc: "NoPrune=false with explicit Remote should delete reference",
+			request: &gitalypb.FetchRemoteRequest{
+				RemoteParams: &gitalypb.Remote{
+					Url:  remoteURL,
+					Name: "my-remote",
+				},
+				NoPrune: false,
+			},
+			ref:         "refs/heads/nonexistent",
+			shouldExist: false,
+		},
+		{
+			desc: "NoPrune=false with explicit Remote should not delete reference outside of refspec",
+			request: &gitalypb.FetchRemoteRequest{
+				RemoteParams: &gitalypb.Remote{
+					Url:  remoteURL,
+					Name: "my-remote",
+					MirrorRefmaps: []string{
+						"refs/heads/*:refs/remotes/my-remote/*",
+					},
+				},
+				NoPrune: false,
+			},
+			ref:         "refs/heads/nonexistent",
+			shouldExist: true,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			targetRepoProto, targetRepoPath := copyRepoWithNewRemote(t, sourceRepo, locator, "my-remote")
+			defer func() {
+				require.NoError(t, os.RemoveAll(targetRepoPath))
+			}()
+			targetRepo := git.NewRepository(targetRepoProto, config.Config)
+
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			require.NoError(t, targetRepo.UpdateRef(ctx, tc.ref, "refs/heads/master", ""))
+
+			tc.request.Repository = targetRepoProto
+			resp, err := client.FetchRemote(ctx, tc.request)
+			require.NoError(t, err)
+			require.NotNil(t, resp)
+
+			hasRevision, err := targetRepo.HasRevision(ctx, tc.ref.Revision())
+			require.NoError(t, err)
+			require.Equal(t, tc.shouldExist, hasRevision)
+		})
+	}
+}
+
 func TestFetchRemoteFailure(t *testing.T) {
 	repo, _, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()