Merge branch 'ce-to-ee-2018-06-21' into 'master'

CE upstream - 2018-06-21 12:22 UTC

Closes gitaly#1138

See merge request gitlab-org/gitlab-ee!6222

VERSION

-11.0.0-pre
+11.1.0-pre

app/controllers/projects/commits_controller.rb

@@ -9,6 +9,7 @@ class Projects::CommitsController < Projects::ApplicationController
   before_action :assign_ref_vars
   before_action :authorize_download_code!
   before_action :set_commits
+  before_action :set_request_format, only: :show
 
   def show
     @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened
@@ -61,6 +62,19 @@ class Projects::CommitsController < Projects::ApplicationController
     @commits = prepare_commits_for_rendering(@commits)
   end
 
+  # Rails 5 sets request.format from the extension.
+  # Explicitly set to :html.
+  def set_request_format
+    request.format = :html if set_request_format?
+  end
+
+  # Rails 5 sets request.format from extension.
+  # In this case if the ref ends with `.atom`, it's expected to be the html response,
+  # not the atom one. So explicitly set request.format as :html to act like rails4.
+  def set_request_format?
+    request.format.to_s == "text/html" || @commits.ref.ends_with?("atom")
+  end
+
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42330')
   end

app/models/label.rb

@@ -85,11 +85,16 @@ class Label < ActiveRecord::Base
       (#{Project.reference_pattern})?
       #{Regexp.escape(reference_prefix)}
       (?:
-        (?<label_id>\d+(?!\S\w)\b) | # Integer-based label ID, or
-        (?<label_name>
-          [A-Za-z0-9_\-\?\.&]+ | # String-based single-word label title, or
-          ".+?"                  # String-based multi-word label surrounded in quotes
-        )
+          (?<label_id>\d+(?!\S\w)\b)
+        | # Integer-based label ID, or
+          (?<label_name>
+              # String-based single-word label title, or
+              [A-Za-z0-9_\-\?\.&]+
+              (?<!\.|\?)
+            |
+              # String-based multi-word label surrounded in quotes
+              ".+?"
+          )
       )
     }x
   end

app/services/concerns/issues/resolve_discussions.rb

@@ -14,7 +14,6 @@ module Issues
     def merge_request_to_resolve_discussions_of
       strong_memoize(:merge_request_to_resolve_discussions_of) do
         MergeRequestsFinder.new(current_user, project_id: project.id)
-          .execute
           .find_by(iid: merge_request_to_resolve_discussions_of_iid)
       end
     end

changelogs/unreleased/blackst0ne-rails5-set-request-format-in--commits-controller.yml

+---
+title: "[Rails5] Set request.format in commits_controller"
+merge_request: 20023
+author: "@blackst0ne"
+type: fixed

changelogs/unreleased/dm-label-reference-period.yml

+---
+title: Properly detect label reference if followed by period or question mark
+merge_request:
+author:
+type: fixed

changelogs/unreleased/rails5-fix-48140.yml

+---
+title: 'Rails 5 fix Capybara::ElementNotFound: Unable to find visible css #modal-revert-commit
+  and expected: "/bar" got: "/foo"'
+merge_request: 20044
+author: Jasper Maes
+type: fixed

config/application.rb

@@ -76,7 +76,7 @@ module Gitlab
     # namespaces/users.
     # https://github.com/rails/rails/blob/5-0-stable/actioncable/lib/action_cable.rb#L38
     # Please change this value when configuring ActionCable for real usage.
-    config.action_cable.mount_path = "-" if rails5?
+    config.action_cable.mount_path = "/-/cable" if rails5?
 
     # Configure sensitive parameters which will be filtered from the log file.
     #

doc/development/documentation/styleguide.md

