Add start_sha to commits API

When passing start_branch on committing from the WebIDE, it's possible
that the branch has changed since editing started, which results in the
change being applied on top of the latest commit in the branch and
overwriting the new changes.

By passing the start_sha instead we can make sure that the change is
applied on top of the commit which the user started editing from.

13 files changed, 240 insertions, 62 deletions

diff --git a/app/services/commits/create_service.rb b/app/services/commits/create_service.rb
index bb34a3d3352..f3be68f9602 100644
--- a/app/services/commits/create_service.rb
+++ b/app/services/commits/create_service.rb
@@ -10,6 +10,7 @@ module Commits
 
       @start_project = params[:start_project] || @project
       @start_branch = params[:start_branch]
+      @start_sha = params[:start_sha]
       @branch_name = params[:branch_name]
       @force = params[:force] || false
     end
@@ -40,7 +41,7 @@ module Commits
     end
 
     def different_branch?
-      @start_branch != @branch_name || @start_project != @project
+      @start_project != @project || @start_branch != @branch_name || @start_sha.present?
     end
 
     def force?
@@ -49,6 +50,7 @@ module Commits
 
     def validate!
       validate_permissions!
+      validate_start_sha!
       validate_on_branch!
       validate_branch_existence!
 
@@ -63,7 +65,21 @@ module Commits
       end
     end
 
+    def validate_start_sha!
+      return unless @start_sha
+
+      if @start_branch
+        raise_error("You can't pass both start_branch and start_sha")
+      elsif !Gitlab::Git.commit_id?(@start_sha)
+        raise_error("Invalid start_sha '#{@start_sha}'")
+      elsif !@start_project.repository.commit(@start_sha)
+        raise_error("Cannot find start_sha '#{@start_sha}'")
+      end
+    end
+
     def validate_on_branch!
+      return unless @start_branch
+
       if !@start_project.empty_repo? && !@start_project.repository.branch_exists?(@start_branch)
         raise_error('You can only create or edit files when you are on a branch')
       end
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index d8c4e5bc5e8..65af4dd5a28 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -47,6 +47,7 @@ module Files
         author_name: @author_name,
         start_project: @start_project,
         start_branch_name: @start_branch,
+        start_sha: @start_sha,
         force: force?
       )
     rescue ArgumentError => e
diff --git a/changelogs/unreleased/add-support-for-start-sha-to-commits-api.yml b/changelogs/unreleased/add-support-for-start-sha-to-commits-api.yml
new file mode 100644
index 00000000000..f810c2c5ada
--- /dev/null
+++ b/changelogs/unreleased/add-support-for-start-sha-to-commits-api.yml
@@ -0,0 +1,5 @@
+---
+title: Add support for start_sha to commits API
+merge_request: 29598
+author:
+type: changed
diff --git a/doc/api/commits.md b/doc/api/commits.md
index 6eb4c47415f..1a835c0a872 100644
--- a/doc/api/commits.md
+++ b/doc/api/commits.md
@@ -72,15 +72,16 @@ POST /projects/:id/repository/commits
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
-| `branch` | string | yes | Name of the branch to commit into. To create a new branch, also provide `start_branch`. |
+| `branch` | string | yes | Name of the branch to commit into. To create a new branch, also provide either `start_branch` or `start_sha`, and optionally `start_project`. |
 | `commit_message` | string | yes | Commit message |
-| `start_branch` | string | no | Name of the branch to start the new commit from |
-| `start_project` | integer/string | no | The project ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) to start the commit from. Defaults to the value of `id`. |
+| `start_branch` | string | no | Name of the branch to start the new branch from |
+| `start_sha` | string | no | SHA of the commit to start the new branch from |
+| `start_project` | integer/string | no | The project ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) to start the new branch from. Defaults to the value of `id`. |
 | `actions[]` | array | yes | An array of action hashes to commit as a batch. See the next table for what attributes it can take. |
 | `author_email` | string | no | Specify the commit author's email address |
 | `author_name` | string | no | Specify the commit author's name |
 | `stats` | boolean | no | Include commit stats. Default is true |
-| `force` | boolean | no | When `true` overwrites the target branch with a new commit based on the `start_branch` |
+| `force` | boolean | no | When `true` overwrites the target branch with a new commit based on the `start_branch` or `start_sha` |
 
 | `actions[]` Attribute | Type | Required | Description |
 | --------------------- | ---- | -------- | ----------- |
diff --git a/lib/api/commits.rb b/lib/api/commits.rb
index c414ad75d9d..0aeb9584641 100644
--- a/lib/api/commits.rb
+++ b/lib/api/commits.rb
@@ -76,7 +76,7 @@ module API
         detail 'This feature was introduced in GitLab 8.13'
       end
       params do
