From f8cecafb07792bcaf9d7ffa85766c3b33c1dd252 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Thu, 13 Jun 2019 12:44:41 +0200 Subject: [PATCH] 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 | 18 +- app/services/files/multi_service.rb | 1 + ...d-support-for-start-sha-to-commits-api.yml | 5 + doc/api/commits.md | 9 +- lib/api/commits.rb | 14 +- lib/gitlab/git.rb | 5 + lib/gitlab/git/repository.rb | 4 +- lib/gitlab/gitaly_client/operation_service.rb | 9 +- spec/lib/gitlab/git_spec.rb | 20 ++ spec/requests/api/commits_spec.rb | 213 ++++++++++++++---- .../submodules/update_service_spec.rb | 2 +- spec/spec_helper.rb | 1 + .../helpers/expect_request_with_status.rb | 11 + 13 files changed, 245 insertions(+), 67 deletions(-) create mode 100644 changelogs/unreleased/add-support-for-start-sha-to-commits-api.yml create mode 100644 spec/support/helpers/expect_request_with_status.rb 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) } } - - 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 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