Merge remote-tracking branch 'upstream/master' into use-string-in-describe

* upstream/master:
  Expire the branch cache after `git gc` runs
  Be explicit on merge request discussion variables
  Improve wording in UI guide
  Fix expandable diffs
  CHANGELOG was updated.
  Style of import project buttons were fixed in the new project page.
  api: expose {should,force}_remove_source_branch
  Fix typo in UI guide
  Add rule about adding new header tab to the ui guide
  Update the health_check gem to the latest release
  Display tooltip for "Copy to Clipboard" button (!5164)
  Remove icons from some buttons which already has text

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.10.0 (unreleased)
+  - Expose {should,force}_remove_source_branch (Ben Boeckel)
   - Fix commit builds API, return all builds for all pipelines for given commit. !4849
   - Replace Haml with Hamlit to make view rendering faster. !3666
+  - Expire the branch cache after `git gc` runs
   - Refactor repository paths handling to allow multiple git mount points
   - Optimize system note visibility checking by memoizing the visible reference count !5070
   - Add Application Setting to configure default Repository Path for new projects
   - Remove pinTo from Flash and make inline flash messages look nicer !4854 (winniehell)
   - Wrap code blocks on Activies and Todos page. !4783 (winniehell)
   - Align flash messages with left side of page content !4959 (winniehell)
+  - Display tooltip for "Copy to Clipboard" button !5164 (winniehell)
   - Display last commit of deleted branch in push events !4699 (winniehell)
   - Escape file extension when parsing search results !5141 (winniehell)
   - Apply the trusted_proxies config to the rack request object for use with rack_attack
@@ -29,6 +32,7 @@ v 8.10.0 (unreleased)
   - Exclude email check from the standard health check
   - Updated layout for Projects, Groups, Users on Admin area !4424
   - Fix changing issue state columns in milestone view
+  - Update health_check gem to version 2.1.0
   - Add notification settings dropdown for groups
   - Wildcards for protected branches. !4665
   - Allow importing from Github using Personal Access Tokens. (Eric K Idema)
@@ -53,6 +57,7 @@ v 8.10.0 (unreleased)
   - Add API endpoint for a group issues !4520 (mahcsig)
   - Add Bugzilla integration !4930 (iamtjg)
   - Instrument Rinku usage
+  - Be explicit to define merge request discussion variables
   - Metrics for Rouge::Plugins::Redcarpet and Rouge::Formatters::HTMLGitlab
   - RailsCache metris now includes fetch_hit/fetch_miss and read_hit/read_miss info.
   - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w)
@@ -72,6 +77,7 @@ v 8.10.0 (unreleased)
   - Add min value for project limit field on user's form !3622 (jastkand)
   - Add reminder to not paste private SSH keys !4399 (Ingo Blechschmidt)
   - Remove duplicate `description` field in `MergeRequest` entities (Ben Boeckel)
+  - Style of import project buttons were fixed in the new project page. !5183 (rdemirbay)
 
 v 8.9.6 (unreleased)
   - Fix importing of events under notes for GitLab projects

Gemfile

@@ -344,7 +344,7 @@ gem 'oauth2', '~> 1.2.0'
 gem 'paranoia', '~> 2.0'
 
 # Health check
-gem 'health_check', '~> 1.5.1'
+gem 'health_check', '~> 2.1.0'
 
 # System information
 gem 'vmstat', '~> 2.1.0'

Gemfile.lock

@@ -322,8 +322,8 @@ GEM
       thor
       tilt
     hashie (3.4.3)
-    health_check (1.5.1)
-      rails (>= 2.3.0)
+    health_check (2.1.0)
+      rails (>= 4.0)
     hipchat (1.5.2)
       httparty
       mimemagic
@@ -870,7 +870,7 @@ DEPENDENCIES
   grape (~> 0.13.0)
   grape-entity (~> 0.4.2)
   hamlit (~> 2.5)
-  health_check (~> 1.5.1)
+  health_check (~> 2.1.0)
   hipchat (~> 1.5.0)
   html-pipeline (~> 1.11.0)
   httparty (~> 0.13.3)

app/assets/javascripts/diff.js.coffee

 class @Diff
   UNFOLD_COUNT = 20
   constructor: ->