-        requires :branch, type: String, desc: 'Name of the branch to commit into. To create a new branch, also provide `start_branch`.', allow_blank: false
+        requires :branch, type: String, desc: 'Name of the branch to commit into. To create a new branch, also provide either `start_branch` or `start_sha`, and optionally `start_project`.', allow_blank: false
         requires :commit_message, type: String, desc: 'Commit message'
         requires :actions, type: Array, desc: 'Actions to perform in commit' do
           requires :action, type: String, desc: 'The action to perform, `create`, `delete`, `move`, `update`, `chmod`', values: %w[create update move delete chmod].freeze
@@ -98,12 +98,16 @@ module API
             requires :execute_filemode, type: Boolean, desc: 'When `true/false` enables/disables the execute flag on the file.'
           end
         end
-        optional :start_branch, type: String, desc: 'Name of the branch to start the new commit from'
-        optional :start_project, types: [Integer, String], desc: 'The ID or path of the project to start the commit from'
+
+        optional :start_branch, type: String, desc: 'Name of the branch to start the new branch from'
+        optional :start_sha, type: String, desc: 'SHA of the commit to start the new branch from'
+        mutually_exclusive :start_branch, :start_sha
+
+        optional :start_project, types: [Integer, String], desc: 'The ID or path of the project to start the new branch from'
         optional :author_email, type: String, desc: 'Author email for commit'
         optional :author_name, type: String, desc: 'Author name for commit'
         optional :stats, type: Boolean, default: true, desc: 'Include commit stats'
-        optional :force, type: Boolean, default: false, desc: 'When `true` overwrites the target branch with a new commit based on the `start_branch`'
+        optional :force, type: Boolean, default: false, desc: 'When `true` overwrites the target branch with a new commit based on the `start_branch` or `start_sha`'
       end
       post ':id/repository/commits' do
         if params[:start_project]
@@ -118,7 +122,7 @@ module API
 
         attrs = declared_params
         attrs[:branch_name] = attrs.delete(:branch)
-        attrs[:start_branch] ||= attrs[:branch_name]
+        attrs[:start_branch] ||= attrs[:branch_name] unless attrs[:start_sha]
         attrs[:start_project] = start_project if start_project
 
         result = ::Files::MultiService.new(user_project, current_user, attrs).execute
diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb
index 44a62586a23..df9f33baec2 100644
--- a/lib/gitlab/git.rb
+++ b/lib/gitlab/git.rb
@@ -9,6 +9,7 @@ module Gitlab
     # https://github.com/git/git/blob/3ad8b5bf26362ac67c9020bf8c30eee54a84f56d/cache.h#L1011-L1012
     EMPTY_TREE_ID = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'.freeze
     BLANK_SHA = ('0' * 40).freeze
+    COMMIT_ID = /\A[0-9a-f]{40}\z/.freeze
     TAG_REF_PREFIX = "refs/tags/".freeze
     BRANCH_REF_PREFIX = "refs/heads/".freeze
 
@@ -65,6 +66,10 @@ module Gitlab
         ref == BLANK_SHA
       end
 
+      def commit_id?(ref)
+        COMMIT_ID.match?(ref)
+      end
+
       def version
         Gitlab::Git::Version.git_version
       end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index a7d9ba51277..6e8aa5d578e 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -873,13 +873,13 @@ module Gitlab
       def multi_action(
         user, branch_name:, message:, actions:,
         author_email: nil, author_name: nil,
-        start_branch_name: nil, start_repository: self,
+        start_branch_name: nil, start_sha: nil, start_repository: self,
         force: false)
 
         wrapped_gitaly_errors do
           gitaly_operation_client.user_commit_files(user, branch_name,
               message, actions, author_email, author_name,
-              start_branch_name, start_repository, force)
+              start_branch_name, start_repository, force, start_sha)
         end
       end
       # rubocop:enable Metrics/ParameterLists
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index 783c2ff0915..33ca428a942 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -325,11 +325,11 @@ module Gitlab
       # rubocop:disable Metrics/ParameterLists
       def user_commit_files(
         user, branch_name, commit_message, actions, author_email, author_name,
-        start_branch_name, start_repository, force = false)
+        start_branch_name, start_repository, force = false, start_sha = nil)
         req_enum = Enumerator.new do |y|
           header = user_commit_files_request_header(user, branch_name,
           commit_message, actions, author_email, author_name,
-          start_branch_name, start_repository, force)
+          start_branch_name, start_repository, force, start_sha)
 
           y.yield Gitaly::UserCommitFilesRequest.new(header: header)
 
