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

Don't execute git hooks if you create branch as part of other change

Closes #23439

See merge request !7237

18 files changed, 277 insertions, 113 deletions

diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb
index 99d0bcfa8d1..80f84a388ce 100644
--- a/spec/controllers/projects/templates_controller_spec.rb
+++ b/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
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index 992580a6b34..715b2a27b30 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -106,6 +106,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
diff --git a/spec/features/projects/files/editing_a_file_spec.rb b/spec/features/projects/files/editing_a_file_spec.rb
index fe047e00409..36a80d7575d 100644
--- a/spec/features/projects/files/editing_a_file_spec.rb
+++ b/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",
diff --git a/spec/features/projects/files/project_owner_creates_license_file_spec.rb b/spec/features/projects/files/project_owner_creates_license_file_spec.rb
index a521ce50f35..64094af29c0 100644
--- a/spec/features/projects/files/project_owner_creates_license_file_spec.rb
+++ b/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)
diff --git a/spec/features/projects/issuable_templates_spec.rb b/spec/features/projects/issuable_templates_spec.rb
index 6dae5c64b30..e90a033b8c4 100644
--- a/spec/features/projects/issuable_templates_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb
index f5822fed37c..ffb9ba86fbb 100644
--- a/spec/lib/gitlab/diff/position_tracer_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index b080be62b34..116ab16ae74 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -209,7 +209,13 @@ 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" }
 
diff --git a/spec/lib/gitlab/template/issue_template_spec.rb b/spec/lib/gitlab/template/issue_template_spec.rb
index 45cec65a284..1335a2b8f35 100644
--- a/spec/lib/gitlab/template/issue_template_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/template/merge_request_template_spec.rb b/spec/lib/gitlab/template/merge_request_template_spec.rb
index ae51b79be22..320b870309a 100644
--- a/spec/lib/gitlab/template/merge_request_template_spec.rb
+++ b/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
diff --git a/spec/models/cycle_analytics/production_spec.rb b/spec/models/cycle_analytics/production_spec.rb
index 2cbee741fb0..b9fe492fe2c 100644
--- a/spec/models/cycle_analytics/production_spec.rb
+++ b/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
diff --git a/spec/models/cycle_analytics/staging_spec.rb b/spec/models/cycle_analytics/staging_spec.rb
index 104e65335dd..9a024d533a1 100644
--- a/spec/models/cycle_analytics/staging_spec.rb
+++ b/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
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 829b69093c9..901cfb907f2 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -289,17 +289,39 @@ describe Repository, models: true do
   describe "#commit_dir" do
     it "commits a change that creates a new directory" do
       expect do
-        repository.commit_dir(user, 'newdir', 'Create newdir', 'master')
+        repository.commit_dir(user, 'newdir',
+          message: 'Create newdir', branch_name: 'master')
       end.to change { repository.commits('master').count }.by(1)
 
       newdir = repository.tree('master', 'newdir')
       expect(newdir.path).to eq('newdir')
     end
 
+    context "when committing to another project" do
+      let(:forked_project) { create(:project) }
+
+      it "creates a fork and commit to the forked project" do
+        expect do
+          repository.commit_dir(user, 'newdir',
+            message: 'Create newdir', branch_name: 'patch',
+            start_branch_name: 'master', start_project: forked_project)
+        end.to change { repository.commits('master').count }.by(0)
+
+        expect(repository.branch_exists?('patch')).to be_truthy
+        expect(forked_project.repository.branch_exists?('patch')).to be_falsy
+
+        newdir = repository.tree('patch', 'newdir')
+        expect(newdir.path).to eq('newdir')
+      end
+    end
+
     context "when an author is specified" do
       it "uses the given email/name to set the commit's author" do
         expect do