+    $('.files .diff-file').singleFileDiff()
     @filesCommentButton = $('.files .diff-file').filesCommentButton()
 
     $(document).off('click', '.js-unfold')

app/assets/stylesheets/pages/commits.scss

@@ -61,7 +61,7 @@
     font-size: 0;
   }
 
-  .btn-transparent {
+  .btn-clipboard, .btn-transparent {
     padding-left: 0;
     padding-right: 0;
   }

app/assets/stylesheets/pages/projects.scss

@@ -340,23 +340,30 @@ a.deploy-project-label {
 
 .project-import {
   .form-group {
-    margin-bottom: 0;
+    margin-bottom: 5px;
   }
+
   .import-buttons {
     padding-left: 0;
     display: -webkit-flex;
     display: flex;
     -webkit-flex-wrap: wrap;
     flex-wrap: wrap;
+
     .btn {
-      margin-right: 10px;
-      padding: 8px 12px;
+      margin: 0 10px 10px 0;
+      padding: 8px;
     }
-    &> div {
-      margin-bottom: 14px;
+
+    > div {
       padding-left: 0;
+
       &:last-child {
         margin-bottom: 0;
+
+        .btn {
+          margin-right: 0;
+        }
       }
     }
   }

app/controllers/projects/merge_requests_controller.rb

@@ -56,7 +56,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def show
     respond_to do |format|
-      format.html
+      format.html { define_discussion_vars }
 
       format.json do
         render json: @merge_request
@@ -82,7 +82,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @merge_request_diff = @merge_request.merge_request_diff
 
     respond_to do |format|
-      format.html
+      format.html { define_discussion_vars }
       format.json { render json: { html: view_to_html_string("projects/merge_requests/show/_diffs") } }
     end
   end
@@ -108,7 +108,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def commits
     respond_to do |format|
-      format.html { render 'show' }
+      format.html do
+        define_discussion_vars
+
+        render 'show'
+      end
       format.json do
         # Get commits from repository
         # or from cache if already merged
@@ -123,7 +127,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
   def builds
     respond_to do |format|
-      format.html { render 'show' }
+      format.html do
+        define_discussion_vars
+
+        render 'show'
+      end
       format.json { render json: { html: view_to_html_string('projects/merge_requests/show/_builds') } }
     end
   end
@@ -353,14 +361,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       @merge_request.unlock_mr
       @merge_request.close
     end
-
-    if request.format == :html || action_name == 'show'
-      define_show_html_vars
-    end
   end
 
-  # Discussion tab data is only required on html requests
-  def define_show_html_vars
+  # Discussion tab data is rendered on html responses of actions
+  # :show, :diff, :commits, :builds. but not when request the data through AJAX
+  def define_discussion_vars
     # Build a note object for comment form
     @note = @project.notes.new(noteable: @noteable)
 

app/helpers/button_helper.rb

@@ -15,29 +15,13 @@ module ButtonHelper
   #
   # See http://clipboardjs.com/#usage
   def clipboard_button(data = {})
+    data = { toggle: 'tooltip', placement: 'bottom', container: 'body' }.merge(data)
     content_tag :button,
       icon('clipboard'),
       class: "btn btn-clipboard",
       data: data,
-      type: :button
-  end
-
-  # Output a "Copy to Clipboard" button with a custom CSS class
-  #
-  # data - Data attributes passed to `content_tag`
-  # css_class - Class passed to the `content_tag`
-  #
-  # Examples:
-  # 
-  #   # Define the target element
-  #   clipboard_button_with_class({clipboard_target: "div#foo"}, css_class: "btn-clipboard")
-  #   # => "<button class='btn btn-clipboard' data-clipboard-target='div#foo'>...</button>"
-  def clipboard_button_with_class(data = {}, css_class: 'btn-clipboard')
-    content_tag :button,
-      icon('clipboard'),
-      class: "btn #{css_class}",
-      data: data,
-      type: :button
+      type: :button,
+      title: "Copy to Clipboard"
   end
 
   def http_clone_button(project, placement = 'right', append_link: true)

app/services/projects/housekeeping_service.rb

@@ -7,8 +7,6 @@
 #
 module Projects
   class HousekeepingService < BaseService
-    include Gitlab::ShellAdapter
-
     LEASE_TIMEOUT = 3600
 
     class LeaseTaken < StandardError
@@ -24,7 +22,7 @@ module Projects
     def execute
       raise LeaseTaken unless try_obtain_lease
 
-      GitlabShellOneShotWorker.perform_async(:gc, @project.repository_storage_path, @project.path_with_namespace)
+      GitGarbageCollectWorker.perform_async(@project.id)
     ensure
       Gitlab::Metrics.measure(:reset_pushes_since_gc) do
         update_pushes_since_gc(0)

app/views/groups/milestones/index.html.haml

@@ -6,7 +6,6 @@
   .nav-controls
     - if can?(current_user, :admin_milestones, @group)
       = link_to new_group_milestone_path(@group), class: "btn btn-new" do
-        = icon('plus')
         New Milestone
 
 .row-content-block

app/views/groups/projects.html.haml

@@ -7,7 +7,6 @@
     - if can? current_user, :admin_group, @group
       .controls
         = link_to new_project_path(namespace_id: @group.id), class: "btn btn-sm btn-success" do
-          = icon('plus')
           New Project
   %ul.well-list
     - @projects.each do |project|

app/views/projects/commits/_commit.html.haml

@@ -25,7 +25,7 @@
         .commit-actions.hidden-xs
           - if commit.status
             = render_commit_status(commit, cssclass: 'btn btn-transparent')
-          = clipboard_button_with_class({ clipboard_text: commit.id }, css_class: 'btn-transparent')
+          = clipboard_button(clipboard_text: commit.id)
           = link_to commit.short_id, namespace_project_commit_path(project.namespace, project, commit), class: "commit-short-id btn btn-transparent"
           = link_to_browse_code(project, commit)
 

app/views/projects/merge_requests/show/_how_to_merge.html.haml

@@ -8,7 +8,7 @@
         %p
           %strong Step 1.
           Fetch and check out the branch for this merge request
-        = clipboard_button_with_class({clipboard_target: "pre#merge-info-1"}, css_class: "btn-clipboard")
+        = clipboard_button(clipboard_target: "pre#merge-info-1")
         %pre.dark#merge-info-1
           - if @merge_request.for_fork?
             :preserve
@@ -25,7 +25,7 @@
         %p
           %strong Step 3.
           Merge the branch and fix any conflicts that come up
-        = clipboard_button_with_class({clipboard_target: "pre#merge-info-3"}, css_class: "btn-clipboard")
+        = clipboard_button(clipboard_target: "pre#merge-info-3")
         %pre.dark#merge-info-3
           - if @merge_request.for_fork?
             :preserve
@@ -38,7 +38,7 @@
         %p
           %strong Step 4.
           Push the result of the merge to GitLab
-        = clipboard_button_with_class({clipboard_target: "pre#merge-info-4"}, css_class: "btn-clipboard")
+        = clipboard_button(clipboard_target: "pre#merge-info-4")
         %pre.dark#merge-info-4
           :preserve
             git push origin #{h @merge_request.target_branch}

app/views/shared/milestones/_summary.html.haml

@@ -26,7 +26,6 @@
     %span.pull-right.tab-issues-buttons
       - if project && can?(current_user, :create_issue, project)
         = link_to new_namespace_project_issue_path(project.namespace, project, issue: { milestone_id: milestone.id }), class: "btn  btn-grouped", title: "New Issue" do
-          %i.fa.fa-plus
           New Issue
       = link_to 'Browse Issues', milestones_browse_issuables_path(milestone, type: :issues), class: "btn btn-grouped"
     %span.pull-right.tab-merge-requests-buttons.hidden

app/workers/git_garbage_collect_worker.rb

+class GitGarbageCollectWorker
+  include Sidekiq::Worker
+  include Gitlab::ShellAdapter
+
+  sidekiq_options queue: :gitlab_shell, retry: false
+
+  def perform(project_id)
+    project = Project.find(project_id)
+
+    gitlab_shell.gc(project.repository_storage_path, project.path_with_namespace)
+    # Expire the branch cache in case garbage collection caused a ref lookup to fail
+    project.repository.after_create_branch
+  end
+end

app/workers/gitlab_shell_one_shot_worker.rb

-class GitlabShellOneShotWorker
-  include Sidekiq::Worker
-  include Gitlab::ShellAdapter
-
-  sidekiq_options queue: :gitlab_shell, retry: false
-
-  def perform(action, *arg)
-    gitlab_shell.send(action, *arg)
-  end
-end

config/initializers/health_check.rb

-# Email forcibly included in the standard checks, but the email health check
-# doesn't support the full range of SMTP options, which can result in failures
-# for valid SMTP configurations.
-# Overwrite the HealthCheck's detection of whether email is configured
-# in order to avoid the email check during standard checks
-module HealthCheck
-  class Utils
-    def self.mailer_configured?
-      false
-    end
-  end
-end
-
 HealthCheck.setup do |config|
   config.standard_checks = ['database', 'migrations', 'cache']
   config.full_checks = ['database', 'migrations', 'cache']

doc/api/merge_requests.md

@@ -68,7 +68,9 @@ Parameters:
     "merge_when_build_succeeds": true,
     "merge_status": "can_be_merged",
     "subscribed" : false,
-    "user_notes_count": 1
+    "user_notes_count": 1,
+    "should_remove_source_branch": true,
+    "force_remove_source_branch": false
   }
 ]
 ```
@@ -132,7 +134,9 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 1
+  "user_notes_count": 1,
+  "should_remove_source_branch": true,
+  "force_remove_source_branch": false
 }
 ```
 
@@ -233,6 +237,8 @@ Parameters:
   "merge_status": "can_be_merged",
   "subscribed" : true,
   "user_notes_count": 1,
+  "should_remove_source_branch": true,
+  "force_remove_source_branch": false,
   "changes": [
     {
     "old_path": "VERSION",
@@ -312,7 +318,9 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 0
+  "user_notes_count": 0,
+  "should_remove_source_branch": true,
+  "force_remove_source_branch": false
 }
 ```
 
@@ -383,7 +391,9 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 1
+  "user_notes_count": 1,
+  "should_remove_source_branch": true,
+  "force_remove_source_branch": false
 }
 ```
 
@@ -481,7 +491,9 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 1
+  "user_notes_count": 1,
+  "should_remove_source_branch": true,
+  "force_remove_source_branch": false
 }
 ```
 
@@ -547,7 +559,9 @@ Parameters:
   "merge_when_build_succeeds": true,
   "merge_status": "can_be_merged",
   "subscribed" : true,
-  "user_notes_count": 1
+  "user_notes_count": 1,
+  "should_remove_source_branch": true,
+  "force_remove_source_branch": false
 }
 ```
 
@@ -866,7 +880,9 @@ Example response:
     "merge_when_build_succeeds": false,
     "merge_status": "unchecked",
     "subscribed": true,
-    "user_notes_count": 7
+    "user_notes_count": 7,
+    "should_remove_source_branch": true,
+    "force_remove_source_branch": false
   },
   "target_url": "https://gitlab.example.com/gitlab-org/gitlab-ci/merge_requests/7",
   "body": "Et voluptas laudantium minus nihil recusandae ut accusamus earum aut non.",

doc/development/ui_guide.md

@@ -27,6 +27,8 @@ We try to keep the amount of tabs in the header navigation between 5 and 10 so t
 tab should represent separate functionality. Everything related to the issue 
 tracker should be under the 'Issues' tab while everything related to the wiki should 
 be under 'Wiki' tab and so on and so forth.
+When adding a new tab to the header don't use more than 2 words for text in the link. 
+We want to keep links short and easy to remember and fit all of them in the small screen.
 
 ## Mobile screen size 
 

lib/api/entities.rb

@@ -207,6 +207,8 @@ module API
         merge_request.subscribed?(options[:current_user])
       end
       expose :user_notes_count
+      expose :should_remove_source_branch?, as: :should_remove_source_branch
+      expose :force_remove_source_branch?, as: :force_remove_source_branch
     end
 
     class MergeRequestChanges < MergeRequest

spec/features/expand_collapse_diffs_spec.rb

@@ -106,7 +106,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do
         let(:comment_text) { 'A comment' }
 
         before do
-          large_diff.find('.line_holder', match: :prefer_exact).hover
+          large_diff.find('.diff-line-num', match: :prefer_exact).hover
           large_diff.find('.add-diff-note').click
           large_diff.find('.note-textarea').send_keys comment_text
           large_diff.find_button('Comment').click
@@ -161,7 +161,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do
         end
 
         it 'does not make a new HTTP request' do
-          expect(evaluate_script('ajaxUris')).to be_empty
+          expect(evaluate_script('ajaxUris')).not_to include(a_string_matching('small_diff.md'))
         end
       end
     end
@@ -199,7 +199,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do
         end
 
         it 'does not make a new HTTP request' do
-          expect(evaluate_script('ajaxUris')).to be_empty
+          expect(evaluate_script('ajaxUris')).not_to include(a_string_matching('small_diff.md'))
         end
       end
     end

spec/features/merge_requests/diffs_spec.rb

+require 'spec_helper'
+
+feature 'Diffs URL', js: true, feature: true do
+  before do
+    login_as :admin
+    @merge_request = create(:merge_request)
+    @project = @merge_request.source_project
+  end
+
+  context 'when visit with */* as accept header' do
+    before(:each) do
+      page.driver.add_header('Accept', '*/*')
+    end
+
+    it 'renders the notes' do
+      create :note_on_merge_request, project: @project, noteable: @merge_request, note: 'Rebasing with master'
+
+      visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)
+
+      # Load notes and diff through AJAX
+      expect(page).to have_css('.note-text', visible: false, text: 'Rebasing with master')
+      expect(page).to have_css('.diffs.tab-pane.active')
+    end
+  end
+end