@@ -445,7 +445,7 @@ module Gitlab
       # rubocop:disable Metrics/ParameterLists
       def user_commit_files_request_header(
         user, branch_name, commit_message, actions, author_email, author_name,
-        start_branch_name, start_repository, force)
+        start_branch_name, start_repository, force, start_sha)
 
         Gitaly::UserCommitFilesRequestHeader.new(
           repository: @gitaly_repo,
@@ -456,7 +456,8 @@ module Gitlab
           commit_author_email: encode_binary(author_email),
           start_branch_name: encode_binary(start_branch_name),
           start_repository: start_repository.gitaly_repository,
-          force: force
+          force: force,
+          start_sha: encode_binary(start_sha)
         )
       end
       # rubocop:enable Metrics/ParameterLists
diff --git a/spec/lib/gitlab/git_spec.rb b/spec/lib/gitlab/git_spec.rb
index ce15057dd7d..6515be85ae3 100644
--- a/spec/lib/gitlab/git_spec.rb
+++ b/spec/lib/gitlab/git_spec.rb
@@ -39,6 +39,26 @@ describe Gitlab::Git do
     end
   end
 
+  describe '.commit_id?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:sha, :result) do
+      ''                                         | false
+      'foobar'                                   | false
+      '4b825dc'                                  | false
+      'zzz25dc642cb6eb9a060e54bf8d69288fbee4904' | false
+
+      '4b825dc642cb6eb9a060e54bf8d69288fbee4904' | true
+      Gitlab::Git::BLANK_SHA                     | true
+    end
+
+    with_them do
+      it 'returns the expected result' do
+        expect(described_class.commit_id?(sha)).to eq(result)
+      end
+    end
+  end
+
   describe '.shas_eql?' do
     using RSpec::Parameterized::TableSyntax
 
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index 204e378f7be..311ef3ee99a 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -320,67 +320,132 @@ describe API::Commits do
         end
       end
 
-      context 'when the API user is a guest' do
+      context 'when committing to a new branch' do
         def last_commit_id(project, branch_name)
           project.repository.find_branch(branch_name)&.dereferenced_target&.id
         end
 
-        let(:public_project) { create(:project, :public, :repository) }
-        let!(:url) { "/projects/#{public_project.id}/repository/commits" }
-        let(:guest) { create(:user).tap { |u| public_project.add_guest(u) } }
+        before do
+          valid_c_params[:start_branch] = 'master'
+          valid_c_params[:branch] = 'patch'
+        end
 
-        it 'returns a 403' do
-          post api(url, guest), params: valid_c_params
+        context 'when the API user is a guest' do
+          let(:public_project) { create(:project, :public, :repository) }
+          let(:url) { "/projects/#{public_project.id}/repository/commits" }
+          let(:guest) { create(:user).tap { |u| public_project.add_guest(u) } }
 
-          expect(response).to have_gitlab_http_status(403)
-        end
+          it 'returns a 403' do
+            post api(url, guest), params: valid_c_params
 
-        context 'when start_project is provided' do
-          context 'when posting to a forked project the user owns' do
-            let!(:forked_project) { fork_project(public_project, guest, namespace: guest.namespace, repository: true) }
-            let!(:url) { "/projects/#{forked_project.id}/repository/commits" }
+            expect(response).to have_gitlab_http_status(403)
+          end
 
-            before do
-              valid_c_params[:start_branch]  = "master"
-              valid_c_params[:branch]        = "patch"
-            end
+          context 'when start_project is provided' do
+            context 'when posting to a forked project the user owns' do
+              let(:forked_project) { fork_project(public_project, guest, namespace: guest.namespace, repository: true) }
+              let(:url) { "/projects/#{forked_project.id}/repository/commits" }
+
+              context 'identified by Integer (id)' do
+                before do
+                  valid_c_params[:start_project] = public_project.id
+                end
+
+                it 'adds a new commit to forked_project and returns a 201' do
+                  expect_request_with_status(201) { post api(url, guest), params: valid_c_params }
+                    .to change { last_commit_id(forked_project, valid_c_params[:branch]) }
+                    .and not_change { last_commit_id(public_project, valid_c_params[:start_branch]) }
+                end
+              end
 
