Merge branch 'gitaly-mandatory-20180702-jv' into 'master'

Make OperationService RPC's mandatory

Closes gitaly#801, gitaly#779, gitaly#755, gitaly#754, gitaly#727, gitaly#680, gitaly#633, gitaly#547, gitaly#546, and gitaly#541

See merge request gitlab-org/gitlab-ce!20309

GITALY_SERVER_VERSION

-0.109.0
+0.110.0

lib/gitlab/gitaly_client/operation_service.rb

@@ -64,6 +64,8 @@ module Gitlab
 
         target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit)
         Gitlab::Git::Branch.new(@repository, branch.name, target_commit.id, target_commit)
+      rescue GRPC::FailedPrecondition => ex
+        raise Gitlab::Git::Repository::InvalidRef, ex
       end
 
       def user_delete_branch(branch_name, user)
@@ -133,6 +135,8 @@ module Gitlab
           request
         ).branch_update
         Gitlab::Git::OperationService::BranchUpdate.from_gitaly(branch_update)
+      rescue GRPC::FailedPrecondition => e
+        raise Gitlab::Git::CommitError, e
       end
 
       def user_cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)

spec/features/tags/master_deletes_tag_spec.rb

@@ -35,30 +35,15 @@ feature 'Master deletes tag' do
   end
 
   context 'when pre-receive hook fails', :js do
-    context 'when Gitaly operation_user_delete_tag feature is enabled' do
-      before do
-        allow_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:rm_tag)
-          .and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
-      end
-
-      scenario 'shows the error message' do
-        delete_first_tag
-
-        expect(page).to have_content('Do not delete tags')
-      end
+    before do
+      allow_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:rm_tag)
+        .and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
     end
 
-    context 'when Gitaly operation_user_delete_tag feature is disabled', :skip_gitaly_mock do
-      before do
-        allow_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
-          .and_raise(Gitlab::Git::PreReceiveError, 'Do not delete tags')
-      end
-
-      scenario 'shows the error message' do
-        delete_first_tag
+    scenario 'shows the error message' do
+      delete_first_tag
 
-        expect(page).to have_content('Do not delete tags')
-      end
+      expect(page).to have_content('Do not delete tags')
     end
   end
 

spec/lib/gitlab/git/repository_spec.rb

@@ -1971,21 +1971,15 @@ describe Gitlab::Git::Repository, seed_helper: true do
       end
     end
 
-    context 'with gitaly' do
-      it "calls Gitaly's OperationService" do
-        expect_any_instance_of(Gitlab::GitalyClient::OperationService)
-          .to receive(:user_ff_branch).with(user, source_sha, target_branch)
-          .and_return(nil)
+    it "calls Gitaly's OperationService" do
+      expect_any_instance_of(Gitlab::GitalyClient::OperationService)
+        .to receive(:user_ff_branch).with(user, source_sha, target_branch)
+        .and_return(nil)
 
-        subject
-      end
-
-      it_behaves_like '#ff_merge'
+      subject
     end
 
-    context 'without gitaly', :skip_gitaly_mock do
-      it_behaves_like '#ff_merge'
-    end
+    it_behaves_like '#ff_merge'
   end
 
   describe '#delete_all_refs_except' do
@@ -2308,92 +2302,95 @@ describe Gitlab::Git::Repository, seed_helper: true do
         expect { subject }.to raise_error(Gitlab::Git::CommandError, 'error')
       end
     end
+  end
 
-    describe '#squash' do
-      let(:squash_id) { '1' }
-      let(:branch_name) { 'fix' }
-      let(:start_sha) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
-      let(:end_sha) { '12d65c8dd2b2676fa3ac47d955accc085a37a9c1' }
+  describe '#squash' do
+    let(:squash_id) { '1' }
+    let(:branch_name) { 'fix' }
+    let(:start_sha) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
+    let(:end_sha) { '12d65c8dd2b2676fa3ac47d955accc085a37a9c1' }
 
-      subject do
-        opts = {
-          branch: branch_name,
-          start_sha: start_sha,
-          end_sha: end_sha,
-          author: user,
-          message: 'Squash commit message'
-        }
+    subject do
+      opts = {
+        branch: branch_name,
+        start_sha: start_sha,
+        end_sha: end_sha,
+        author: user,
+        message: 'Squash commit message'
+      }
 