spec/requests/api/merge_requests_spec.rb

@@ -138,6 +138,8 @@ describe API::API, api: true  do
       expect(json_response['work_in_progress']).to be_falsy
       expect(json_response['merge_when_build_succeeds']).to be_falsy
       expect(json_response['merge_status']).to eq('can_be_merged')
+      expect(json_response['should_close_merge_request']).to be_falsy
+      expect(json_response['force_close_merge_request']).to be_falsy
     end
 
     it "should return merge_request" do
@@ -147,6 +149,8 @@ describe API::API, api: true  do
       expect(json_response['iid']).to eq(merge_request.iid)
       expect(json_response['work_in_progress']).to eq(false)
       expect(json_response['merge_status']).to eq('can_be_merged')
+      expect(json_response['should_close_merge_request']).to be_falsy
+      expect(json_response['force_close_merge_request']).to be_falsy
     end
 
     it 'should return merge_request by iid' do

spec/services/projects/housekeeping_service_spec.rb

@@ -12,7 +12,7 @@ describe Projects::HousekeepingService do
 
     it 'enqueues a sidekiq job' do
       expect(subject).to receive(:try_obtain_lease).and_return(true)
-      expect(GitlabShellOneShotWorker).to receive(:perform_async).with(:gc, project.repository_storage_path, project.path_with_namespace)
+      expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id)
 
       subject.execute
       expect(project.pushes_since_gc).to eq(0)
@@ -20,7 +20,7 @@ describe Projects::HousekeepingService do
 
     it 'does not enqueue a job when no lease can be obtained' do
       expect(subject).to receive(:try_obtain_lease).and_return(false)
-      expect(GitlabShellOneShotWorker).not_to receive(:perform_async)
+      expect(GitGarbageCollectWorker).not_to receive(:perform_async)
 
       expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken)
       expect(project.pushes_since_gc).to eq(0)

spec/workers/git_garbage_collect_worker_spec.rb

+require 'spec_helper'
+
+describe GitGarbageCollectWorker do
+  let(:project) { create(:project) }
+  let(:shell) { Gitlab::Shell.new }
+
+  subject { GitGarbageCollectWorker.new }
+
+  before do
+    allow(subject).to receive(:gitlab_shell).and_return(shell)
+  end
+
+  describe "#perform" do
+    it "runs `git gc`" do
+      expect(shell).to receive(:gc).with(
+        project.repository_storage_path,
+        project.path_with_namespace).
+      and_return(true)
+      expect_any_instance_of(Repository).to receive(:after_create_branch)
+
+      subject.perform(project.id)
+    end
+  end
+end