Merge branch 'reenable-gitaly-ref-name' into 'master'

Re-enable gitaly migration for ref_name_for_sha after bugfixes

See merge request !11310

4 files changed, 53 insertions, 33 deletions

diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index d380c5021ee..a2c01ec4432 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -471,19 +471,19 @@ module Gitlab
 
       # Returns a RefName for a given SHA
       def ref_name_for_sha(ref_path, sha)
-        # NOTE: This feature is intentionally disabled until
-        # https://gitlab.com/gitlab-org/gitaly/issues/180 is resolved
-        # gitaly_migrate(:find_ref_name) do |is_enabled|
-        #   if is_enabled
-        #     gitaly_ref_client.find_ref_name(sha, ref_path)
-        #   else
-        args = %W(#{Gitlab.config.git.bin_path} for-each-ref --count=1 #{ref_path} --contains #{sha})
-
-        # Not found -> ["", 0]
-        # Found -> ["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0]
-        Gitlab::Popen.popen(args, @path).first.split.last
-        #   end
-        # end
+        raise ArgumentError, "sha can't be empty" unless sha.present?
+
+        gitaly_migrate(:find_ref_name) do |is_enabled|
+          if is_enabled
+            gitaly_ref_client.find_ref_name(sha, ref_path)
+          else
+            args = %W(#{Gitlab.config.git.bin_path} for-each-ref --count=1 #{ref_path} --contains #{sha})
+
+            # Not found -> ["", 0]
+            # Found -> ["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0]
+            Gitlab::Popen.popen(args, @path).first.split.last
+          end
+        end
       end
 
       # Returns commits collection
diff --git a/lib/gitlab/gitaly_client/ref.rb b/lib/gitlab/gitaly_client/ref.rb
index bf04e1fa50b..53c43e28df8 100644
--- a/lib/gitlab/gitaly_client/ref.rb
+++ b/lib/gitlab/gitaly_client/ref.rb
@@ -28,7 +28,7 @@ module Gitlab
 
       def find_ref_name(commit_id, ref_prefix)
         request = Gitaly::FindRefNameRequest.new(
-          repository: @repository,
+          repository: @gitaly_repo,
           commit_id: commit_id,
           prefix: ref_prefix
         )
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index fea186fd4f4..53d492b8f74 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -26,6 +26,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     context 'with gitaly enabled' do
       before { stub_gitaly }
+      after { Gitlab::GitalyClient.clear_stubs! }
 
       it 'gets the branch name from GitalyClient' do
         expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name)
@@ -120,6 +121,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     context 'with gitaly enabled' do
       before { stub_gitaly }
+      after { Gitlab::GitalyClient.clear_stubs! }
 
       it 'gets the branch names from GitalyClient' do
         expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names)
@@ -157,6 +159,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     context 'with gitaly enabled' do
       before { stub_gitaly }
+      after { Gitlab::GitalyClient.clear_stubs! }
 
       it 'gets the tag names from GitalyClient' do
         expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names)
@@ -1046,6 +1049,28 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe '#ref_name_for_sha' do
+    let(:ref_path) { 'refs/heads' }
+    let(:sha) { repository.find_branch('master').dereferenced_target.id }
+    let(:ref_name) { 'refs/heads/master' }
+
+    it 'returns the ref name for the given sha' do
+      expect(repository.ref_name_for_sha(ref_path, sha)).to eq(ref_name)
+    end
+
+    it "returns an empty name if the ref doesn't exist" do
+      expect(repository.ref_name_for_sha(ref_path, "000000")).to eq("")
+    end
+
+    it "raise an exception if the ref is empty" do
+      expect { repository.ref_name_for_sha(ref_path, "") }.to raise_error(ArgumentError)
+    end
+
+    it "raise an exception if the ref is nil" do
+      expect { repository.ref_name_for_sha(ref_path, nil) }.to raise_error(ArgumentError)
+    end
+  end
+
   describe '#find_commits' do
     it 'should return a return a collection of commits' do
       commits = repository.find_commits
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 61b748429d7..718b7d5e86b 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -110,22 +110,11 @@ describe Repository, models: true do
   end
 
   describe '#ref_name_for_sha' do
-    context 'ref found' do
-      it 'returns the ref' do
-        allow_any_instance_of(Gitlab::Popen).to receive(:popen).
-          and_return(["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0])
+    it 'returns the ref' do
+      allow(repository.raw_repository).to receive(:ref_name_for_sha).
+        and_return('refs/environments/production/77')
 
-        expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq 'refs/environments/production/77'
-      end
-    end
-
-    context 'ref not found' do
-      it 'returns nil' do
-        allow_any_instance_of(Gitlab::Popen).to receive(:popen).
-          and_return(["", 0])
-
-        expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq nil
-      end
+      expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq 'refs/environments/production/77'
     end
   end
 
@@ -1917,12 +1906,18 @@ describe Repository, models: true do
 
   describe '#is_ancestor?' do
     context 'Gitaly is_ancestor feature enabled' do
-      it "asks Gitaly server if it's an ancestor" do
-        commit = repository.commit
-        expect(repository.raw_repository).to receive(:is_ancestor?).and_call_original
+      let(:commit) { repository.commit }
+      let(:ancestor) { commit.parents.first }
+
+      before do
+        allow(Gitlab::GitalyClient).to receive(:enabled?).and_return(true)
         allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(:is_ancestor).and_return(true)
+      end
+
+      it "asks Gitaly server if it's an ancestor" do
+        expect_any_instance_of(Gitlab::GitalyClient::Commit).to receive(:is_ancestor).with(ancestor.id, commit.id)
 
-        expect(repository.is_ancestor?(commit.id, commit.id)).to be true
+        repository.is_ancestor?(ancestor.id, commit.id)
       end
     end
   end