-        repository.squash(user, squash_id, opts)
+      repository.squash(user, squash_id, opts)
+    end
+
+    # Should be ported to gitaly-ruby rspec suite https://gitlab.com/gitlab-org/gitaly/issues/1234
+    skip 'sparse checkout' do
+      let(:expected_files) { %w(files files/js files/js/application.js) }
+
+      it 'checks out only the files in the diff' do
+        allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args|
+          m.call(*args) do
+            worktree_path = args[0]
+            files_pattern = File.join(worktree_path, '**', '*')
+            expected = expected_files.map do |path|
+              File.expand_path(path, worktree_path)
+            end
+
+            expect(Dir[files_pattern]).to eq(expected)
+          end
+        end
+
+        subject
       end
 
-      context 'sparse checkout', :skip_gitaly_mock do
-        let(:expected_files) { %w(files files/js files/js/application.js) }
+      context 'when the diff contains a rename' do
+        let(:repo) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged }
+        let(:end_sha) { new_commit_move_file(repo).oid }
 
-        it 'checks out only the files in the diff' do
+        after do
+          # Erase our commits so other tests get the original repo
+          repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
+          repo.references.update('refs/heads/master', SeedRepo::LastCommit::ID)
+        end
+
+        it 'does not include the renamed file in the sparse checkout' do
           allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args|
             m.call(*args) do
               worktree_path = args[0]
               files_pattern = File.join(worktree_path, '**', '*')
-              expected = expected_files.map do |path|
-                File.expand_path(path, worktree_path)
-              end
 
-              expect(Dir[files_pattern]).to eq(expected)
+              expect(Dir[files_pattern]).not_to include('CHANGELOG')
+              expect(Dir[files_pattern]).not_to include('encoding/CHANGELOG')
             end
           end
 
           subject
         end
-
-        context 'when the diff contains a rename' do
-          let(:repo) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged }
-          let(:end_sha) { new_commit_move_file(repo).oid }
-
-          after do
-            # Erase our commits so other tests get the original repo
-            repo = Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '').rugged
-            repo.references.update('refs/heads/master', SeedRepo::LastCommit::ID)
-          end
-
-          it 'does not include the renamed file in the sparse checkout' do
-            allow(repository).to receive(:with_worktree).and_wrap_original do |m, *args|
-              m.call(*args) do
-                worktree_path = args[0]
-                files_pattern = File.join(worktree_path, '**', '*')
-
-                expect(Dir[files_pattern]).not_to include('CHANGELOG')
-                expect(Dir[files_pattern]).not_to include('encoding/CHANGELOG')
-              end
-            end
-
-            subject
-          end
-        end
       end
+    end
 
-      context 'with an ASCII-8BIT diff', :skip_gitaly_mock do
-        let(:diff) { "diff --git a/README.md b/README.md\nindex faaf198..43c5edf 100644\n--- a/README.md\n+++ b/README.md\n@@ -1,4 +1,4 @@\n-testme\n+✓ testme\n ======\n \n Sample repo for testing gitlab features\n" }
+    # Should be ported to gitaly-ruby rspec suite https://gitlab.com/gitlab-org/gitaly/issues/1234
+    skip 'with an ASCII-8BIT diff' do
+      let(:diff) { "diff --git a/README.md b/README.md\nindex faaf198..43c5edf 100644\n--- a/README.md\n+++ b/README.md\n@@ -1,4 +1,4 @@\n-testme\n+✓ testme\n ======\n \n Sample repo for testing gitlab features\n" }
 
-        it 'applies a ASCII-8BIT diff' do
-          allow(repository).to receive(:run_git!).and_call_original
-          allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT'))
+      it 'applies a ASCII-8BIT diff' do
+        allow(repository).to receive(:run_git!).and_call_original
+        allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT'))
 
-          expect(subject).to match(/\h{40}/)
-        end
+        expect(subject).to match(/\h{40}/)
       end
+    end
 
-      context 'with trailing whitespace in an invalid patch', :skip_gitaly_mock do
-        let(:diff) { "diff --git a/README.md b/README.md\nindex faaf198..43c5edf 100644\n--- a/README.md\n+++ b/README.md\n@@ -1,4 +1,4 @@\n-testme\n+   \n ======   \n \n Sample repo for testing gitlab features\n" }
+    # Should be ported to gitaly-ruby rspec suite https://gitlab.com/gitlab-org/gitaly/issues/1234
+    skip 'with trailing whitespace in an invalid patch' do
+      let(:diff) { "diff --git a/README.md b/README.md\nindex faaf198..43c5edf 100644\n--- a/README.md\n+++ b/README.md\n@@ -1,4 +1,4 @@\n-testme\n+   \n ======   \n \n Sample repo for testing gitlab features\n" }
 