-          repository.commit_dir(user, "newdir", "Add newdir", 'master', author_email: author_email, author_name: author_name)
+          repository.commit_dir(user, 'newdir',
+            message: 'Add newdir',
+            branch_name: 'master',
+            author_email: author_email, author_name: author_name)
         end.to change { repository.commits('master').count }.by(1)
 
         last_commit = repository.commit
@@ -314,8 +336,9 @@ describe Repository, models: true do
     it 'commits change to a file successfully' do
       expect do
         repository.commit_file(user, 'CHANGELOG', 'Changelog!',
-                              'Updates file content',
-                              'master', true)
+                               message: 'Updates file content',
+                               branch_name: 'master',
+                               update: true)
       end.to change { repository.commits('master').count }.by(1)
 
       blob = repository.blob_at('master', 'CHANGELOG')
@@ -326,8 +349,12 @@ describe Repository, models: true do
     context "when an author is specified" do
       it "uses the given email/name to set the commit's author" do
         expect do
-          repository.commit_file(user, "README", 'README!', 'Add README',
-                                'master', true, author_email: author_email, author_name: author_name)
+          repository.commit_file(user, 'README', 'README!',
+                                 message: 'Add README',
+                                 branch_name: 'master',
+                                 update: true,
+                                 author_email: author_email,
+                                 author_name: author_name)
         end.to change { repository.commits('master').count }.by(1)
 
         last_commit = repository.commit
@@ -342,7 +369,7 @@ describe Repository, models: true do
     it 'updates filename successfully' do
       expect do
         repository.update_file(user, 'NEWLICENSE', 'Copyright!',
-                                     branch: 'master',
+                                     branch_name: 'master',
                                      previous_path: 'LICENSE',
                                      message: 'Changes filename')
       end.to change { repository.commits('master').count }.by(1)
@@ -355,15 +382,16 @@ describe Repository, models: true do
 
     context "when an author is specified" do
       it "uses the given email/name to set the commit's author" do
-        repository.commit_file(user, "README", 'README!', 'Add README', 'master', true)
+        repository.commit_file(user, 'README', 'README!',
+          message: 'Add README', branch_name: 'master', update: true)
 
         expect do
-          repository.update_file(user, 'README', "Updated README!",
-                                branch: 'master',
-                                previous_path: 'README',
-                                message: 'Update README',
-                                author_email: author_email,
-                                author_name: author_name)
+          repository.update_file(user, 'README', 'Updated README!',
+                                 branch_name: 'master',
+                                 previous_path: 'README',
+                                 message: 'Update README',
+                                 author_email: author_email,
+                                 author_name: author_name)
         end.to change { repository.commits('master').count }.by(1)
 
         last_commit = repository.commit
@@ -376,10 +404,12 @@ describe Repository, models: true do
 
   describe "#remove_file" do
     it 'removes file successfully' do
-      repository.commit_file(user, "README", 'README!', 'Add README', 'master', true)
+      repository.commit_file(user, 'README', 'README!',
+        message: 'Add README', branch_name: 'master', update: true)
 
       expect do
-        repository.remove_file(user, "README", "Remove README", 'master')
+        repository.remove_file(user, 'README',
+          message: 'Remove README', branch_name: 'master')
       end.to change { repository.commits('master').count }.by(1)
 
       expect(repository.blob_at('master', 'README')).to be_nil
@@ -387,10 +417,13 @@ describe Repository, models: true do
 
     context "when an author is specified" do
       it "uses the given email/name to set the commit's author" do
-        repository.commit_file(user, "README", 'README!', 'Add README', 'master', true)
+        repository.commit_file(user, 'README', 'README!',
+          message: 'Add README', branch_name: 'master', update: true)
 
         expect do
-          repository.remove_file(user, "README", "Remove README", 'master', author_email: author_email, author_name: author_name)
+          repository.remove_file(user, 'README',
+            message: 'Remove README', branch_name: 'master',
+            author_email: author_email, author_name: author_name)
         end.to change { repository.commits('master').count }.by(1)
 
         last_commit = repository.commit
