Merge branch 'webide-commit-use-correct-parent' into 'master'

Use correct parent when committing in WebIDE

See merge request gitlab-org/gitlab-ce!29598

app/assets/javascripts/ide/services/index.js

@@ -56,13 +56,7 @@ export default {
     return Api.branchSingle(projectId, currentBranchId);
   },
   commit(projectId, payload) {
-    // Currently the `commit` endpoint does not support `start_sha` so we
-    // have to make the request in the FE. This is not ideal and will be
-    // resolved soon. https://gitlab.com/gitlab-org/gitlab-ce/issues/59023
-    const { branch, start_sha: ref } = payload;
-    const branchPromise = ref ? Api.createBranch(projectId, { ref, branch }) : Promise.resolve();
-
-    return branchPromise.then(() => Api.commitMultiple(projectId, payload));
+    return Api.commitMultiple(projectId, payload);
   },
   getFiles(projectUrl, branchId) {
     const url = `${projectUrl}/files/${branchId}`;

app/assets/javascripts/ide/stores/utils.js

@@ -155,7 +155,7 @@ export const createCommitPayload = ({
     last_commit_id:
       newBranch || f.deleted || f.prevPath || f.replaces ? undefined : f.lastCommitSha,
   })),
-  start_sha: newBranch ? rootGetters.lastCommit.short_id : undefined,
+  start_sha: newBranch ? rootGetters.lastCommit.id : undefined,
 });
 
 export const createNewMergeRequestUrl = (projectUrl, source, target) =>

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

+---
+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/frontend/ide/services/index_spec.js

@@ -16,40 +16,16 @@ describe('IDE services', () => {
         branch: TEST_BRANCH,
         commit_message: 'Hello world',
         actions: [],
-        start_sha: undefined,
+        start_sha: TEST_COMMIT_SHA,
       };
 
-      Api.createBranch.mockReturnValue(Promise.resolve());
       Api.commitMultiple.mockReturnValue(Promise.resolve());
     });
 
-    describe.each`
-      startSha           | shouldCreateBranch
-      ${undefined}       | ${false}
-      ${TEST_COMMIT_SHA} | ${true}
-    `('when start_sha is $startSha', ({ startSha, shouldCreateBranch }) => {
-      beforeEach(() => {
-        payload.start_sha = startSha;
+    it('should commit', () => {
+      services.commit(TEST_PROJECT_ID, payload);
 
-        return services.commit(TEST_PROJECT_ID, payload);
-      });
-
-      if (shouldCreateBranch) {
-        it('should create branch', () => {
-          expect(Api.createBranch).toHaveBeenCalledWith(TEST_PROJECT_ID, {
-            ref: TEST_COMMIT_SHA,
-            branch: TEST_BRANCH,
-          });
-        });
-      } else {
-        it('should not create branch', () => {
-          expect(Api.createBranch).not.toHaveBeenCalled();
-        });
-      }
-
-      it('should commit', () => {
-        expect(Api.commitMultiple).toHaveBeenCalledWith(TEST_PROJECT_ID, payload);
-      });
+      expect(Api.commitMultiple).toHaveBeenCalledWith(TEST_PROJECT_ID, payload);
     });
   });
 });

spec/javascripts/ide/stores/modules/commit/actions_spec.js

@@ -245,7 +245,7 @@ describe('IDE commit module actions', () => {
           master: {
             workingReference: '1',
             commit: {
-              short_id: TEST_COMMIT_SHA,
+              id: TEST_COMMIT_SHA,
             },
           },
         },

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) } }
+        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

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

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