-        it 'does not include whitespace warnings in the error' do
-          allow(repository).to receive(:run_git!).and_call_original
-          allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT'))
+      it 'does not include whitespace warnings in the error' do
+        allow(repository).to receive(:run_git!).and_call_original
+        allow(repository).to receive(:run_git!).with(%W(diff --binary #{start_sha}...#{end_sha})).and_return(diff.force_encoding('ASCII-8BIT'))
 
-          expect { subject }.to raise_error do |error|
-            expect(error).to be_a(described_class::GitError)
-            expect(error.message).not_to include('trailing whitespace')
-          end
+        expect { subject }.to raise_error do |error|
+          expect(error).to be_a(described_class::GitError)
+          expect(error.message).not_to include('trailing whitespace')
         end
       end
     end

spec/models/repository_spec.rb

@@ -1861,155 +1861,61 @@ describe Repository do
   describe '#add_tag' do
     let(:user) { build_stubbed(:user) }
 
-    shared_examples 'adding tag' do
-      context 'with a valid target' do
-        it 'creates the tag' do
-          repository.add_tag(user, '8.5', 'master', 'foo')
-
-          tag = repository.find_tag('8.5')
-          expect(tag).to be_present
-          expect(tag.message).to eq('foo')
-          expect(tag.dereferenced_target.id).to eq(repository.commit('master').id)
-        end
-
-        it 'returns a Gitlab::Git::Tag object' do
-          tag = repository.add_tag(user, '8.5', 'master', 'foo')
-
-          expect(tag).to be_a(Gitlab::Git::Tag)
-        end
-      end
+    context 'with a valid target' do
+      it 'creates the tag' do
+        repository.add_tag(user, '8.5', 'master', 'foo')
 
-      context 'with an invalid target' do
-        it 'returns false' do
-          expect(repository.add_tag(user, '8.5', 'bar', 'foo')).to be false
-        end
+        tag = repository.find_tag('8.5')
+        expect(tag).to be_present
+        expect(tag.message).to eq('foo')
+        expect(tag.dereferenced_target.id).to eq(repository.commit('master').id)
       end
-    end
-
-    context 'when Gitaly operation_user_add_tag feature is enabled' do
-      it_behaves_like 'adding tag'
-    end
-
-    context 'when Gitaly operation_user_add_tag feature is disabled', :disable_gitaly do
-      it_behaves_like 'adding tag'
-
-      it 'passes commit SHA to pre-receive and update hooks and tag SHA to post-receive hook' do
-        pre_receive_hook = Gitlab::Git::Hook.new('pre-receive', project)
-        update_hook = Gitlab::Git::Hook.new('update', project)
-        post_receive_hook = Gitlab::Git::Hook.new('post-receive', project)
-
-        allow(Gitlab::Git::Hook).to receive(:new)
-          .and_return(pre_receive_hook, update_hook, post_receive_hook)
-
-        allow(pre_receive_hook).to receive(:trigger).and_call_original
-        allow(update_hook).to receive(:trigger).and_call_original
-        allow(post_receive_hook).to receive(:trigger).and_call_original
 
+      it 'returns a Gitlab::Git::Tag object' do
         tag = repository.add_tag(user, '8.5', 'master', 'foo')
 
-        commit_sha = repository.commit('master').id
-        tag_sha = tag.target
+        expect(tag).to be_a(Gitlab::Git::Tag)
+      end
+    end
 
-        expect(pre_receive_hook).to have_received(:trigger)
-          .with(anything, anything, anything, commit_sha, anything)
-        expect(update_hook).to have_received(:trigger)
-          .with(anything, anything, anything, commit_sha, anything)
-        expect(post_receive_hook).to have_received(:trigger)
-          .with(anything, anything, anything, tag_sha, anything)
+    context 'with an invalid target' do
+      it 'returns false' do
+        expect(repository.add_tag(user, '8.5', 'bar', 'foo')).to be false
       end
     end
   end
 
   describe '#rm_branch' do
-    shared_examples "user deleting a branch" do
-      it 'removes a branch' do
-        expect(repository).to receive(:before_remove_branch)
-        expect(repository).to receive(:after_remove_branch)
+    it 'removes a branch' do
+      expect(repository).to receive(:before_remove_branch)
+      expect(repository).to receive(:after_remove_branch)
 
-        repository.rm_branch(user, 'feature')
-      end
+      repository.rm_branch(user, 'feature')
     end
 
-    context 'with gitaly enabled' do
-      it_behaves_like "user deleting a branch"
-
-      context 'when pre hooks failed' do
-        before do
-          allow_any_instance_of(Gitlab::GitalyClient::OperationService)
-            .to receive(:user_delete_branch).and_raise(Gitlab::Git::PreReceiveError)
-        end
-
-        it 'gets an error and does not delete the branch' do
-          expect do
-            repository.rm_branch(user, 'feature')
-          end.to raise_error(Gitlab::Git::PreReceiveError)
-
-          expect(repository.find_branch('feature')).not_to be_nil
-        end
-      end
-    end
-
-    context 'with gitaly disabled', :disable_gitaly do
-      it_behaves_like "user deleting a branch"
-
-      let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature
-      let(:blank_sha) { '0000000000000000000000000000000000000000' }
-
-      context 'when pre hooks were successful' do
-        it 'runs without errors' do
-          expect_any_instance_of(Gitlab::Git::HooksService).to receive(:execute)
-            .with(git_user, repository.raw_repository, old_rev, blank_sha, 'refs/heads/feature')
-
-          expect { repository.rm_branch(user, 'feature') }.not_to raise_error
-        end
-
-        it 'deletes the branch' do
-          allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil])
-
-          expect { repository.rm_branch(user, 'feature') }.not_to raise_error
-
-          expect(repository.find_branch('feature')).to be_nil
-        end
+    context 'when pre hooks failed' do
+      before do
+        allow_any_instance_of(Gitlab::GitalyClient::OperationService)
+          .to receive(:user_delete_branch).and_raise(Gitlab::Git::PreReceiveError)
       end
 