@@ -538,11 +571,14 @@ describe Repository, models: true do
 
   describe "#license_blob", caching: true do
     before do
-      repository.remove_file(user, 'LICENSE', 'Remove LICENSE', 'master')
+      repository.remove_file(
+        user, 'LICENSE', message: 'Remove LICENSE', branch_name: 'master')
     end
 
     it 'handles when HEAD points to non-existent ref' do
-      repository.commit_file(user, 'LICENSE', 'Copyright!', 'Add LICENSE', 'master', false)
+      repository.commit_file(
+        user, 'LICENSE', 'Copyright!',
+        message: 'Add LICENSE', branch_name: 'master', update: false)
 
       allow(repository).to receive(:file_on_head).
         and_raise(Rugged::ReferenceError)
@@ -551,21 +587,27 @@ describe Repository, models: true do
     end
 
     it 'looks in the root_ref only' do
-      repository.remove_file(user, 'LICENSE', 'Remove LICENSE', 'markdown')
-      repository.commit_file(user, 'LICENSE', Licensee::License.new('mit').content, 'Add LICENSE', 'markdown', false)
+      repository.remove_file(user, 'LICENSE',
+        message: 'Remove LICENSE', branch_name: 'markdown')
+      repository.commit_file(user, 'LICENSE',
+        Licensee::License.new('mit').content,
+        message: 'Add LICENSE', branch_name: 'markdown', update: false)
 
       expect(repository.license_blob).to be_nil
     end
 
     it 'detects license file with no recognizable open-source license content' do
-      repository.commit_file(user, 'LICENSE', 'Copyright!', 'Add LICENSE', 'master', false)
+      repository.commit_file(user, 'LICENSE', 'Copyright!',
+        message: 'Add LICENSE', branch_name: 'master', update: false)
 
       expect(repository.license_blob.name).to eq('LICENSE')
     end
 
     %w[LICENSE LICENCE LiCensE LICENSE.md LICENSE.foo COPYING COPYING.md].each do |filename|
       it "detects '#{filename}'" do
-        repository.commit_file(user, filename, Licensee::License.new('mit').content, "Add #{filename}", 'master', false)
+        repository.commit_file(user, filename,
+          Licensee::License.new('mit').content,
+          message: "Add #{filename}", branch_name: 'master', update: false)
 
         expect(repository.license_blob.name).to eq(filename)
       end
@@ -574,7 +616,8 @@ describe Repository, models: true do
 
   describe '#license_key', caching: true do
     before do
-      repository.remove_file(user, 'LICENSE', 'Remove LICENSE', 'master')
+      repository.remove_file(user, 'LICENSE',
+        message: 'Remove LICENSE', branch_name: 'master')
     end
 
     it 'returns nil when no license is detected' do
@@ -588,13 +631,16 @@ describe Repository, models: true do
     end
 
     it 'detects license file with no recognizable open-source license content' do
-      repository.commit_file(user, 'LICENSE', 'Copyright!', 'Add LICENSE', 'master', false)
+      repository.commit_file(user, 'LICENSE', 'Copyright!',
+        message: 'Add LICENSE', branch_name: 'master', update: false)
 
       expect(repository.license_key).to be_nil
     end
 
     it 'returns the license key' do
-      repository.commit_file(user, 'LICENSE', Licensee::License.new('mit').content, 'Add LICENSE', 'master', false)
+      repository.commit_file(user, 'LICENSE',
+        Licensee::License.new('mit').content,
+        message: 'Add LICENSE', branch_name: 'master', update: false)
 
       expect(repository.license_key).to eq('mit')
     end
@@ -707,7 +753,7 @@ describe Repository, models: true do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
-          repository.rm_branch(user, 'new_feature')
+          repository.rm_branch(user, 'feature')
         end.to raise_error(GitHooksService::PreReceiveError)
       end
 
@@ -728,36 +774,51 @@ describe Repository, models: true do
 
     context 'when pre hooks were successful' do
       before do
