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