Merge branch 'pks-fetch-remote-fix-pruning' into 'master'

repository: Fix regressions in FetchRemote

See merge request gitlab-org/gitaly!3043

6 files changed, 264 insertions, 25 deletions

diff --git a/changelogs/unreleased/pks-fetch-remote-fix-pruning.yml b/changelogs/unreleased/pks-fetch-remote-fix-pruning.yml
new file mode 100644
index 000000000..19d1411ee
--- /dev/null
+++ b/changelogs/unreleased/pks-fetch-remote-fix-pruning.yml
@@ -0,0 +1,5 @@
+---
+title: 'repository: Fix regressions in FetchRemote'
+merge_request: 3043
+author:
+type: fixed
diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go
index 29f7d56de..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,12 +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 + ".mirror", Value: "true"},
-			git.ConfigPair{Key: "remote." + remoteName + ".prune", Value: "true"},
-			git.ConfigPair{Key: "http.followRedirects", Value: "false"},
-		)
-
 		if params.GetHttpAuthorizationHeader() != "" {
 			client, err := s.ruby.RepositoryServiceClient(ctx)
 			if err != nil {
@@ -129,6 +128,10 @@ func (s *server) FetchRemote(ctx context.Context, req *gitalypb.FetchRemoteReque
 		defer cancel()
 	}
 
+	opts.Global = append(opts.Global,
+		git.ConfigPair{Key: "http.followRedirects", Value: "false"},
+	)
+
 	if err := repo.FetchRemote(ctx, remoteName, opts); err != nil {
 		if _, ok := status.FromError(err); ok {
 			// this check is used because of internal call to alternates.PathAndEnv
@@ -201,6 +204,10 @@ func (s *server) validateFetchRemoteRequest(req *gitalypb.FetchRemoteRequest) er
 }
 
 func (s *server) getRefspecs(refmaps []string) []string {
+	if len(refmaps) == 0 {
+		return []string{"+refs/*:refs/*"}
+	}
+
 	refspecs := make([]string, 0, len(refmaps))
 
 	for _, refmap := range refmaps {
diff --git a/internal/gitaly/service/repository/fetch_remote_test.go b/internal/gitaly/service/repository/fetch_remote_test.go
index c0577df6a..84c049c5d 100644
--- a/internal/gitaly/service/repository/fetch_remote_test.go
+++ b/internal/gitaly/service/repository/fetch_remote_test.go
@@ -13,6 +13,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/storage"
@@ -73,6 +74,165 @@ func TestFetchRemoteSuccess(t *testing.T) {
 	require.Equal(t, resp.TagsChanged, true)
 }
 
+func TestFetchRemote_withDefaultRefmaps(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()
+
+	sourceRepoProto, sourceRepoPath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+	sourceRepo := git.NewRepository(sourceRepoProto, config.Config)
+
+	targetRepoProto, targetRepoPath := copyRepoWithNewRemote(t, sourceRepoProto, locator, "my-remote")
+	defer func() {
+		require.NoError(t, os.RemoveAll(targetRepoPath))
+	}()
+	targetRepo := git.NewRepository(targetRepoProto, config.Config)
+
+	port, stopGitServer := testhelper.GitServer(t, config.Config, sourceRepoPath, nil)
+	defer func() { require.NoError(t, stopGitServer()) }()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	require.NoError(t, sourceRepo.UpdateRef(ctx, "refs/heads/foobar", "refs/heads/master", ""))
+
+	// With no refmap given, FetchRemote should fall back to
+	// "refs/heads/*:refs/heads/*" and thus mirror what's in the source
+	// repository.
+	resp, err := client.FetchRemote(ctx, &gitalypb.FetchRemoteRequest{
+		Repository: targetRepoProto,
+		RemoteParams: &gitalypb.Remote{
+			Url:  fmt.Sprintf("http://127.0.0.1:%d/%s", port, filepath.Base(sourceRepoPath)),
+			Name: "my-remote",
+		},
+	})
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+
+	sourceRefs, err := sourceRepo.GetReferences(ctx, "")
+	require.NoError(t, err)
+	targetRefs, err := targetRepo.GetReferences(ctx, "")
+	require.NoError(t, err)
+	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()
diff --git a/proto/go/gitalypb/repository-service.pb.go b/proto/go/gitalypb/repository-service.pb.go
index b2834ab21..00f18bfef 100644
--- a/proto/go/gitalypb/repository-service.pb.go
+++ b/proto/go/gitalypb/repository-service.pb.go
@@ -780,15 +780,26 @@ func (m *ApplyGitattributesResponse) XXX_DiscardUnknown() {
 var xxx_messageInfo_ApplyGitattributesResponse proto.InternalMessageInfo
 
 type FetchRemoteRequest struct {
-	Repository   *Repository `protobuf:"bytes,1,opt,name=repository,proto3" json:"repository,omitempty"`
-	Remote       string      `protobuf:"bytes,2,opt,name=remote,proto3" json:"remote,omitempty"`
-	Force        bool        `protobuf:"varint,3,opt,name=force,proto3" json:"force,omitempty"`
-	NoTags       bool        `protobuf:"varint,4,opt,name=no_tags,json=noTags,proto3" json:"no_tags,omitempty"`
-	Timeout      int32       `protobuf:"varint,5,opt,name=timeout,proto3" json:"timeout,omitempty"`
-	SshKey       string      `protobuf:"bytes,6,opt,name=ssh_key,json=sshKey,proto3" json:"ssh_key,omitempty"`
-	KnownHosts   string      `protobuf:"bytes,7,opt,name=known_hosts,json=knownHosts,proto3" json:"known_hosts,omitempty"`
-	NoPrune      bool        `protobuf:"varint,9,opt,name=no_prune,json=noPrune,proto3" json:"no_prune,omitempty"`
-	RemoteParams *Remote     `protobuf:"bytes,10,opt,name=remote_params,json=remoteParams,proto3" json:"remote_params,omitempty"`
+	Repository *Repository `protobuf:"bytes,1,opt,name=repository,proto3" json:"repository,omitempty"`
+	// remote is the name of the remote that shall be fetched. This remote must
+	// exist in the repository's configuration already. This parameter is
+	// deprecated in favor of remote_params.
+	Remote string `protobuf:"bytes,2,opt,name=remote,proto3" json:"remote,omitempty"`
+	// force determines if references should be force-updated in case they have
+	// diverged.
+	Force bool `protobuf:"varint,3,opt,name=force,proto3" json:"force,omitempty"`
+	// no_tags determines whether tags should be fetched.
+	NoTags bool `protobuf:"varint,4,opt,name=no_tags,json=noTags,proto3" json:"no_tags,omitempty"`
+	// timeout specifies a timeout for the fetch.
+	Timeout    int32  `protobuf:"varint,5,opt,name=timeout,proto3" json:"timeout,omitempty"`
+	SshKey     string `protobuf:"bytes,6,opt,name=ssh_key,json=sshKey,proto3" json:"ssh_key,omitempty"`
+	KnownHosts string `protobuf:"bytes,7,opt,name=known_hosts,json=knownHosts,proto3" json:"known_hosts,omitempty"`
+	// no_prune will the fetch to not prune remote references which do not exist
+	// in the remote repository anymore.
+	NoPrune bool `protobuf:"varint,9,opt,name=no_prune,json=noPrune,proto3" json:"no_prune,omitempty"`
+	// remote_params specifies the remote repository which should be fetched
+	// from.
+	RemoteParams *Remote `protobuf:"bytes,10,opt,name=remote_params,json=remoteParams,proto3" json:"remote_params,omitempty"`
 	// If check_tags_changed is true, the FetchRemote RPC will check whether any
 	// tags were modified, returning the result in the tags_changed field of
 	// FetchRemoteResponse
@@ -3335,14 +3346,32 @@ func (m *SearchFilesByContentResponse) GetEndOfMatch() bool {
 	return false
 }
 
+// Remote represents a git remote repository.
 type Remote struct {
-	Url                     string   `protobuf:"bytes,1,opt,name=url,proto3" json:"url,omitempty"`
-	Name                    string   `protobuf:"bytes,2,opt,name=name,proto3" json:"name,omitempty"`
-	HttpAuthorizationHeader string   `protobuf:"bytes,3,opt,name=http_authorization_header,json=httpAuthorizationHeader,proto3" json:"http_authorization_header,omitempty"`
-	MirrorRefmaps           []string `protobuf:"bytes,4,rep,name=mirror_refmaps,json=mirrorRefmaps,proto3" json:"mirror_refmaps,omitempty"`
-	XXX_NoUnkeyedLiteral    struct{} `json:"-"`
-	XXX_unrecognized        []byte   `json:"-"`
-	XXX_sizecache           int32    `json:"-"`
+	// url is the URL of the remote repository.
+	Url string `protobuf:"bytes,1,opt,name=url,proto3" json:"url,omitempty"`
+	// name is the name of the remote repository. This is mainly used to
+	// determine where fetched references should end up, e.g. in
+	// `refs/remotes/$name/`.
+	Name string `protobuf:"bytes,2,opt,name=name,proto3" json:"name,omitempty"`
+	// http_authorization_header is the HTTP header which should be added to
+	// the request in order to authenticate against the repository.
+	HttpAuthorizationHeader string `protobuf:"bytes,3,opt,name=http_authorization_header,json=httpAuthorizationHeader,proto3" json:"http_authorization_header,omitempty"`
+	// mirror_refmaps contains the refspecs which shall be fetched. Some special
+	// refspecs are accepted:
+	//
+	// - "all_refs" gets translated to "+refs/*:refs/*", which mirrors all
+	//   references of the source repository.
+	// - "heads" gets translated to "+refs/heads/*:refs/heads/*", which mirrors
+	//   all branches of the source repository.
+	// - "tags" gets translated to "+refs/tags/*:refs/tags/*", which mirrors all
+	//   tags of the source repository.
+	//
+	// If no refspecs are given, this defaults to "all_refs".
+	MirrorRefmaps        []string `protobuf:"bytes,4,rep,name=mirror_refmaps,json=mirrorRefmaps,proto3" json:"mirror_refmaps,omitempty"`
+	XXX_NoUnkeyedLiteral struct{} `json:"-"`
+	XXX_unrecognized     []byte   `json:"-"`
+	XXX_sizecache        int32    `json:"-"`
 }
 
 func (m *Remote) Reset()         { *m = Remote{} }
@@ -4257,6 +4286,8 @@ type RepositoryServiceClient interface {
 	WriteCommitGraph(ctx context.Context, in *WriteCommitGraphRequest, opts ...grpc.CallOption) (*WriteCommitGraphResponse, error)
 	RepositorySize(ctx context.Context, in *RepositorySizeRequest, opts ...grpc.CallOption) (*RepositorySizeResponse, error)
 	ApplyGitattributes(ctx context.Context, in *ApplyGitattributesRequest, opts ...grpc.CallOption) (*ApplyGitattributesResponse, error)
+	// FetchRemote fetches references from a remote repository into the local
+	// repository.
 	FetchRemote(ctx context.Context, in *FetchRemoteRequest, opts ...grpc.CallOption) (*FetchRemoteResponse, error)
 	CreateRepository(ctx context.Context, in *CreateRepositoryRequest, opts ...grpc.CallOption) (*CreateRepositoryResponse, error)
 	GetArchive(ctx context.Context, in *GetArchiveRequest, opts ...grpc.CallOption) (RepositoryService_GetArchiveClient, error)
@@ -4925,6 +4956,8 @@ type RepositoryServiceServer interface {
 	WriteCommitGraph(context.Context, *WriteCommitGraphRequest) (*WriteCommitGraphResponse, error)
 	RepositorySize(context.Context, *RepositorySizeRequest) (*RepositorySizeResponse, error)
 	ApplyGitattributes(context.Context, *ApplyGitattributesRequest) (*ApplyGitattributesResponse, error)
+	// FetchRemote fetches references from a remote repository into the local
+	// repository.
 	FetchRemote(context.Context, *FetchRemoteRequest) (*FetchRemoteResponse, error)
 	CreateRepository(context.Context, *CreateRepositoryRequest) (*CreateRepositoryResponse, error)
 	GetArchive(*GetArchiveRequest, RepositoryService_GetArchiveServer) error
diff --git a/proto/repository-service.proto b/proto/repository-service.proto
index ed32e64f5..3f7b3f405 100644
--- a/proto/repository-service.proto
+++ b/proto/repository-service.proto
@@ -48,6 +48,9 @@ service RepositoryService {
       op: MUTATOR
     };
   }
+
+  // FetchRemote fetches references from a remote repository into the local
+  // repository.
   rpc FetchRemote(FetchRemoteRequest) returns (FetchRemoteResponse) {
     option (op_type) = {
       op: MUTATOR
@@ -294,14 +297,25 @@ message ApplyGitattributesResponse {}
 
 message FetchRemoteRequest {
   Repository repository = 1 [(target_repository)=true];
+  // remote is the name of the remote that shall be fetched. This remote must
+  // exist in the repository's configuration already. This parameter is
+  // deprecated in favor of remote_params.
   string remote = 2;
+  // force determines if references should be force-updated in case they have
+  // diverged.
   bool force = 3;
+  // no_tags determines whether tags should be fetched.
   bool no_tags = 4;
+  // timeout specifies a timeout for the fetch.
   int32 timeout = 5;
   string ssh_key = 6;
   string known_hosts = 7;
   reserved 8;
+  // no_prune will the fetch to not prune remote references which do not exist
+  // in the remote repository anymore.
   bool no_prune = 9;
+  // remote_params specifies the remote repository which should be fetched
+  // from.
   Remote remote_params = 10;
   // If check_tags_changed is true, the FetchRemote RPC will check whether any
   // tags were modified, returning the result in the tags_changed field of
@@ -607,10 +621,28 @@ message SearchFilesByContentResponse {
   bool end_of_match = 3;
 }
 
+// Remote represents a git remote repository.
 message Remote {
+  // url is the URL of the remote repository.
   string url = 1;
+  // name is the name of the remote repository. This is mainly used to
+  // determine where fetched references should end up, e.g. in
+  // `refs/remotes/$name/`.
   string name = 2;
+  // http_authorization_header is the HTTP header which should be added to
+  // the request in order to authenticate against the repository.
   string http_authorization_header = 3;
+  // mirror_refmaps contains the refspecs which shall be fetched. Some special
+  // refspecs are accepted:
+  //
+  // - "all_refs" gets translated to "+refs/*:refs/*", which mirrors all
+  //   references of the source repository.
+  // - "heads" gets translated to "+refs/heads/*:refs/heads/*", which mirrors
+  //   all branches of the source repository.
+  // - "tags" gets translated to "+refs/tags/*:refs/tags/*", which mirrors all
+  //   tags of the source repository.
+  //
+  // If no refspecs are given, this defaults to "all_refs".
   repeated string mirror_refmaps = 4;
 }
 
diff --git a/ruby/proto/gitaly/repository-service_services_pb.rb b/ruby/proto/gitaly/repository-service_services_pb.rb
index 3fc818dc5..b67eaf506 100644
--- a/ruby/proto/gitaly/repository-service_services_pb.rb
+++ b/ruby/proto/gitaly/repository-service_services_pb.rb
@@ -22,6 +22,8 @@ module Gitaly
       rpc :WriteCommitGraph, Gitaly::WriteCommitGraphRequest, Gitaly::WriteCommitGraphResponse
       rpc :RepositorySize, Gitaly::RepositorySizeRequest, Gitaly::RepositorySizeResponse
       rpc :ApplyGitattributes, Gitaly::ApplyGitattributesRequest, Gitaly::ApplyGitattributesResponse
+      # FetchRemote fetches references from a remote repository into the local
+      # repository.
       rpc :FetchRemote, Gitaly::FetchRemoteRequest, Gitaly::FetchRemoteResponse
       rpc :CreateRepository, Gitaly::CreateRepositoryRequest, Gitaly::CreateRepositoryResponse
       rpc :GetArchive, Gitaly::GetArchiveRequest, stream(Gitaly::GetArchiveResponse)