From 80543e0abd65a575bcc43f2482279af3980f4acc Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 1 Mar 2017 12:08:51 -0600 Subject: Fix creating a file in an empty repo using the API --- app/controllers/concerns/creates_commit.rb | 7 +--- app/models/repository.rb | 2 ++ app/services/files/base_service.rb | 16 ++++------ app/services/git_operation_service.rb | 37 ++++++---------------- .../dm-fix-api-create-file-on-empty-repo.yml | 4 +++ spec/requests/api/files_spec.rb | 14 ++++++++ spec/requests/api/v3/files_spec.rb | 14 ++++++++ 7 files changed, 50 insertions(+), 44 deletions(-) create mode 100644 changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 2fe03020d2d..f897f828cec 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -4,7 +4,7 @@ module CreatesCommit def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil) set_commit_variables - start_branch = @mr_target_branch unless initial_commit? + start_branch = @mr_target_branch commit_params = @commit_params.merge( start_project: @mr_target_project, start_branch: start_branch, @@ -117,11 +117,6 @@ module CreatesCommit @mr_source_branch = guess_mr_source_branch end - def initial_commit? - @mr_target_branch.nil? || - !@mr_target_project.repository.branch_exists?(@mr_target_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, diff --git a/app/models/repository.rb b/app/models/repository.rb index 0dbf246c3a4..d410d60dbad 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -995,6 +995,8 @@ 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 31869c2f01e..3480cb70f67 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -58,16 +58,12 @@ module Files raise_error("You are not allowed to push into this branch") end - 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 + 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, 'Branch with such name already exists. You need to switch to this branch in order to make changes' end end diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb index 27bcc047601..507e0a6680a 100644 --- a/app/services/git_operation_service.rb +++ b/app/services/git_operation_service.rb @@ -56,13 +56,14 @@ class GitOperationService start_project: repository.project, &block) - check_with_branch_arguments!( - branch_name, start_branch_name, start_project) + start_branch ||= branch_name + + verify_start_branch_exists!(start_project.repository, start_branch_name) update_branch_with_hooks(branch_name) do repository.with_repo_branch_commit( start_project.repository, - start_branch_name || branch_name, + start_branch_name, &block) end end @@ -150,30 +151,10 @@ class GitOperationService 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 + def verify_start_branch_exists!(start_repository, start_branch_name) + return if start_repository.empty_repo? + return if start_repository.branch_exists?(start_branch_name) + + raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.path_with_namespace}" 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 new file mode 100644 index 00000000000..7ac25c0a83e --- /dev/null +++ b/changelogs/unreleased/dm-fix-api-create-file-on-empty-repo.yml @@ -0,0 +1,4 @@ +--- +title: Fix creating a file in an empty repo using the API +merge_request: 9632 +author: diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 31b1aca6d73..91f8a35e045 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -147,6 +147,20 @@ 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 index 93637053626..dc5691a106f 100644 --- a/spec/requests/api/v3/files_spec.rb +++ b/spec/requests/api/v3/files_spec.rb @@ -148,6 +148,20 @@ describe API::V3::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 -- cgit v1.2.1 From 8ca9bf1d3b9c4afc81abeeecee2a97167547809c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 1 Mar 2017 14:29:33 -0600 Subject: Fix variable name and change copy --- app/services/files/base_service.rb | 2 +- app/services/git_operation_service.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 3480cb70f67..c8a60422bf4 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -63,7 +63,7 @@ module Files end if !project.empty_repo? && different_branch? && repository.branch_exists?(@branch_name) - raise ValidationError, 'Branch with such name already exists. You need to switch to this branch in order to make changes' + raise ValidationError, "A branch called #{@branch_name} already exists. Switch to that branch in order to make changes" end end diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb index 507e0a6680a..82ef34a4863 100644 --- a/app/services/git_operation_service.rb +++ b/app/services/git_operation_service.rb @@ -56,7 +56,7 @@ class GitOperationService start_project: repository.project, &block) - start_branch ||= branch_name + start_branch_name ||= branch_name verify_start_branch_exists!(start_project.repository, start_branch_name) @@ -154,7 +154,7 @@ class GitOperationService def verify_start_branch_exists!(start_repository, start_branch_name) return if start_repository.empty_repo? return if start_repository.branch_exists?(start_branch_name) - + raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.path_with_namespace}" end end -- cgit v1.2.1 From e7bf621ab1fc04d17cd7edd8ca2350cb05e7c0e0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 1 Mar 2017 17:17:35 -0600 Subject: Fix spec --- spec/requests/api/v3/files_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/v3/files_spec.rb b/spec/requests/api/v3/files_spec.rb index dc5691a106f..3b61139a2cd 100644 --- a/spec/requests/api/v3/files_spec.rb +++ b/spec/requests/api/v3/files_spec.rb @@ -153,7 +153,7 @@ describe API::V3::Files, api: true 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 + 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') -- cgit v1.2.1 From f2464a13210fe77e2a01b1a5ef9b6466444da426 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 2 Mar 2017 08:59:57 -0600 Subject: Don't require start branch to exist if we're just creating a new branch --- app/services/git_operation_service.rb | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/app/services/git_operation_service.rb b/app/services/git_operation_service.rb index 82ef34a4863..ed6ea638235 100644 --- a/app/services/git_operation_service.rb +++ b/app/services/git_operation_service.rb @@ -56,14 +56,17 @@ class GitOperationService start_project: repository.project, &block) - start_branch_name ||= branch_name + start_repository = start_project.repository + start_branch_name = nil if start_repository.empty_repo? - verify_start_branch_exists!(start_project.repository, start_branch_name) + 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 update_branch_with_hooks(branch_name) do repository.with_repo_branch_commit( - start_project.repository, - start_branch_name, + start_repository, + start_branch_name || branch_name, &block) end end @@ -150,11 +153,4 @@ class GitOperationService repository.raw_repository.autocrlf = :input end end - - def verify_start_branch_exists!(start_repository, start_branch_name) - return if start_repository.empty_repo? - return if start_repository.branch_exists?(start_branch_name) - - raise ArgumentError, "Cannot find branch #{start_branch_name} in #{start_repository.path_with_namespace}" - end end -- cgit v1.2.1 From e26b4f0c1e2a35331bb9ec83fdb36829f2210ba9 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 2 Mar 2017 12:55:30 -0600 Subject: Delete hooks from project with empty repository --- spec/factories/projects.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 586efdefdb3..04de3512125 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -39,6 +39,10 @@ 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 -- cgit v1.2.1