-            context 'identified by Integer (id)' do
-              before do
-                valid_c_params[:start_project] = public_project.id
+              context 'identified by String (full_path)' do
+                before do
+                  valid_c_params[:start_project] = public_project.full_path
+                end
+
+                it 'adds a new commit to forked_project and returns a 201' do
+                  expect_request_with_status(201) { post api(url, guest), params: valid_c_params }
+                    .to change { last_commit_id(forked_project, valid_c_params[:branch]) }
+                    .and not_change { last_commit_id(public_project, valid_c_params[:start_branch]) }
+                end
               end
 
-              it 'adds a new commit to forked_project and returns a 201' do
-                expect { post api(url, guest), params: valid_c_params }
-                  .to change { last_commit_id(forked_project, valid_c_params[:branch]) }
-                  .and not_change { last_commit_id(public_project, valid_c_params[:start_branch]) }
+              context 'when branch already exists' do
+                before do
+                  valid_c_params.delete(:start_branch)
+                  valid_c_params[:branch] = 'master'
+                  valid_c_params[:start_project] = public_project.id
+                end
+
+                it 'returns a 400' do
+                  post api(url, guest), params: valid_c_params
+
+                  expect(response).to have_gitlab_http_status(400)
+                  expect(json_response['message']).to eq("A branch called 'master' already exists. Switch to that branch in order to make changes")
+                end
+
+                context 'when force is set to true' do
+                  before do
+                    valid_c_params[:force] = true
+                  end
+
+                  it 'adds a new commit to forked_project and returns a 201' do
+                    expect_request_with_status(201) { post api(url, guest), params: valid_c_params }
+                      .to change { last_commit_id(forked_project, valid_c_params[:branch]) }
+                      .and not_change { last_commit_id(public_project, valid_c_params[:branch]) }
+                  end
+                end
+              end
+
+              context 'when start_sha is also provided' do
+                let(:forked_project) { fork_project(public_project, guest, namespace: guest.namespace, repository: false) }
+                let(:start_sha) { public_project.repository.commit.parent.sha }
+
+                before do
+                  # initialize an empty repository to force fetching from the original project
+                  forked_project.repository.create_if_not_exists
 
-                expect(response).to have_gitlab_http_status(201)
+                  valid_c_params[:start_project] = public_project.id
+                  valid_c_params[:start_sha] = start_sha
+                  valid_c_params.delete(:start_branch)
+                end
+
+                it 'fetches the start_sha from the original project to use as parent commit and returns a 201' do
+                  expect_request_with_status(201) { post api(url, guest), params: valid_c_params }
+                    .to change { last_commit_id(forked_project, valid_c_params[:branch]) }
+                    .and not_change { last_commit_id(forked_project, 'master') }
+
+                  last_commit = forked_project.repository.find_branch(valid_c_params[:branch]).dereferenced_target
+                  expect(last_commit.parent_id).to eq(start_sha)
+                end
               end
             end
 
-            context 'identified by String (full_path)' do
+            context 'when the target project is not part of the fork network of start_project' do
+              let(:unrelated_project) { create(:project, :public, :repository, creator: guest) }
+              let(:url) { "/projects/#{unrelated_project.id}/repository/commits" }
+
               before do
-                valid_c_params[:start_project] = public_project.full_path
+                valid_c_params[:start_branch] = 'master'
+                valid_c_params[:branch] = 'patch'
+                valid_c_params[:start_project] = public_project.id
               end
 
-              it 'adds a new commit to forked_project and returns a 201' do
-                expect { post api(url, guest), params: valid_c_params }
-                  .to change { last_commit_id(forked_project, valid_c_params[:branch]) }
-                  .and not_change { last_commit_id(public_project, valid_c_params[:start_branch]) }
+              it 'returns a 403' do
+                post api(url, guest), params: valid_c_params
 
-                expect(response).to have_gitlab_http_status(201)
+                expect(response).to have_gitlab_http_status(403)
               end
             end
           end
 
-          context 'when the target project is not part of the fork network of start_project' do
-            let(:unrelated_project) { create(:project, :public, :repository, creator: guest) }
-            let!(:url) { "/projects/#{unrelated_project.id}/repository/commits" }
+          context 'when posting to a forked project the user does not have write access' do
+            let(:forked_project) { fork_project(public_project, user, namespace: user.namespace, repository: true) }
+            let(:url) { "/projects/#{forked_project.id}/repository/commits" }
 
             before do
-              valid_c_params[:start_branch]  = "master"
-              valid_c_params[:branch]        = "patch"
+              valid_c_params[:start_branch] = 'master'
+              valid_c_params[:branch] = 'patch'
               valid_c_params[:start_project] = public_project.id
             end
 
@@ -392,20 +457,68 @@ describe API::Commits do
           end
         end
 
