diff options
-rw-r--r-- | app/controllers/concerns/creates_commit.rb | 21 | ||||
-rw-r--r-- | app/models/repository.rb | 2 | ||||
-rw-r--r-- | app/services/files/base_service.rb | 16 | ||||
-rw-r--r-- | app/services/git_operation_service.rb | 37 | ||||
-rw-r--r-- | changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml | 4 | ||||
-rw-r--r-- | spec/factories/projects.rb | 4 | ||||
-rw-r--r-- | spec/requests/api/files_spec.rb | 14 | ||||
-rw-r--r-- | spec/requests/api/v3/files_spec.rb | 285 |
8 files changed, 42 insertions, 341 deletions
diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index d62130ce044..9ac8197e45a 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -4,8 +4,6 @@ module CreatesCommit def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil) set_commit_variables - - start_branch = @mr_target_branch commit_params = @commit_params.merge( start_project: @mr_target_project, start_branch: @mr_target_branch, @@ -119,23 +117,8 @@ module CreatesCommit # Merge request to this project @mr_target_project = @project + @mr_target_branch ||= @ref || @target_branch - @mr_target_branch = @ref || @target_branch - - @mr_source_branch = guess_mr_source_branch - end - - def guess_mr_source_branch - # XXX: Happens when viewing a commit without a branch. In this case, - # @target_branch would be the default branch for @mr_source_project, - # however we want a generated new branch here. Thus we can't use - # @target_branch, but should pass nil to indicate that we want a new - # branch instead of @target_branch. - return if - create_merge_request? && - # XXX: Don't understand why rubocop prefers this indention - @mr_source_project.repository.branch_exists?(@target_branch) - - @target_branch + @mr_source_branch = @target_branch end end diff --git a/app/models/repository.rb b/app/models/repository.rb index 5085e5f436c..1a6418a4491 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1075,8 +1075,6 @@ class Repository end def with_repo_branch_commit(start_repository, start_branch_name) - return yield(nil) if start_repository.empty_repo? - branch_name_or_sha = if start_repository == self start_branch_name diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 0f31f4d7cbb..0a25f56d24c 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -58,12 +58,16 @@ module Files raise_error("You are not allowed to push into this branch") end - if !@start_project.empty_repo? && !@start_project.repository.branch_exists?(@start_branch) - raise ValidationError, 'You can only create or edit files when you are on a branch' - end - - if !project.empty_repo? && different_branch? && repository.branch_exists?(@branch_name) - raise ValidationError, "A branch called #{@branch_name} already exists. Switch to that branch in order to make changes" + unless project.empty_repo? + unless @start_project.repository.branch_exists?(@start_branch) + raise_error('You can only create or edit files when you are on a branch') + end + + if different_branch? + if repository.branch_exists?(@target_branch) + raise_error('Branch with such name already exists. You need to switch to this branch in order to make changes') + end + end end end diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb index ed6ea638235..27bcc047601 100644 --- a/app/services/git_operation_service.rb +++ b/app/services/git_operation_service.rb @@ -56,16 +56,12 @@ class GitOperationService start_project: repository.project, &block) - start_repository = start_project.repository - start_branch_name = nil if start_repository.empty_repo? - - if start_branch_name && !start_repository.branch_exists?(start_branch_name) - raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.path_with_namespace}" - end + check_with_branch_arguments!( + branch_name, start_branch_name, start_project) update_branch_with_hooks(branch_name) do repository.with_repo_branch_commit( - start_repository, + start_project.repository, start_branch_name || branch_name, &block) end @@ -153,4 +149,31 @@ class GitOperationService repository.raw_repository.autocrlf = :input end end + + def check_with_branch_arguments!( + branch_name, start_branch_name, start_project) + return if repository.branch_exists?(branch_name) + + if repository.project != start_project + unless start_branch_name + raise ArgumentError, + 'Should also pass :start_branch_name if' + + ' :start_project is different from current project' + end + + unless start_project.repository.branch_exists?(start_branch_name) + raise ArgumentError, + "Cannot find branch #{branch_name} nor" \ + " #{start_branch_name} from" \ + " #{start_project.path_with_namespace}" + end + elsif start_branch_name + unless repository.branch_exists?(start_branch_name) + raise ArgumentError, + "Cannot find branch #{branch_name} nor" \ + " #{start_branch_name} from" \ + " #{repository.project.path_with_namespace}" + end + end + end end diff --git a/changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml b/changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml deleted file mode 100644 index 7ac25c0a83e..00000000000 --- a/changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -title: Fix creating a file in an empty repo using the API -merge_request: 9632 -author: diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 3d8b7e879c7..c80b09e9b9d 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -39,10 +39,6 @@ FactoryGirl.define do trait :empty_repo do after(:create) do |project| project.create_repository - - # We delete hooks so that gitlab-shell will not try to authenticate with - # an API that isn't running - FileUtils.rm_r(File.join(project.repository_storage_path, "#{project.path_with_namespace}.git", 'hooks')) end end diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 07061548320..5e26e779366 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -147,20 +147,6 @@ describe API::Files, api: true do expect(last_commit.author_name).to eq(author_name) end end - - context 'when the repo is empty' do - let!(:project) { create(:project_empty_repo, namespace: user.namespace ) } - - it "creates a new file in project repo" do - post api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(201) - expect(json_response['file_path']).to eq('newfile.rb') - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(user.email) - expect(last_commit.author_name).to eq(user.name) - end - end end describe "PUT /projects/:id/repository/files" do diff --git a/spec/requests/api/v3/files_spec.rb b/spec/requests/api/v3/files_spec.rb deleted file mode 100644 index 3b61139a2cd..00000000000 --- a/spec/requests/api/v3/files_spec.rb +++ /dev/null @@ -1,285 +0,0 @@ -require 'spec_helper' - -describe API::V3::Files, api: true do - include ApiHelpers - - # I have to remove periods from the end of the name - # This happened when the user's name had a suffix (i.e. "Sr.") - # This seems to be what git does under the hood. For example, this commit: - # - # $ git commit --author='Foo Sr. <foo@example.com>' -m 'Where's my trailing period?' - # - # results in this: - # - # $ git show --pretty - # ... - # Author: Foo Sr <foo@example.com> - # ... - - let(:user) { create(:user) } - let!(:project) { create(:project, :repository, namespace: user.namespace ) } - let(:guest) { create(:user) { |u| project.add_guest(u) } } - let(:file_path) { 'files/ruby/popen.rb' } - let(:params) do - { - file_path: file_path, - ref: 'master' - } - end - let(:author_email) { FFaker::Internet.email } - let(:author_name) { FFaker::Name.name.chomp("\.") } - - before { project.team << [user, :developer] } - - describe "GET /projects/:id/repository/files" do - let(:route) { "/projects/#{project.id}/repository/files" } - - shared_examples_for 'repository files' do - it "returns file info" do - get v3_api(route, current_user), params - - expect(response).to have_http_status(200) - expect(json_response['file_path']).to eq(file_path) - expect(json_response['file_name']).to eq('popen.rb') - expect(json_response['last_commit_id']).to eq('570e7b2abdd848b95f2f578043fc23bd6f6fd24d') - expect(Base64.decode64(json_response['content']).lines.first).to eq("require 'fileutils'\n") - end - - context 'when no params are given' do - it_behaves_like '400 response' do - let(:request) { get v3_api(route, current_user) } - end - end - - context 'when file_path does not exist' do - let(:params) do - { - file_path: 'app/models/application.rb', - ref: 'master', - } - end - - it_behaves_like '404 response' do - let(:request) { get v3_api(route, current_user), params } - let(:message) { '404 File Not Found' } - end - end - - context 'when repository is disabled' do - include_context 'disabled repository' - - it_behaves_like '403 response' do - let(:request) { get v3_api(route, current_user), params } - end - end - end - - context 'when unauthenticated', 'and project is public' do - it_behaves_like 'repository files' do - let(:project) { create(:project, :public) } - let(:current_user) { nil } - end - end - - context 'when unauthenticated', 'and project is private' do - it_behaves_like '404 response' do - let(:request) { get v3_api(route), params } - let(:message) { '404 Project Not Found' } - end - end - - context 'when authenticated', 'as a developer' do - it_behaves_like 'repository files' do - let(:current_user) { user } - end - end - - context 'when authenticated', 'as a guest' do - it_behaves_like '403 response' do - let(:request) { get v3_api(route, guest), params } - end - end - end - - describe "POST /projects/:id/repository/files" do - let(:valid_params) do - { - file_path: 'newfile.rb', - branch_name: 'master', - content: 'puts 8', - commit_message: 'Added newfile' - } - end - - it "creates a new file in project repo" do - post v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(201) - expect(json_response['file_path']).to eq('newfile.rb') - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(user.email) - expect(last_commit.author_name).to eq(user.name) - end - - it "returns a 400 bad request if no params given" do - post v3_api("/projects/#{project.id}/repository/files", user) - - expect(response).to have_http_status(400) - end - - it "returns a 400 if editor fails to create file" do - allow_any_instance_of(Repository).to receive(:create_file). - and_return(false) - - post v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(400) - end - - context "when specifying an author" do - it "creates a new file with the specified author" do - valid_params.merge!(author_email: author_email, author_name: author_name) - - post v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(201) - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(author_email) - expect(last_commit.author_name).to eq(author_name) - end - end - - context 'when the repo is empty' do - let!(:project) { create(:project_empty_repo, namespace: user.namespace ) } - - it "creates a new file in project repo" do - post v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(201) - expect(json_response['file_path']).to eq('newfile.rb') - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(user.email) - expect(last_commit.author_name).to eq(user.name) - end - end - end - - describe "PUT /projects/:id/repository/files" do - let(:valid_params) do - { - file_path: file_path, - branch_name: 'master', - content: 'puts 8', - commit_message: 'Changed file' - } - end - - it "updates existing file in project repo" do - put v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(200) - expect(json_response['file_path']).to eq(file_path) - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(user.email) - expect(last_commit.author_name).to eq(user.name) - end - - it "returns a 400 bad request if no params given" do - put v3_api("/projects/#{project.id}/repository/files", user) - - expect(response).to have_http_status(400) - end - - context "when specifying an author" do - it "updates a file with the specified author" do - valid_params.merge!(author_email: author_email, author_name: author_name, content: "New content") - - put v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(200) - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(author_email) - expect(last_commit.author_name).to eq(author_name) - end - end - end - - describe "DELETE /projects/:id/repository/files" do - let(:valid_params) do - { - file_path: file_path, - branch_name: 'master', - commit_message: 'Changed file' - } - end - - it "deletes existing file in project repo" do - delete v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(200) - expect(json_response['file_path']).to eq(file_path) - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(user.email) - expect(last_commit.author_name).to eq(user.name) - end - - it "returns a 400 bad request if no params given" do - delete v3_api("/projects/#{project.id}/repository/files", user) - - expect(response).to have_http_status(400) - end - - it "returns a 400 if fails to create file" do - allow_any_instance_of(Repository).to receive(:delete_file).and_return(false) - - delete v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(400) - end - - context "when specifying an author" do - it "removes a file with the specified author" do - valid_params.merge!(author_email: author_email, author_name: author_name) - - delete v3_api("/projects/#{project.id}/repository/files", user), valid_params - - expect(response).to have_http_status(200) - last_commit = project.repository.commit.raw - expect(last_commit.author_email).to eq(author_email) - expect(last_commit.author_name).to eq(author_name) - end - end - end - - describe "POST /projects/:id/repository/files with binary file" do - let(:file_path) { 'test.bin' } - let(:put_params) do - { - file_path: file_path, - branch_name: 'master', - content: 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAAACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII=', - commit_message: 'Binary file with a \n should not be touched', - encoding: 'base64' - } - end - let(:get_params) do - { - file_path: file_path, - ref: 'master', - } - end - - before do - post v3_api("/projects/#{project.id}/repository/files", user), put_params - end - - it "remains unchanged" do - get v3_api("/projects/#{project.id}/repository/files", user), get_params - - expect(response).to have_http_status(200) - expect(json_response['file_path']).to eq(file_path) - expect(json_response['file_name']).to eq(file_path) - expect(json_response['content']).to eq(put_params[:content]) - end - end -end |