-        expect_any_instance_of(GitHooksService).to receive(:execute).
-          with(user, repository.path_to_repo, old_rev, new_rev, 'refs/heads/feature').
-          and_yield.and_return(true)
+        service = GitHooksService.new
+        expect(GitHooksService).to receive(:new).and_return(service)
+        expect(service).to receive(:execute).
+          with(
+            user,
+            repository.path_to_repo,
+            old_rev,
+            new_rev,
+            'refs/heads/feature').
+          and_yield(service).and_return(true)
       end
 
       it 'runs without errors' do
         expect do
-          repository.update_branch_with_hooks(user, 'feature') { new_rev }
+          GitOperationService.new(user, repository).with_branch('feature') do
+            new_rev
+          end
         end.not_to raise_error
       end
 
       it 'ensures the autocrlf Git option is set to :input' do
-        expect(repository).to receive(:update_autocrlf_option)
+        service = GitOperationService.new(user, repository)
+
+        expect(service).to receive(:update_autocrlf_option)
 
-        repository.update_branch_with_hooks(user, 'feature') { new_rev }
+        service.with_branch('feature') { new_rev }
       end
 
       context "when the branch wasn't empty" do
         it 'updates the head' do
           expect(repository.find_branch('feature').dereferenced_target.id).to eq(old_rev)
-          repository.update_branch_with_hooks(user, 'feature') { new_rev }
+
+          GitOperationService.new(user, repository).with_branch('feature') do
+            new_rev
+          end
+
           expect(repository.find_branch('feature').dereferenced_target.id).to eq(new_rev)
         end
       end
     end
 
     context 'when the update adds more than one commit' do
-      it 'runs without errors' do
-        old_rev = '33f3729a45c02fc67d00adb1b8bca394b0e761d9'
+      let(:old_rev) { '33f3729a45c02fc67d00adb1b8bca394b0e761d9' }
 
+      it 'runs without errors' do
         # old_rev is an ancestor of new_rev
         expect(repository.rugged.merge_base(old_rev, new_rev)).to eq(old_rev)
 
@@ -767,22 +828,28 @@ describe Repository, models: true do
         branch = 'feature-ff-target'
         repository.add_branch(user, branch, old_rev)
 
-        expect { repository.update_branch_with_hooks(user, branch) { new_rev } }.not_to raise_error
+        expect do
+          GitOperationService.new(user, repository).with_branch(branch) do
+            new_rev
+          end
+        end.not_to raise_error
       end
     end
 
     context 'when the update would remove commits from the target branch' do
-      it 'raises an exception' do
-        branch = 'master'
-        old_rev = repository.find_branch(branch).dereferenced_target.sha
+      let(:branch) { 'master' }
+      let(:old_rev) { repository.find_branch(branch).dereferenced_target.sha }
 
+      it 'raises an exception' do
         # The 'master' branch is NOT an ancestor of new_rev.
         expect(repository.rugged.merge_base(old_rev, new_rev)).not_to eq(old_rev)
 
         # Updating 'master' to new_rev would lose the commits on 'master' that
         # are not contained in new_rev. This should not be allowed.
         expect do
-          repository.update_branch_with_hooks(user, branch) { new_rev }
+          GitOperationService.new(user, repository).with_branch(branch) do
+            new_rev
+          end
         end.to raise_error(Repository::CommitError)
       end
     end
@@ -792,7 +859,9 @@ describe Repository, models: true do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
 
         expect do
-          repository.update_branch_with_hooks(user, 'feature') { new_rev }
+          GitOperationService.new(user, repository).with_branch('feature') do
+            new_rev
+          end
         end.to raise_error(GitHooksService::PreReceiveError)
       end
     end
@@ -800,7 +869,6 @@ describe Repository, models: true do
     context 'when target branch is different from source branch' do
       before do
         allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, ''])
-        allow(repository).to receive(:update_ref!)
       end
 
       it 'expires branch cache' do
