Merge branch 'dm-fix-editing-files-on-forks-ee' into 'master'

Refactor changing files in web UI

See merge request !1268

app/assets/javascripts/blob/blob_file_dropzone.js

@@ -35,7 +35,7 @@ export default class BlobFileDropzone {
           this.removeFile(file);
         });
         this.on('sending', function (file, xhr, formData) {
-          formData.append('target_branch', form.find('input[name="target_branch"]').val());
+          formData.append('branch_name', form.find('input[name="branch_name"]').val());
           formData.append('create_merge_request', form.find('.js-create-merge-request').val());
           formData.append('commit_message', form.find('.js-commit-message').val());
         });

app/controllers/concerns/creates_commit.rb

 module CreatesCommit
   extend ActiveSupport::Concern
 
+  def set_start_branch_to_branch_name
+    branch_exists = @repository.find_branch(@branch_name)
+    @start_branch = @branch_name if branch_exists
+  end
+
   def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil)
-    set_commit_variables
+    if can?(current_user, :push_code, @project)
+      @project_to_commit_into = @project
+      @branch_name ||= @ref
+    else
+      @project_to_commit_into = current_user.fork_of(@project)
+      @branch_name ||= @project_to_commit_into.repository.next_branch('patch')
+    end
+
+    @start_branch ||= @ref || @branch_name
 
     commit_params = @commit_params.merge(
-      start_project: @mr_target_project,
-      start_branch: @mr_target_branch,
-      target_branch: @mr_source_branch
+      start_project: @project,
+      start_branch: @start_branch,
+      branch_name: @branch_name
     )
 
-    result = service.new(
-      @mr_source_project, current_user, commit_params).execute
+    result = service.new(@project_to_commit_into, current_user, commit_params).execute
 
     if result[:status] == :success
       update_flash_notice(success_notice)
@@ -72,30 +84,30 @@ module CreatesCommit
 
   def new_merge_request_path
     new_namespace_project_merge_request_path(
-      @mr_source_project.namespace,
-      @mr_source_project,
+      @project_to_commit_into.namespace,
+      @project_to_commit_into,
       merge_request: {
-        source_project_id: @mr_source_project.id,
-        target_project_id: @mr_target_project.id,
-        source_branch: @mr_source_branch,
-        target_branch: @mr_target_branch
+        source_project_id: @project_to_commit_into.id,
+        target_project_id: @project.id,
+        source_branch: @branch_name,
+        target_branch: @start_branch
       }
     )
   end
 
   def existing_merge_request_path
-    namespace_project_merge_request_path(@mr_target_project.namespace, @mr_target_project, @merge_request)
+    namespace_project_merge_request_path(@project.namespace, @project, @merge_request)
   end
 
   def merge_request_exists?
     return @merge_request if defined?(@merge_request)
 
-    @merge_request = MergeRequestsFinder.new(current_user, project_id: @mr_target_project.id).execute.opened.
-      find_by(source_branch: @mr_source_branch, target_branch: @mr_target_branch, source_project_id: @mr_source_project)
+    @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.
+      find_by(source_project_id: @project_to_commit_into, source_branch: @branch_name, target_branch: @start_branch)
   end
 
   def different_project?
-    @mr_source_project != @mr_target_project
+    @project_to_commit_into != @project
   end
 
   def create_merge_request?
@@ -103,22 +115,6 @@ module CreatesCommit
     # as the target branch in the same project,
     # we don't want to create a merge request.
     params[:create_merge_request].present? &&
-      (different_project? || @mr_target_branch != @mr_source_branch)
-  end
-
-  def set_commit_variables
-    if can?(current_user, :push_code, @project)
-      @mr_source_project = @project
-      @target_branch ||= @ref
-    else
-      @mr_source_project = current_user.fork_of(@project)
-      @target_branch ||= @mr_source_project.repository.next_branch('patch')
-    end
-
-    # Merge request to this project
-    @mr_target_project = @project
-    @mr_target_branch ||= @ref || @target_branch
-
-    @mr_source_branch = @target_branch
+      (different_project? || @start_branch != @branch_name)
   end
 end

app/controllers/projects/application_controller.rb

@@ -89,9 +89,4 @@ class Projects::ApplicationController < ApplicationController
   def builds_enabled
     return render_404 unless @project.feature_available?(:builds, current_user)
   end
-
-  def update_ref
-    branch_exists = @repository.find_branch(@target_branch)
-    @ref = @target_branch if branch_exists
-  end
 end

app/controllers/projects/blob_controller.rb

@@ -25,10 +25,10 @@ class Projects::BlobController < Projects::ApplicationController
   end
 
   def create
-    update_ref
+    set_start_branch_to_branch_name
 
     create_commit(Files::CreateService, success_notice: "The file has been successfully created.",
-                                        success_path: -> { namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @file_path)) },
+                                        success_path: -> { namespace_project_blob_path(@project.namespace, @project, File.join(@branch_name, @file_path)) },
                                         failure_view: :new,
                                         failure_path: namespace_project_new_blob_path(@project.namespace, @project, @ref))
   end
@@ -69,10 +69,10 @@ class Projects::BlobController < Projects::ApplicationController
   end
 
   def destroy
-    create_commit(Files::DestroyService, success_notice: "The file has been successfully deleted.",
-                                         success_path: -> { namespace_project_tree_path(@project.namespace, @project, @target_branch) },
-                                         failure_view: :show,
-                                         failure_path: namespace_project_blob_path(@project.namespace, @project, @id))
+    create_commit(Files::DeleteService, success_notice: "The file has been successfully deleted.",
+                                        success_path: -> { namespace_project_tree_path(@project.namespace, @project, @branch_name) },
+                                        failure_view: :show,
+                                        failure_path: namespace_project_blob_path(@project.namespace, @project, @id))
   end
 
   def diff
@@ -127,16 +127,16 @@ class Projects::BlobController < Projects::ApplicationController
 
   def after_edit_path
     from_merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.find_by(iid: params[:from_merge_request_iid])
-    if from_merge_request && @target_branch == @ref
+    if from_merge_request && @branch_name == @ref
       diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) +
         "##{hexdigest(@path)}"
     else
-      namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path))
+      namespace_project_blob_path(@project.namespace, @project, File.join(@branch_name, @path))
     end
   end
 
   def editor_variables
