Merge branch 'gitaly-ff-branch-nil' into 'master'

Add missing Gitaly branch_update nil checks

Closes #49263

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

4 files changed, 141 insertions, 11 deletions

diff --git a/changelogs/unreleased/gitaly-ff-branch-nil.yml b/changelogs/unreleased/gitaly-ff-branch-nil.yml
new file mode 100644
index 00000000000..e7e689e6053
--- /dev/null
+++ b/changelogs/unreleased/gitaly-ff-branch-nil.yml
@@ -0,0 +1,5 @@
+---
+title: Add missing Gitaly branch_update nil checks
+merge_request: 20711
+author:
+type: fixed
diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb
index 280def182d5..57d748343be 100644
--- a/lib/gitlab/git/operation_service.rb
+++ b/lib/gitlab/git/operation_service.rb
@@ -8,6 +8,8 @@ module Gitlab
         alias_method :branch_created?, :branch_created
 
         def self.from_gitaly(branch_update)
+          return if branch_update.nil?
+
           new(
             branch_update.commit_id,
             branch_update.repo_created,
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index ab2c61f6782..88391de7078 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -128,13 +128,14 @@ module Gitlab
           branch: encode_binary(target_branch)
         )
 
-        branch_update = GitalyClient.call(
+        response = GitalyClient.call(
           @repository.storage,
           :operation_service,
           :user_ff_branch,
           request
-        ).branch_update
-        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(branch_update)
+        )
+
+        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
       rescue GRPC::FailedPrecondition => e
         raise Gitlab::Git::CommitError, e
       end
@@ -290,9 +291,9 @@ module Gitlab
           raise Gitlab::Git::CommitError, response.commit_error
         elsif response.create_tree_error.presence
           raise Gitlab::Git::Repository::CreateTreeError, response.create_tree_error
-        else
-          Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
         end
+
+        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
       end
 
       def user_commit_files_request_header(
diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
index 9709f1f5646..a82be474e4e 100644
--- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb
@@ -1,10 +1,10 @@
 require 'spec_helper'
 
 describe Gitlab::GitalyClient::OperationService do
-  let(:project) { create(:project) }
+  set(:project) { create(:project, :repository) }
   let(:repository) { project.repository.raw }
   let(:client) { described_class.new(repository) }
-  let(:user) { create(:user) }
+  set(:user) { create(:user) }
   let(:gitaly_user) { Gitlab::Git::User.from_gitlab(user).to_gitaly }
 
   describe '#user_create_branch' do
@@ -110,18 +110,104 @@ describe Gitlab::GitalyClient::OperationService do
     end
     let(:response) { Gitaly::UserFFBranchResponse.new(branch_update: branch_update) }
 
-    subject { client.user_ff_branch(user, source_sha, target_branch) }
-
-    it 'sends a user_ff_branch message and returns a BranchUpdate object' do
+    before do
       expect_any_instance_of(Gitaly::OperationService::Stub)
         .to receive(:user_ff_branch).with(request, kind_of(Hash))
         .and_return(response)
+    end
 
+    subject { client.user_ff_branch(user, source_sha, target_branch) }
+
+    it 'sends a user_ff_branch message and returns a BranchUpdate object' do
       expect(subject).to be_a(Gitlab::Git::OperationService::BranchUpdate)
       expect(subject.newrev).to eq(source_sha)
       expect(subject.repo_created).to be(false)
       expect(subject.branch_created).to be(false)
     end
+
+    context 'when the response has no branch_update' do
+      let(:response) { Gitaly::UserFFBranchResponse.new }
+
+      it { expect(subject).to be_nil }
+    end
+  end
+
+  shared_examples 'cherry pick and revert errors' do
+    context 'when a pre_receive_error is present' do
+      let(:response) { response_class.new(pre_receive_error: "something failed") }
+
+      it 'raises a PreReceiveError' do
+        expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
+      end
+    end
+
+    context 'when a commit_error is present' do
+      let(:response) { response_class.new(commit_error: "something failed") }
+
+      it 'raises a CommitError' do
+        expect { subject }.to raise_error(Gitlab::Git::CommitError, "something failed")
+      end
+    end
+
+    context 'when a create_tree_error is present' do
+      let(:response) { response_class.new(create_tree_error: "something failed") }
+
+      it 'raises a CreateTreeError' do
+        expect { subject }.to raise_error(Gitlab::Git::Repository::CreateTreeError, "something failed")
+      end
+    end
+
+    context 'when branch_update is nil' do
+      let(:response) { response_class.new }
+
+      it { expect(subject).to be_nil }
+    end
+  end
+
+  describe '#user_cherry_pick' do
+    let(:response_class) { Gitaly::UserCherryPickResponse }
+
+    subject do
+      client.user_cherry_pick(
+        user: user,
+        commit: repository.commit,
+        branch_name: 'master',
+        message: 'Cherry-pick message',
+        start_branch_name: 'master',
+        start_repository: repository
+      )
+    end
+
+    before do
+      expect_any_instance_of(Gitaly::OperationService::Stub)
+        .to receive(:user_cherry_pick).with(kind_of(Gitaly::UserCherryPickRequest), kind_of(Hash))
+        .and_return(response)
+    end
+
+    it_behaves_like 'cherry pick and revert errors'
+  end
+
+  describe '#user_revert' do
+    let(:response_class) { Gitaly::UserRevertResponse }
+
+    subject do
+      client.user_revert(
+        user: user,
+        commit: repository.commit,
+        branch_name: 'master',
+        message: 'Revert message',
+        start_branch_name: 'master',
+        start_repository: repository
+      )
+    end
+
+    before do
+      expect_any_instance_of(Gitaly::OperationService::Stub)
+        .to receive(:user_revert).with(kind_of(Gitaly::UserRevertRequest), kind_of(Hash))
+        .and_return(response)
+    end
+
+    it_behaves_like 'cherry pick and revert errors'
   end
 
   describe '#user_squash' do
@@ -162,7 +248,7 @@ describe Gitlab::GitalyClient::OperationService do
         Gitaly::UserSquashResponse.new(git_error: "something failed")
       end
 
-      it "throws a PreReceive exception" do
+      it "raises a GitError exception" do
         expect_any_instance_of(Gitaly::OperationService::Stub)
           .to receive(:user_squash).with(request, kind_of(Hash))
           .and_return(response)
@@ -171,5 +257,41 @@ describe Gitlab::GitalyClient::OperationService do
           Gitlab::Git::Repository::GitError, "something failed")
       end
     end
+
+    describe '#user_commit_files' do
+      subject do
+        client.user_commit_files(
+          gitaly_user, 'my-branch', 'Commit files message', [], 'janedoe@example.com', 'Jane Doe',
+          'master', repository)
+      end
+
+      before do
+        expect_any_instance_of(Gitaly::OperationService::Stub)
+          .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash))
+          .and_return(response)
+      end
+
+      context 'when a pre_receive_error is present' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "something failed") }
+
+        it 'raises a PreReceiveError' do
+          expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
+        end
+      end
+
+      context 'when an index_error is present' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new(index_error: "something failed") }
+
+        it 'raises a PreReceiveError' do
+          expect { subject }.to raise_error(Gitlab::Git::Index::IndexError, "something failed")
+        end
+      end
+
+      context 'when branch_update is nil' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new }
+
+        it { expect(subject).to be_nil }
+      end
+    end
   end
 end