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.

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

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

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

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 |
 | --------------------- | ---- | -------- | ----------- |

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

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

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

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

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
 

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) } }
-
-        it 'returns a 403' do
-          post api(url, guest), params: valid_c_params
-
-          expect(response).to have_gitlab_http_status(403)
+        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 '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) } }
 
-            before do
-              valid_c_params[:start_branch]  = "master"
-              valid_c_params[:branch]        = "patch"
+          it 'returns a 403' do
+            post api(url, guest), params: valid_c_params
+
+            expect(response).to have_gitlab_http_status(403)
+          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 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
+
+              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
+
+                  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 Integer (id)' 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_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)
-              end
-            end
-
-            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 { 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]) }
-
-                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

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
 

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.

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