-    @target_branch = params[:target_branch]
+    @branch_name = params[:branch_name]
 
     @file_path =
       if action_name.to_s == 'create'

app/controllers/projects/commit_controller.rb

@@ -56,9 +56,7 @@ class Projects::CommitController < Projects::ApplicationController
 
     return render_404 if @start_branch.blank?
 
-    @target_branch = create_new_branch? ? @commit.revert_branch_name : @start_branch
-
-    @mr_target_branch = @start_branch
+    @branch_name = create_new_branch? ? @commit.revert_branch_name : @start_branch
 
     create_commit(Commits::RevertService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully reverted.",
                                           success_path: -> { successful_change_path }, failure_path: failed_change_path)
@@ -69,9 +67,7 @@ class Projects::CommitController < Projects::ApplicationController
 
     return render_404 if @start_branch.blank?
 
-    @target_branch = create_new_branch? ? @commit.cherry_pick_branch_name : @start_branch
-
-    @mr_target_branch = @start_branch
+    @branch_name = create_new_branch? ? @commit.cherry_pick_branch_name : @start_branch
 
     create_commit(Commits::CherryPickService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully cherry-picked.",
                                               success_path: -> { successful_change_path }, failure_path: failed_change_path)
@@ -84,7 +80,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def successful_change_path
-    referenced_merge_request_url || namespace_project_commits_url(@project.namespace, @project, @target_branch)
+    referenced_merge_request_url || namespace_project_commits_url(@project.namespace, @project, @branch_name)
   end
 
   def failed_change_path

app/controllers/projects/tree_controller.rb

@@ -34,16 +34,16 @@ class Projects::TreeController < Projects::ApplicationController
   def create_dir
     return render_404 unless @commit_params.values.all?
 
-    update_ref
+    set_start_branch_to_branch_name
     create_commit(Files::CreateDirService,  success_notice: "The directory has been successfully created.",
-                                            success_path: namespace_project_tree_path(@project.namespace, @project, File.join(@target_branch, @dir_name)),
+                                            success_path: namespace_project_tree_path(@project.namespace, @project, File.join(@branch_name, @dir_name)),
                                             failure_path: namespace_project_tree_path(@project.namespace, @project, @ref))
   end
 
   private
 
   def assign_dir_vars
-    @target_branch = params[:target_branch]
+    @branch_name = params[:branch_name]
 
     @dir_name = File.join(@path, params[:dir_name])
     @commit_params = {

app/helpers/projects_helper.rb

@@ -291,14 +291,14 @@ module ProjectsHelper
     end
   end
 
-  def add_special_file_path(project, file_name:, commit_message: nil, target_branch: nil, context: nil)
+  def add_special_file_path(project, file_name:, commit_message: nil, branch_name: nil, context: nil)
     namespace_project_new_blob_path(
       project.namespace,
       project,
       project.default_branch || 'master',
       file_name:      file_name,
       commit_message: commit_message || "Add #{file_name.downcase}",
-      target_branch: target_branch,
+      branch_name: branch_name,
       context: context
     )
   end

app/helpers/tree_helper.rb

@@ -35,7 +35,7 @@ module TreeHelper
   end
 
   def on_top_of_branch?(project = @project, ref = @ref)
-    project.repository.branch_names.include?(ref)
+    project.repository.branch_exists?(ref)
   end
 
   def can_edit_tree?(project = nil, ref = nil)

app/services/commits/change_service.rb

 module Commits
-  class ChangeService < ::BaseService
-    ValidationError = Class.new(StandardError)
-    ChangeError = Class.new(StandardError)
+  class ChangeService < Commits::CreateService
+    def initialize(*args)
+      super
 
-    def execute
-      @start_project = params[:start_project] || @project
-      @start_branch = params[:start_branch]
-      @target_branch = params[:target_branch]
       @commit = params[:commit]
-
-      check_push_permissions
-
-      commit
-    rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError,
-           ValidationError, ChangeError => ex
-      error(ex.message)
     end
 
     private
 
-    def commit
-      raise NotImplementedError
-    end
-
     def commit_change(action)
       raise NotImplementedError unless repository.respond_to?(action)
 
-      validate_target_branch if different_branch?
-
       repository.public_send(
         action,
         current_user,
         @commit,
-        @target_branch,
+        @branch_name,
         start_project: @start_project,
         start_branch_name: @start_branch)
-
-      success
     rescue Repository::CreateTreeError
       error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically.
-                     A #{action.to_s.dasherize} may have already been performed with this #{@commit.change_type_title(current_user)}, or a more recent commit may have updated some of its content."
+                   This #{@commit.change_type_title(current_user)} may already have been #{action.to_s.dasherize}ed, or a more recent commit may have updated some of its content."
       raise ChangeError, error_msg
     end
-
-    def check_push_permissions
-      allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
-
-      unless allowed
-        raise ValidationError.new('You are not allowed to push into this branch')
-      end
-
-      true
-    end
-
-    def validate_target_branch
-      result = ValidateNewBranchService.new(@project, current_user)
-        .execute(@target_branch)
-
-      if result[:status] == :error
-        raise ChangeError, "There was an error creating the source branch: #{result[:message]}"
-      end
-    end
-
-    def different_branch?
-      @start_branch != @target_branch || @start_project != @project
-    end
   end
 end

app/services/commits/cherry_pick_service.rb

 module Commits
   class CherryPickService < ChangeService
-    def commit
+    def create_commit!
       commit_change(:cherry_pick)
     end
   end

app/services/commits/create_service.rb

+module Commits
+  class CreateService < ::BaseService
+    ValidationError = Class.new(StandardError)
+    ChangeError = Class.new(StandardError)
+
+    def initialize(*args)
+      super
+
+      @start_project = params[:start_project] || @project
+      @start_branch = params[:start_branch]
+      @branch_name = params[:branch_name]
+    end
+
+    def execute
+      validate!
+
+      new_commit = create_commit!
+
+      success(result: new_commit)
+    rescue ValidationError, ChangeError, Gitlab::Git::Index::IndexError, Repository::CommitError, GitHooksService::PreReceiveError => ex
+      error(ex.message)
+    end
+
+    private
+
+    def create_commit!
+      raise NotImplementedError
+    end
+
+    def raise_error(message)
+      raise ValidationError, message
+    end
+
+    def different_branch?
+      @start_branch != @branch_name || @start_project != @project
+    end
+
+    def validate!
+      validate_permissions!
+      validate_repository_size!
+      validate_on_branch!
+      validate_branch_existance!
+
+      validate_new_branch_name! if different_branch?
+    end
+
+    def validate_permissions!
+      allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@branch_name)
+
+      unless allowed
+        raise_error("You are not allowed to push into this branch")
+      end
+    end
+
+    def validate_repository_size!
+      if project.above_size_limit?
+        raise_error(Gitlab::RepositorySizeError.new(project).commit_error)
+      end
+    end
+
+    def validate_on_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
+    end
+
+    def validate_branch_existance!
+      if !project.empty_repo? && different_branch? && repository.branch_exists?(@branch_name)
+        raise_error("A branch called '#{@branch_name}' already exists. Switch to that branch in order to make changes")
+      end
+    end
+
+    def validate_new_branch_name!
+      result = ValidateNewBranchService.new(project, current_user).execute(@branch_name)
+
+      if result[:status] == :error
+        raise_error("Something went wrong when we tried to create '#{@branch_name}' for you: #{result[:message]}")
+      end
+    end
+  end
+end