-      context 'when pre hooks failed' do
-        it 'gets an error' do
-          allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
-
-          expect do
-            repository.rm_branch(user, 'feature')
-          end.to raise_error(Gitlab::Git::PreReceiveError)
-        end
-
-        it 'does not delete the branch' do
-          allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([false, ''])
+      it 'gets an error and does not delete the branch' do
+        expect do
+          repository.rm_branch(user, 'feature')
+        end.to raise_error(Gitlab::Git::PreReceiveError)
 
-          expect do
-            repository.rm_branch(user, 'feature')
-          end.to raise_error(Gitlab::Git::PreReceiveError)
-          expect(repository.find_branch('feature')).not_to be_nil
-        end
+        expect(repository.find_branch('feature')).not_to be_nil
       end
     end
   end
 
   describe '#rm_tag' do
-    shared_examples 'removing tag' do
-      it 'removes a tag' do
-        expect(repository).to receive(:before_remove_tag)
+    it 'removes a tag' do
+      expect(repository).to receive(:before_remove_tag)
 
-        repository.rm_tag(build_stubbed(:user), 'v1.1.0')
-
-        expect(repository.find_tag('v1.1.0')).to be_nil
-      end
-    end
-
-    context 'when Gitaly operation_user_delete_tag feature is enabled' do
-      it_behaves_like 'removing tag'
-    end
+      repository.rm_tag(build_stubbed(:user), 'v1.1.0')
 
-    context 'when Gitaly operation_user_delete_tag feature is disabled', :skip_gitaly_mock do
-      it_behaves_like 'removing tag'
+      expect(repository.find_tag('v1.1.0')).to be_nil
     end
   end
 

spec/services/files/update_service_spec.rb

@@ -71,17 +71,5 @@ describe Files::UpdateService do
         expect(results.data).to eq(new_contents)
       end
     end
-
-    context 'with gitaly disabled', :skip_gitaly_mock do
-      context 'when target branch is different than source branch' do
-        let(:branch_name) { "#{project.default_branch}-new" }
-
-        it 'fires hooks only once' do
-          expect(Gitlab::Git::HooksService).to receive(:new).once.and_call_original
-
-          subject.execute
-        end
-      end
-    end
   end
 end

