handle non-existent start branch in UserCommitFiles

UserCommitFiles receives requests where the StartBranch is set even
for an empty repository. Such requests fail when attempting to resolve
the start branch' commit as the branch does not exist. The Ruby
implementation of the RPC went around this case by checking whether the
repository has no branches. If so, it ignored the start branch. This
scenario caused failures in the QA pipeline. It doesn't appear such
requests are sent in production though.

This commit fixes the failing pipeline by mirroring the Ruby
implementations behavior and ignoring the start branch when the
repository has no branches. The Go implementation already handled this
for the remote repository. This changes the handling to also take place
when the start repository is not set.

2 files changed, 47 insertions, 24 deletions

diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go
index 449176f33..dbb01a8f1 100644
--- a/internal/gitaly/service/operations/commit_files.go
+++ b/internal/gitaly/service/operations/commit_files.go
@@ -374,30 +374,30 @@ func (s *Server) resolveParentCommit(ctx context.Context, local git.Repository,
 		if err != nil {
 			return "", fmt.Errorf("remote repository: %w", err)
 		}
+	}
 
-		if hasBranches, err := repo.HasBranches(ctx); err != nil {
-			return "", fmt.Errorf("has branches: %w", err)
-		} else if !hasBranches {
-			// GitLab sends requests to UserCommitFiles where target repository
-			// and start repository are the same. If the request hits Gitaly directly,
-			// Gitaly could check if the repos are the same by comparing their storages
-			// and relative paths and simply resolve the branch locally. When request is proxied
-			// through Praefect, the start repository's storage is not rewritten, thus Gitaly can't
-			// identify the repos as being the same.
-			//
-			// If the start repository is set, we have to resolve the branch there as it
-			// might be on a different commit than the local repository. As Gitaly can't identify
-			// the repositories are the same behind Praefect, it has to perform an RPC to resolve
-			// the branch. The resolving would fail as the branch does not yet exist in the start
-			// repository, which is actually the local repository.
-			//
-			// Due to this, we check if the remote has any branches. If not, we likely hit this case
-			// and we're creating the first branch. If so, we'll just return the commit that was
-			// already resolved locally.
-			//
-			// See: https://gitlab.com/gitlab-org/gitaly/-/issues/3294
-			return targetBranchCommit, nil
-		}
+	if hasBranches, err := repo.HasBranches(ctx); err != nil {
+		return "", fmt.Errorf("has branches: %w", err)
+	} else if !hasBranches {
+		// GitLab sends requests to UserCommitFiles where target repository
+		// and start repository are the same. If the request hits Gitaly directly,
+		// Gitaly could check if the repos are the same by comparing their storages
+		// and relative paths and simply resolve the branch locally. When request is proxied
+		// through Praefect, the start repository's storage is not rewritten, thus Gitaly can't
+		// identify the repos as being the same.
+		//
+		// If the start repository is set, we have to resolve the branch there as it
+		// might be on a different commit than the local repository. As Gitaly can't identify
+		// the repositories are the same behind Praefect, it has to perform an RPC to resolve
+		// the branch. The resolving would fail as the branch does not yet exist in the start
+		// repository, which is actually the local repository.
+		//
+		// Due to this, we check if the remote has any branches. If not, we likely hit this case
+		// and we're creating the first branch. If so, we'll just return the commit that was
+		// already resolved locally.
+		//
+		// See: https://gitlab.com/gitlab-org/gitaly/-/issues/3294
+		return targetBranchCommit, nil
 	}
 
 	branch := targetBranch
diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go
index 3853a7437..fa0a2ab5c 100644
--- a/internal/gitaly/service/operations/commit_files_test.go
+++ b/internal/gitaly/service/operations/commit_files_test.go
@@ -87,6 +87,7 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 	type step struct {
 		actions         []*gitalypb.UserCommitFilesRequest
 		startRepository *gitalypb.Repository
+		startBranch     string
 		error           error
 		indexError      string
 		repoCreated     bool
@@ -790,13 +791,31 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 			},
 		},
 		{
-			desc: "start repository refers to an empty repository",
+			desc: "empty target repository with start branch set",
 			steps: []step{
 				{
 					actions: []*gitalypb.UserCommitFilesRequest{
 						createFileHeaderRequest("file-1"),
 						actionContentRequest("content-1"),
 					},
+					startBranch:   "master",
+					branchCreated: true,
+					repoCreated:   true,
+					treeEntries: []testhelper.TreeEntry{
+						{Mode: DefaultMode, Path: "file-1", Content: "content-1"},
+					},
+				},
+			},
+		},
+		{
+			desc: "start repository refers to an empty remote repository",
+			steps: []step{
+				{
+					actions: []*gitalypb.UserCommitFilesRequest{
+						createFileHeaderRequest("file-1"),
+						actionContentRequest("content-1"),
+					},
+					startBranch:     "master",
 					startRepository: startRepo,
 					branchCreated:   true,
 					repoCreated:     true,
@@ -828,6 +847,10 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) {
 					setStartRepository(headerRequest, step.startRepository)
 				}
 
+				if step.startBranch != "" {
+					setStartBranchName(headerRequest, []byte(step.startBranch))
+				}
+
 				stream, err := client.UserCommitFiles(ctx)
 				require.NoError(t, err)
 				require.NoError(t, stream.Send(headerRequest))