-        context 'when posting to a forked project the user does not have write access' do
-          let!(:forked_project) { fork_project(public_project, user, namespace: user.namespace, repository: true) }
-          let!(:url) { "/projects/#{forked_project.id}/repository/commits" }
+        context 'when start_sha is provided' do
+          let(:start_sha) { project.repository.commit.parent.sha }
 
           before do
-            valid_c_params[:start_branch]  = "master"
-            valid_c_params[:branch]        = "patch"
-            valid_c_params[:start_project] = public_project.id
+            valid_c_params[:start_sha] = start_sha
+            valid_c_params.delete(:start_branch)
           end
 
-          it 'returns a 403' do
-            post api(url, guest), params: valid_c_params
+          it 'returns a 400 if start_branch is also provided' do
+            valid_c_params[:start_branch] = 'master'
+            post api(url, user), params: valid_c_params
 
-            expect(response).to have_gitlab_http_status(403)
+            expect(response).to have_gitlab_http_status(400)
+            expect(json_response['error']).to eq('start_branch, start_sha are mutually exclusive')
+          end
+
+          it 'returns a 400 if branch already exists' do
+            valid_c_params[:branch] = 'master'
+            post api(url, user), params: valid_c_params
+
+            expect(response).to have_gitlab_http_status(400)
+            expect(json_response['message']).to eq("A branch called 'master' already exists. Switch to that branch in order to make changes")
+          end
+
+          it 'returns a 400 if start_sha does not exist' do
+            valid_c_params[:start_sha] = '1' * 40
+            post api(url, user), params: valid_c_params
+
+            expect(response).to have_gitlab_http_status(400)
+            expect(json_response['message']).to eq("Cannot find start_sha '#{valid_c_params[:start_sha]}'")
+          end
+
+          it 'returns a 400 if start_sha is not a full SHA' do
+            valid_c_params[:start_sha] = start_sha.slice(0, 7)
+            post api(url, user), params: valid_c_params
+
+            expect(response).to have_gitlab_http_status(400)
+            expect(json_response['message']).to eq("Invalid start_sha '#{valid_c_params[:start_sha]}'")
+          end
+
+          it 'uses the start_sha as parent commit and returns a 201' do
+            expect_request_with_status(201) { post api(url, user), params: valid_c_params }
+              .to change { last_commit_id(project, valid_c_params[:branch]) }
+              .and not_change { last_commit_id(project, 'master') }
+
+            last_commit = project.repository.find_branch(valid_c_params[:branch]).dereferenced_target
+            expect(last_commit.parent_id).to eq(start_sha)
+          end
+
+          context 'when force is set to true and branch already exists' do
+            before do
+              valid_c_params[:force] = true
+              valid_c_params[:branch] = 'master'
+            end
+
+            it 'uses the start_sha as parent commit and returns a 201' do
+              expect_request_with_status(201) { post api(url, user), params: valid_c_params }
+                .to change { last_commit_id(project, valid_c_params[:branch]) }
+
+              last_commit = project.repository.find_branch(valid_c_params[:branch]).dereferenced_target
+              expect(last_commit.parent_id).to eq(start_sha)
+            end
           end
         end
       end
diff --git a/spec/services/submodules/update_service_spec.rb b/spec/services/submodules/update_service_spec.rb
index cf92350c1b2..47b31d4bcbf 100644
--- a/spec/services/submodules/update_service_spec.rb
+++ b/spec/services/submodules/update_service_spec.rb
@@ -142,7 +142,7 @@ describe Submodules::UpdateService do
             let(:branch_name) { nil }
 
             it_behaves_like 'returns error result' do
-              let(:error_message) { 'You can only create or edit files when you are on a branch' }
+              let(:error_message) { 'Invalid parameters' }
             end
           end
 
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 95e0d8858b9..fb9e3b295f8 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -104,6 +104,7 @@ RSpec.configure do |config|
   config.include Rails.application.routes.url_helpers, type: :routing
   config.include PolicyHelpers, type: :policy
   config.include MemoryUsageHelper
+  config.include ExpectRequestWithStatus, type: :request
 
   if ENV['CI']
     # This includes the first try, i.e. tests will be run 4 times before failing.
diff --git a/spec/support/helpers/expect_request_with_status.rb b/spec/support/helpers/expect_request_with_status.rb
new file mode 100644
index 00000000000..0469a94e336
--- /dev/null
+++ b/spec/support/helpers/expect_request_with_status.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+module ExpectRequestWithStatus
+  def expect_request_with_status(status)
+    expect do
+      yield
+
+      expect(response).to have_gitlab_http_status(status)
+    end
+  end
+end