Support start_sha parameter in UserCommitFiles

This supports passing a `start_sha` parameter to the commits API in
the Rails app, see https://gitlab.com/gitlab-org/gitlab-ce/issues/60609

9 files changed, 286 insertions, 58 deletions

diff --git a/changelogs/unreleased/webide-commit-use-correct-parent.yml b/changelogs/unreleased/webide-commit-use-correct-parent.yml
new file mode 100644
index 000000000..6f03d06f4
--- /dev/null
+++ b/changelogs/unreleased/webide-commit-use-correct-parent.yml
@@ -0,0 +1,5 @@
+---
+title: Support start_sha parameter in UserCommitFiles
+merge_request: 1308
+author:
+type: changed
diff --git a/internal/service/operations/commit_files.go b/internal/service/operations/commit_files.go
index e9496d1af..1959e061c 100644
--- a/internal/service/operations/commit_files.go
+++ b/internal/service/operations/commit_files.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -77,5 +78,14 @@ func validateUserCommitFilesHeader(header *gitalypb.UserCommitFilesRequestHeader
 	if len(header.GetBranchName()) == 0 {
 		return fmt.Errorf("empty BranchName")
 	}
+
+	startSha := header.GetStartSha()
+	if len(startSha) > 0 {
+		err := git.ValidateCommitID(startSha)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
diff --git a/internal/service/operations/commit_files_test.go b/internal/service/operations/commit_files_test.go
index 19b8eeedd..61ef23c15 100644
--- a/internal/service/operations/commit_files_test.go
+++ b/internal/service/operations/commit_files_test.go
@@ -107,10 +107,10 @@ func TestSuccessfulUserCommitFilesRequest(t *testing.T) {
 			require.NoError(t, stream.Send(actionsRequest3))
 			require.NoError(t, stream.Send(actionsRequest4))
 
-			r, err := stream.CloseAndRecv()
+			resp, err := stream.CloseAndRecv()
 			require.NoError(t, err)
-			require.Equal(t, tc.repoCreated, r.GetBranchUpdate().GetRepoCreated())
-			require.Equal(t, tc.branchCreated, r.GetBranchUpdate().GetBranchCreated())
+			require.Equal(t, tc.repoCreated, resp.GetBranchUpdate().GetRepoCreated())
+			require.Equal(t, tc.branchCreated, resp.GetBranchUpdate().GetBranchCreated())
 
 			headCommit, err := log.GetCommit(ctxOuter, tc.repo, tc.branchName)
 			require.NoError(t, err)
@@ -179,10 +179,10 @@ func TestSuccessfulUserCommitFilesRequestMove(t *testing.T) {
 				require.NoError(t, stream.Send(actionsRequest2))
 			}
 
-			r, err := stream.CloseAndRecv()
+			resp, err := stream.CloseAndRecv()
 			require.NoError(t, err)
 
-			update := r.GetBranchUpdate()
+			update := resp.GetBranchUpdate()
 			require.NotNil(t, update)
 
 			fileContent := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "show", update.CommitId+":"+filePath)
@@ -237,10 +237,10 @@ func TestSuccessfulUserCommitFilesRequestForceCommit(t *testing.T) {
 	require.NoError(t, stream.Send(createFileHeaderRequest("TEST.md")))
 	require.NoError(t, stream.Send(actionContentRequest("Test")))
 
-	r, err := stream.CloseAndRecv()
+	resp, err := stream.CloseAndRecv()
 	require.NoError(t, err)
 
-	update := r.GetBranchUpdate()
+	update := resp.GetBranchUpdate()
 	newTargetBranchCommit, err := log.GetCommit(ctxOuter, testRepo, targetBranchName)
 	require.NoError(t, err)
 
@@ -248,6 +248,90 @@ func TestSuccessfulUserCommitFilesRequestForceCommit(t *testing.T) {
 	require.Equal(t, newTargetBranchCommit.ParentIds, []string{startBranchCommit.Id})
 }
 
+func TestSuccessfulUserCommitFilesRequestStartSha(t *testing.T) {
+	server, serverSocketPath := runFullServer(t)
+	defer server.Stop()
+
+	client, conn := operations.NewOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctxOuter, cancel := testhelper.Context()
+	defer cancel()
+
+	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
+	ctx := metadata.NewOutgoingContext(ctxOuter, md)
+	targetBranchName := "new"
+
+	startCommit, err := log.GetCommit(ctxOuter, testRepo, "master")
+	require.NoError(t, err)
+
+	headerRequest := headerRequest(testRepo, user, targetBranchName, commitFilesMessage)
+	setStartSha(headerRequest, startCommit.Id)
+
+	stream, err := client.UserCommitFiles(ctx)
+	require.NoError(t, err)
+	require.NoError(t, stream.Send(headerRequest))
+	require.NoError(t, stream.Send(createFileHeaderRequest("TEST.md")))
+	require.NoError(t, stream.Send(actionContentRequest("Test")))
+
+	resp, err := stream.CloseAndRecv()
+	require.NoError(t, err)
+
+	update := resp.GetBranchUpdate()
+	newTargetBranchCommit, err := log.GetCommit(ctxOuter, testRepo, targetBranchName)
+	require.NoError(t, err)
+
+	require.Equal(t, newTargetBranchCommit.Id, update.CommitId)
+	require.Equal(t, newTargetBranchCommit.ParentIds, []string{startCommit.Id})
+}
+
+func TestSuccessfulUserCommitFilesRequestStartShaRemoteRepository(t *testing.T) {
+	server, serverSocketPath := runFullServer(t)
+	defer server.Stop()
+
+	client, conn := operations.NewOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctxOuter, cancel := testhelper.Context()
+	defer cancel()
+
+	newRepo, _, newRepoCleanupFn := testhelper.InitBareRepo(t)
+	defer newRepoCleanupFn()
+
+	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
+	ctx := metadata.NewOutgoingContext(ctxOuter, md)
+	targetBranchName := "new"
+
+	startCommit, err := log.GetCommit(ctxOuter, testRepo, "master")
+	require.NoError(t, err)
+
+	headerRequest := headerRequest(newRepo, user, targetBranchName, commitFilesMessage)
+	setStartSha(headerRequest, startCommit.Id)
+	setStartRepository(headerRequest, testRepo)
+
+	stream, err := client.UserCommitFiles(ctx)
+	require.NoError(t, err)
+	require.NoError(t, stream.Send(headerRequest))
+	require.NoError(t, stream.Send(createFileHeaderRequest("TEST.md")))
+	require.NoError(t, stream.Send(actionContentRequest("Test")))
+
+	resp, err := stream.CloseAndRecv()
+	require.NoError(t, err)
+
+	update := resp.GetBranchUpdate()
+	newTargetBranchCommit, err := log.GetCommit(ctxOuter, newRepo, targetBranchName)
+	require.NoError(t, err)
+
+	require.Equal(t, newTargetBranchCommit.Id, update.CommitId)
+	require.Equal(t, newTargetBranchCommit.ParentIds, []string{startCommit.Id})
+}
+
 func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) {
 	server, serverSocketPath := runFullServer(t)
 	defer server.Stop()
@@ -282,11 +366,11 @@ func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) {
 			require.NoError(t, stream.Send(actionsRequest1))
 			require.NoError(t, stream.Send(actionsRequest2))
 
-			r, err := stream.CloseAndRecv()
+			resp, err := stream.CloseAndRecv()
 			require.NoError(t, err)
 
-			require.Contains(t, r.PreReceiveError, "GL_ID="+user.GlId)
-			require.Contains(t, r.PreReceiveError, "GL_USERNAME="+user.GlUsername)
+			require.Contains(t, resp.PreReceiveError, "GL_ID="+user.GlId)
+			require.Contains(t, resp.PreReceiveError, "GL_USERNAME="+user.GlUsername)
 		})
 	}
 }
