Merge branch 'gitaly-more-annotations' into 'master'

Branch and tag refactors for Gitaly

See merge request !12872

7 files changed, 67 insertions, 139 deletions

diff --git a/lib/gitlab/git/branch.rb b/lib/gitlab/git/branch.rb
index e2be9d784b9..c53882787f1 100644
--- a/lib/gitlab/git/branch.rb
+++ b/lib/gitlab/git/branch.rb
@@ -3,39 +3,8 @@
 module Gitlab
   module Git
     class Branch < Ref
-      def initialize(repository, name, target)
-        if target.is_a?(Gitaly::FindLocalBranchResponse)
-          target = target_from_gitaly_local_branches_response(target)
-        end
-
-        super(repository, name, target)
-      end
-
-      def target_from_gitaly_local_branches_response(response)
-        # Git messages have no encoding enforcements. However, in the UI we only
-        # handle UTF-8, so basically we cross our fingers that the message force
-        # encoded to UTF-8 is readable.
-        message = response.commit_subject.dup.force_encoding('UTF-8')
-
-        # NOTE: For ease of parsing in Gitaly, we have only the subject of
-        # the commit and not the full message. This is ok, since all the
-        # code that uses `local_branches` only cares at most about the
-        # commit message.
-        # TODO: Once gitaly "takes over" Rugged consider separating the
-        # subject from the message to make it clearer when there's one
-        # available but not the other.
-        hash = {
-          id: response.commit_id,
-          message: message,
-          authored_date: Time.at(response.commit_author.date.seconds),
-          author_name: response.commit_author.name,
-          author_email: response.commit_author.email,
-          committed_date: Time.at(response.commit_committer.date.seconds),
-          committer_name: response.commit_committer.name,
-          committer_email: response.commit_committer.email
-        }
-
-        Gitlab::Git::Commit.decorate(hash)
+      def initialize(repository, name, target, target_commit)
+        super(repository, name, target, target_commit)
       end
     end
   end
diff --git a/lib/gitlab/git/ref.rb b/lib/gitlab/git/ref.rb
index c1a10688285..372ce005b94 100644
--- a/lib/gitlab/git/ref.rb
+++ b/lib/gitlab/git/ref.rb
@@ -33,10 +33,9 @@ module Gitlab
         object
       end
 
-      def initialize(repository, name, target)
-        encode! name
-        @name = name.gsub(/\Arefs\/(tags|heads)\//, '')
-        @dereferenced_target = Gitlab::Git::Commit.find(repository, target)
+      def initialize(repository, name, target, derefenced_target)
+        @name = Gitlab::Git.ref_name(name)
+        @dereferenced_target = derefenced_target
         @target = if target.respond_to?(:oid)
                     target.oid
                   elsif target.respond_to?(:name)
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 639f5625d59..73fa42e3a8e 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -80,16 +80,10 @@ module Gitlab
       end
 
       # Returns an Array of Branches
-      def branches(filter: nil, sort_by: nil)
-        branches = rugged.branches.each(filter).map do |rugged_ref|
-          begin
-            Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target)
-          rescue Rugged::ReferenceError
-            # Omit invalid branch
-          end
-        end.compact
-
-        sort_branches(branches, sort_by)
+      #
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/389
+      def branches(sort_by: nil)
+        branches_filter(sort_by: sort_by)
       end
 
       def reload_rugged
@@ -107,7 +101,10 @@ module Gitlab
         reload_rugged if force_reload
 
         rugged_ref = rugged.branches[name]
-        Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target) if rugged_ref
+        if rugged_ref
+          target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
+          Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
+        end
       end
 
       def local_branches(sort_by: nil)
@@ -115,7 +112,7 @@ module Gitlab
           if is_enabled
             gitaly_ref_client.local_branches(sort_by: sort_by)
           else
-            branches(filter: :local, sort_by: sort_by)
+            branches_filter(filter: :local, sort_by: sort_by)
           end
         end
       end
@@ -162,6 +159,8 @@ module Gitlab
       end
 
       # Returns an Array of Tags