app/services/commits/revert_service.rb

 module Commits
   class RevertService < ChangeService
-    def commit
+    def create_commit!
       commit_change(:revert)
     end
   end

app/services/files/base_service.rb

 module Files
-  class BaseService < ::BaseService
-    ValidationError = Class.new(StandardError)
-
-    def execute
-      @start_project = params[:start_project] || @project
-      @start_branch = params[:start_branch]
-      @target_branch = params[:target_branch]
+  class BaseService < Commits::CreateService
+    def initialize(*args)
+      super
 
+      @author_email = params[:author_email]
+      @author_name = params[:author_name]
       @commit_message = params[:commit_message]
-      @file_path      = params[:file_path]
-      @previous_path  = params[:previous_path]
-      @file_content   = if params[:file_content_encoding] == 'base64'
-                          Base64.decode64(params[:file_content])
-                        else
-                          params[:file_content]
-                        end
-      @last_commit_sha = params[:last_commit_sha]
-      @author_email    = params[:author_email]
-      @author_name     = params[:author_name]
-
-      # Validate parameters
-      validate
-
-      # Create new branch if it different from start_branch
-      validate_target_branch if different_branch?
-
-      result = commit
-      if result
-        success(result: result)
-      else
-        error('Something went wrong. Your changes were not committed')
-      end
-    rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError, ValidationError => ex
-      error(ex.message)
-    end
-
-    private
-
-    def different_branch?
-      @start_branch != @target_branch || @start_project != @project
-    end
-
-    def file_has_changed?
-      return false unless @last_commit_sha && last_commit
-
-      @last_commit_sha != last_commit.sha
-    end
 
-    def raise_error(message)
-      raise ValidationError.new(message)
-    end
-
-    def validate
-      allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
-
-      if project.above_size_limit?
-        raise_error(Gitlab::RepositorySizeError.new(project).commit_error)
-      end
-
-      unless allowed
-        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"
-      end
-    end
-
-    def validate_target_branch
-      result = ValidateNewBranchService.new(project, current_user).
-        execute(@target_branch)
-
-      if result[:status] == :error
-        raise_error("Something went wrong when we tried to create #{@target_branch} for you: #{result[:message]}")
-      end
-    end
+      @file_path = params[:file_path]
+      @previous_path = params[:previous_path]
 
-    def push_rule
-      project.push_rule
+      @file_content = params[:file_content]
+      @file_content = Base64.decode64(@file_content) if params[:file_content_encoding] == 'base64'
     end
   end
 end

app/services/files/create_dir_service.rb

 module Files
   class CreateDirService < Files::BaseService
-    def commit
+    def create_commit!
       repository.create_dir(
         current_user,
         @file_path,
         message: @commit_message,
-        branch_name: @target_branch,
+        branch_name: @branch_name,
         author_email: @author_email,
         author_name: @author_name,
         start_project: @start_project,
         start_branch_name: @start_branch)
     end
-
-    def validate
-      super
-
-      unless @file_path =~ Gitlab::Regex.file_path_regex
-        raise_error(
-          'Your changes could not be committed, because the file path ' +
-          Gitlab::Regex.file_path_regex_message
-        )
-      end
-    end
   end
 end

app/services/files/create_service.rb

 module Files
   class CreateService < Files::BaseService
-    def commit
+    def create_commit!
       repository.create_file(
         current_user,
         @file_path,
         @file_content,
         message: @commit_message,
-        branch_name: @target_branch,
+        branch_name: @branch_name,
         author_email: @author_email,
         author_name: @author_name,
         start_project: @start_project,
         start_branch_name: @start_branch)
     end
-
-    def validate
-      super
-
-      if @file_content.nil?
-        raise_error("You must provide content.")
-      end
-
-      if @file_path =~ Gitlab::Regex.directory_traversal_regex
-        raise_error(
-          'Your changes could not be committed, because the file name ' +
-          Gitlab::Regex.directory_traversal_regex_message
-        )
-      end
-
-      unless @file_path =~ Gitlab::Regex.file_path_regex
-        raise_error(
-          'Your changes could not be committed, because the file name ' +
-          Gitlab::Regex.file_path_regex_message
-        )
-      end
-
-      unless project.empty_repo?
-        @file_path.slice!(0) if @file_path.start_with?('/')
-
-        blob = repository.blob_at_branch(@start_branch, @file_path)
-
-        if blob
-          raise_error('Your changes could not be committed because a file with the same name already exists')
-        end
-      end
-    end
   end
 end

app/services/files/destroy_service.rb → app/services/files/delete_service.rb

 module Files