spec/services/merge_requests/rebase_service_spec.rb

@@ -36,9 +36,9 @@ describe MergeRequests::RebaseService do
       end
     end
 
-    context 'when unexpected error occurs', :disable_gitaly do
+    context 'when unexpected error occurs' do
       before do
-        allow(repository).to receive(:run_git!).and_raise('Something went wrong')
+        allow(repository).to receive(:gitaly_operation_client).and_raise('Something went wrong')
       end
 
       it 'saves a generic error message' do
@@ -53,9 +53,9 @@ describe MergeRequests::RebaseService do
       end
     end
 
-    context 'with git command failure', :disable_gitaly do
+    context 'with git command failure' do
       before do
-        allow(repository).to receive(:run_git!).and_raise(Gitlab::Git::Repository::GitError, 'Something went wrong')
+        allow(repository).to receive(:gitaly_operation_client).and_raise(Gitlab::Git::Repository::GitError, 'Something went wrong')
       end
 
       it 'saves a generic error message' do
@@ -71,7 +71,7 @@ describe MergeRequests::RebaseService do
     end
 
     context 'valid params' do
-      shared_examples 'successful rebase' do
+      describe 'successful rebase' do
         before do
           service.execute(merge_request)
         end
@@ -97,26 +97,8 @@ describe MergeRequests::RebaseService do
         end
       end
 
-      context 'when Gitaly rebase feature is enabled' do
-        it_behaves_like 'successful rebase'
-      end
-
-      context 'when Gitaly rebase feature is disabled', :disable_gitaly do
-        it_behaves_like 'successful rebase'
-      end
-
-      context 'git commands', :disable_gitaly do
-        it 'sets GL_REPOSITORY env variable when calling git commands' do
-          expect(repository).to receive(:popen).exactly(3)
-            .with(anything, anything, hash_including('GL_REPOSITORY'), anything)
-            .and_return(['', 0])
-
-          service.execute(merge_request)
-        end
-      end
-
       context 'fork' do
-        shared_examples 'successful fork rebase' do
+        describe 'successful fork rebase' do
           let(:forked_project) do
             fork_project(project, user, repository: true)
           end
@@ -140,14 +122,6 @@ describe MergeRequests::RebaseService do
             expect(parent_sha).to eq(target_branch_sha)
           end
         end
-
-        context 'when Gitaly rebase feature is enabled' do
-          it_behaves_like 'successful fork rebase'
-        end
-
-        context 'when Gitaly rebase feature is disabled', :disable_gitaly do
-          it_behaves_like 'successful fork rebase'
-        end
       end
     end
   end

spec/services/merge_requests/squash_service_spec.rb

@@ -124,51 +124,6 @@ describe MergeRequests::SquashService do
                                                           message: a_string_including('squash'))
         end
       end
-
-      context 'with Gitaly disabled', :skip_gitaly_mock do
-        stages = {
-          'add worktree for squash' => 'worktree',
-          'configure sparse checkout' => 'config',
-          'get files in diff' => 'diff --name-only',
-          'check out target branch' => 'checkout',
-          'apply patch' => 'diff --binary',
-          'commit squashed changes' => 'commit',
-          'get SHA of squashed commit' => 'rev-parse'
-        }
-
-        stages.each do |stage, command|
-          context "when the #{stage} stage fails" do
-            before do
-              git_command = a_collection_containing_exactly(
-                a_string_starting_with("#{Gitlab.config.git.bin_path} #{command}")
-              ).or(
-                a_collection_starting_with([Gitlab.config.git.bin_path] + command.split)
-              )
-
-              allow(repository).to receive(:popen).and_return(['', 0])
-              allow(repository).to receive(:popen).with(git_command, anything, anything, anything).and_return([error, 1])
-            end
-
-            it 'logs the stage and output' do
-              expect(service).to receive(:log_error).with(log_error)
-              expect(service).to receive(:log_error).with(error)
-
-              service.execute(merge_request)
-            end
-
-            it 'returns an error' do
-              expect(service.execute(merge_request)).to match(status: :error,
-                                                              message: a_string_including('squash'))
-            end
-
-            it 'cleans up the temporary directory' do
-              expect(File.exist?(squash_dir_path)).to be(false)
-
-              service.execute(merge_request)
-            end
-          end
-        end
-      end
     end
 
     context 'when any other exception is thrown' do