+      #
+      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/390
       def tags
         rugged.references.each("refs/tags/*").map do |ref|
           message = nil
@@ -174,7 +173,8 @@ module Gitlab
             end
           end
 
-          Gitlab::Git::Tag.new(self, ref.name, ref.target, message)
+          target_commit = Gitlab::Git::Commit.find(self, ref.target)
+          Gitlab::Git::Tag.new(self, ref.name, ref.target, target_commit, message)
         end.sort_by(&:name)
       end
 
@@ -204,13 +204,6 @@ module Gitlab
         branch_names + tag_names
       end
 
-      # Deprecated. Will be removed in 5.2
-      def heads
-        rugged.references.each("refs/heads/*").map do |head|
-          Gitlab::Git::Ref.new(self, head.name, head.target)
-        end.sort_by(&:name)
-      end
-
       def has_commits?
         !empty?
       end
@@ -707,7 +700,8 @@ module Gitlab
       #   create_branch("other-feature", "master")
       def create_branch(ref, start_point = "HEAD")
         rugged_ref = rugged.branches.create(ref, start_point)
-        Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target)
+        target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
+        Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
       rescue Rugged::ReferenceError => e
         raise InvalidRef.new("Branch #{ref} already exists") if e.to_s =~ /'refs\/heads\/#{ref}'/
         raise InvalidRef.new("Invalid reference #{start_point}")
@@ -837,6 +831,20 @@ module Gitlab
 
       private
 