@@ -26,7 +26,11 @@ Check the GitLab handbook for the [writing styles guidelines](https://about.gitl
 - Jump a line between different markups (e.g., after every paragraph, header, list, etc)
 - Capitalize "G" and "L" in GitLab
 - Use sentence case for titles, headings, labels, menu items, and buttons.
-- Use title case when referring to [features](https://about.gitlab.com/features/) or [products](https://about.gitlab.com/pricing/), and methods. Note that some features are also objects (e.g. "Merge Requests" and "merge requests"). E.g.: GitLab Runner, Geo, Issue Boards, Git, Prometheus, Continuous Integration.
+- Use title case when referring to [features](https://about.gitlab.com/features/) or
+[products](https://about.gitlab.com/pricing/) (e.g., GitLab Runner, Geo,
+Issue Boards, GitLab Core, Git, Prometheus, Kubernetes, etc), and methods or methodologies
+(e.g., Continuous Integration, Continuous Deployment, Scrum, Agile, etc). Note that
+some features are also objects (e.g. "Merge Requests" and "merge requests").
 
 ## Formatting
 
@@ -72,7 +76,7 @@ For punctuation rules, please refer to the [GitLab UX guide](https://design.gitl
   This is to ensure that no document with wrong heading is going
   live without an audit, thus preventing dead links and redirection issues when
   corrected
-- Leave exactly one newline after a heading
+- Leave exactly one new line after a heading
 
 ## Links
 
@@ -95,6 +99,16 @@ For punctuation rules, please refer to the [GitLab UX guide](https://design.gitl
   E.g., instead of writing something like `Read more about GitLab Issue Boards [here](LINK)`,
   write `Read more about [GitLab Issue Boards](LINK)`.
 
+## Navigation
+
+To indicate the steps of navigation through the UI:
+
+- Use the exact word as shown in the UI, including any capital letters as-is
+- Use bold text for navigation items and the char `>` as separator
+(e.g., `Navigate to your project's **Settings > CI/CD**` )
+- If there are any expandable menus, make sure to mention that the user
+needs to expand the tab to find the settings you're referring to
+
 ## Images
 
 - Place images in a separate directory named `img/` in the same directory where

doc/development/fe_guide/vuex.md

@@ -78,7 +78,7 @@ In this file, we will write the actions that will call the respective mutations:
 
 ```javascript
   import * as types from './mutation_types';
-  import axios from '~/lib/utils/axios-utils';
+  import axios from '~/lib/utils/axios_utils';
   import createFlash from '~/flash';
 
   export const requestUsers = ({ commit }) => commit(types.REQUEST_USERS);
@@ -214,7 +214,7 @@ import { mapGetters } from 'vuex';
 };
 ```
 
-### `mutations_types.js`
+### `mutation_types.js`
 From [vuex mutations docs][vuex-mutations]:
 > It is a commonly seen pattern to use constants for mutation types in various Flux implementations. This allows the code to take advantage of tooling like linters, and putting all constants in a single file allows your collaborators to get an at-a-glance view of what mutations are possible in the entire application.
 

ee/app/controllers/projects/issue_links_controller.rb

@@ -22,7 +22,6 @@ module Projects
     def issue
       @issue ||=
         IssuesFinder.new(current_user, project_id: @project.id)
-                    .execute
                     .find_by!(iid: params[:issue_id])
     end
 

lib/gitlab/git/repository.rb

@@ -529,32 +529,17 @@ module Gitlab
       def raw_changes_between(old_rev, new_rev)
         @raw_changes_between ||= {}
 
-        @raw_changes_between[[old_rev, new_rev]] ||= begin
-          return [] if new_rev.blank? || new_rev == Gitlab::Git::BLANK_SHA
+        @raw_changes_between[[old_rev, new_rev]] ||=
+          begin
+            return [] if new_rev.blank? || new_rev == Gitlab::Git::BLANK_SHA
 
-          gitaly_migrate(:raw_changes_between) do |is_enabled|
-            if is_enabled
+            wrapped_gitaly_errors do
               gitaly_repository_client.raw_changes_between(old_rev, new_rev)
                 .each_with_object([]) do |msg, arr|
                 msg.raw_changes.each { |change| arr << ::Gitlab::Git::RawDiffChange.new(change) }
               end
-            else
-              result = []
-
-              circuit_breaker.perform do
-                Open3.pipeline_r(git_diff_cmd(old_rev, new_rev), format_git_cat_file_script, git_cat_file_cmd) do |last_stdout, wait_threads|
-                  last_stdout.each_line { |line| result << ::Gitlab::Git::RawDiffChange.new(line.chomp!) }
-
-                  if wait_threads.any? { |waiter| !waiter.value&.success? }
-                    raise ::Gitlab::Git::Repository::GitError, "Unabled to obtain changes between #{old_rev} and #{new_rev}"
-                  end
-                end
-              end
-
-              result
             end
           end
-        end
       rescue ArgumentError => e
         raise Gitlab::Git::Repository::GitError.new(e)
       end

rubocop/cop/gitlab/finder_with_find_by.rb

+module RuboCop
+  module Cop
+    module Gitlab
+      class FinderWithFindBy < RuboCop::Cop::Cop
+        FIND_PATTERN = /\Afind(_by\!?)?\z/
+        ALLOWED_MODULES = ['FinderMethods'].freeze
+
+        def message(used_method)
+          <<~MSG
+          Don't chain finders `#execute` method with `##{used_method}`.
+          Instead include `FinderMethods` in the Finder and call `##{used_method}`
+          directly on the finder instance.
+
+          This will make sure all authorization checks are performed on the resource.
+          MSG
+        end
+
+        def on_send(node)
+          if find_on_execute?(node) && !allowed_module?(node)
+            add_offense(node, location: :selector, message: message(node.method_name))
+          end
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            upto_including_execute = node.descendants.first.source_range
+            before_execute = node.descendants[1].source_range
+            range_to_remove = node.source_range
+                                .with(begin_pos: before_execute.end_pos,
+                                      end_pos: upto_including_execute.end_pos)
+
+            corrector.remove(range_to_remove)
+          end
+        end
+
+        def find_on_execute?(node)
+          chained_on_node = node.descendants.first
+          node.method_name.to_s =~ FIND_PATTERN &&
+            chained_on_node&.method_name == :execute
+        end
+
+        def allowed_module?(node)
+          ALLOWED_MODULES.include?(node.parent_module_name)
+        end
+
+        def method_name_for_node(node)
+          children[1].to_s
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -2,6 +2,7 @@
 require_relative 'cop/gitlab/module_with_instance_variables'
 require_relative 'cop/gitlab/predicate_memoization'
 require_relative 'cop/gitlab/httparty'
+require_relative 'cop/gitlab/finder_with_find_by'
 require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/avoid_return_from_blocks'
 require_relative 'cop/avoid_break_from_strong_memoize'

spec/features/profiles/account_spec.rb

@@ -67,4 +67,6 @@ def update_username(new_username)
   page.within('.modal') do
     find('.js-modal-primary-action').click
   end
+
+  wait_for_requests
 end

spec/features/projects/merge_requests/user_reverts_merge_request_spec.rb

@@ -13,6 +13,8 @@ describe 'User reverts a merge request', :js do
 
     click_button('Merge')
 
+    wait_for_requests
+
     visit(merge_request_path(merge_request))
   end
 

spec/lib/banzai/filter/label_reference_filter_spec.rb

@@ -148,9 +148,11 @@ describe Banzai::Filter::LabelReferenceFilter do
       expect(doc.text).to eq 'See ?g.fm&'
     end
 
-    it 'links with adjacent text' do
-      doc = reference_filter("Label (#{reference}).")
-      expect(doc.to_html).to match(%r(\(<a.+><span.+>\?g\.fm&amp;</span></a>\)\.))
+    it 'does not include trailing punctuation', :aggregate_failures do
+      ['.', ', ok?', '...', '?', '!', ': is that ok?'].each do |trailing_punctuation|
+        doc = filter("Label #{reference}#{trailing_punctuation}")
+        expect(doc.to_html).to match(%r(<a.+><span.+>\?g\.fm&amp;</span></a>#{Regexp.escape(trailing_punctuation)}))
+      end
     end
 
     it 'ignores invalid label names' do

spec/lib/gitlab/git/repository_spec.rb

@@ -1043,50 +1043,40 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#raw_changes_between' do
-    shared_examples 'raw changes' do
-      let(:old_rev) { }
-      let(:new_rev) { }
-      let(:changes) { repository.raw_changes_between(old_rev, new_rev) }
+    let(:old_rev) { }
+    let(:new_rev) { }
+    let(:changes) { repository.raw_changes_between(old_rev, new_rev) }
 
-      context 'initial commit' do
-        let(:old_rev) { Gitlab::Git::BLANK_SHA }
-        let(:new_rev) { '1a0b36b3cdad1d2ee32457c102a8c0b7056fa863' }
+    context 'initial commit' do
+      let(:old_rev) { Gitlab::Git::BLANK_SHA }
+      let(:new_rev) { '1a0b36b3cdad1d2ee32457c102a8c0b7056fa863' }
 
-        it 'returns the changes' do
-          expect(changes).to be_present
-          expect(changes.size).to eq(3)
-        end
+      it 'returns the changes' do
+        expect(changes).to be_present
+        expect(changes.size).to eq(3)
       end
+    end
 
-      context 'with an invalid rev' do
-        let(:old_rev) { 'foo' }
-        let(:new_rev) { 'bar' }
+    context 'with an invalid rev' do
+      let(:old_rev) { 'foo' }
+      let(:new_rev) { 'bar' }
 
-        it 'returns an error' do
-          expect { changes }.to raise_error(Gitlab::Git::Repository::GitError)
-        end
-      end
-
-      context 'with valid revs' do
-        let(:old_rev) { 'fa1b1e6c004a68b7d8763b86455da9e6b23e36d6' }
-        let(:new_rev) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
-
-        it 'returns the changes' do
-          expect(changes.size).to eq(9)
-          expect(changes.first.operation).to eq(:modified)
-          expect(changes.first.new_path).to eq('.gitmodules')
-          expect(changes.last.operation).to eq(:added)
-          expect(changes.last.new_path).to eq('files/lfs/picture-invalid.png')
-        end
+      it 'returns an error' do
+        expect { changes }.to raise_error(Gitlab::Git::Repository::GitError)
       end
     end
 
-    context 'when gitaly is enabled' do
-      it_behaves_like 'raw changes'
-    end
+    context 'with valid revs' do
+      let(:old_rev) { 'fa1b1e6c004a68b7d8763b86455da9e6b23e36d6' }
+      let(:new_rev) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
 
-    context 'when gitaly is disabled', :disable_gitaly do
-      it_behaves_like 'raw changes'
+      it 'returns the changes' do
+        expect(changes.size).to eq(9)
+        expect(changes.first.operation).to eq(:modified)
+        expect(changes.first.new_path).to eq('.gitmodules')
+        expect(changes.last.operation).to eq(:added)
+        expect(changes.last.new_path).to eq('files/lfs/picture-invalid.png')
+      end
     end
   end
 

spec/rubocop/cop/gitlab/finder_with_find_by_spec.rb

+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/gitlab/finder_with_find_by'
+
+describe RuboCop::Cop::Gitlab::FinderWithFindBy do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'when calling execute.find' do
+    let(:source) do
+      <<~SRC
+      DummyFinder.new(some_args)
+        .execute
+        .find_by!(1)
+      SRC
+    end
+    let(:corrected_source) do
+      <<~SRC
+      DummyFinder.new(some_args)
+        .find_by!(1)
+      SRC
+    end
+
+    it 'registers an offence' do
+      inspect_source(source)
+
+      expect(cop.offenses.size).to eq(1)
+    end
+
+    it 'can autocorrect the source' do
+      expect(autocorrect_source(source)).to eq(corrected_source)
+    end
+
+    context 'when called within the `FinderMethods` module' do
+      let(:source) do
+        <<~SRC
+          module FinderMethods
+            def find_by!(*args)
+              execute.find_by!(args)
+            end
+          end
+        SRC
+      end
+
+      it 'does not register an offence' do
+        inspect_source(source)
+
+        expect(cop.offenses).to be_empty
+      end
+    end
+  end
+end

spec/services/issues/resolve_discussions_spec.rb

@@ -31,10 +31,8 @@ describe Issues::ResolveDiscussions do
 
       it "only queries for the merge request once" do
         fake_finder = double
-        fake_results = double
 
-        expect(fake_finder).to receive(:execute).and_return(fake_results).exactly(1)
-        expect(fake_results).to receive(:find_by).exactly(1)
+        expect(fake_finder).to receive(:find_by).exactly(1)
         expect(MergeRequestsFinder).to receive(:new).and_return(fake_finder).exactly(1)
 
         2.times { service.merge_request_to_resolve_discussions_of }