Merge branch 'fix-git-hooks-when-creating-file-ee' into 'master'

Git operations cleanup

See merge request !1151

app/controllers/concerns/creates_commit.rb

@@ -4,13 +4,15 @@ 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?
     commit_params = @commit_params.merge(
-      source_project: @project,
-      source_branch: @ref,
-      target_branch: @target_branch
+      start_project: @mr_target_project,
+      start_branch: start_branch,
+      target_branch: @mr_source_branch
     )
 
-    result = service.new(@tree_edit_project, current_user, commit_params).execute
+    result = service.new(
+      @mr_source_project, current_user, commit_params).execute
 
     if result[:status] == :success
       update_flash_notice(success_notice)
@@ -89,20 +91,18 @@ module CreatesCommit
     @mr_source_project != @mr_target_project
   end
 
-  def different_branch?
-    @mr_source_branch != @mr_target_branch || different_project?
-  end
-
   def create_merge_request?
-    params[:create_merge_request].present? && different_branch?
+    # XXX: Even if the field is set, if we're checking the same branch
+    # as the target branch in the same project,
+    # we don't want to create a merge request.
+    params[:create_merge_request].present? &&
+      (different_project? || @ref != @target_branch)
   end
 
+  # TODO: We should really clean this up
   def set_commit_variables
-    @mr_source_branch ||= @target_branch
-
     if can?(current_user, :push_code, @project)
       # Edit file in this project
-      @tree_edit_project = @project
       @mr_source_project = @project
 
       if @project.forked?
@@ -112,15 +112,34 @@ module CreatesCommit
       else
         # Merge request to this project
         @mr_target_project = @project
-        @mr_target_branch ||= @ref
+        @mr_target_branch = @ref || @target_branch
       end
     else
-      # Edit file in fork
-      @tree_edit_project = current_user.fork_of(@project)
       # Merge request from fork to this project
-      @mr_source_project = @tree_edit_project
+      @mr_source_project = current_user.fork_of(@project)
       @mr_target_project = @project
-      @mr_target_branch ||= @ref
+      @mr_target_branch = @ref || @target_branch
     end
+
+    @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,
+    # 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
   end
 end

app/controllers/projects/commit_controller.rb

@@ -39,7 +39,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def revert
-    assign_change_commit_vars(@commit.revert_branch_name)
+    assign_change_commit_vars
 
     return render_404 if @target_branch.blank?
 
@@ -48,7 +48,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def cherry_pick
-    assign_change_commit_vars(@commit.cherry_pick_branch_name)
+    assign_change_commit_vars
 
     return render_404 if @target_branch.blank?
 
@@ -105,11 +105,9 @@ class Projects::CommitController < Projects::ApplicationController
     }
   end
 
-  def assign_change_commit_vars(mr_source_branch)
+  def assign_change_commit_vars
     @commit = project.commit(params[:id])
     @target_branch = params[:target_branch]