+      # Gitaly note: JV: Trying to get rid of the 'filter' option so we can implement this with 'git'.
+      def branches_filter(filter: nil, sort_by: nil)
+        branches = rugged.branches.each(filter).map do |rugged_ref|
+          begin
+            target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
+            Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
+          rescue Rugged::ReferenceError
+            # Omit invalid branch
+          end
+        end.compact
+
+        sort_branches(branches, sort_by)
+      end
+
       def raw_log(options)
         default_options = {
           limit: 10,
diff --git a/lib/gitlab/git/tag.rb b/lib/gitlab/git/tag.rb
index 9a39de6ad07..bc4e160dce9 100644
--- a/lib/gitlab/git/tag.rb
+++ b/lib/gitlab/git/tag.rb
@@ -5,8 +5,8 @@ module Gitlab
     class Tag < Ref
       attr_reader :object_sha
 
-      def initialize(repository, name, target, message = nil)
-        super(repository, name, target)
+      def initialize(repository, name, target, target_commit, message = nil)
+        super(repository, name, target, target_commit)
 
         @message = message
       end
diff --git a/lib/gitlab/gitaly_client/ref.rb b/lib/gitlab/gitaly_client/ref.rb
index 6edc69de078..2c17b67d4b7 100644
--- a/lib/gitlab/gitaly_client/ref.rb
+++ b/lib/gitlab/gitaly_client/ref.rb
@@ -72,11 +72,39 @@ module Gitlab
             Gitlab::Git::Branch.new(
               @repository,
               encode!(gitaly_branch.name.dup),
-              gitaly_branch.commit_id
+              gitaly_branch.commit_id,
+              commit_from_local_branches_response(gitaly_branch)
             )
           end
         end
       end
+
+      def commit_from_local_branches_response(response)
+        # Git messages have no encoding enforcements. However, in the UI we only
+        # handle UTF-8, so basically we cross our fingers that the message force
+        # encoded to UTF-8 is readable.
+        message = response.commit_subject.dup.force_encoding('UTF-8')
+
+        # NOTE: For ease of parsing in Gitaly, we have only the subject of
+        # the commit and not the full message. This is ok, since all the
+        # code that uses `local_branches` only cares at most about the
+        # commit message.
+        # TODO: Once gitaly "takes over" Rugged consider separating the
+        # subject from the message to make it clearer when there's one
+        # available but not the other.
+        hash = {
+          id: response.commit_id,
+          message: message,
+          authored_date: Time.at(response.commit_author.date.seconds),
+          author_name: response.commit_author.name,
+          author_email: response.commit_author.email,
+          committed_date: Time.at(response.commit_committer.date.seconds),
+          committer_name: response.commit_committer.name,
+          committer_email: response.commit_committer.email
+        }
+
+        Gitlab::Git::Commit.decorate(hash)
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb
index d1d7ed1d02a..cdf1b8beee3 100644
--- a/spec/lib/gitlab/git/branch_spec.rb
+++ b/spec/lib/gitlab/git/branch_spec.rb
@@ -7,51 +7,6 @@ describe Gitlab::Git::Branch, seed_helper: true do
 
   it { is_expected.to be_kind_of Array }
 
-  describe 'initialize' do
-    let(:commit_id) { 'f00' }
-    let(:commit_subject) { "My commit".force_encoding('ASCII-8BIT') }
-    let(:committer) do
-      Gitaly::FindLocalBranchCommitAuthor.new(
-        name: generate(:name),
-        email: generate(:email),
-        date: Google::Protobuf::Timestamp.new(seconds: 123)
-      )
-    end
-    let(:author) do
-      Gitaly::FindLocalBranchCommitAuthor.new(
-        name: generate(:name),
-        email: generate(:email),
-        date: Google::Protobuf::Timestamp.new(seconds: 456)
-      )
-    end
-    let(:gitaly_branch) do
-      Gitaly::FindLocalBranchResponse.new(
-        name: 'foo', commit_id: commit_id, commit_subject: commit_subject,
-        commit_author: author, commit_committer: committer
-      )
-    end
-    let(:attributes) do
-      {
-        id: commit_id,
-        message: commit_subject,
-        authored_date: Time.at(author.date.seconds),
-        author_name: author.name,
-        author_email: author.email,
-        committed_date: Time.at(committer.date.seconds),
-        committer_name: committer.name,
-        committer_email: committer.email
-      }
-    end
-    let(:branch) { described_class.new(repository, 'foo', gitaly_branch) }
-
-    it 'parses Gitaly::FindLocalBranchResponse correctly' do
-      expect(Gitlab::Git::Commit).to receive(:decorate)
-        .with(hash_including(attributes)).and_call_original
-
-      expect(branch.dereferenced_target.message).to be_utf8
-    end
-  end
-
   describe '#size' do
     subject { super().size }
     it { is_expected.to eq(SeedRepo::Repo::BRANCHES.size) }
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 10fa5f4044b..f5893294e34 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -234,33 +234,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     it { expect(repository.bare?).to be_truthy }
   end
 
-  describe '#heads' do
-    let(:heads) { repository.heads }
-    subject { heads }
-
-    it { is_expected.to be_kind_of Array }
-
-    describe '#size' do
-      subject { super().size }
-      it { is_expected.to eq(SeedRepo::Repo::BRANCHES.size) }
-    end
-
-    context :head do
-      subject { heads.first }
-
-      describe '#name' do
-        subject { super().name }
-        it { is_expected.to eq("feature") }
-      end
-
-      context :commit do
-        subject { heads.first.dereferenced_target.sha }
-
-        it { is_expected.to eq("0b4bc9a49b562e85de7cc9e834518ea6828729b9") }
-      end
-    end
-  end
-
   describe '#ref_names' do
     let(:ref_names) { repository.ref_names }
     subject { ref_names }
@@ -521,7 +494,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
       end
 
       it "should refresh the repo's #heads collection" do
-        head_names = @normal_repo.heads.map { |h| h.name }
+        head_names = @normal_repo.branches.map { |h| h.name }
         expect(head_names).to include(new_branch)
       end
 
@@ -542,7 +515,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
             eq(normal_repo.rugged.branches["master"].target.oid)
           )
 
-          head_names = normal_repo.heads.map { |h| h.name }
+          head_names = normal_repo.branches.map { |h| h.name }
           expect(head_names).not_to include(new_branch)
         end
 
@@ -589,10 +562,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
       expect(@repo.rugged.branches["feature"]).to be_nil
     end
 
-    it "should update the repo's #heads collection" do
-      expect(@repo.heads).not_to include("feature")
-    end
-
     after(:all) do
       FileUtils.rm_rf(TEST_MUTABLE_REPO_PATH)
       ensure_seeds