@@ -809,7 +877,10 @@ describe Repository, models: true do
         expect(repository).not_to receive(:expire_emptiness_caches)
         expect(repository).to     receive(:expire_branches_cache)
 
-        repository.update_branch_with_hooks(user, 'new-feature') { new_rev }
+        GitOperationService.new(user, repository).
+          with_branch('new-feature') do
+            new_rev
+          end
       end
     end
 
@@ -827,7 +898,9 @@ describe Repository, models: true do
         expect(empty_repository).to receive(:expire_branches_cache)
 
         empty_repository.commit_file(user, 'CHANGELOG', 'Changelog!',
-                                     'Updates file content', 'master', false)
+                                     message: 'Updates file content',
+                                     branch_name: 'master',
+                                     update: false)
       end
     end
   end
@@ -877,7 +950,7 @@ describe Repository, models: true do
       end
 
       it 'sets autocrlf to :input' do
-        repository.update_autocrlf_option
+        GitOperationService.new(nil, repository).send(:update_autocrlf_option)
 
         expect(repository.raw_repository.autocrlf).to eq(:input)
       end
@@ -892,7 +965,7 @@ describe Repository, models: true do
         expect(repository.raw_repository).not_to receive(:autocrlf=).
           with(:input)
 
-        repository.update_autocrlf_option
+        GitOperationService.new(nil, repository).send(:update_autocrlf_option)
       end
     end
   end
@@ -1388,9 +1461,10 @@ describe Repository, models: true do
   describe '#rm_tag' do
     it 'removes a tag' do
       expect(repository).to receive(:before_remove_tag)
-      expect(repository.rugged.tags).to receive(:delete).with('v1.1.0')
 
-      repository.rm_tag('v1.1.0')
+      repository.rm_tag(create(:user), 'v1.1.0')
+
+      expect(repository.find_tag('v1.1.0')).to be_nil
     end
   end
 
@@ -1458,16 +1532,16 @@ describe Repository, models: true do
     end
   end
 
-  describe '#update_ref!' do
+  describe '#update_ref' do
     it 'can create a ref' do
-      repository.update_ref!('refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+      GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/foobar', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
 
       expect(repository.find_branch('foobar')).not_to be_nil
     end
 
     it 'raises CommitError when the ref update fails' do
       expect do
-        repository.update_ref!('refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
+        GitOperationService.new(nil, repository).send(:update_ref, 'refs/heads/master', 'refs/heads/master', Gitlab::Git::BLANK_SHA)
       end.to raise_error(Repository::CommitError)
     end
   end
diff --git a/spec/services/compare_service_spec.rb b/spec/services/compare_service_spec.rb
index 3760f19aaa2..0a7fc58523f 100644
--- a/spec/services/compare_service_spec.rb
+++ b/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
diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb
index d3c37c7820f..35e6e139238 100644
--- a/spec/services/files/update_service_spec.rb
+++ b/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
diff --git a/spec/services/git_hooks_service_spec.rb b/spec/services/git_hooks_service_spec.rb
index 41b0968b8b4..3318dfb22b6 100644
--- a/spec/services/git_hooks_service_spec.rb
+++ b/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
 
diff --git a/spec/services/merge_requests/resolve_service_spec.rb b/spec/services/merge_requests/resolve_service_spec.rb
index 388abb6a0df..a0e51681725 100644
--- a/spec/services/merge_requests/resolve_service_spec.rb
+++ b/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
diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb
index 75c95d70951..6ed55289ed9 100644
--- a/spec/support/cycle_analytics_helpers.rb
+++ b/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 = {
diff --git a/spec/workers/git_garbage_collect_worker_spec.rb b/spec/workers/git_garbage_collect_worker_spec.rb
index e471a68a49a..5ef8cf1105b 100644
--- a/spec/workers/git_garbage_collect_worker_spec.rb
+++ b/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