-    @mr_source_branch = mr_source_branch
-    @mr_target_branch = @target_branch
     @commit_params = {
       commit: @commit,
       create_merge_request: params[:create_merge_request].present? || different_project?

app/controllers/projects/compare_controller.rb

@@ -46,7 +46,8 @@ class Projects::CompareController < Projects::ApplicationController
   end
 
   def define_diff_vars
-    @compare = CompareService.new.execute(@project, @head_ref, @project, @start_ref)
+    @compare = CompareService.new(@project, @head_ref)
+      .execute(@project, @start_ref)
 
     if @compare
       @commits = @compare.commits

app/models/merge_request_diff.rb

@@ -169,7 +169,8 @@ class MergeRequestDiff < ActiveRecord::Base
     # When compare merge request versions we want diff A..B instead of A...B
     # so we handle cases when user does squash and rebase of the commits between versions.
     # For this reason we set straight to true by default.
-    CompareService.new.execute(project, head_commit_sha, project, sha, straight: straight)
+    CompareService.new(project, head_commit_sha)
+      .execute(project, sha, straight: straight)
   end
 
   def commits_count

app/services/commits/change_service.rb

@@ -4,7 +4,8 @@ module Commits
     class ChangeError < StandardError; end
 
     def execute
-      @source_project = params[:source_project] || @project
+      @start_project = params[:start_project] || @project
+      @start_branch = params[:start_branch]
       @target_branch = params[:target_branch]
       @commit = params[:commit]
       @create_merge_request = params[:create_merge_request].present?
@@ -25,13 +26,28 @@ module Commits
     def commit_change(action)
       raise NotImplementedError unless repository.respond_to?(action)
 
-      into = @create_merge_request ? @commit.public_send("#{action}_branch_name") : @target_branch
-      tree_id = repository.public_send("check_#{action}_content", @commit, @target_branch)
+      if @create_merge_request
+        into = @commit.public_send("#{action}_branch_name")
+        tree_branch = @start_branch
+      else
+        into = tree_branch = @target_branch
+      end
+
+      tree_id = repository.public_send(
+        "check_#{action}_content", @commit, tree_branch)
 
       if tree_id
-        create_target_branch(into) if @create_merge_request
+        validate_target_branch(into) if @create_merge_request
+
+        repository.public_send(
+          action,
+          current_user,
+          @commit,
+          into,
+          tree_id,
+          start_project: @start_project,
+          start_branch_name: @start_branch)
 
-        repository.public_send(action, current_user, @commit, into, tree_id)
         success
       else
         error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically.
@@ -50,12 +66,12 @@ module Commits
       true
     end
 
-    def create_target_branch(new_branch)
+    def validate_target_branch(new_branch)
       # Temporary branch exists and contains the change commit
-      return success if repository.find_branch(new_branch)
+      return if repository.find_branch(new_branch)
 
-      result = CreateBranchService.new(@project, current_user)
-                                  .execute(new_branch, @target_branch, source_project: @source_project)
+      result = ValidateNewBranchService.new(@project, current_user)
+        .execute(new_branch)
 
       if result[:status] == :error
         raise ChangeError, "There was an error creating the source branch: #{result[:message]}"

app/services/compare_service.rb

@@ -3,23 +3,27 @@ require 'securerandom'
 # Compare 2 branches for one repo or between repositories
 # and return Gitlab::Git::Compare object that responds to commits and diffs
 class CompareService
-  def execute(source_project, source_branch, target_project, target_branch, straight: false)
-    source_commit = source_project.commit(source_branch)
-    return unless source_commit
+  attr_reader :start_project, :start_branch_name
 
-    source_sha = source_commit.sha
+  def initialize(new_start_project, new_start_branch_name)
+    @start_project = new_start_project
+    @start_branch_name = new_start_branch_name
+  end
 
+  def execute(target_project, target_branch, straight: false)
     # If compare with other project we need to fetch ref first
-    unless target_project == source_project
-      random_string = SecureRandom.hex
+    target_project.repository.with_repo_branch_commit(
+      start_project.repository,
+      start_branch_name) do |commit|
+      break unless commit
 
-      target_project.repository.fetch_ref(
-        source_project.repository.path_to_repo,
-        "refs/heads/#{source_branch}",
-        "refs/tmp/#{random_string}/head"
-      )
+      compare(commit.sha, target_project, target_branch, straight)
     end
+  end
+
+  private
 
+  def compare(source_sha, target_project, target_branch, straight)
     raw_compare = Gitlab::Git::Compare.new(
       target_project.repository.raw_repository,
       target_branch,

app/services/create_branch_service.rb

 class CreateBranchService < BaseService
-  def execute(branch_name, ref, source_project: @project)
-    valid_branch = Gitlab::GitRefValidator.validate(branch_name)
+  def execute(branch_name, ref)
+    result = ValidateNewBranchService.new(project, current_user)
+      .execute(branch_name)
 
-    unless valid_branch
-      return error('Branch name is invalid')
-    end
-
-    repository = project.repository
-    existing_branch = repository.find_branch(branch_name)
-
-    if existing_branch
-      return error('Branch already exists')
-    end
-
-    new_branch = if source_project != @project
-                   repository.fetch_ref(
-                     source_project.repository.path_to_repo,
-                     "refs/heads/#{ref}",
-                     "refs/heads/#{branch_name}"
-                   )
-
-                   repository.after_create_branch
+    return result if result[:status] == :error
 
-                   repository.find_branch(branch_name)
-                 else
-                   repository.add_branch(current_user, branch_name, ref)
-                 end
+    new_branch = repository.add_branch(current_user, branch_name, ref)
 
     if new_branch
       success(new_branch)

app/services/delete_tag_service.rb

@@ -7,7 +7,7 @@ class DeleteTagService < BaseService
       return error('No such tag', 404)
     end
 
-    if repository.rm_tag(tag_name)
+    if repository.rm_tag(current_user, tag_name)
       release = project.releases.find_by(tag: tag_name)
       release.destroy if release
 

app/services/files/base_service.rb

@@ -3,9 +3,9 @@ module Files
     class ValidationError < StandardError; end
 
     def execute
-      @source_project = params[:source_project] || @project
-      @source_branch = params[:source_branch]
-      @target_branch  = params[:target_branch]
+      @start_project = params[:start_project] || @project
+      @start_branch = params[:start_branch]
+      @target_branch = params[:target_branch]
 
       @commit_message = params[:commit_message]
       @file_path      = params[:file_path]
@@ -22,10 +22,8 @@ module Files
       # Validate parameters
       validate
 
-      # Create new branch if it different from source_branch
-      if different_branch?
-        create_target_branch
-      end
+      # Create new branch if it different from start_branch
+      validate_target_branch if different_branch?
 
       result = commit
       if result
@@ -40,7 +38,7 @@ module Files
     private
 
     def different_branch?
-      @source_branch != @target_branch || @source_project != @project
+      @start_branch != @target_branch || @start_project != @project
     end
 
     def file_has_changed?
@@ -65,22 +63,23 @@ module Files
       end
 
       unless project.empty_repo?
-        unless @source_project.repository.branch_names.include?(@source_branch)
+        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_names.include?(@target_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
 
-    def create_target_branch
-      result = CreateBranchService.new(project, current_user).execute(@target_branch, @source_branch, source_project: @source_project)
+    def validate_target_branch
+      result = ValidateNewBranchService.new(project, current_user).
+        execute(@target_branch)
 
-      unless result[:status] == :success
+      if result[:status] == :error
         raise_error("Something went wrong when we tried to create #{@target_branch} for you: #{result[:message]}")
       end
     end

app/services/files/create_dir_service.rb

 module Files
   class CreateDirService < Files::BaseService
     def commit
-      repository.commit_dir(current_user, @file_path, @commit_message, @target_branch, author_email: @author_email, author_name: @author_name)
+      repository.commit_dir(
+        current_user,
+        @file_path,
+        message: @commit_message,
+        branch_name: @target_branch,
+        author_email: @author_email,
+        author_name: @author_name,
+        start_project: @start_project,
+        start_branch_name: @start_branch)
     end
 
     def validate

app/services/files/create_service.rb

 module Files
   class CreateService < Files::BaseService
     def commit
-      repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch, false, author_email: @author_email, author_name: @author_name)
+      repository.commit_file(
+        current_user,
+        @file_path,
+        @file_content,
+        message: @commit_message,
+        branch_name: @target_branch,
+        update: false,
+        author_email: @author_email,
+        author_name: @author_name,
+        start_project: @start_project,
+        start_branch_name: @start_branch)
     end
 
     def validate
@@ -24,7 +34,7 @@ module Files
       unless project.empty_repo?
         @file_path.slice!(0) if @file_path.start_with?('/')
 
-        blob = repository.blob_at_branch(@source_branch, @file_path)
+        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')

app/services/files/delete_service.rb

 module Files
   class DeleteService < Files::BaseService
     def commit
-      repository.remove_file(current_user, @file_path, @commit_message, @target_branch, author_email: @author_email, author_name: @author_name)
+      repository.remove_file(
+        current_user,
+        @file_path,
+        message: @commit_message,
+        branch_name: @target_branch,
+        author_email: @author_email,
+        author_name: @author_name,
+        start_project: @start_project,
+        start_branch_name: @start_branch)
     end
   end
 end

app/services/files/multi_service.rb

@@ -5,11 +5,13 @@ module Files
     def commit
       repository.multi_action(
         user: current_user,
-        branch: @target_branch,
         message: @commit_message,
+        branch_name: @target_branch,
         actions: params[:actions],
         author_email: @author_email,
-        author_name: @author_name
+        author_name: @author_name,
+        start_project: @start_project,
+        start_branch_name: @start_branch
       )
     end
 
@@ -61,7 +63,7 @@ module Files
     end
 
     def last_commit
-      Gitlab::Git::Commit.last_for_path(repository, @source_branch, @file_path)
+      Gitlab::Git::Commit.last_for_path(repository, @start_branch, @file_path)
     end
 
     def regex_check(file)

app/services/files/update_service.rb

@@ -4,11 +4,13 @@ module Files
 
     def commit
       repository.update_file(current_user, @file_path, @file_content,
-                             branch: @target_branch,
-                             previous_path: @previous_path,
                              message: @commit_message,
+                             branch_name: @target_branch,
+                             previous_path: @previous_path,
                              author_email: @author_email,
-                             author_name: @author_name)
+                             author_name: @author_name,
+                             start_project: @start_project,
+                             start_branch_name: @start_branch)
     end
 
     private
@@ -23,7 +25,7 @@ module Files
 
     def last_commit
       @last_commit ||= Gitlab::Git::Commit.
-        last_for_path(@source_project.repository, @source_branch, @file_path)
+        last_for_path(@start_project.repository, @start_branch, @file_path)
     end
   end
 end

app/services/git_hooks_service.rb

@@ -18,9 +18,9 @@ class GitHooksService
       end
     end
 
-    yield self
-
-    run_hook('post-receive')
+    yield(self).tap do
+      run_hook('post-receive')
+    end
   end
 
   private

app/services/git_operation_service.rb

+class GitOperationService
+  attr_reader :user, :repository
+
+  def initialize(new_user, new_repository)
+    @user = new_user
+    @repository = new_repository
+  end
+
+  def add_branch(branch_name, newrev)
+    ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
+    oldrev = Gitlab::Git::BLANK_SHA
+
+    update_ref_in_hooks(ref, newrev, oldrev)
+  end
+
+  def rm_branch(branch)
+    ref = Gitlab::Git::BRANCH_REF_PREFIX + branch.name
+    oldrev = branch.target
+    newrev = Gitlab::Git::BLANK_SHA
+
+    update_ref_in_hooks(ref, newrev, oldrev)
+  end
+
+  def add_tag(tag_name, newrev, options = {})
+    ref = Gitlab::Git::TAG_REF_PREFIX + tag_name
+    oldrev = Gitlab::Git::BLANK_SHA
+
+    with_hooks(ref, newrev, oldrev) do |service|
+      # We want to pass the OID of the tag object to the hooks. For an
+      # annotated tag we don't know that OID until after the tag object
+      # (raw_tag) is created in the repository. That is why we have to
+      # update the value after creating the tag object. Only the
+      # "post-receive" hook will receive the correct value in this case.
+      raw_tag = repository.rugged.tags.create(tag_name, newrev, options)
+      service.newrev = raw_tag.target_id
+    end
+  end
+
+  def rm_tag(tag)
+    ref = Gitlab::Git::TAG_REF_PREFIX + tag.name
+    oldrev = tag.target
+    newrev = Gitlab::Git::BLANK_SHA
+
+    update_ref_in_hooks(ref, newrev, oldrev) do
+      repository.rugged.tags.delete(tag_name)
+    end
+  end
+
+  # Whenever `start_branch_name` is passed, if `branch_name` doesn't exist,
+  # it would be created from `start_branch_name`.
+  # If `start_project` is passed, and the branch doesn't exist,
+  # it would try to find the commits from it instead of current repository.
+  def with_branch(
+    branch_name,
+    start_branch_name: nil,
+    start_project: repository.project,
+    &block)
+
+    check_with_branch_arguments!(
+      branch_name, start_branch_name, start_project)
+
+    update_branch_with_hooks(branch_name) do
+      repository.with_repo_branch_commit(
+        start_project.repository,
+        start_branch_name || branch_name,
+        &block)
+    end
+  end
+
+  private
+
+  def update_branch_with_hooks(branch_name)
+    update_autocrlf_option
+
+    was_empty = repository.empty?
+
+    # Make commit
+    newrev = yield
+
+    unless newrev
+      raise Repository::CommitError.new('Failed to create commit')
+    end
+
+    branch = repository.find_branch(branch_name)
+    oldrev = find_oldrev_from_branch(newrev, branch)
+
+    ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
+    update_ref_in_hooks(ref, newrev, oldrev)
+
+    # If repo was empty expire cache
+    repository.after_create if was_empty
+    repository.after_create_branch if
+      was_empty || Gitlab::Git.blank_ref?(oldrev)
+
+    newrev
+  end
+
+  def find_oldrev_from_branch(newrev, branch)
+    return Gitlab::Git::BLANK_SHA unless branch
+
+    oldrev = branch.target
+
+    if oldrev == repository.rugged.merge_base(newrev, branch.target)
+      oldrev
+    else
+      raise Repository::CommitError.new('Branch diverged')
+    end
+  end
+
+  def update_ref_in_hooks(ref, newrev, oldrev)
+    with_hooks(ref, newrev, oldrev) do
+      update_ref(ref, newrev, oldrev)
+    end
+  end
+
+  def with_hooks(ref, newrev, oldrev)
+    GitHooksService.new.execute(
+      user,
+      repository.path_to_repo,
+      oldrev,
+      newrev,
+      ref) do |service|
+
+      yield(service)
+    end
+  end
+
+  def update_ref(ref, newrev, oldrev)
+    # We use 'git update-ref' because libgit2/rugged currently does not
+    # offer 'compare and swap' ref updates. Without compare-and-swap we can
+    # (and have!) accidentally reset the ref to an earlier state, clobbering
+    # commits. See also https://github.com/libgit2/libgit2/issues/1534.
+    command = %W[#{Gitlab.config.git.bin_path} update-ref --stdin -z]
+    _, status = Gitlab::Popen.popen(
+      command,
+      repository.path_to_repo) do |stdin|
+      stdin.write("update #{ref}\x00#{newrev}\x00#{oldrev}\x00")
+    end
+
+    unless status.zero?
+      raise Repository::CommitError.new(
+        "Could not update branch #{Gitlab::Git.branch_name(ref)}." \
+        " Please refresh and try again.")
+    end
+  end
+
+  def update_autocrlf_option
+    if repository.raw_repository.autocrlf != :input
+      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

app/services/merge_requests/build_service.rb

@@ -16,11 +16,12 @@ module MergeRequests
       messages = validate_branches(merge_request)
       return build_failed(merge_request, messages) unless messages.empty?
 
-      compare = CompareService.new.execute(
+      compare = CompareService.new(
         merge_request.source_project,
-        merge_request.source_branch,
+        merge_request.source_branch
+      ).execute(
         merge_request.target_project,
-        merge_request.target_branch,
+        merge_request.target_branch
       )
 
       merge_request.compare_commits = compare.commits

app/services/validate_new_branch_service.rb

+require_relative 'base_service'
+
+class ValidateNewBranchService < BaseService
+  def execute(branch_name)
+    valid_branch = Gitlab::GitRefValidator.validate(branch_name)
+
+    unless valid_branch
+      return error('Branch name is invalid')
+    end
+
+    repository = project.repository
+    existing_branch = repository.find_branch(branch_name)
+
+    if existing_branch
+      return error('Branch already exists')
+    end
+
+    success
+  rescue GitHooksService::PreReceiveError => ex
+    error(ex.message)
+  end
+end

app/workers/emails_on_push_worker.rb

@@ -33,13 +33,15 @@ class EmailsOnPushWorker
     reverse_compare = false
 
     if action == :push
-      compare = CompareService.new.execute(project, after_sha, project, before_sha)
+      compare = CompareService.new(project, after_sha)
+        .execute(project, before_sha)
       diff_refs = compare.diff_refs
 
       return false if compare.same
 
       if compare.commits.empty?
-        compare = CompareService.new.execute(project, before_sha, project, after_sha)
+        compare = CompareService.new(project, before_sha)
+          .execute(project, after_sha)
         diff_refs = compare.diff_refs
 
         reverse_compare = true

lib/api/commits.rb

@@ -54,7 +54,7 @@ module API
         authorize! :push_code, user_project
 
         attrs = declared_params
-        attrs[:source_branch] = attrs[:branch_name]
+        attrs[:start_branch] = attrs[:branch_name]
         attrs[:target_branch] = attrs[:branch_name]
         attrs[:actions].map! do |action|
           action[:action] = action[:action].to_sym
@@ -139,8 +139,6 @@ module API
         commit_params = {
           commit: commit,
           create_merge_request: false,
-          source_project: user_project,
-          source_branch: commit.cherry_pick_branch_name,
           target_branch: params[:branch]
         }
 

lib/api/files.rb

@@ -5,7 +5,7 @@ module API
       def commit_params(attrs)
         {
           file_path: attrs[:file_path],
-          source_branch: attrs[:branch_name],
+          start_branch: attrs[:branch_name],
           target_branch: attrs[:branch_name],
           commit_message: attrs[:commit_message],
           file_content: attrs[:content],

lib/gitlab/git.rb

@@ -6,7 +6,7 @@ module Gitlab
 
     class << self
       def ref_name(ref)
-        ref.gsub(/\Arefs\/(tags|heads)\//, '')
+        ref.sub(/\Arefs\/(tags|heads)\//, '')
       end
 
       def branch_name(ref)

spec/controllers/projects/templates_controller_spec.rb

@@ -14,7 +14,8 @@ describe Projects::TemplatesController do
 
   before do
     project.add_user(user, Gitlab::Access::MASTER)
-    project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
+    project.repository.commit_file(user, file_path_1, 'something valid',
+      message: 'test 3', branch_name: 'master', update: false)
   end
 
   describe '#show' do

spec/factories/projects.rb

@@ -127,6 +127,42 @@ FactoryGirl.define do
     path { 'gitlabhq' }
 
     test_repo
+
+    transient do
+      create_template nil
+    end
+
+    after :create do |project, evaluator|
+      TestEnv.copy_repo(project)
+
+      if evaluator.create_template
+        args = evaluator.create_template
+
+        project.add_user(args[:user], args[:access])
+
+        project.repository.commit_file(
+          args[:user],
+          ".gitlab/#{args[:path]}/bug.md",
+          'something valid',
+          message: 'test 3',
+          branch_name: 'master',
+          update: false)
+        project.repository.commit_file(
+          args[:user],
+          ".gitlab/#{args[:path]}/template_test.md",
+          'template_test',
+          message: 'test 1',
+          branch_name: 'master',
+          update: false)
+        project.repository.commit_file(
+          args[:user],
+          ".gitlab/#{args[:path]}/feature_proposal.md",
+          'feature_proposal',
+          message: 'test 2',
+          branch_name: 'master',
+          update: false)
+      end
+    end
   end
 
   factory :forked_project_with_submodules, parent: :empty_project do

spec/features/projects/files/editing_a_file_spec.rb

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

spec/features/projects/files/project_owner_creates_license_file_spec.rb

@@ -6,7 +6,8 @@ feature 'project owner creates a license file', feature: true, js: true do
   let(:project_master) { create(:user) }
   let(:project) { create(:project) }
   background do
-    project.repository.remove_file(project_master, 'LICENSE', 'Remove LICENSE', 'master')
+    project.repository.remove_file(project_master, 'LICENSE',
+      message: 'Remove LICENSE', branch_name: 'master')
     project.team << [project_master, :master]
     login_as(project_master)
     visit namespace_project_path(project.namespace, project)

spec/features/projects/issuable_templates_spec.rb

@@ -18,8 +18,20 @@ feature 'issuable templates', feature: true, js: true do
     let(:description_addition) { ' appending to description' }
 
     background do
-      project.repository.commit_file(user, '.gitlab/issue_templates/bug.md', template_content, 'added issue template', 'master', false)
-      project.repository.commit_file(user, '.gitlab/issue_templates/test.md', longtemplate_content, 'added issue template', 'master', false)
+      project.repository.commit_file(
+        user,
+        '.gitlab/issue_templates/bug.md',
+        template_content,
+        message: 'added issue template',
+        branch_name: 'master',
+        update: false)
+      project.repository.commit_file(
+        user,
+        '.gitlab/issue_templates/test.md',
+        longtemplate_content,
+        message: 'added issue template',
+        branch_name: 'master',
+        update: false)
       visit edit_namespace_project_issue_path project.namespace, project, issue
       fill_in :'issue[title]', with: 'test issue title'
     end
@@ -67,7 +79,13 @@ feature 'issuable templates', feature: true, js: true do
     let(:issue) { create(:issue, author: user, assignee: user, project: project) }
 
     background do
-      project.repository.commit_file(user, '.gitlab/issue_templates/bug.md', template_content, 'added issue template', 'master', false)
+      project.repository.commit_file(
+        user,
+        '.gitlab/issue_templates/bug.md',
+        template_content,
+        message: 'added issue template',
+        branch_name: 'master',
+        update: false)
       visit edit_namespace_project_issue_path project.namespace, project, issue
       fill_in :'issue[title]', with: 'test issue title'
       fill_in :'issue[description]', with: prior_description
@@ -86,7 +104,13 @@ feature 'issuable templates', feature: true, js: true do
     let(:merge_request) { create(:merge_request, :with_diffs, source_project: project) }
 
     background do
-      project.repository.commit_file(user, '.gitlab/merge_request_templates/feature-proposal.md', template_content, 'added merge request template', 'master', false)
+      project.repository.commit_file(
+        user,
+        '.gitlab/merge_request_templates/feature-proposal.md',
+        template_content,
+        message: 'added merge request template',
+        branch_name: 'master',
+        update: false)
       visit edit_namespace_project_merge_request_path project.namespace, project, merge_request
       fill_in :'merge_request[title]', with: 'test merge request title'
     end
@@ -111,7 +135,13 @@ feature 'issuable templates', feature: true, js: true do
       fork_project.team << [fork_user, :master]
       create(:forked_project_link, forked_to_project: fork_project, forked_from_project: project)
       login_as fork_user
-      project.repository.commit_file(fork_user, '.gitlab/merge_request_templates/feature-proposal.md', template_content, 'added merge request template', 'master', false)
+      project.repository.commit_file(
+        fork_user,
+        '.gitlab/merge_request_templates/feature-proposal.md',
+        template_content,
+        message: 'added merge request template',
+        branch_name: 'master',
+        update: false)
       visit edit_namespace_project_merge_request_path project.namespace, project, merge_request
       fill_in :'merge_request[title]', with: 'test merge request title'
     end

spec/lib/gitlab/checks/change_access_spec.rb

@@ -193,7 +193,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
             white_listed.each do |file_path|
               old_rev = 'be93687618e4b132087f430a4d8fc3a609c9b77c'
               old_rev = new_rev if new_rev
-              new_rev = project.repository.commit_file(user, file_path, "commit #{file_path}", "commit #{file_path}", "master", false)
+              new_rev = project.repository.commit_file(user, file_path, "commit #{file_path}", message: "commit #{file_path}", branch_name: "master", update: false)
 
               allow(project.repository).to receive(:new_commits).and_return(
                 project.repository.commits_between(old_rev, new_rev)
@@ -216,7 +216,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
             black_listed.each do |file_path|
               old_rev = 'be93687618e4b132087f430a4d8fc3a609c9b77c'
               old_rev = new_rev if new_rev
-              new_rev = project.repository.commit_file(user, file_path, "commit #{file_path}", "commit #{file_path}", "master", false)
+              new_rev = project.repository.commit_file(user, file_path, "commit #{file_path}", message: "commit #{file_path}", branch_name: "master", update: false)
 
               allow(project.repository).to receive(:new_commits).and_return(
                 project.repository.commits_between(old_rev, new_rev)

spec/lib/gitlab/diff/position_tracer_spec.rb

@@ -99,7 +99,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
     Files::CreateService.new(
       project,
       current_user,
-      source_branch: branch_name,
+      start_branch: branch_name,
       target_branch: branch_name,
       commit_message: "Create file",
       file_path: file_name,
@@ -112,7 +112,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
     Files::UpdateService.new(
       project,
       current_user,
-      source_branch: branch_name,
+      start_branch: branch_name,
       target_branch: branch_name,
       commit_message: "Update file",
       file_path: file_name,
@@ -125,7 +125,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do
     Files::DeleteService.new(
       project,
       current_user,
-      source_branch: branch_name,
+      start_branch: branch_name,
       target_branch: branch_name,
       commit_message: "Delete file",
       file_path: file_name

spec/lib/gitlab/elastic/search_results_spec.rb

@@ -561,9 +561,9 @@ describe Gitlab::Elastic::SearchResults, lib: true do
             user,
             'test-file',
             'search test',
-            'search test',
-            'master',
-            false
+            message: 'search test',
+            branch_name: 'master',
+            update: false
           )
 
           project.repository.index_commits
@@ -594,9 +594,9 @@ describe Gitlab::Elastic::SearchResults, lib: true do
             user,
             'test-file',
             'tesla',
-            'search test',
-            'master',
-            false
+            message: 'search test',
+            branch_name: 'master',
+            update: false
           )
 
           project.repository.index_blobs

spec/lib/gitlab/git_access_spec.rb

@@ -217,7 +217,7 @@ describe Gitlab::GitAccess, lib: true do
 
     def stub_git_hooks
       # Running the `pre-receive` hook is expensive, and not necessary for this test.
-      allow_any_instance_of(GitHooksService).to receive(:execute).and_yield
+      allow_any_instance_of(GitHooksService).to receive(:execute).and_yield(GitHooksService.new)
     end
 
     def merge_into_protected_branch
@@ -225,7 +225,7 @@ describe Gitlab::GitAccess, lib: true do
                                            stub_git_hooks
                                            project.repository.add_branch(user, unprotected_branch, 'feature')
                                            target_branch = project.repository.lookup('feature')
-                                           source_branch = project.repository.commit_file(user, FFaker::InternetSE.login_user_name, FFaker::HipsterIpsum.paragraph, FFaker::HipsterIpsum.sentence, unprotected_branch, false)
+                                           source_branch = project.repository.commit_file(user, FFaker::InternetSE.login_user_name, FFaker::HipsterIpsum.paragraph, message: FFaker::HipsterIpsum.sentence, branch_name: unprotected_branch, update: false)
                                            rugged = project.repository.rugged
                                            author = { email: "email@example.com", time: Time.now, name: "Example Git User" }
 

spec/lib/gitlab/template/issue_template_spec.rb

@@ -4,16 +4,14 @@ describe Gitlab::Template::IssueTemplate do
   subject { described_class }
 
   let(:user) { create(:user) }
-  let(:project) { create(:project, :repository) }
-  let(:file_path_1) { '.gitlab/issue_templates/bug.md' }
-  let(:file_path_2) { '.gitlab/issue_templates/template_test.md' }
-  let(:file_path_3) { '.gitlab/issue_templates/feature_proposal.md' }
-
-  before do
-    project.add_user(user, Gitlab::Access::MASTER)
-    project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
-    project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false)
-    project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false)
+
+  let(:project) do
+    create(:project,
+      :repository,
+      create_template: {
+        user: user,
+        access: Gitlab::Access::MASTER,
+        path: 'issue_templates' })
   end
 
   describe '.all' do

spec/lib/gitlab/template/merge_request_template_spec.rb

@@ -4,16 +4,14 @@ describe Gitlab::Template::MergeRequestTemplate do
   subject { described_class }
 
   let(:user) { create(:user) }
-  let(:project) { create(:project, :repository) }
-  let(:file_path_1) { '.gitlab/merge_request_templates/bug.md' }
-  let(:file_path_2) { '.gitlab/merge_request_templates/template_test.md' }
-  let(:file_path_3) { '.gitlab/merge_request_templates/feature_proposal.md' }
-
-  before do
-    project.add_user(user, Gitlab::Access::MASTER)
-    project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false)
-    project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false)
-    project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false)
+
+  let(:project) do
+    create(:project,
+      :repository,
+      create_template: {
+        user: user,
+        access: Gitlab::Access::MASTER,
+        path: 'merge_request_templates' })
   end
 
   describe '.all' do

spec/models/cycle_analytics/production_spec.rb

@@ -21,7 +21,13 @@ describe 'CycleAnalytics#production', feature: true do
        ["production deploy happens after merge request is merged (along with other changes)",
         lambda do |context, data|
           # Make other changes on master
-          sha = context.project.repository.commit_file(context.user, context.random_git_name, "content", "commit message", 'master', false)
+          sha = context.project.repository.commit_file(
+            context.user,
+            context.random_git_name,
+            'content',
+            message: 'commit message',
+            branch_name: 'master',
+            update: false)
           context.project.repository.commit(sha)
 
           context.deploy_master

spec/models/cycle_analytics/staging_spec.rb

@@ -29,10 +29,10 @@ describe 'CycleAnalytics#staging', feature: true do
                                sha = context.project.repository.commit_file(
                                  context.user,
                                  context.random_git_name,
-                                 "content",
-                                 "commit message",
-                                 'master',
-                                 false)
+                                 'content',
+                                 message: 'commit message',
+                                 branch_name: 'master',
+                                 update: false)
                                context.project.repository.commit(sha)
 
                                context.deploy_master

spec/services/compare_service_spec.rb

@@ -3,17 +3,17 @@ require 'spec_helper'
 describe CompareService, services: true do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
-  let(:service) { described_class.new }
+  let(:service) { described_class.new(project, 'feature') }
 
   describe '#execute' do
     context 'compare with base, like feature...fix' do
-      subject { service.execute(project, 'feature', project, 'fix', straight: false) }
+      subject { service.execute(project, 'fix', straight: false) }
 
       it { expect(subject.diffs.size).to eq(1) }
     end
 
     context 'straight compare, like feature..fix' do
-      subject { service.execute(project, 'feature', project, 'fix', straight: true) }
+      subject { service.execute(project, 'fix', straight: true) }
 
       it { expect(subject.diffs.size).to eq(3) }
     end

spec/services/files/update_service_spec.rb

@@ -6,7 +6,10 @@ describe Files::UpdateService do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
   let(:file_path) { 'files/ruby/popen.rb' }
-  let(:new_contents) { "New Content" }
+  let(:new_contents) { 'New Content' }
+  let(:target_branch) { project.default_branch }
+  let(:last_commit_sha) { nil }
+
   let(:commit_params) do
     {
       file_path: file_path,
@@ -14,9 +17,9 @@ describe Files::UpdateService do
       file_content: new_contents,
       file_content_encoding: "text",
       last_commit_sha: last_commit_sha,
-      source_project: project,
-      source_branch: project.default_branch,
-      target_branch: project.default_branch,
+      start_project: project,
+      start_branch: project.default_branch,
+      target_branch: target_branch
     }
   end
 
@@ -54,18 +57,6 @@ describe Files::UpdateService do
     end
 
     context "when the last_commit_sha is not supplied" do
-      let(:commit_params) do
-        {
-          file_path: file_path,
-          commit_message: "Update File",
-          file_content: new_contents,
-          file_content_encoding: "text",
-          source_project: project,
-          source_branch: project.default_branch,
-          target_branch: project.default_branch,
-        }
-      end
-
       it "returns a hash with the :success status " do
         results = subject.execute
 
@@ -80,5 +71,15 @@ describe Files::UpdateService do
         expect(results.data).to eq(new_contents)
       end
     end
+
+    context 'when target branch is different than source branch' do
+      let(:target_branch) { "#{project.default_branch}-new" }
+
+      it 'fires hooks only once' do
+        expect(GitHooksService).to receive(:new).once.and_call_original
+
+        subject.execute
+      end
+    end
   end
 end

spec/services/git_hooks_service_spec.rb

@@ -21,7 +21,7 @@ describe GitHooksService, services: true do
         hook = double(trigger: [true, nil])
         expect(Gitlab::Git::Hook).to receive(:new).exactly(3).times.and_return(hook)
 
-        expect(service.execute(user, @repo_path, @blankrev, @newrev, @ref) { }).to eq([true, nil])
+        service.execute(user, @repo_path, @blankrev, @newrev, @ref) { }
       end
     end
 

spec/services/merge_requests/resolve_service_spec.rb

@@ -66,7 +66,13 @@ describe MergeRequests::ResolveService do
 
       context 'when the source project is a fork and does not contain the HEAD of the target branch' do
         let!(:target_head) do
-          project.repository.commit_file(user, 'new-file-in-target', '', 'Add new file in target', 'conflict-start', false)
+          project.repository.commit_file(
+            user,
+            'new-file-in-target',
+            '',
+            message: 'Add new file in target',
+            branch_name: 'conflict-start',
+            update: false)
         end
 
         before do

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -152,7 +152,7 @@ describe Projects::UpdateRemoteMirrorService do
           remote_tags = generate_tags(repository, 'v1.0.0', 'v1.1.0')
           allow(repository).to receive(:remote_tags) { remote_tags }
 
-          repository.rm_tag('v1.0.0')
+          repository.rm_tag(create(:user), 'v1.0.0')
 
           expect(repository).to receive(:delete_remote_branches).with(remote_mirror.ref_name, ['v1.0.0'])
 

spec/support/cycle_analytics_helpers.rb

@@ -35,7 +35,13 @@ module CycleAnalyticsHelpers
       project.repository.add_branch(user, source_branch, 'master')
     end
 
-    sha = project.repository.commit_file(user, random_git_name, "content", "commit message", source_branch, false)
+    sha = project.repository.commit_file(
+      user,
+      random_git_name,
+      'content',
+      message: 'commit message',
+      branch_name: source_branch,
+      update: false)
     project.repository.commit(sha)
 
     opts = {

spec/workers/git_garbage_collect_worker_spec.rb

@@ -107,7 +107,8 @@ describe GitGarbageCollectWorker do
       tree: old_commit.tree,
       parents: [old_commit],
     )
-    project.repository.update_ref!(
+    GitOperationService.new(nil, project.repository).send(
+      :update_ref,
       "refs/heads/#{SecureRandom.hex(6)}",
       new_commit_sha,
       Gitlab::Git::BLANK_SHA