Revert "Merge branch 'dm-fix-api-create-file-on-empty-repo' into 'master'"

This reverts commit 4b692487f54da497d3159fe34373b6de14c53d15.

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