-  class DestroyService < Files::BaseService
-    def commit
+  class DeleteService < Files::BaseService
+    def create_commit!
       repository.delete_file(
         current_user,
         @file_path,
         message: @commit_message,
-        branch_name: @target_branch,
+        branch_name: @branch_name,
         author_email: @author_email,
         author_name: @author_name,
         start_project: @start_project,

app/services/files/multi_service.rb

 module Files
   class MultiService < Files::BaseService
-    FileChangedError = Class.new(StandardError)
-
-    ACTIONS = %w[create update delete move].freeze
-
-    def commit
+    def create_commit!
       repository.multi_action(
         user: current_user,
         message: @commit_message,
-        branch_name: @target_branch,
+        branch_name: @branch_name,
         actions: params[:actions],
         author_email: @author_email,
         author_name: @author_name,
@@ -19,122 +15,17 @@ module Files
 
     private
 
-    def validate
+    def validate!
       super
 
-      params[:actions].each_with_index do |action, index|
-        if ACTIONS.include?(action[:action].to_s)
-          action[:action] = action[:action].to_sym
-        else
-          raise_error("Unknown action type `#{action[:action]}`.")
-        end
-
-        unless action[:file_path].present?
-          raise_error("You must specify a file_path.")
-        end
-
-        action[:file_path].slice!(0) if action[:file_path] && action[:file_path].start_with?('/')
-        action[:previous_path].slice!(0) if action[:previous_path] && action[:previous_path].start_with?('/')
-
-        regex_check(action[:file_path])
-        regex_check(action[:previous_path]) if action[:previous_path]
-
-        if project.empty_repo? && action[:action] != :create
-          raise_error("No files to #{action[:action]}.")
-        end
-
-        validate_file_exists(action)
-
-        case action[:action]
-        when :create
-          validate_create(action)
-        when :update
-          validate_update(action)
-        when :delete
-          validate_delete(action)
-        when :move
-          validate_move(action, index)
-        end
-      end
-    end
-
-    def validate_file_exists(action)
-      return if action[:action] == :create
-
-      file_path = action[:file_path]
-      file_path = action[:previous_path] if action[:action] == :move
-
-      blob = repository.blob_at_branch(params[:branch], file_path)
-
-      unless blob
-        raise_error("File to be #{action[:action]}d `#{file_path}` does not exist.")
+      params[:actions].each do |action|
+        validate_action!(action)
       end
     end
 
-    def last_commit
-      Gitlab::Git::Commit.last_for_path(repository, @start_branch, @file_path)
-    end
-
-    def regex_check(file)
-      if file =~ Gitlab::Regex.directory_traversal_regex
-        raise_error(
-          'Your changes could not be committed, because the file name, `' +
-          file +
-          '` ' +
-          Gitlab::Regex.directory_traversal_regex_message
-        )
-      end
-
-      unless file =~ Gitlab::Regex.file_path_regex
-        raise_error(
-          'Your changes could not be committed, because the file name, `' +
-          file +
-          '` ' +
-          Gitlab::Regex.file_path_regex_message
-        )
-      end
-    end
-
-    def validate_create(action)
-      return if project.empty_repo?
-
-      if repository.blob_at_branch(params[:branch], action[:file_path])
-        raise_error("Your changes could not be committed because a file with the name `#{action[:file_path]}` already exists.")
-      end
-
-      if action[:content].nil?
-        raise_error("You must provide content.")
-      end
-    end
-
-    def validate_update(action)
-      if action[:content].nil?
-        raise_error("You must provide content.")
-      end
-
-      if file_has_changed?
-        raise FileChangedError.new("You are attempting to update a file `#{action[:file_path]}` that has changed since you started editing it.")
-      end
-    end
-
-    def validate_delete(action)
-    end
-
-    def validate_move(action, index)
-      if action[:previous_path].nil?
-        raise_error("You must supply the original file path when moving file `#{action[:file_path]}`.")
-      end
-
-      blob = repository.blob_at_branch(params[:branch], action[:file_path])
-
-      if blob
-        raise_error("Move destination `#{action[:file_path]}` already exists.")
-      end
-
-      if action[:content].nil?
-        blob = repository.blob_at_branch(params[:branch], action[:previous_path])
-        blob.load_all_data!(repository) if blob.truncated?
-        params[:actions][index][:content] = blob.data
+    def validate_action!(action)
+      unless Gitlab::Git::Index::ACTIONS.include?(action[:action].to_s)
+        raise_error("Unknown action '#{action[:action]}'")
       end
     end
   end

app/services/files/update_service.rb

@@ -2,10 +2,16 @@ module Files
   class UpdateService < Files::BaseService
     FileChangedError = Class.new(StandardError)
 
-    def commit
+    def initialize(*args)
+      super
+
+      @last_commit_sha = params[:last_commit_sha]
+    end
+
+    def create_commit!
       repository.update_file(current_user, @file_path, @file_content,
                              message: @commit_message,
-                             branch_name: @target_branch,
+                             branch_name: @branch_name,
                              previous_path: @previous_path,
                              author_email: @author_email,
                              author_name: @author_name,
@@ -15,21 +21,23 @@ module Files
 
     private
 
-    def validate
-      super
-
-      if @file_content.nil?
-        raise_error("You must provide content.")
-      end
+    def file_has_changed?
+      return false unless @last_commit_sha && last_commit
 
-      if file_has_changed?
-        raise FileChangedError.new("You are attempting to update a file that has changed since you started editing it.")
-      end
+      @last_commit_sha != last_commit.sha
     end
 
     def last_commit
       @last_commit ||= Gitlab::Git::Commit.
         last_for_path(@start_project.repository, @start_branch, @file_path)
     end
+
+    def validate!
+      super
+
+      if file_has_changed?
+        raise FileChangedError, "You are attempting to update a file that has changed since you started editing it."
+      end
+    end
   end
 end

app/services/validate_new_branch_service.rb

@@ -8,10 +8,7 @@ class ValidateNewBranchService < BaseService
       return error('Branch name is invalid')
     end
 
-    repository = project.repository
-    existing_branch = repository.find_branch(branch_name)
-
-    if existing_branch
+    if project.repository.branch_exists?(branch_name)
       return error('Branch already exists')
     end
 

app/views/projects/blob/edit.html.haml

@@ -9,7 +9,7 @@
   - if @conflict
     .alert.alert-danger
       Someone edited the file the same time you did. Please check out
-      = link_to "the file", namespace_project_blob_path(@project.namespace, @project, tree_join(@target_branch, @file_path)), target: "_blank", rel: 'noopener noreferrer'
+      = link_to "the file", namespace_project_blob_path(@project.namespace, @project, tree_join(@branch_name, @file_path)), target: "_blank", rel: 'noopener noreferrer'
       and make sure your changes will not unintentionally remove theirs.
   .editor-title-row
     %h3.page-title.blob-edit-page-title

app/views/projects/show.html.haml

@@ -73,7 +73,7 @@
             = link_to 'Set up Koding', add_koding_stack_path(@project)
         - if @repository.gitlab_ci_yml.blank? && @project.deployment_service.present?
           %li.missing
-            = link_to add_special_file_path(@project, file_name: '.gitlab-ci.yml', commit_message: 'Set up auto deploy', target_branch: 'auto-deploy', context: 'autodeploy') do
+            = link_to add_special_file_path(@project, file_name: '.gitlab-ci.yml', commit_message: 'Set up auto deploy', branch_name: 'auto-deploy', context: 'autodeploy') do
               Set up auto deploy
 
   - if @repository.commit

app/views/shared/_branch_switcher.html.haml

-- dropdown_toggle_text = @target_branch || tree_edit_branch
-= hidden_field_tag 'target_branch', dropdown_toggle_text
+- dropdown_toggle_text = @branch_name || tree_edit_branch
+= hidden_field_tag 'branch_name', dropdown_toggle_text
 
 .dropdown
-  = dropdown_toggle dropdown_toggle_text, { toggle: 'dropdown', selected: dropdown_toggle_text, field_name: 'target_branch', form_id: '.js-edit-blob-form', refs_url: namespace_project_branches_path(@project.namespace, @project) }, { toggle_class: 'js-project-branches-dropdown js-target-branch' }
+  = dropdown_toggle dropdown_toggle_text, { toggle: 'dropdown', selected: dropdown_toggle_text, field_name: 'branch_name', form_id: '.js-edit-blob-form', refs_url: namespace_project_branches_path(@project.namespace, @project) }, { toggle_class: 'js-project-branches-dropdown js-target-branch' }
   .dropdown-menu.dropdown-menu-selectable.dropdown-menu-paging.dropdown-menu-branches
     = render partial: 'shared/projects/blob/branch_page_default'
     = render partial: 'shared/projects/blob/branch_page_create'

app/views/shared/_new_commit_form.html.haml

 = render 'shared/commit_message_container', placeholder: placeholder
 
 - if @project.empty_repo?
-  = hidden_field_tag 'target_branch', @ref
+  = hidden_field_tag 'branch_name', @ref
 - else
   - if can?(current_user, :push_code, @project)
     .form-group.branch
-      = label_tag 'target_branch', 'Target branch', class: 'control-label'
+      = label_tag 'branch_name', 'Target branch', class: 'control-label'
       .col-sm-10
         = render 'shared/branch_switcher'
 
@@ -16,7 +16,7 @@
               = check_box_tag 'create_merge_request', 1, true, class: 'js-create-merge-request', id: "create_merge_request-#{nonce}"
               Start a <strong>new merge request</strong> with these changes
   - else
-    = hidden_field_tag 'target_branch', @target_branch || tree_edit_branch
+    = hidden_field_tag 'branch_name', @branch_name || tree_edit_branch
     = hidden_field_tag 'create_merge_request', 1
 
   = hidden_field_tag 'original_branch', @ref, class: 'js-original-branch'

features/project/merge_requests/revert.feature

@@ -25,7 +25,5 @@ Feature: Revert Merge Requests
   @javascript
   Scenario: I revert a merge request in a new merge request
     Given I click on the revert button
-    And I am on the Merge Request detail page
-    And I click on the revert button
     And I revert the changes in a new merge request
     Then I should see the new merge request notice

features/project/source/browse_files.feature

@@ -135,7 +135,7 @@ Feature: Project Source Browse Files
     And I fill the commit message
     And I click on "Commit changes"
     Then I am on the new file page
-    And I see a commit error message
+    And I see "Path can contain only..."
 
   @javascript
   Scenario: I can create file with a directory name

features/steps/project/source/browse_files.rb

@@ -284,7 +284,11 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
   end
 
   step 'I see "Unable to create directory"' do
-    expect(page).to have_content('Directory already exists')
+    expect(page).to have_content('A directory with this name already exists')
+  end
+
+  step 'I see "Path can contain only..."' do
+    expect(page).to have_content('Path can contain only')
   end
 
   step 'I see a commit error message' do

lib/api/commits.rb

@@ -62,7 +62,7 @@ module API
       post ":id/repository/commits" do
         authorize! :push_code, user_project
 
-        attrs = declared_params.merge(start_branch: declared_params[:branch], target_branch: declared_params[:branch])
+        attrs = declared_params.merge(start_branch: declared_params[:branch], branch_name: declared_params[:branch])
 
         result = ::Files::MultiService.new(user_project, current_user, attrs).execute
 
@@ -140,7 +140,7 @@ module API
         commit_params = {
           commit: commit,
           start_branch: params[:branch],
-          target_branch: params[:branch]
+          branch_name: params[:branch]
         }
 
         result = ::Commits::CherryPickService.new(user_project, current_user, commit_params).execute

lib/api/files.rb

@@ -5,7 +5,7 @@ module API
         {
           file_path: attrs[:file_path],
           start_branch: attrs[:branch],
-          target_branch: attrs[:branch],
+          branch_name: attrs[:branch],
           commit_message: attrs[:commit_message],
           file_content: attrs[:content],
           file_content_encoding: attrs[:encoding],
@@ -130,7 +130,7 @@ module API
         authorize! :push_code, user_project
 
         file_params = declared_params(include_missing: false)
-        result = ::Files::DestroyService.new(user_project, current_user, commit_params(file_params)).execute
+        result = ::Files::DeleteService.new(user_project, current_user, commit_params(file_params)).execute
 
         if result[:status] != :success
           render_api_error!(result[:message], 400)

lib/api/v3/commits.rb

@@ -53,7 +53,7 @@ module API
 
           attrs = declared_params.dup
           branch = attrs.delete(:branch_name)
-          attrs.merge!(branch: branch, start_branch: branch, target_branch: branch)
+          attrs.merge!(start_branch: branch, branch_name: branch)
 
           result = ::Files::MultiService.new(user_project, current_user, attrs).execute
 
@@ -131,7 +131,7 @@ module API
           commit_params = {
             commit: commit,
             start_branch: params[:branch],
-            target_branch: params[:branch]
+            branch_name: params[:branch]
           }
 
           result = ::Commits::CherryPickService.new(user_project, current_user, commit_params).execute

lib/api/v3/files.rb

@@ -6,7 +6,7 @@ module API
           {
             file_path: attrs[:file_path],
             start_branch: attrs[:branch],
-            target_branch: attrs[:branch],
+            branch_name: attrs[:branch],
             commit_message: attrs[:commit_message],
             file_content: attrs[:content],
             file_content_encoding: attrs[:encoding],
@@ -123,7 +123,7 @@ module API
           file_params = declared_params(include_missing: false)
           file_params[:branch] = file_params.delete(:branch_name)
 
-          result = ::Files::DestroyService.new(user_project, current_user, commit_params(file_params)).execute
+          result = ::Files::DeleteService.new(user_project, current_user, commit_params(file_params)).execute
 
           if result[:status] == :success
             status(200)

lib/gitlab/git/index.rb

 module Gitlab
   module Git
     class Index
+      IndexError = Class.new(StandardError)
+
       DEFAULT_MODE = 0o100644
 
+      ACTIONS = %w(create create_dir update move delete).freeze
+
       attr_reader :repository, :raw_index
 
       def initialize(repository)
@@ -23,9 +27,8 @@ module Gitlab
       def create(options)
         options = normalize_options(options)
 
-        file_entry = get(options[:file_path])
-        if file_entry
-          raise Gitlab::Git::Repository::InvalidBlobName.new("Filename already exists")
+        if get(options[:file_path])
+          raise IndexError, "A file with this name already exists"
         end
 
         add_blob(options)
@@ -34,13 +37,12 @@ module Gitlab
       def create_dir(options)
         options = normalize_options(options)
 
-        file_entry = get(options[:file_path])
-        if file_entry
-          raise Gitlab::Git::Repository::InvalidBlobName.new("Directory already exists as a file")
+        if get(options[:file_path])
+          raise IndexError, "A file with this name already exists"
         end
 
         if dir_exists?(options[:file_path])
-          raise Gitlab::Git::Repository::InvalidBlobName.new("Directory already exists")
+          raise IndexError, "A directory with this name already exists"
         end
 
         options = options.dup
@@ -55,7 +57,7 @@ module Gitlab
 
         file_entry = get(options[:file_path])
         unless file_entry
-          raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist")
+          raise IndexError, "A file with this name doesn't exist"
         end
 
         add_blob(options, mode: file_entry[:mode])
@@ -66,7 +68,11 @@ module Gitlab
 
         file_entry = get(options[:previous_path])
         unless file_entry
-          raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist")
+          raise IndexError, "A file with this name doesn't exist"
+        end
+
+        if get(options[:file_path])
+          raise IndexError, "A file with this name already exists"
         end
 
         raw_index.remove(options[:previous_path])
@@ -77,9 +83,8 @@ module Gitlab
       def delete(options)
         options = normalize_options(options)
 
-        file_entry = get(options[:file_path])
-        unless file_entry
-          raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist")
+        unless get(options[:file_path])
+          raise IndexError, "A file with this name doesn't exist"
         end
 
         raw_index.remove(options[:file_path])
@@ -95,10 +100,20 @@ module Gitlab
       end
 
       def normalize_path(path)
+        unless path
+          raise IndexError, "You must provide a file path"
+        end
+
         pathname = Gitlab::Git::PathHelper.normalize_path(path.dup)
 
-        if pathname.each_filename.include?('..')
-          raise Gitlab::Git::Repository::InvalidBlobName.new('Invalid path')
+        pathname.each_filename do |segment|
+          if segment == '..'
+            raise IndexError, 'Path cannot include directory traversal'
+          end
+
+          unless segment =~ Gitlab::Regex.file_name_regex
+            raise IndexError, "Path #{Gitlab::Regex.file_name_regex_message}"
+          end
         end
 
         pathname.to_s
@@ -106,6 +121,10 @@ module Gitlab
 
       def add_blob(options, mode: nil)
         content = options[:content]
+        unless content
+          raise IndexError, "You must provide content"
+        end
+
         content = Base64.decode64(content) if options[:encoding] == 'base64'
 
         detect = CharlockHolmes::EncodingDetector.new.detect(content)
@@ -119,7 +138,7 @@ module Gitlab
 
         raw_index.add(path: options[:file_path], oid: oid, mode: mode || DEFAULT_MODE)
       rescue Rugged::IndexError => e
-        raise Gitlab::Git::Repository::InvalidBlobName.new(e.message)
+        raise IndexError, e.message
       end
     end
   end

lib/gitlab/regex.rb

@@ -73,22 +73,6 @@ module Gitlab
       "can contain only letters, digits, '_', '-', '@', '+' and '.'."
     end
 
-    def file_path_regex
-      @file_path_regex ||= /\A[[[:alnum:]]_\-\.\/\@]*\z/.freeze
-    end
-
-    def file_path_regex_message
-      "can contain only letters, digits, '_', '-', '@' and '.'. Separate directories with a '/'."
-    end
-
-    def directory_traversal_regex
-      @directory_traversal_regex ||= /\.{2}/.freeze
-    end
-
-    def directory_traversal_regex_message
-      "cannot include directory traversal."
-    end
-
     def archive_formats_regex
       #                           |zip|tar|    tar.gz    |         tar.bz2         |
       @archive_formats_regex ||= /(zip|tar|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)/.freeze

spec/controllers/projects/blob_controller_spec.rb

@@ -106,7 +106,7 @@ describe Projects::BlobController do
         namespace_id: project.namespace,
         project_id: project,
         id: 'master/CHANGELOG',
-        target_branch: 'master',
+        branch_name: 'master',
         content: 'Added changes',
         commit_message: 'Update CHANGELOG'
       }
@@ -178,7 +178,7 @@ describe Projects::BlobController do
 
       context 'when editing on the original repository' do
         it "redirects to forked project new merge request" do
-          default_params[:target_branch] = "fork-test-1"
+          default_params[:branch_name] = "fork-test-1"
           default_params[:create_merge_request] = 1
 
           put :update, default_params

spec/controllers/projects/tree_controller_spec.rb

@@ -97,29 +97,29 @@ describe Projects::TreeController do
            project_id: project,
            id: 'master',
            dir_name: path,
-           target_branch: target_branch,
+           branch_name: branch_name,
            commit_message: 'Test commit message')
     end
 
     context 'successful creation' do
       let(:path) { 'files/new_dir'}
-      let(:target_branch) { 'master-test'}
+      let(:branch_name) { 'master-test'}
 
       it 'redirects to the new directory' do
         expect(subject).
-            to redirect_to("/#{project.path_with_namespace}/tree/#{target_branch}/#{path}")
+            to redirect_to("/#{project.path_with_namespace}/tree/#{branch_name}/#{path}")
         expect(flash[:notice]).to eq('The directory has been successfully created.')
       end
     end
 
     context 'unsuccessful creation' do
       let(:path) { 'README.md' }
-      let(:target_branch) { 'master'}
+      let(:branch_name) { 'master'}
 
       it 'does not allow overwriting of existing files' do
         expect(subject).
             to redirect_to("/#{project.path_with_namespace}/tree/master")
-        expect(flash[:alert]).to eq('Directory already exists as a file')
+        expect(flash[:alert]).to eq('A file with this name already exists')
       end
     end
   end

spec/features/projects/blobs/user_create_spec.rb

@@ -77,7 +77,7 @@ feature 'New blob creation', feature: true, js: true do
         project,
         user,
         start_branch: 'master',
-        target_branch: 'master',
+        branch_name: 'master',
         commit_message: 'Create file',
         file_path: 'feature.rb',
         file_content: content
@@ -87,7 +87,7 @@ feature 'New blob creation', feature: true, js: true do
     end
 
     scenario 'shows error message' do
-      expect(page).to have_content('Your changes could not be committed because a file with the same name already exists')
+      expect(page).to have_content('A file with this name already exists')
       expect(page).to have_content('New file')
       expect(page).to have_content('NextFeature')
     end

spec/features/projects/files/creating_a_file_spec.rb

@@ -29,16 +29,16 @@ feature 'User wants to create a file', feature: true do
 
   scenario 'directory name contains Chinese characters' do
     submit_new_file(file_name: '中文/测试.md')
-    expect(page).to have_content 'The file has been successfully created.'
+    expect(page).to have_content 'The file has been successfully created'
   end
 
   scenario 'file name contains invalid characters' do
     submit_new_file(file_name: '\\')
-    expect(page).to have_content 'Your changes could not be committed, because the file name can contain only'
+    expect(page).to have_content 'Path can contain only'
   end
 
   scenario 'file name contains directory traversal' do
     submit_new_file(file_name: '../README.md')
-    expect(page).to have_content 'Your changes could not be committed, because the file name cannot include directory traversal.'
+    expect(page).to have_content 'Path cannot include directory traversal'
   end
 end

spec/features/projects/files/editing_a_file_spec.rb

@@ -8,7 +8,7 @@ feature 'User wants to edit a file', feature: true do
   let(:commit_params) do
     {
       start_branch: project.default_branch,
-      target_branch: project.default_branch,
+      branch_name: project.default_branch,
       commit_message: "Committing First Update",
       file_path: ".gitignore",
       file_content: "First Update",

spec/features/projects/view_on_env_spec.rb

@@ -25,7 +25,7 @@ describe 'View on environment', js: true do
         project,
         user,
         start_branch: branch_name,
-        target_branch: branch_name,
+        branch_name: branch_name,
         commit_message: "Add .gitlab/route-map.yml",
         file_path: '.gitlab/route-map.yml',
         file_content: route_map
@@ -36,7 +36,7 @@ describe 'View on environment', js: true do
         project,
         user,
         start_branch: branch_name,
-        target_branch: branch_name,
+        branch_name: branch_name,
         commit_message: "Update feature",
         file_path: file_path,
         file_content: "# Noop"

spec/lib/gitlab/checks/change_access_spec.rb

@@ -262,7 +262,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
               [
                 'aws/credentials', '.ssh/personal_rsa', 'config/server_rsa', '.ssh/id_rsa', '.ssh/id_dsa',
                 '.ssh/personal_dsa', 'config/server_ed25519', 'any/id_ed25519', '.ssh/personal_ecdsa', 'config/server_ecdsa',
-                'any_place/id_ecdsa', 'some_pLace/file.key', 'other_PlAcE/other_file.pem', 'bye_bug.history, pg_sql_history'
+                'any_place/id_ecdsa', 'some_pLace/file.key', 'other_PlAcE/other_file.pem', 'bye_bug.history', 'pg_sql_history'
               ]
 
             black_listed.each do |file_path|

spec/lib/gitlab/diff/position_tracer_spec.rb

@@ -100,7 +100,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
       project,
       current_user,
       start_branch: branch_name,
-      target_branch: branch_name,
+      branch_name: branch_name,
       commit_message: "Create file",
       file_path: file_name,
       file_content: content
@@ -113,7 +113,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
       project,
       current_user,
       start_branch: branch_name,
-      target_branch: branch_name,
+      branch_name: branch_name,
       commit_message: "Update file",
       file_path: file_name,
       file_content: content
@@ -122,11 +122,11 @@ describe Gitlab::Diff::PositionTracer, lib: true do
   end
 
   def delete_file(branch_name, file_name)
-    Files::DestroyService.new(
+    Files::DeleteService.new(
       project,
       current_user,
       start_branch: branch_name,
-      target_branch: branch_name,
+      branch_name: branch_name,
       commit_message: "Delete file",
       file_path: file_name
     ).execute

spec/lib/gitlab/git/index_spec.rb

@@ -33,7 +33,7 @@ describe Gitlab::Git::Index, seed_helper: true do
       end
 
       it 'raises an error' do
-        expect { index.create(options) }.to raise_error('Filename already exists')
+        expect { index.create(options) }.to raise_error('A file with this name already exists')
       end
     end
 
@@ -89,7 +89,7 @@ describe Gitlab::Git::Index, seed_helper: true do
       end
 
       it 'raises an error' do
-        expect { index.create_dir(options) }.to raise_error('Directory already exists as a file')
+        expect { index.create_dir(options) }.to raise_error('A file with this name already exists')
       end
     end
 
@@ -99,7 +99,7 @@ describe Gitlab::Git::Index, seed_helper: true do
       end
 
       it 'raises an error' do
-        expect { index.create_dir(options) }.to raise_error('Directory already exists')
+        expect { index.create_dir(options) }.to raise_error('A directory with this name already exists')
       end
     end
   end
@@ -118,7 +118,7 @@ describe Gitlab::Git::Index, seed_helper: true do
       end
 
       it 'raises an error' do
-        expect { index.update(options) }.to raise_error("File doesn't exist")
+        expect { index.update(options) }.to raise_error("A file with this name doesn't exist")
       end
     end
 
@@ -156,7 +156,15 @@ describe Gitlab::Git::Index, seed_helper: true do
       it 'raises an error' do
         options[:previous_path] = 'documents/story.txt'
 
-        expect { index.move(options) }.to raise_error("File doesn't exist")
+        expect { index.move(options) }.to raise_error("A file with this name doesn't exist")
+      end
+    end
+
+    context 'when a file at the new path already exists' do
+      it 'raises an error' do
+        options[:file_path] = 'CHANGELOG'
+
+        expect { index.move(options) }.to raise_error("A file with this name already exists")
       end
     end
 
@@ -203,7 +211,7 @@ describe Gitlab::Git::Index, seed_helper: true do
       end
 
       it 'raises an error' do
-        expect { index.delete(options) }.to raise_error("File doesn't exist")
+        expect { index.delete(options) }.to raise_error("A file with this name doesn't exist")
       end
     end
 

spec/lib/gitlab/git_access_spec.rb

@@ -223,7 +223,7 @@ describe Gitlab::GitAccess, lib: true do
         target_branch = project.repository.lookup('feature')
         source_branch = project.repository.create_file(
           user,
-          'John Doe',
+          'filename',
           'This is the file content',
           message: 'This is a good commit message',
           branch_name: unprotected_branch)

spec/lib/gitlab/regex_spec.rb

@@ -32,12 +32,6 @@ describe Gitlab::Regex, lib: true do
     it { is_expected.to match('foo@bar') }
   end
 
-  describe '.file_path_regex' do
-    subject { described_class.file_path_regex }
-
-    it { is_expected.to match('foo@/bar') }
-  end
-
   describe '.environment_slug_regex' do
     subject { described_class.environment_slug_regex }
 

spec/requests/api/commits_spec.rb

@@ -599,8 +599,7 @@ describe API::Commits, api: true  do
         post api("/projects/#{project.id}/repository/commits/#{master_pickable_commit.id}/cherry_pick", user), branch: 'markdown'
 
         expect(response).to have_http_status(400)
-        expect(json_response['message']).to eq('Sorry, we cannot cherry-pick this commit automatically.
-                     A cherry-pick may have already been performed with this commit, or a more recent commit may have updated some of its content.')
+        expect(json_response['message']).to include('Sorry, we cannot cherry-pick this commit automatically.')
       end
 
       it 'returns 400 if you are not allowed to push to the target branch' do

spec/requests/api/files_spec.rb

@@ -205,7 +205,7 @@ describe API::Files, api: true  do
 
     it "returns a 400 if editor fails to create file" do
       allow_any_instance_of(Repository).to receive(:create_file).
-        and_return(false)
+        and_raise(Repository::CommitError, 'Cannot create file')
 
       post api(route("any%2Etxt"), user), valid_params
 
@@ -299,8 +299,8 @@ describe API::Files, api: true  do
       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)
+    it "returns a 400 if fails to delete file" do
+      allow_any_instance_of(Repository).to receive(:delete_file).and_raise(Repository::CommitError, 'Cannot delete file')
 
       delete api(route(file_path), user), valid_params
 

spec/requests/api/v3/commits_spec.rb

@@ -485,8 +485,7 @@ describe API::V3::Commits, api: true do
         post v3_api("/projects/#{project.id}/repository/commits/#{master_pickable_commit.id}/cherry_pick", user), branch: 'markdown'
 
         expect(response).to have_http_status(400)
-        expect(json_response['message']).to eq('Sorry, we cannot cherry-pick this commit automatically.
-                     A cherry-pick may have already been performed with this commit, or a more recent commit may have updated some of its content.')
+        expect(json_response['message']).to include('Sorry, we cannot cherry-pick this commit automatically.')
       end
 
       it 'returns 400 if you are not allowed to push to the target branch' do

spec/requests/api/v3/files_spec.rb

@@ -129,7 +129,7 @@ describe API::V3::Files, api: true  do
 
     it "returns a 400 if editor fails to create file" do
       allow_any_instance_of(Repository).to receive(:create_file).
-        and_return(false)
+        and_raise(Repository::CommitError, 'Cannot create file')
 
       post v3_api("/projects/#{project.id}/repository/files", user), valid_params
 
@@ -229,8 +229,8 @@ describe API::V3::Files, api: true  do
       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)
+    it "returns a 400 if fails to delete file" do
+      allow_any_instance_of(Repository).to receive(:delete_file).and_raise(Repository::CommitError, 'Cannot delete file')
 
       delete v3_api("/projects/#{project.id}/repository/files", user), valid_params
 

spec/services/files/update_service_spec.rb

@@ -7,7 +7,7 @@ describe Files::UpdateService do
   let(:user) { create(:user) }
   let(:file_path) { 'files/ruby/popen.rb' }
   let(:new_contents) { 'New Content' }
-  let(:target_branch) { project.default_branch }
+  let(:branch_name) { project.default_branch }
   let(:last_commit_sha) { nil }
 
   let(:commit_params) do
@@ -19,7 +19,7 @@ describe Files::UpdateService do
       last_commit_sha: last_commit_sha,
       start_project: project,
       start_branch: project.default_branch,
-      target_branch: target_branch
+      branch_name: branch_name
     }
   end
 
@@ -73,7 +73,7 @@ describe Files::UpdateService do
     end
 
     context 'when target branch is different than source branch' do
-      let(:target_branch) { "#{project.default_branch}-new" }
+      let(:branch_name) { "#{project.default_branch}-new" }
 
       it 'fires hooks only once' do
         expect(GitHooksService).to receive(:new).once.and_call_original