@@ -356,9 +440,9 @@ func TestFailedUserCommitFilesRequestDueToIndexError(t *testing.T) {
 				require.NoError(t, stream.Send(req))
 			}
 
-			r, err := stream.CloseAndRecv()
+			resp, err := stream.CloseAndRecv()
 			require.NoError(t, err)
-			require.Equal(t, tc.indexError, r.GetIndexError())
+			require.Equal(t, tc.indexError, resp.GetIndexError())
 		})
 	}
 }
@@ -399,6 +483,10 @@ func TestFailedUserCommitFilesRequest(t *testing.T) {
 			desc: "empty CommitMessage",
 			req:  headerRequest(testRepo, user, branchName, nil),
 		},
+		{
+			desc: "invalid commit ID: \"foobar\"",
+			req:  setStartSha(headerRequest(testRepo, user, branchName, commitFilesMessage), "foobar"),
+		},
 	}
 
 	for _, tc := range testCases {
@@ -441,6 +529,18 @@ func setStartBranchName(headerRequest *gitalypb.UserCommitFilesRequest, startBra
 	header.StartBranchName = startBranchName
 }
 
+func setStartRepository(headerRequest *gitalypb.UserCommitFilesRequest, startRepository *gitalypb.Repository) {
+	header := getHeader(headerRequest)
+	header.StartRepository = startRepository
+}
+
+func setStartSha(headerRequest *gitalypb.UserCommitFilesRequest, startSha string) *gitalypb.UserCommitFilesRequest {
+	header := getHeader(headerRequest)
+	header.StartSha = startSha
+
+	return headerRequest
+}
+
 func setForce(headerRequest *gitalypb.UserCommitFilesRequest, force bool) {
 	header := getHeader(headerRequest)
 	header.Force = force
diff --git a/ruby/lib/gitaly_server/operations_service.rb b/ruby/lib/gitaly_server/operations_service.rb
index 89655fb3f..f9538e5e2 100644
--- a/ruby/lib/gitaly_server/operations_service.rb
+++ b/ruby/lib/gitaly_server/operations_service.rb
@@ -282,6 +282,8 @@ module GitalyServer
       Gitaly::UserCommitFilesResponse.new(pre_receive_error: set_utf8!(e.message))
     rescue Gitlab::Git::CommitError => e
       raise GRPC::FailedPrecondition.new(e.message)
+    rescue ArgumentError => e
+      raise GRPC::InvalidArgument.new(e.message)
     end
 
     def user_squash(request, call)
@@ -354,6 +356,7 @@ module GitalyServer
 
       optional_fields = {
         start_branch_name: 'start_branch_name',
+        start_sha: 'start_sha',
         author_name: 'commit_author_name',
         author_email: 'commit_author_email',
         force: 'force'
diff --git a/ruby/lib/gitlab/git.rb b/ruby/lib/gitlab/git.rb
index 33ff52453..8c041e534 100644
--- a/ruby/lib/gitlab/git.rb
+++ b/ruby/lib/gitlab/git.rb
@@ -45,7 +45,7 @@ module Gitlab
     # See http://stackoverflow.com/a/40884093/1856239 and
     # https://github.com/git/git/blob/3ad8b5bf26362ac67c9020bf8c30eee54a84f56d/cache.h#L1011-L1012
     EMPTY_TREE_ID = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'.freeze
-    BLANK_SHA = '0' * 40
+    BLANK_SHA = ('0' * 40).freeze
     TAG_REF_PREFIX = "refs/tags/".freeze
     BRANCH_REF_PREFIX = "refs/heads/".freeze
 
diff --git a/ruby/lib/gitlab/git/operation_service.rb b/ruby/lib/gitlab/git/operation_service.rb
index 9bf7ae80d..c2869039f 100644
--- a/ruby/lib/gitlab/git/operation_service.rb
+++ b/ruby/lib/gitlab/git/operation_service.rb
@@ -65,12 +65,15 @@ module Gitlab
         end
       end
 
-      # Whenever `start_branch_name` is passed, if `branch_name` doesn't exist,
-      # it would be created from `start_branch_name`.
+      # Whenever `start_branch_name` or `start_sha` is passed, if `branch_name`
+      # doesn't exist, it will be created from the commit pointed to by
+      # `start_branch_name` or `start_sha`.
+      #
       # If `start_repository` is passed, and the branch doesn't exist,
       # it would try to find the commits from it instead of current repository.
       def with_branch(branch_name,
                       start_branch_name: nil,
+                      start_sha: nil,
                       start_repository: repository,
                       force: false,
                       &block)
@@ -78,12 +81,16 @@ module Gitlab
 
         start_branch_name = nil if start_repository.empty?
 
-        raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.relative_path}" if start_branch_name && !start_repository.branch_exists?(start_branch_name)
+        if start_branch_name.present? && !start_repository.branch_exists?(start_branch_name)
+          raise ArgumentError, "Cannot find branch '#{start_branch_name}'"
+        elsif start_sha.present? && !start_repository.commit_id(start_sha)
+          raise ArgumentError, "Cannot find commit '#{start_sha}'"
+        end
 
         update_branch_with_hooks(branch_name, force) do
           repository.with_repo_branch_commit(
             start_repository,
-            start_branch_name || branch_name,
+            start_sha.presence || start_branch_name.presence || branch_name,
             &block
           )
         end
diff --git a/ruby/lib/gitlab/git/remote_repository.rb b/ruby/lib/gitlab/git/remote_repository.rb
index 494da0c55..df857176d 100644
--- a/ruby/lib/gitlab/git/remote_repository.rb
+++ b/ruby/lib/gitlab/git/remote_repository.rb
@@ -35,12 +35,12 @@ module Gitlab
           gitaly_repository.relative_path == other_repository.relative_path
       end
 
-      def fetch_env
+      def fetch_env(git_config_options: [])
         gitaly_ssh = File.absolute_path(File.join(Gitlab.config.gitaly.client_path, 'gitaly-ssh'))
         gitaly_address = gitaly_client.address(storage)
         gitaly_token = gitaly_client.token(storage)
 
-        request = Gitaly::SSHUploadPackRequest.new(repository: gitaly_repository)
+        request = Gitaly::SSHUploadPackRequest.new(repository: gitaly_repository, git_config_options: git_config_options)
         env = {
           'GITALY_ADDRESS' => gitaly_address,
           'GITALY_PAYLOAD' => request.to_json,
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index 8aa33709e..f98886877 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -26,6 +26,7 @@ module Gitlab
       GITALY_INTERNAL_URL = 'ssh://gitaly/internal.git'.freeze
       AUTOCRLF_VALUES = { 'true' => true, 'false' => false, 'input' => :input }.freeze
       RUGGED_KEY = :rugged_list
+      GIT_ALLOW_SHA_UPLOAD = 'uploadpack.allowAnySHA1InWant=true'.freeze
 
       NoRepository = Class.new(StandardError)
       InvalidRef = Class.new(StandardError)
@@ -455,10 +456,11 @@ module Gitlab
       # rubocop:disable Metrics/ParameterLists
       def multi_action(user, branch_name:, message:, actions:,
                        author_email: nil, author_name: nil,
-                       start_branch_name: nil, start_repository: self, force: false)
+                       start_branch_name: nil, start_sha: nil, start_repository: self, force: false)
         OperationService.new(user, self).with_branch(
           branch_name,
           start_branch_name: start_branch_name,
+          start_sha: start_sha,
           start_repository: start_repository,
           force: force
         ) do |start_commit|
@@ -488,44 +490,27 @@ module Gitlab
       end
       # rubocop:enable Metrics/ParameterLists
 
-      def with_repo_branch_commit(start_repository, start_branch_name)
+      def with_repo_branch_commit(start_repository, start_ref)
         start_repository = RemoteRepository.new(start_repository) unless start_repository.is_a?(RemoteRepository)
 
-        return yield nil if start_repository.empty?
-
-        if start_repository.same_repository?(self)
-          yield commit(start_branch_name)
-        else
-          start_commit_id = start_repository.commit_id(start_branch_name)
-
-          return yield nil unless start_commit_id
-
-          if branch_commit = commit(start_commit_id)
-            yield branch_commit
-          else
-            with_repo_tmp_commit(
-              start_repository, start_branch_name, start_commit_id
-            ) do |tmp_commit|
-              yield tmp_commit
-            end
-          end
+        if start_repository.empty?
+          return yield nil
+        elsif start_repository.same_repository?(self)
+          # Directly return the commit from this repository
+          return yield commit(start_ref)
         end
-      end
 
-      def with_repo_tmp_commit(start_repository, start_branch_name, sha)
-        source_ref = start_branch_name
+        # Find the commit from the remote repository (this triggers an RPC)
+        commit_id = start_repository.commit_id(start_ref)
+        return yield nil unless commit_id
 
-        source_ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{source_ref}" unless Gitlab::Git.branch_ref?(source_ref)
-
-        tmp_ref = fetch_ref(
-          start_repository,
-          source_ref: source_ref,
-          target_ref: "refs/tmp/#{SecureRandom.hex}"
-        )
-
-        yield commit(sha)
-      ensure
-        delete_refs(tmp_ref) if tmp_ref
+        if existing_commit = commit(commit_id)
+          # Commit is already present (e.g. in a fork, or through a previous fetch)
+          yield existing_commit
+        else
+          fetch_sha(start_repository, commit_id)
+          yield commit(commit_id)
+        end
       end
 
       def fetch_source_branch!(source_repository, source_branch, local_ref)
@@ -614,14 +599,17 @@ module Gitlab
         end
       end
 
-      def fetch_ref(source_repository, source_ref:, target_ref:)
+      # Fetch a commit from the given source repository
+      def fetch_sha(source_repository, sha)
         source_repository = RemoteRepository.new(source_repository) unless source_repository.is_a?(RemoteRepository)
 
-        args = %W[fetch --no-tags -f #{GITALY_INTERNAL_URL} #{source_ref}:#{target_ref}]
-        message, status = run_git(args, env: source_repository.fetch_env, include_stderr: true)
-        raise Gitlab::Git::CommandError, message if status != 0
+        env = source_repository.fetch_env(git_config_options: [GIT_ALLOW_SHA_UPLOAD])
+
+        args = %W[fetch --no-tags #{GITALY_INTERNAL_URL} #{sha}]
+        message, status = run_git(args, env: env, include_stderr: true)
+        raise Gitlab::Git::CommandError, message unless status.zero?
 
-        target_ref
+        sha
       end
 
       # Lookup for rugged object by oid or ref name
diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb
index 672c95361..8cb8558a6 100644
--- a/ruby/spec/lib/gitlab/git/repository_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_spec.rb
@@ -227,6 +227,91 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
     end
   end
 
+  describe '#with_repo_branch_commit' do
+    let(:start_repository) { Gitlab::Git::RemoteRepository.new(source_repository) }
+    let(:start_commit) { source_repository.commit }
+
+    context 'when start_repository is empty' do
+      let(:source_repository) { gitlab_git_from_gitaly(new_empty_test_repo) }
+
+      before do
+        expect(start_repository).not_to receive(:commit_id)
+        expect(repository).not_to receive(:fetch_sha)
+      end
+
+      it 'yields nil' do
+        expect do |block|
+          repository.with_repo_branch_commit(start_repository, 'master', &block)
+        end.to yield_with_args(nil)
+      end
+    end
+
+    context 'when start_repository is the same repository' do
+      let(:source_repository) { repository }
+
+      before do
+        expect(start_repository).not_to receive(:commit_id)
+        expect(repository).not_to receive(:fetch_sha)
+      end
+
+      it 'yields the commit for the SHA' do
+        expect do |block|
+          repository.with_repo_branch_commit(start_repository, start_commit.sha, &block)
+        end.to yield_with_args(start_commit)
+      end
+
+      it 'yields the commit for the branch' do
+        expect do |block|
+          repository.with_repo_branch_commit(start_repository, 'master', &block)
+        end.to yield_with_args(start_commit)
+      end
+    end
+
+    context 'when start_repository is different' do
+      let(:source_repository) { gitlab_git_from_gitaly(test_repo_read_only) }
+
+      context 'when start commit already exists' do
+        let(:start_commit) { repository.commit }
+
+        before do
+          expect(start_repository).to receive(:commit_id).and_return(start_commit.sha)
+          expect(repository).not_to receive(:fetch_sha)
+        end
+
+        it 'yields the commit for the SHA' do
+          expect do |block|
+            repository.with_repo_branch_commit(start_repository, start_commit.sha, &block)
+          end.to yield_with_args(start_commit)
+        end
+
+        it 'yields the commit for the branch' do
+          expect do |block|
+            repository.with_repo_branch_commit(start_repository, 'master', &block)
+          end.to yield_with_args(start_commit)
+        end
+      end
+
+      context 'when start commit does not exist' do
+        before do
+          expect(start_repository).to receive(:commit_id).and_return(start_commit.sha)
+          expect(repository).to receive(:fetch_sha).with(start_repository, start_commit.sha)
+        end
+
+        it 'yields the fetched commit for the SHA' do
+          expect do |block|
+            repository.with_repo_branch_commit(start_repository, start_commit.sha, &block)
+          end.to yield_with_args(nil) # since fetch_sha is mocked
+        end
+
+        it 'yields the fetched commit for the branch' do
+          expect do |block|
+            repository.with_repo_branch_commit(start_repository, 'master', &block)
+          end.to yield_with_args(nil) # since fetch_sha is mocked
+        end
+      end
+    end
+  end
+
   describe '#fetch_source_branch!' do
     let(:local_ref) { 'refs/merge-requests/1/head' }
     let(:repository) { mutable_repository }
@@ -321,6 +406,36 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
     end
   end
 
+  describe '#fetch_sha' do
+    let(:source_repository) { Gitlab::Git::RemoteRepository.new(repository) }
+    let(:sha) { 'b971194ee2d047f24cb897b6fb0d7ae99c8dd0ca' }
+    let(:git_args) { %W[fetch --no-tags ssh://gitaly/internal.git #{sha}] }
+
+    before do
+      expect(source_repository).to receive(:fetch_env)
+        .with(git_config_options: ['uploadpack.allowAnySHA1InWant=true'])
+        .and_return({})
+    end
+
+    it 'fetches the commit from the source repository' do
+      expect(repository).to receive(:run_git)
+        .with(git_args, env: {}, include_stderr: true)
+        .and_return(['success', 0])
+
+      expect(repository.fetch_sha(source_repository, sha)).to eq(sha)
+    end
+
+    it 'raises an error if the commit does not exist in the source repository' do
+      expect(repository).to receive(:run_git)
+        .with(git_args, env: {}, include_stderr: true)
+        .and_return(['error', 1])
+
+      expect do
+        repository.fetch_sha(source_repository, sha)
+      end.to raise_error(Gitlab::Git::CommandError, 'error')
+    end
+  end
+
   describe '#write_config' do
     before do
       repository_rugged.config["gitlab.fullpath"] = repository_path