Merge branch 'pks-rpcs-branch-confusion' into 'master'

git: Fix ambiguous lookups of branches

See merge request gitlab-org/gitaly!3074

12 files changed, 91 insertions, 47 deletions

diff --git a/internal/git/reference.go b/internal/git/reference.go
index fee318bf8..4822a171f 100644
--- a/internal/git/reference.go
+++ b/internal/git/reference.go
@@ -22,18 +22,10 @@ func (r Revision) String() string {
 // Revision does and must always contain a fully qualified reference.
 type ReferenceName string
 
-// NewBranchReferenceName returns a new ReferenceName for the given branch. The
-// branch may either be a fully qualified branch name ("refs/heads/master") or
-// an unqualified name ("master"). In the latter case, the returned
-// ReferenceName will still be a fully qualified reference by prepending
-// "refs/heads".
-func NewBranchReferenceName(branch string) ReferenceName {
-	if strings.HasPrefix(branch, "refs/heads/") {
-		return ReferenceName(branch)
-	}
-	if strings.HasPrefix(branch, "heads/") {
-		return ReferenceName("refs/" + branch)
-	}
+// NewReferenceNameFromBranchName returns a new ReferenceName from a given
+// branch name. Note that branch is treated as an unqualified branch name.
+// This function will thus always prepend "refs/heads/".
+func NewReferenceNameFromBranchName(branch string) ReferenceName {
 	return ReferenceName("refs/heads/" + branch)
 }
 
diff --git a/internal/git/reference_test.go b/internal/git/reference_test.go
index 7b3b58738..01687a2e3 100644
--- a/internal/git/reference_test.go
+++ b/internal/git/reference_test.go
@@ -52,26 +52,26 @@ func TestCheckRefFormat(t *testing.T) {
 	}
 }
 
-func TestReferenceName_NewBranchReferenceName(t *testing.T) {
+func TestReferenceName_NewReferenceNameFromBranchName(t *testing.T) {
 	for _, tc := range []struct {
 		desc      string
 		reference string
 		expected  string
 	}{
 		{
-			desc:      "fully qualified reference",
-			reference: "refs/heads/master",
+			desc:      "unqualified reference",
+			reference: "master",
 			expected:  "refs/heads/master",
 		},
 		{
 			desc:      "partly qualified reference",
 			reference: "heads/master",
-			expected:  "refs/heads/master",
+			expected:  "refs/heads/heads/master",
 		},
 		{
-			desc:      "unqualified reference",
-			reference: "master",
-			expected:  "refs/heads/master",
+			desc:      "fully qualified reference",
+			reference: "refs/heads/master",
+			expected:  "refs/heads/refs/heads/master",
 		},
 		{
 			desc:      "weird branch name",
@@ -85,7 +85,7 @@ func TestReferenceName_NewBranchReferenceName(t *testing.T) {
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
-			ref := NewBranchReferenceName(tc.reference)
+			ref := NewReferenceNameFromBranchName(tc.reference)
 			require.Equal(t, ref.String(), tc.expected)
 		})
 	}
diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go
index 287dd4ba7..581045085 100644
--- a/internal/gitaly/service/operations/submodules.go
+++ b/internal/gitaly/service/operations/submodules.go
@@ -89,7 +89,7 @@ func (s *Server) userUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda
 		}, nil
 	}
 
-	branchRef, err := repo.GetReference(ctx, git.NewBranchReferenceName(string(req.GetBranch())))
+	branchRef, err := repo.GetReference(ctx, git.NewReferenceNameFromBranchName(string(req.GetBranch())))
 	if err != nil {
 		if errors.Is(err, git.ErrReferenceNotFound) {
 			return nil, helper.ErrInvalidArgumentf("Cannot find branch")
@@ -156,7 +156,7 @@ func (s *Server) userUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda
 		ctx,
 		req.GetRepository(),
 		req.GetUser(),
-		"refs/heads/"+string(req.GetBranch()),
+		branchRef.Name.String(),
 		result.CommitID,
 		branchRef.Target,
 	); err != nil {
diff --git a/internal/gitaly/service/operations/submodules_test.go b/internal/gitaly/service/operations/submodules_test.go
index f788c1f0f..7fd7b7ec6 100644
--- a/internal/gitaly/service/operations/submodules_test.go
+++ b/internal/gitaly/service/operations/submodules_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/git/lstree"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
@@ -33,9 +34,24 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context)
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
-	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
+	testRepoProto, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
+	testRepo := localrepo.New(testRepoProto, config.Config)
+
+	// This reference is created to check that we can correctly commit onto
+	// a branch which has a name starting with "refs/heads/".
+	currentOID, err := testRepo.ResolveRevision(ctx, "refs/heads/master")
+	require.NoError(t, err)
+	require.NoError(t, testRepo.UpdateRef(ctx, "refs/heads/refs/heads/master", currentOID, git.ZeroOID))
+
+	// If something uses the branch name as an unqualified reference, then
+	// git would return the tag instead of the branch. We thus create a tag
+	// with a different OID than the current master branch.
+	prevOID, err := testRepo.ResolveRevision(ctx, "refs/heads/master~")
+	require.NoError(t, err)
+	require.NoError(t, testRepo.UpdateRef(ctx, "refs/tags/master", prevOID, git.ZeroOID))
+
 	commitMessage := []byte("Update Submodule message")
 
 	testCases := []struct {
@@ -51,6 +67,12 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context)
 			branch:    "master",
 		},
 		{
+			desc:      "Update submodule on weird branch",
+			submodule: "gitlab-grack",
+			commitSha: "41fa1bc9e0f0630ced6a8a211d60c2af425ecc2d",
+			branch:    "refs/heads/master",
+		},
+		{
 			desc:      "Update submodule inside folder",
 			submodule: "test_inside_folder/another_folder/six",
 			commitSha: "e25eda1fece24ac7a03624ed1320f82396f35bd8",
@@ -61,7 +83,7 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context)
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			request := &gitalypb.UserUpdateSubmoduleRequest{
-				Repository:    testRepo,
+				Repository:    testRepoProto,
 				User:          testhelper.TestUser,
 				Submodule:     []byte(testCase.submodule),
 				CommitSha:     testCase.commitSha,
@@ -74,7 +96,7 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context)
 			require.Empty(t, response.GetCommitError())
 			require.Empty(t, response.GetPreReceiveError())
 
-			commit, err := log.GetCommit(ctx, locator, testRepo, git.Revision(response.BranchUpdate.CommitId))
+			commit, err := log.GetCommit(ctx, locator, testRepoProto, git.Revision(response.BranchUpdate.CommitId))
 			require.NoError(t, err)
 			require.Equal(t, commit.Author.Email, testhelper.TestUser.Email)
 			require.Equal(t, commit.Committer.Email, testhelper.TestUser.Email)
diff --git a/internal/gitaly/service/ref/branches.go b/internal/gitaly/service/ref/branches.go
index eb23e6f7e..e7d6779cf 100644
--- a/internal/gitaly/service/ref/branches.go
+++ b/internal/gitaly/service/ref/branches.go
@@ -20,7 +20,7 @@ func (s *server) FindBranch(ctx context.Context, req *gitalypb.FindBranchRequest
 
 	repo := req.GetRepository()
 
-	branchName := git.NewBranchReferenceName(string(req.GetName()))
+	branchName := git.NewReferenceNameFromBranchName(string(req.GetName()))
 	branchRef, err := localrepo.New(repo, config.Config).GetReference(ctx, branchName)
 	if err != nil {
 		if errors.Is(err, git.ErrReferenceNotFound) {
diff --git a/internal/gitaly/service/ref/branches_test.go b/internal/gitaly/service/ref/branches_test.go
index d2f596d01..65ea84618 100644
--- a/internal/gitaly/service/ref/branches_test.go
+++ b/internal/gitaly/service/ref/branches_test.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -16,23 +17,37 @@ func TestSuccessfulFindBranchRequest(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	locator := config.NewLocator(config.Config)
 	stop, serverSocketPath := runRefServiceServer(t)
 	defer stop()
 
 	client, conn := newRefServiceClient(t, serverSocketPath)
 	defer conn.Close()
 
-	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	testRepoProto, _, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
-	branchNameInput := "master"
-	branchTarget, err := log.GetCommit(ctx, locator, testRepo, git.Revision(branchNameInput))
-	require.NoError(t, err)
+	repo := localrepo.New(testRepoProto, config.Config)
+	locator := config.NewLocator(config.Config)
 
-	branch := &gitalypb.Branch{
-		Name:         []byte(branchNameInput),
-		TargetCommit: branchTarget,
+	branchesByName := make(map[git.ReferenceName]*gitalypb.Branch)
+	for branchName, revision := range map[git.ReferenceName]git.Revision{
+		"refs/heads/branch":            "refs/heads/master~0",
+		"refs/heads/heads/branch":      "refs/heads/master~1",
+		"refs/heads/refs/heads/branch": "refs/heads/master~2",
+	} {
+		oid, err := repo.ResolveRevision(ctx, revision)
+		require.NoError(t, err)
+
+		err = repo.UpdateRef(ctx, branchName, oid, "")
+		require.NoError(t, err)
+
+		commit, err := log.GetCommit(ctx, locator, testRepoProto, branchName.Revision())
+		require.NoError(t, err)
+
+		branchesByName[branchName] = &gitalypb.Branch{
+			Name:         []byte(branchName.String()[len("refs/heads/"):]),
+			TargetCommit: commit,
+		}
 	}
 
 	testCases := []struct {
@@ -42,18 +57,18 @@ func TestSuccessfulFindBranchRequest(t *testing.T) {
 	}{
 		{
 			desc:           "regular branch name",
-			branchName:     branchNameInput,
-			expectedBranch: branch,
+			branchName:     "branch",
+			expectedBranch: branchesByName["refs/heads/branch"],
 		},
 		{
 			desc:           "absolute reference path",
-			branchName:     "refs/heads/" + branchNameInput,
-			expectedBranch: branch,
+			branchName:     "heads/branch",
+			expectedBranch: branchesByName["refs/heads/heads/branch"],
 		},
 		{
 			desc:           "heads path",
-			branchName:     "heads/" + branchNameInput,
-			expectedBranch: branch,
+			branchName:     "refs/heads/branch",
+			expectedBranch: branchesByName["refs/heads/refs/heads/branch"],
 		},
 		{
 			desc:       "non-existent branch",
@@ -64,7 +79,7 @@ func TestSuccessfulFindBranchRequest(t *testing.T) {
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			request := &gitalypb.FindBranchRequest{
-				Repository: testRepo,
+				Repository: testRepoProto,
 				Name:       []byte(testCase.branchName),
 			}
 
diff --git a/proto/go/gitalypb/operations.pb.go b/proto/go/gitalypb/operations.pb.go
index 8938b24d7..27850cfe9 100644
--- a/proto/go/gitalypb/operations.pb.go
+++ b/proto/go/gitalypb/operations.pb.go
@@ -2715,7 +2715,8 @@ type UserUpdateSubmoduleRequest struct {
 	User *User `protobuf:"bytes,2,opt,name=user,proto3" json:"user,omitempty"`
 	// commit_sha is the object ID the submodule shall be updated to.
 	CommitSha string `protobuf:"bytes,3,opt,name=commit_sha,json=commitSha,proto3" json:"commit_sha,omitempty"`
-	// branch is the branch which shall be updated.
+	// branch is the branch which shall be updated. This is the unqualified name
+	// of the branch, it must not have a "refs/heads/" prefix.
 	Branch []byte `protobuf:"bytes,4,opt,name=branch,proto3" json:"branch,omitempty"`
 	// submodule is the path to the submodule which shall be updated.
 	Submodule []byte `protobuf:"bytes,5,opt,name=submodule,proto3" json:"submodule,omitempty"`
diff --git a/proto/go/gitalypb/ref.pb.go b/proto/go/gitalypb/ref.pb.go
index 940c3d9a9..5b924cba6 100644
--- a/proto/go/gitalypb/ref.pb.go
+++ b/proto/go/gitalypb/ref.pb.go
@@ -1328,8 +1328,10 @@ func (m *DeleteBranchResponse) XXX_DiscardUnknown() {
 var xxx_messageInfo_DeleteBranchResponse proto.InternalMessageInfo
 
 type FindBranchRequest struct {
+	// repository is the repository in which the branch should be looked up.
 	Repository *Repository `protobuf:"bytes,1,opt,name=repository,proto3" json:"repository,omitempty"`
-	// Name can be 'master' but also 'refs/heads/master'
+	// name is the name of the branch which should be looked up. This must be the
+	// branch name only, it must not have the "refs/heads/" prefix.
 	Name                 []byte   `protobuf:"bytes,2,opt,name=name,proto3" json:"name,omitempty"`
 	XXX_NoUnkeyedLiteral struct{} `json:"-"`
 	XXX_unrecognized     []byte   `json:"-"`
@@ -2237,6 +2239,8 @@ type RefServiceClient interface {
 	FindTag(ctx context.Context, in *FindTagRequest, opts ...grpc.CallOption) (*FindTagResponse, error)
 	FindAllRemoteBranches(ctx context.Context, in *FindAllRemoteBranchesRequest, opts ...grpc.CallOption) (RefService_FindAllRemoteBranchesClient, error)
 	RefExists(ctx context.Context, in *RefExistsRequest, opts ...grpc.CallOption) (*RefExistsResponse, error)
+	// FindBranch finds a branch by its unqualified name (like "master") and
+	// returns the commit it currently points to.
 	FindBranch(ctx context.Context, in *FindBranchRequest, opts ...grpc.CallOption) (*FindBranchResponse, error)
 	DeleteRefs(ctx context.Context, in *DeleteRefsRequest, opts ...grpc.CallOption) (*DeleteRefsResponse, error)
 	ListBranchNamesContainingCommit(ctx context.Context, in *ListBranchNamesContainingCommitRequest, opts ...grpc.CallOption) (RefService_ListBranchNamesContainingCommitClient, error)
@@ -2685,6 +2689,8 @@ type RefServiceServer interface {
 	FindTag(context.Context, *FindTagRequest) (*FindTagResponse, error)
 	FindAllRemoteBranches(*FindAllRemoteBranchesRequest, RefService_FindAllRemoteBranchesServer) error
 	RefExists(context.Context, *RefExistsRequest) (*RefExistsResponse, error)
+	// FindBranch finds a branch by its unqualified name (like "master") and
+	// returns the commit it currently points to.
 	FindBranch(context.Context, *FindBranchRequest) (*FindBranchResponse, error)
 	DeleteRefs(context.Context, *DeleteRefsRequest) (*DeleteRefsResponse, error)
 	ListBranchNamesContainingCommit(*ListBranchNamesContainingCommitRequest, RefService_ListBranchNamesContainingCommitServer) error
diff --git a/proto/operations.proto b/proto/operations.proto
index 70484113f..981e5864f 100644
--- a/proto/operations.proto
+++ b/proto/operations.proto
@@ -670,7 +670,8 @@ message UserUpdateSubmoduleRequest {
   User user = 2;
   // commit_sha is the object ID the submodule shall be updated to.
   string commit_sha = 3;
-  // branch is the branch which shall be updated.
+  // branch is the branch which shall be updated. This is the unqualified name
+  // of the branch, it must not have a "refs/heads/" prefix.
   bytes branch = 4;
   // submodule is the path to the submodule which shall be updated.
   bytes submodule = 5;
diff --git a/proto/ref.proto b/proto/ref.proto
index 1943b83c4..cd43ed5ca 100644
--- a/proto/ref.proto
+++ b/proto/ref.proto
@@ -63,6 +63,9 @@ service RefService {
       op: ACCESSOR
     };
   }
+
+  // FindBranch finds a branch by its unqualified name (like "master") and
+  // returns the commit it currently points to.
   rpc FindBranch(FindBranchRequest) returns (FindBranchResponse) {
     option (op_type) = {
       op: ACCESSOR
@@ -264,8 +267,10 @@ message DeleteBranchRequest {
 message DeleteBranchResponse {}
 
 message FindBranchRequest {
+  // repository is the repository in which the branch should be looked up.
   Repository repository = 1 [(target_repository)=true];
-  // Name can be 'master' but also 'refs/heads/master'
+  // name is the name of the branch which should be looked up. This must be the
+  // branch name only, it must not have the "refs/heads/" prefix.
   bytes name = 2;
 }
 
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index d24ccb35c..e248ad774 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -406,7 +406,7 @@ module Gitlab
       end
 
       def update_submodule(submodule_path, commit_sha, branch, committer, message)
-        target = rugged.rev_parse(branch)
+        target = rugged.rev_parse("refs/heads/" + branch)
         raise CommitError, 'Invalid branch' unless target.is_a?(Rugged::Commit)
 
         current_entry = rugged_submodule_entry(target, submodule_path)
@@ -505,7 +505,7 @@ module Gitlab
       def find_branch(name, force_reload = false)
         reload_rugged if force_reload
 
-        rugged_ref = rugged.branches[name]
+        rugged_ref = rugged.ref("refs/heads/" + name)
         if rugged_ref
           target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
           Gitlab::Git::Branch.new(self, rugged_ref.canonical_name, rugged_ref.target, target_commit)
diff --git a/ruby/proto/gitaly/ref_services_pb.rb b/ruby/proto/gitaly/ref_services_pb.rb
index e09800677..a3979c5e3 100644
--- a/ruby/proto/gitaly/ref_services_pb.rb
+++ b/ruby/proto/gitaly/ref_services_pb.rb
@@ -26,6 +26,8 @@ module Gitaly
       rpc :FindTag, Gitaly::FindTagRequest, Gitaly::FindTagResponse
       rpc :FindAllRemoteBranches, Gitaly::FindAllRemoteBranchesRequest, stream(Gitaly::FindAllRemoteBranchesResponse)
       rpc :RefExists, Gitaly::RefExistsRequest, Gitaly::RefExistsResponse
+      # FindBranch finds a branch by its unqualified name (like "master") and
+      # returns the commit it currently points to.
       rpc :FindBranch, Gitaly::FindBranchRequest, Gitaly::FindBranchResponse
       rpc :DeleteRefs, Gitaly::DeleteRefsRequest, Gitaly::DeleteRefsResponse
       rpc :ListBranchNamesContainingCommit, Gitaly::ListBranchNamesContainingCommitRequest, stream(Gitaly::ListBranchNamesContainingCommitResponse)