Merge branch 'master' into sh-support-bitbucket-server-import

GITALY_SERVER_VERSION

-0.112.0
+0.113.0

Gemfile

@@ -418,7 +418,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.105.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.106.0', require: 'gitaly'
 gem 'grpc', '~> 1.11.0'
 
 # Locked until https://github.com/google/protobuf/issues/4210 is closed

Gemfile.lock

@@ -282,7 +282,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.105.0)
+    gitaly-proto (0.106.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.10)
     github-linguist (5.3.3)
@@ -1037,7 +1037,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.105.0)
+  gitaly-proto (~> 0.106.0)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-gollum-lib (~> 4.2)

Gemfile.rails5.lock

@@ -285,7 +285,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.105.0)
+    gitaly-proto (0.106.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.10)
     github-linguist (5.3.3)
@@ -1047,7 +1047,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.105.0)
+  gitaly-proto (~> 0.106.0)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-gollum-lib (~> 4.2)

app/workers/emails_on_push_worker.rb

@@ -51,7 +51,7 @@ class EmailsOnPushWorker
       end
     end
 
-    recipients.split.each do |recipient|
+    valid_recipients(recipients).each do |recipient|
       begin
         send_email(
           recipient,
@@ -89,4 +89,10 @@ class EmailsOnPushWorker
     email.header[:skip_premailer] = true if skip_premailer
     email.deliver_now
   end
+
+  def valid_recipients(recipients)
+    recipients.split.select do |recipient|
+      recipient.include?('@')
+    end
+  end
 end

changelogs/unreleased/accept-rf3-2822-compliant-addresses.yml

+---
+title: Emails on push recipients now accepts formats like John Doe <johndoe@example.com>
+merge_request:
+author: George Thomas
+type: added

doc/user/project/integrations/emails_on_push.md

@@ -6,7 +6,7 @@ every change that is pushed to your project.
 Navigate to the [Integrations page](project_services.md#accessing-the-project-services)
 and select the **Emails on push** service to configure it.
 
-In the _Recipients_ area, provide a list of emails separated by commas.
+In the _Recipients_ area, provide a list of emails separated by spaces or newlines.
 
 You can configure any of the following settings depending on your preference.
 

lib/gitlab/git/repository_mirroring.rb

@@ -17,33 +17,19 @@ module Gitlab
         rugged.config["remote.#{remote_name}.prune"] = true
       end
 
-      def remote_tags(remote)
-        # Each line has this format: "dc872e9fa6963f8f03da6c8f6f264d0845d6b092\trefs/tags/v1.10.0\n"
-        # We want to convert it to: [{ 'v1.10.0' => 'dc872e9fa6963f8f03da6c8f6f264d0845d6b092' }, ...]
-        list_remote_tags(remote).map do |line|
-          target, path = line.strip.split("\t")
-
-          # When the remote repo does not have tags.
-          if target.nil? || path.nil?
-            Rails.logger.info "Empty or invalid list of tags for remote: #{remote}. Output: #{output}"
-            break []
+      def remote_branches(remote_name)
+        gitaly_migrate(:ref_find_all_remote_branches) do |is_enabled|
+          if is_enabled
+            gitaly_ref_client.remote_branches(remote_name)
+          else
+            rugged_remote_branches(remote_name)
           end
-
-          name = path.split('/', 3).last
-          # We're only interested in tag references
-          # See: http://stackoverflow.com/questions/15472107/when-listing-git-ls-remote-why-theres-after-the-tag-name
-          next if name =~ /\^\{\}\Z/
-
-          target_commit = Gitlab::Git::Commit.find(self, target)
-          Gitlab::Git::Tag.new(self, {
-            name: name,
-            target: target,
-            target_commit: target_commit
-          })
-        end.compact
+        end
       end
 
-      def remote_branches(remote_name)
+      private
+
+      def rugged_remote_branches(remote_name)
         branches = []
 
         rugged.references.each("refs/remotes/#{remote_name}/*").map do |ref|
@@ -60,8 +46,6 @@ module Gitlab
         branches
       end
 
-      private
-
       def set_remote_refmap(remote_name, refmap)
         Array(refmap).each_with_index do |refspec, i|
           refspec = REFMAPS[refspec] || refspec
@@ -75,21 +59,6 @@ module Gitlab
           end
         end
       end
-
-      def list_remote_tags(remote)
-        tag_list, exit_code, error = nil
-        cmd = %W(#{Gitlab.config.git.bin_path} --git-dir=#{path} ls-remote --tags #{remote})
-
-        Open3.popen3(*cmd) do |stdin, stdout, stderr, wait_thr|
-          tag_list  = stdout.read
-          error     = stderr.read
-          exit_code = wait_thr.value.exitstatus
-        end
-
-        raise RemoteError, error unless exit_code.zero?
-
-        tag_list.split("\n")
-      end
     end
   end
 end

lib/gitlab/gitaly_client/ref_service.rb

@@ -17,6 +17,13 @@ module Gitlab
         consume_find_all_branches_response(response)
       end
 
+      def remote_branches(remote_name)
+        request = Gitaly::FindAllRemoteBranchesRequest.new(repository: @gitaly_repo, remote_name: remote_name)
+        response = GitalyClient.call(@repository.storage, :ref_service, :find_all_remote_branches, request)
+
+        consume_find_all_remote_branches_response(remote_name, response)
+      end
+
       def merged_branches(branch_names = [])
         request = Gitaly::FindAllBranchesRequest.new(
           repository: @gitaly_repo,
@@ -243,6 +250,18 @@ module Gitlab
         end
       end
 
+      def consume_find_all_remote_branches_response(remote_name, response)
+        remote_name += '/' unless remote_name.ends_with?('/')
+
+        response.flat_map do |message|
+          message.branches.map do |branch|
+            target_commit = Gitlab::Git::Commit.decorate(@repository, branch.target_commit)
+            branch_name = branch.name.sub(remote_name, '')
+            Gitlab::Git::Branch.new(@repository, branch_name, branch.target_commit.id, target_commit)
+          end
+        end
+      end
+
       def consume_tags_response(response)
         response.flat_map do |message|
           message.tags.map { |gitaly_tag| Gitlab::Git::Tag.new(@repository, gitaly_tag) }

spec/lib/gitlab/git/repository_spec.rb

@@ -602,40 +602,6 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
-  describe '#remote_tags' do
-    let(:remote_name) { 'upstream' }
-    let(:target_commit_id) { SeedRepo::Commit::ID }
-    let(:tag_name) { 'v0.0.1' }
-    let(:tag_message) { 'My tag' }
-    let(:remote_repository) do
-      Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
-    end
-
-    around do |example|
-      Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-        example.run
-      end
-    end
-
-    subject { repository.remote_tags(remote_name) }
-
-    before do
-      remote_repository_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { remote_repository.path }
-      repository.add_remote(remote_name, remote_repository_path)
-      remote_repository.add_tag(tag_name, user: user, target: target_commit_id)
-    end
-
-    after do
-      ensure_seeds
-    end
-
-    it 'gets the remote tags' do
-      expect(subject.first).to be_an_instance_of(Gitlab::Git::Tag)
-      expect(subject.first.name).to eq(tag_name)
-      expect(subject.first.dereferenced_target.id).to eq(target_commit_id)
-    end
-  end
-
   describe "#log" do
     shared_examples 'repository log' do
       let(:commit_with_old_name) do

spec/lib/gitlab/gitaly_client/ref_service_spec.rb

@@ -18,6 +18,44 @@ describe Gitlab::GitalyClient::RefService do
     end
   end
 
+  describe '#remote_branches' do
+    let(:remote_name) { 'my_remote' }
+    subject { client.remote_branches(remote_name) }
+
+    it 'sends a find_all_remote_branches message' do
+      expect_any_instance_of(Gitaly::RefService::Stub)
+        .to receive(:find_all_remote_branches)
+        .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
+        .and_return([])
+
+      subject
+    end
+
+    it 'concantes and returns the response branches as Gitlab::Git::Branch objects' do
+      target_commits = create_list(:gitaly_commit, 4)
+      response_branches = target_commits.each_with_index.map do |gitaly_commit, i|
+        Gitaly::Branch.new(name: "#{remote_name}/#{i}", target_commit: gitaly_commit)
+      end
+      response = [
+        Gitaly::FindAllRemoteBranchesResponse.new(branches: response_branches[0, 2]),
+        Gitaly::FindAllRemoteBranchesResponse.new(branches: response_branches[2, 2])
+      ]
+
+      expect_any_instance_of(Gitaly::RefService::Stub)
+        .to receive(:find_all_remote_branches).and_return(response)
+
+      expect(subject.length).to be(response_branches.length)
+
+      response_branches.each_with_index do |gitaly_branch, i|
+        branch = subject[i]
+        commit = Gitlab::Git::Commit.new(repository, gitaly_branch.target_commit)
+
+        expect(branch.name).to eq(i.to_s) # It removes the `remote/` prefix
+        expect(branch.dereferenced_target).to eq(commit)
+      end
+    end
+  end
+
   describe '#branch_names' do
     it 'sends a find_all_branch_names message' do
       expect_any_instance_of(Gitaly::RefService::Stub)

spec/models/repository_spec.rb

@@ -2225,20 +2225,6 @@ describe Repository do
     end
   end
 
-  describe '#remote_branches' do
-    it 'returns the remote branches' do
-      masterrev = repository.find_branch('master').dereferenced_target
-      create_remote_branch('joe', 'remote_branch', masterrev)
-      repository.add_branch(user, 'local_branch', masterrev.id)
-
-      # TODO: move this test to gitaly https://gitlab.com/gitlab-org/gitaly/issues/1243
-      Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-        expect(repository.remote_branches('joe').any? { |branch| branch.name == 'local_branch' }).to eq(false)
-        expect(repository.remote_branches('joe').any? { |branch| branch.name == 'remote_branch' }).to eq(true)
-      end
-    end
-  end
-
   describe '#commit_count' do
     context 'with a non-existing repository' do
       it 'returns 0' do

spec/workers/emails_on_push_worker_spec.rb

@@ -100,10 +100,6 @@ describe EmailsOnPushWorker, :mailer do
     end
 
     context "when there are multiple recipients" do
-      let(:recipients) do
-        1.upto(5).map { |i| user.email.sub('@', "+#{i}@") }.join("\n")
-      end
-
       before do
         # This is a hack because we modify the mail object before sending, for efficency,
         # but the TestMailer adapter just appends the objects to an array. To clone a mail
@@ -114,16 +110,57 @@ describe EmailsOnPushWorker, :mailer do
         end
       end
 
-      it "sends the mail to each of the recipients" do
-        perform
-        expect(ActionMailer::Base.deliveries.count).to eq(5)
-        expect(ActionMailer::Base.deliveries.map(&:to).flatten).to contain_exactly(*recipients.split)
+      context "when the recipient addresses are a list of email addresses" do
+        let(:recipients) do
+          1.upto(5).map { |i| user.email.sub('@', "+#{i}@") }.join("\n")
+        end
+
+        it "sends the mail to each of the recipients" do
+          perform
+
+          expect(ActionMailer::Base.deliveries.count).to eq(5)
+          expect(email_recipients).to contain_exactly(*recipients.split)
+        end
+
+        it "only generates the mail once" do
+          expect(Notify).to receive(:repository_push_email).once.and_call_original
+          expect(Premailer::Rails::CustomizedPremailer).to receive(:new).once.and_call_original
+
+          perform
+        end
       end
 
-      it "only generates the mail once" do
-        expect(Notify).to receive(:repository_push_email).once.and_call_original
-        expect(Premailer::Rails::CustomizedPremailer).to receive(:new).once.and_call_original
-        perform
+      context "when the recipient addresses contains angle brackets and are separated by spaces" do
+        let(:recipients) { "John Doe <johndoe@example.com> Jane Doe <janedoe@example.com>" }
+
+        it "accepts emails separated by whitespace" do
+          perform
+
+          expect(ActionMailer::Base.deliveries.count).to eq(2)
+          expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com")
+        end
+      end
+
+      context "when the recipient addresses contain a mix of emails with and without angle brackets" do
+        let(:recipients) { "johndoe@example.com Jane Doe <janedoe@example.com>" }
+
+        it "accepts both kind of emails" do
+          perform
+
+          expect(ActionMailer::Base.deliveries.count).to eq(2)
+          expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com")
+        end
+      end
+
+      context "when the recipient addresses contains angle brackets and are separated by newlines" do
+        let(:recipients) { "John Doe <johndoe@example.com>\nJane Doe <janedoe@example.com>" }
+
+        it "accepts emails separated by newlines" do
+          perform
+
+          expect(ActionMailer::Base.deliveries.count).to eq(2)
+          expect(email_recipients).to contain_exactly("johndoe@example.com", "janedoe@example.com")
+        end
       end
     end
   end