Merge remote-tracking branch 'ce/master' into ce-to-ee-2017-08-03

Gemfile

@@ -406,7 +406,7 @@ gem 'sys-filesystem', '~> 1.1.6'
 gem 'net-ntp'
 
 # Gitaly GRPC client
-gem 'gitaly', '~> 0.21.0'
+gem 'gitaly', '~> 0.23.0'
 
 gem 'toml-rb', '~> 0.3.15', require: false
 

Gemfile.lock

@@ -293,7 +293,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly (0.21.0)
+    gitaly (0.23.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -1012,7 +1012,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly (~> 0.21.0)
+  gitaly (~> 0.23.0)
   github-linguist (~> 4.7.0)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-license (~> 1.0)

app/assets/javascripts/diff_notes/components/jump_to_discussion.js

@@ -94,7 +94,7 @@ const JumpToDiscussion = Vue.extend({
             hasDiscussionsToJumpTo = false;
           }
         }
-      } else if (activeTab !== 'notes') {
+      } else if (activeTab !== 'show') {
         // If we are on the commits or builds tabs,
         // there are no discussions to jump to.
         hasDiscussionsToJumpTo = false;
@@ -103,12 +103,12 @@ const JumpToDiscussion = Vue.extend({
       if (!hasDiscussionsToJumpTo) {
         // If there are no discussions to jump to on the current page,
         // switch to the notes tab and jump to the first disucssion there.
-        window.mrTabs.activateTab('notes');
-        activeTab = 'notes';
+        window.mrTabs.activateTab('show');
+        activeTab = 'show';
         jumpToFirstDiscussion = true;
       }
 
-      if (activeTab === 'notes') {
+      if (activeTab === 'show') {
         discussionsSelector = '.discussion[data-discussion-id]';
         discussionIdsInScope = discussionIdsForElements($(discussionsSelector));
       }
@@ -156,7 +156,7 @@ const JumpToDiscussion = Vue.extend({
 
       let $target = $(`${discussionsSelector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`);
 
-      if (activeTab === 'notes') {
+      if (activeTab === 'show') {
         $target = $target.closest('.note-discussion');
 
         // If the next discussion is closed, toggle it open.

app/assets/javascripts/main.js

@@ -151,6 +151,7 @@ import './syntax_highlight';
 
 import './dispatcher';
 
+<<<<<<< HEAD
 // EE-only scripts
 import './admin_email_select';
 import './application_settings';
@@ -159,6 +160,8 @@ import './ldap_groups_select';
 import './path_locks';
 import './weight_select';
 
+=======
+>>>>>>> ce/master
 // eslint-disable-next-line global-require, import/no-commonjs
 if (process.env.NODE_ENV !== 'production') require('./test_utils/');
 

app/assets/javascripts/profile/gl_crop.js

 /* eslint-disable no-useless-escape, max-len, quotes, no-var, no-underscore-dangle, func-names, space-before-function-paren, no-unused-vars, no-return-assign, object-shorthand, one-var, one-var-declaration-per-line, comma-dangle, consistent-return, class-methods-use-this, new-parens */
+<<<<<<< HEAD
 import _ from 'underscore';
 import 'vendor/cropper';
+=======
+
+import 'cropper';
+import _ from 'underscore';
+>>>>>>> ce/master
 
 ((global) => {
   // Matches everything but the file name

app/assets/stylesheets/framework/dropdowns.scss

@@ -725,7 +725,8 @@
 
 // TODO: change global style and remove mixin
 @mixin new-style-dropdown {
-  .dropdown-menu {
+  .dropdown-menu,
+  .dropdown-menu-nav {
     li {
       padding: 0 1px;
 
@@ -766,4 +767,8 @@
       }
     }
   }
+
+  .dropdown-menu-align-right {
+    margin-top: 2px;
+  }
 }

app/assets/stylesheets/framework/header.scss

@@ -4,6 +4,8 @@
  */
 
 header {
+  @include new-style-dropdown;
+
   transition: padding $sidebar-transition-duration;
 
   &.navbar-empty {
@@ -313,25 +315,6 @@ header {
   .impersonation i {
     color: $red-500;
   }
-
-  // TODO: fallback to global style
-  .dropdown-menu,
-  .dropdown-menu-nav {
-    li {
-      padding: 0 1px;
-
-      a {
-        border-radius: 0;
-        padding: 8px 16px;
-
-        &:hover,
-        &:active,
-        &:focus {
-          background-color: $gray-darker;
-        }
-      }
-    }
-  }
 }
 
 .with-performance-bar header.navbar-gitlab {

app/assets/stylesheets/pages/cycle_analytics.scss

 #cycle-analytics {
+  @include new-style-dropdown;
+
   max-width: 1000px;
   margin: 24px auto 0;
   position: relative;
@@ -110,10 +112,6 @@
 
   .js-ca-dropdown {
     top: $gl-padding-top;
-
-    .dropdown-menu-align-right {
-      margin-top: 2px;
-    }
   }
 
   .content-list {
@@ -446,24 +444,6 @@
       margin-bottom: 20px;
     }
   }
-
-  // TODO: fallback to global style
-  .dropdown-menu {
-    li {
-      padding: 0 1px;
-
-      a {
-        border-radius: 0;
-        padding: 8px 16px;
-
-        &:hover,
-        &:active,
-        &:focus {
-          background-color: $gray-darker;
-        }
-      }
-    }
-  }
 }
 
 .cycle-analytics-overview {

app/assets/stylesheets/pages/tree.scss

 .tree-holder {
+  @include new-style-dropdown;
 
   .nav-block {
     margin: 10px 0;
@@ -202,28 +203,6 @@
       }
     }
   }
-
-  // TODO: fallback to global style
-  .dropdown-menu:not(.dropdown-menu-selectable) {
-    li {
-      padding: 0 1px;
-
-      &.dropdown-header {
-        padding: 8px 16px;
-      }
-
-      a {
-        border-radius: 0;
-        padding: 8px 16px;
-
-        &:hover,
-        &:active,
-        &:focus {
-          background-color: $gray-darker;
-        }
-      }
-    }
-  }
 }
 
 .blob-commit-info {

app/controllers/dashboard/todos_controller.rb

@@ -13,7 +13,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def destroy
-    TodoService.new.mark_todos_as_done_by_ids([params[:id]], current_user)
+    TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
 
     respond_to do |format|
       format.html do
@@ -37,7 +37,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def restore
-    TodoService.new.mark_todos_as_pending_by_ids([params[:id]], current_user)
+    TodoService.new.mark_todos_as_pending_by_ids(params[:id], current_user)
 
     render json: todos_counts
   end

app/finders/todos_finder.rb

@@ -95,9 +95,18 @@ class TodosFinder
     @project
   end
 
+  def project_ids(items)
+    ids = items.except(:order).select(:project_id)
+    if Gitlab::Database.mysql?
+      # To make UPDATE work on MySQL, wrap it in a SELECT with an alias
+      ids = Todo.except(:order).select('*').from("(#{ids.to_sql}) AS t")
+    end
+
+    ids
+  end
+
   def projects(items)
-    item_project_ids = items.reorder(nil).select(:project_id)
-    ProjectsFinder.new(current_user: current_user, project_ids_relation: item_project_ids).execute
+    ProjectsFinder.new(current_user: current_user, project_ids_relation: project_ids(items)).execute
   end
 
   def type?

app/models/concerns/referable.rb

@@ -25,6 +25,18 @@ module Referable
     to_reference(from_project)
   end
 
+  def referable_inspect
+    if respond_to?(:id)
+      "#<#{self.class.name} id:#{id} #{to_reference(full: true)}>"
+    else
+      "#<#{self.class.name} #{to_reference(full: true)}>"
+    end
+  end
+
+  def inspect
+    referable_inspect
+  end
+
   module ClassMethods
     # The character that prefixes the actual reference identifier
     #

app/models/key.rb

@@ -16,8 +16,6 @@ class Key < ActiveRecord::Base
     presence: true,
     length: { maximum: 5000 },
     format: { with: /\A(ssh|ecdsa)-.*\Z/ }
-  validates :key,
-    format: { without: /\n|\r/, message: 'should be a single line' }
   validates :fingerprint,
     uniqueness: true,
     presence: { message: 'cannot be generated' }
@@ -33,6 +31,7 @@ class Key < ActiveRecord::Base
   after_destroy :post_destroy_hook
 
   def key=(value)
+    value&.delete!("\n\r")
     value.strip! unless value.blank?
     write_attribute(:key, value)
   end

app/models/merge_request_diff.rb

@@ -85,11 +85,7 @@ class MergeRequestDiff < ActiveRecord::Base
 
   def raw_diffs(options = {})
     if options[:ignore_whitespace_change]
-      @diffs_no_whitespace ||=
-        Gitlab::Git::Compare.new(
-          repository.raw_repository,
-          safe_start_commit_sha,
-          head_commit_sha).diffs(options)
+      @diffs_no_whitespace ||= compare.diffs(options)
     else
       @raw_diffs ||= {}
       @raw_diffs[options] ||= load_diffs(options)

app/models/user.rb

@@ -50,6 +50,11 @@ class User < ActiveRecord::Base
   devise :lockable, :recoverable, :rememberable, :trackable,
     :validatable, :omniauthable, :confirmable, :registerable
 
+  # devise overrides #inspect, so we manually use the Referable one
+  def inspect
+    referable_inspect
+  end
+
   # Override Devise::Models::Trackable#update_tracked_fields!
   # to limit database writes to at most once every hour
   def update_tracked_fields!(request)

app/services/issuable_base_service.rb

@@ -290,7 +290,7 @@ class IssuableBaseService < BaseService
       todo_service.mark_todo(issuable, current_user)
     when 'done'
       todo = TodosFinder.new(current_user).execute.find_by(target: issuable)
-      todo_service.mark_todos_as_done([todo], current_user) if todo
+      todo_service.mark_todos_as_done_by_ids(todo, current_user) if todo
     end
   end
 

app/services/todo_service.rb

@@ -178,20 +178,22 @@ class TodoService
 
   # When user marks some todos as done
   def mark_todos_as_done(todos, current_user)
-    update_todos_state_by_ids(todos.select(&:id), current_user, :done)
+    update_todos_state(todos, current_user, :done)
   end
 
   def mark_todos_as_done_by_ids(ids, current_user)
-    update_todos_state_by_ids(ids, current_user, :done)
+    todos = todos_by_ids(ids, current_user)
+    mark_todos_as_done(todos, current_user)
   end
 
   # When user marks some todos as pending
   def mark_todos_as_pending(todos, current_user)
-    update_todos_state_by_ids(todos.select(&:id), current_user, :pending)
+    update_todos_state(todos, current_user, :pending)
   end
 
   def mark_todos_as_pending_by_ids(ids, current_user)
-    update_todos_state_by_ids(ids, current_user, :pending)
+    todos = todos_by_ids(ids, current_user)
+    mark_todos_as_pending(todos, current_user)
   end
 
   # When user marks an issue as todo
@@ -206,9 +208,11 @@ class TodoService
 
   private
 
-  def update_todos_state_by_ids(ids, current_user, state)
-    todos = current_user.todos.where(id: ids)
+  def todos_by_ids(ids, current_user)
+    current_user.todos.where(id: Array(ids))
+  end
 
+  def update_todos_state(todos, current_user, state)
     # Only update those that are not really on that state
     todos = todos.where.not(state: state)
     todos_ids = todos.pluck(:id)

changelogs/unreleased/35232-next-unresolved.yml

+---
+title: fix jump to next discussion button
+merge_request:
+author:

changelogs/unreleased/3686_make_tarball_download_url.yml

+---
+title: repository archive download url now ends with selected file extension
+merge_request: 13178
+author: haseebeqx

changelogs/unreleased/reorganise-issues-indexes-for-sorting.yml

+---
+title: Re-organise "issues" indexes for faster ordering
+merge_request:
+author:

changelogs/unreleased/tc-no-todo-service-select.yml

+---
+title: Avoid plucking Todo ids in TodoService
+merge_request: 10845
+author:

config/routes/repository.rb

@@ -2,7 +2,7 @@
 
 resource :repository, only: [:create] do
   member do
-    get 'archive', constraints: { format: Gitlab::PathRegex.archive_formats_regex }
+    get ':ref/archive', constraints: { format: Gitlab::PathRegex.archive_formats_regex, ref: /.+/ }, action: 'archive', as: 'archive'
   end
 end
 

db/migrate/20170803130232_reorganise_issues_indexes_for_faster_sorting.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class ReorganiseIssuesIndexesForFasterSorting < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  REMOVE_INDEX_COLUMNS = %i[project_id created_at due_date updated_at].freeze
+
+  ADD_INDEX_COLUMNS = [
+    %i[project_id created_at id state],
+    %i[project_id due_date id state],
+    %i[project_id updated_at id state]
+  ].freeze
+
+  TABLE = :issues
+
+  def up
+    add_indexes(ADD_INDEX_COLUMNS)
+    remove_indexes(REMOVE_INDEX_COLUMNS)
+  end
+
+  def down
+    add_indexes(REMOVE_INDEX_COLUMNS)
+    remove_indexes(ADD_INDEX_COLUMNS)
+  end
+
+  def add_indexes(columns)
+    columns.each do |column|
+      add_concurrent_index(TABLE, column) unless index_exists?(TABLE, column)
+    end
+  end
+
+  def remove_indexes(columns)
+    columns.each do |column|
+      remove_concurrent_index(TABLE, column) if index_exists?(TABLE, column)
+    end
+  end
+end

db/post_migrate/20170703130158_schedule_merge_request_diff_migrations.rb

+class ScheduleMergeRequestDiffMigrations < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 2500
+  MIGRATION = 'DeserializeMergeRequestDiffsAndCommits'
+
+  disable_ddl_transaction!
+
+  class MergeRequestDiff < ActiveRecord::Base
+    self.table_name = 'merge_request_diffs'
+
+    include ::EachBatch
+  end
+
+  # Assuming that there are 5 million rows affected (which is more than on
+  # GitLab.com), and that each batch of 2,500 rows takes up to 5 minutes, then
+  # we can migrate all the rows in 7 days.
+  #
+  # On staging, plucking the IDs themselves takes 5 seconds.
+  def up
+    non_empty = 'st_commits IS NOT NULL OR st_diffs IS NOT NULL'
+
+    MergeRequestDiff.where(non_empty).each_batch(of: BATCH_SIZE) do |relation, index|
+      range = relation.pluck('MIN(id)', 'MAX(id)').first
+
+      BackgroundMigrationWorker.perform_in(index * 5.minutes, MIGRATION, range)
+    end
+  end
+
+  def down
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170728101014) do
+ActiveRecord::Schema.define(version: 20170803130232) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -816,12 +816,13 @@ ActiveRecord::Schema.define(version: 20170728101014) do
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
   add_index "issues", ["author_id"], name: "index_issues_on_author_id", using: :btree
   add_index "issues", ["confidential"], name: "index_issues_on_confidential", using: :btree
-  add_index "issues", ["created_at"], name: "index_issues_on_created_at", using: :btree
   add_index "issues", ["deleted_at"], name: "index_issues_on_deleted_at", using: :btree
   add_index "issues", ["description"], name: "index_issues_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
-  add_index "issues", ["due_date"], name: "index_issues_on_due_date", using: :btree
   add_index "issues", ["milestone_id"], name: "index_issues_on_milestone_id", using: :btree
+  add_index "issues", ["project_id", "created_at", "id", "state"], name: "index_issues_on_project_id_and_created_at_and_id_and_state", using: :btree
+  add_index "issues", ["project_id", "due_date", "id", "state"], name: "index_issues_on_project_id_and_due_date_and_id_and_state", using: :btree
   add_index "issues", ["project_id", "iid"], name: "index_issues_on_project_id_and_iid", unique: true, using: :btree
+  add_index "issues", ["project_id", "updated_at", "id", "state"], name: "index_issues_on_project_id_and_updated_at_and_id_and_state", using: :btree
   add_index "issues", ["relative_position"], name: "index_issues_on_relative_position", using: :btree
   add_index "issues", ["state"], name: "index_issues_on_state", using: :btree
   add_index "issues", ["title"], name: "index_issues_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"}

doc/administration/reply_by_email_postfix_setup.md

@@ -177,6 +177,20 @@ Courier, which we will install later to add IMAP authentication, requires mailbo
     ```sh
     sudo apt-get install courier-imap
     ```
+    
+    And start `imapd`:
+    ```sh
+    imapd start
+    ```
+    
+1. The courier-authdaemon isn't started after installation. Without it, imap authentication will fail:
+    ```sh
+    sudo service courier-authdaemon start
+    ```
+    You can also configure courier-authdaemon to start on boot:
+    ```sh
+    sudo systemctl enable courier-authdaemon
+    ```
 
 ## Configure Postfix to receive email from the internet
 

doc/api/README.md

@@ -78,6 +78,38 @@ controller-specific endpoints. GraphQL has a number of benefits:
 It will co-exist with the current v4 REST API. If we have a v5 API, this should
 be a compatibility layer on top of GraphQL.
 
+## Basic usage
+
+API requests should be prefixed with `api` and the API version. The API version
+is defined in [`lib/api.rb`][lib-api-url]. For example, the root of the v4 API
+is at `/api/v4`.
+
+For endpoints that require [authentication](#authentication), you need to pass
+a `private_token` parameter via query string or header. If passed as a header,
+the header name must be `PRIVATE-TOKEN` (uppercase and with a dash instead of
+an underscore).
+
+Example of a valid API request:
+
+```
+GET /projects?private_token=9koXpg98eAheJpvBs5tK
+```
+
+Example of a valid API request using cURL and authentication via header:
+
+```shell
+curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects"
+```
+
+Example of a valid API request using cURL and authentication via a query string:
+
+```shell
+curl "https://gitlab.example.com/api/v4/projects?private_token=9koXpg98eAheJpvBs5tK"
+```
+
+The API uses JSON to serialize data. You don't need to specify `.json` at the
+end of an API URL.
+
 ## Authentication
 
 Most API requests require authentication via a session cookie or token. For
@@ -208,37 +240,6 @@ GET /projects?private_token=9koXpg98eAheJpvBs5tK&sudo=23
 curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --header "SUDO: 23" "https://gitlab.example.com/api/v4/projects"
 ```
 
-## Basic usage
-
-API requests should be prefixed with `api` and the API version. The API version
-is defined in [`lib/api.rb`][lib-api-url].
-
-For endpoints that require [authentication](#authentication), you need to pass
-a `private_token` parameter via query string or header. If passed as a header,
-the header name must be `PRIVATE-TOKEN` (uppercase and with a dash instead of
-an underscore).
-
-Example of a valid API request:
-
-```
-GET /projects?private_token=9koXpg98eAheJpvBs5tK
-```
-
-Example of a valid API request using cURL and authentication via header:
-
-```shell
-curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects"
-```
-
-Example of a valid API request using cURL and authentication via a query string:
-
-```shell
-curl "https://gitlab.example.com/api/v4/projects?private_token=9koXpg98eAheJpvBs5tK"
-```
-
-The API uses JSON to serialize data. You don't need to specify `.json` at the
-end of an API URL.
-
 ## Status codes
 
 The API is designed to return different status codes according to context and

lib/api/todos.rb

@@ -59,10 +59,10 @@ module API
         requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
       end
       post ':id/mark_as_done' do
-        todo = current_user.todos.find(params[:id])
-        TodoService.new.mark_todos_as_done([todo], current_user)
+        TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
+        todo = Todo.find(params[:id])
 
-        present todo.reload, with: Entities::Todo, current_user: current_user
+        present todo, with: Entities::Todo, current_user: current_user
       end
 
       desc 'Mark all todos as done'

lib/api/v3/todos.rb

@@ -11,10 +11,10 @@ module API
           requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
         end
         delete ':id' do
-          todo = current_user.todos.find(params[:id])
-          TodoService.new.mark_todos_as_done([todo], current_user)
+          TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
+          todo = Todo.find(params[:id])
 
-          present todo.reload, with: ::API::Entities::Todo, current_user: current_user
+          present todo, with: ::API::Entities::Todo, current_user: current_user
         end
 
         desc 'Mark all todos as done'

lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits.rb

+module Gitlab
+  module BackgroundMigration
+    class DeserializeMergeRequestDiffsAndCommits
+      attr_reader :diff_ids, :commit_rows, :file_rows
+
+      class MergeRequestDiff < ActiveRecord::Base
+        self.table_name = 'merge_request_diffs'
+      end
+
+      BUFFER_ROWS = 1000
+
+      def perform(start_id, stop_id)
+        merge_request_diffs = MergeRequestDiff
+                               .select(:id, :st_commits, :st_diffs)
+                               .where('st_commits IS NOT NULL OR st_diffs IS NOT NULL')
+                               .where(id: start_id..stop_id)
+
+        reset_buffers!
+
+        merge_request_diffs.each do |merge_request_diff|
+          commits, files = single_diff_rows(merge_request_diff)
+
+          diff_ids << merge_request_diff.id
+          commit_rows.concat(commits)
+          file_rows.concat(files)
+
+          if diff_ids.length > BUFFER_ROWS ||
+              commit_rows.length > BUFFER_ROWS ||
+              file_rows.length > BUFFER_ROWS
+
+            flush_buffers!
+          end
+        end
+
+        flush_buffers!
+      end
+
+      private
+
+      def reset_buffers!
+        @diff_ids = []
+        @commit_rows = []
+        @file_rows = []
+      end
+
+      def flush_buffers!
+        if diff_ids.any?
+          MergeRequestDiff.transaction do
+            Gitlab::Database.bulk_insert('merge_request_diff_commits', commit_rows)
+            Gitlab::Database.bulk_insert('merge_request_diff_files', file_rows)
+
+            MergeRequestDiff.where(id: diff_ids).update_all(st_commits: nil, st_diffs: nil)
+          end
+        end
+
+        reset_buffers!
+      end
+
+      def single_diff_rows(merge_request_diff)
+        sha_attribute = Gitlab::Database::ShaAttribute.new
+        commits = YAML.load(merge_request_diff.st_commits) rescue []
+
+        commit_rows = commits.map.with_index do |commit, index|
+          commit_hash = commit.to_hash.with_indifferent_access.except(:parent_ids)
+          sha = commit_hash.delete(:id)
+
+          commit_hash.merge(
+            merge_request_diff_id: merge_request_diff.id,
+            relative_order: index,
+            sha: sha_attribute.type_cast_for_database(sha)
+          )
+        end
+
+        diffs = YAML.load(merge_request_diff.st_diffs) rescue []
+        diffs = [] unless valid_raw_diffs?(diffs)
+
+        file_rows = diffs.map.with_index do |diff, index|
+          diff_hash = diff.to_hash.with_indifferent_access.merge(
+            binary: false,
+            merge_request_diff_id: merge_request_diff.id,
+            relative_order: index
+          )
+
+          # Compatibility with old diffs created with Psych.
+          diff_hash.tap do |hash|
+            diff_text = hash[:diff]
+
+            if diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only?
+              hash[:binary] = true
+              hash[:diff] = [diff_text].pack('m0')
+            end
+          end
+        end
+
+        [commit_rows, file_rows]
+      end
+
+      # Unlike MergeRequestDiff#valid_raw_diff?, don't count Rugged objects as
+      # valid, because we don't render them usefully anyway.
+      def valid_raw_diffs?(diffs)
+        return false unless diffs.respond_to?(:each)
+
+        diffs.all? { |diff| diff.is_a?(Hash) }
+      end
+    end
+  end
+end

lib/gitlab/git/repository.rb

@@ -300,17 +300,14 @@ module Gitlab
         raw_log(options).map { |c| Commit.decorate(c) }
       end
 
-      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/382
       def count_commits(options)
-        cmd = %W[#{Gitlab.config.git.bin_path} --git-dir=#{path} rev-list]
-        cmd << "--after=#{options[:after].iso8601}" if options[:after]
-        cmd << "--before=#{options[:before].iso8601}" if options[:before]
-        cmd += %W[--count #{options[:ref]}]
-        cmd += %W[-- #{options[:path]}] if options[:path].present?
-
-        raw_output = IO.popen(cmd) { |io| io.read }
-
-        raw_output.to_i
+        gitaly_migrate(:count_commits) do |is_enabled|
+          if is_enabled
+            count_commits_by_gitaly(options)
+          else
+            count_commits_by_shelling_out(options)
+          end
+        end
       end
 
       def sha_from_ref(ref)
@@ -1005,6 +1002,22 @@ module Gitlab
         gitaly_ref_client.tags
       end
 
+      def count_commits_by_gitaly(options)
+        gitaly_commit_client.commit_count(options[:ref], options)
+      end
+
+      def count_commits_by_shelling_out(options)
+        cmd = %W[#{Gitlab.config.git.bin_path} --git-dir=#{path} rev-list]
+        cmd << "--after=#{options[:after].iso8601}" if options[:after]
+        cmd << "--before=#{options[:before].iso8601}" if options[:before]
+        cmd += %W[--count #{options[:ref]}]
+        cmd += %W[-- #{options[:path]}] if options[:path].present?
+
+        raw_output = IO.popen(cmd) { |io| io.read }
+
+        raw_output.to_i
+      end
+
       def gitaly_migrate(method, &block)
         Gitlab::GitalyClient.migrate(method, &block)
       rescue GRPC::NotFound => e

lib/gitlab/gitaly_client/commit_service.rb

@@ -85,11 +85,14 @@ module Gitlab
         end
       end
 
-      def commit_count(ref)
+      def commit_count(ref, options = {})
         request = Gitaly::CountCommitsRequest.new(
           repository: @gitaly_repo,
           revision: ref
         )
+        request.after = Google::Protobuf::Timestamp.new(seconds: options[:after].to_i) if options[:after].present?
+        request.before = Google::Protobuf::Timestamp.new(seconds: options[:before].to_i) if options[:before].present?
+        request.path = options[:path] if options[:path].present?
 
         GitalyClient.call(@repository.storage, :commit_service, :count_commits, request).count
       end

spec/controllers/projects/repositories_controller_spec.rb

@@ -6,7 +6,7 @@ describe Projects::RepositoriesController do
   describe "GET archive" do
     context 'as a guest' do
       it 'responds with redirect in correct format' do
-        get :archive, namespace_id: project.namespace, project_id: project, format: "zip"
+        get :archive, namespace_id: project.namespace, project_id: project, format: "zip", ref: 'master'
 
         expect(response.header["Content-Type"]).to start_with('text/html')
         expect(response).to be_redirect

spec/lib/gitlab/background_migration/deserialize_merge_request_diffs_and_commits_spec.rb

+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::DeserializeMergeRequestDiffsAndCommits do
+  describe '#perform' do
+    set(:merge_request) { create(:merge_request) }
+    set(:merge_request_diff) { merge_request.merge_request_diff }
+    let(:updated_merge_request_diff) { MergeRequestDiff.find(merge_request_diff.id) }
+
+    def diffs_to_hashes(diffs)
+      diffs.as_json(only: Gitlab::Git::Diff::SERIALIZE_KEYS).map(&:with_indifferent_access)
+    end
+
+    def quote_yaml(value)
+      MergeRequestDiff.connection.quote(YAML.dump(value))
+    end
+
+    def convert_to_yaml(merge_request_diff_id, commits, diffs)
+      MergeRequestDiff.where(id: merge_request_diff_id).update_all(
+        "st_commits = #{quote_yaml(commits)}, st_diffs = #{quote_yaml(diffs)}"
+      )
+    end
+
+    shared_examples 'updated MR diff' do
+      before do
+        convert_to_yaml(merge_request_diff.id, commits, diffs)
+
+        MergeRequestDiffCommit.delete_all
+        MergeRequestDiffFile.delete_all
+
+        subject.perform(merge_request_diff.id, merge_request_diff.id)
+      end
+
+      it 'creates correct entries in the merge_request_diff_commits table' do
+        expect(updated_merge_request_diff.merge_request_diff_commits.count).to eq(commits.count)
+        expect(updated_merge_request_diff.commits.map(&:to_hash)).to eq(commits)
+      end
+
+      it 'creates correct entries in the merge_request_diff_files table' do
+        expect(updated_merge_request_diff.merge_request_diff_files.count).to eq(expected_diffs.count)
+        expect(diffs_to_hashes(updated_merge_request_diff.raw_diffs)).to eq(expected_diffs)
+      end
+
+      it 'sets the st_commits and st_diffs columns to nil' do
+        expect(updated_merge_request_diff.st_commits_before_type_cast).to be_nil
+        expect(updated_merge_request_diff.st_diffs_before_type_cast).to be_nil
+      end
+    end
+
+    context 'when the diff IDs passed do not exist' do
+      it 'does not raise' do
+        expect { subject.perform(0, 0) }.not_to raise_exception
+      end
+    end
+
+    context 'when the merge request diff has no serialised commits or diffs' do
+      before do
+        merge_request_diff.update(st_commits: nil, st_diffs: nil)
+      end
+
+      it 'does not raise' do
+        expect { subject.perform(merge_request_diff.id, merge_request_diff.id) }
+          .not_to raise_exception
+      end
+    end
+
+    context 'processing multiple merge request diffs' do
+      let(:start_id) { described_class::MergeRequestDiff.minimum(:id) }
+      let(:stop_id) { described_class::MergeRequestDiff.maximum(:id) }
+
+      before do
+        merge_request.reload_diff(true)
+
+        convert_to_yaml(start_id, merge_request_diff.commits, merge_request_diff.diffs)
+        convert_to_yaml(stop_id, updated_merge_request_diff.commits, updated_merge_request_diff.diffs)
+
+        MergeRequestDiffCommit.delete_all
+        MergeRequestDiffFile.delete_all
+      end
+
+      context 'when BUFFER_ROWS is exceeded' do
+        before do
+          stub_const("#{described_class}::BUFFER_ROWS", 1)
+        end
+
+        it 'updates and continues' do
+          expect(described_class::MergeRequestDiff).to receive(:transaction).twice
+
+          subject.perform(start_id, stop_id)
+        end
+      end
+
+      context 'when BUFFER_ROWS is not exceeded' do
+        it 'only updates once' do
+          expect(described_class::MergeRequestDiff).to receive(:transaction).once
+
+          subject.perform(start_id, stop_id)
+        end
+      end
+    end
+
+    context 'when the merge request diff update fails' do
+      before do
+        allow(described_class::MergeRequestDiff)
+          .to receive(:update_all).and_raise(ActiveRecord::Rollback)
+      end
+
+      it 'does not add any diff commits' do
+        expect { subject.perform(merge_request_diff.id, merge_request_diff.id) }
+          .not_to change { MergeRequestDiffCommit.count }
+      end
+
+      it 'does not add any diff files' do
+        expect { subject.perform(merge_request_diff.id, merge_request_diff.id) }
+          .not_to change { MergeRequestDiffFile.count }
+      end
+    end
+
+    context 'when the merge request diff has valid commits and diffs' do
+      let(:commits) { merge_request_diff.commits.map(&:to_hash) }
+      let(:diffs) { diffs_to_hashes(merge_request_diff.merge_request_diff_files) }
+      let(:expected_diffs) { diffs }
+
+      include_examples 'updated MR diff'
+    end
+
+    context 'when the merge request diffs have binary content' do
+      let(:commits) { merge_request_diff.commits.map(&:to_hash) }
+      let(:expected_diffs) { diffs }
+
+      # The start of a PDF created by Illustrator
+      let(:binary_string) do
+        "\x25\x50\x44\x46\x2d\x31\x2e\x35\x0d\x25\xe2\xe3\xcf\xd3\x0d\x0a".force_encoding(Encoding::BINARY)
+      end
+
+      let(:diffs) do
+        [
+          {
+            'diff' => binary_string,
+            'new_path' => 'path',
+            'old_path' => 'path',
+            'a_mode' => '100644',
+            'b_mode' => '100644',
+            'new_file' => false,
+            'renamed_file' => false,
+            'deleted_file' => false,
+            'too_large' => false
+          }
+        ]
+      end
+
+      include_examples 'updated MR diff'
+    end
+
+    context 'when the merge request diff has commits, but no diffs' do
+      let(:commits) { merge_request_diff.commits.map(&:to_hash) }
+      let(:diffs) { [] }
+      let(:expected_diffs) { diffs }
+
+      include_examples 'updated MR diff'
+    end
+
+    context 'when the merge request diffs have invalid content' do
+      let(:commits) { merge_request_diff.commits.map(&:to_hash) }
+      let(:diffs) { ['--broken-diff'] }
+      let(:expected_diffs) { [] }
+
+      include_examples 'updated MR diff'
+    end
+
+    context 'when the merge request diffs are Rugged::Patch instances' do
+      let(:commits) { merge_request_diff.commits.map(&:to_hash) }
+      let(:first_commit) { merge_request.project.repository.commit(merge_request_diff.head_commit_sha) }
+      let(:diffs) { first_commit.diff_from_parent.patches }
+      let(:expected_diffs) { [] }
+
+      include_examples 'updated MR diff'
+    end
+
+    context 'when the merge request diffs are Rugged::Diff::Delta instances' do
+      let(:commits) { merge_request_diff.commits.map(&:to_hash) }
+      let(:first_commit) { merge_request.project.repository.commit(merge_request_diff.head_commit_sha) }
+      let(:diffs) { first_commit.diff_from_parent.deltas }
+      let(:expected_diffs) { [] }
+
+      include_examples 'updated MR diff'
+    end
+  end
+end

spec/lib/gitlab/git/repository_spec.rb

@@ -361,20 +361,20 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#commit_count' do
-    shared_examples 'counting commits' do
+    shared_examples 'simple commit counting' do
       it { expect(repository.commit_count("master")).to eq(25) }
       it { expect(repository.commit_count("feature")).to eq(9) }
     end
 
     context 'when Gitaly commit_count feature is enabled' do
-      it_behaves_like 'counting commits'
+      it_behaves_like 'simple commit counting'
       it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::CommitService, :commit_count do
         subject { repository.commit_count('master') }
       end
     end
 
     context 'when Gitaly commit_count feature is disabled', skip_gitaly_mock: true  do
-      it_behaves_like 'counting commits'
+      it_behaves_like 'simple commit counting'
     end
   end
 
@@ -797,29 +797,39 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#count_commits' do
-    context 'with after timestamp' do
-      it 'returns the number of commits after timestamp' do
-        options = { ref: 'master', limit: nil, after: Time.iso8601('2013-03-03T20:15:01+00:00') }
+    shared_examples 'extended commit counting' do
+      context 'with after timestamp' do
+        it 'returns the number of commits after timestamp' do
+          options = { ref: 'master', limit: nil, after: Time.iso8601('2013-03-03T20:15:01+00:00') }
 
-        expect(repository.count_commits(options)).to eq(25)
+          expect(repository.count_commits(options)).to eq(25)
+        end
       end
-    end
 
-    context 'with before timestamp' do
-      it 'returns the number of commits after timestamp' do
-        options = { ref: 'feature', limit: nil, before: Time.iso8601('2015-03-03T20:15:01+00:00') }
+      context 'with before timestamp' do
+        it 'returns the number of commits before timestamp' do
+          options = { ref: 'feature', limit: nil, before: Time.iso8601('2015-03-03T20:15:01+00:00') }
 
-        expect(repository.count_commits(options)).to eq(9)
+          expect(repository.count_commits(options)).to eq(9)
+        end
       end
-    end
 
-    context 'with path' do
-      it 'returns the number of commits with path ' do
-        options = { ref: 'master', limit: nil, path: "encoding" }
+      context 'with path' do
+        it 'returns the number of commits with path ' do
+          options = { ref: 'master', limit: nil, path: "encoding" }
 
-        expect(repository.count_commits(options)).to eq(2)
+          expect(repository.count_commits(options)).to eq(2)
+        end
       end
     end
+
+    context 'when Gitaly count_commits feature is enabled' do
+      it_behaves_like 'extended commit counting'
+    end
+
+    context 'when Gitaly count_commits feature is disabled', skip_gitaly_mock: true do
+      it_behaves_like 'extended commit counting'
+    end
   end
 
   describe "branch_names_contains" do

spec/migrations/schedule_merge_request_diff_migrations_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20170703130158_schedule_merge_request_diff_migrations')
+
+describe ScheduleMergeRequestDiffMigrations, :migration, :sidekiq do
+  matcher :be_scheduled_migration do |time, *expected|
+    match do |migration|
+      BackgroundMigrationWorker.jobs.any? do |job|
+        job['args'] == [migration, expected] &&
+          job['at'].to_i == time.to_i
+      end
+    end
+
+    failure_message do |migration|
+      "Migration `#{migration}` with args `#{expected.inspect}` not scheduled!"
+    end
+  end
+
+  let(:merge_request_diffs) { table(:merge_request_diffs) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:projects) { table(:projects) }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+
+    projects.create!(id: 1, name: 'gitlab', path: 'gitlab')
+
+    merge_requests.create!(id: 1, target_project_id: 1, source_project_id: 1, target_branch: 'feature', source_branch: 'master')
+
+    merge_request_diffs.create!(id: 1, merge_request_id: 1, st_commits: YAML.dump([]), st_diffs: nil)
+    merge_request_diffs.create!(id: 2, merge_request_id: 1, st_commits: nil, st_diffs: YAML.dump([]))
+    merge_request_diffs.create!(id: 3, merge_request_id: 1, st_commits: nil, st_diffs: nil)
+    merge_request_diffs.create!(id: 4, merge_request_id: 1, st_commits: YAML.dump([]), st_diffs: YAML.dump([]))
+  end
+
+  it 'correctly schedules background migrations' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(described_class::MIGRATION).to be_scheduled_migration(5.minutes.from_now, 1, 1)
+        expect(described_class::MIGRATION).to be_scheduled_migration(10.minutes.from_now, 2, 2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(15.minutes.from_now, 4, 4)
+        expect(BackgroundMigrationWorker.jobs.size).to eq 3
+      end
+    end
+  end
+
+  it 'schedules background migrations' do
+    Sidekiq::Testing.inline! do
+      non_empty = 'st_commits IS NOT NULL OR st_diffs IS NOT NULL'
+
+      expect(merge_request_diffs.where(non_empty).count).to eq 3
+
+      migrate!
+
+      expect(merge_request_diffs.where(non_empty).count).to eq 0
+    end
+  end
+end

spec/models/key_spec.rb

@@ -92,15 +92,17 @@ describe Key, :mailer do
       expect(key).not_to be_valid
     end
 
-    it 'rejects the unfingerprintable key (not a key)' do
-      expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid
+    it 'accepts a key with newline charecters after stripping them' do
+      key = build(:key)
+      key.key = key.key.insert(100, "\n")
+      key.key = key.key.insert(40, "\r\n")
+      expect(key).to be_valid
     end
 
-    it 'rejects the multiple line key' do
-      key = build(:key)
-      key.key.tr!(' ', "\n")
-      expect(key).not_to be_valid
+    it 'rejects the unfingerprintable key (not a key)' do
+      expect(build(:key, key: 'ssh-rsa an-invalid-key==')).not_to be_valid
     end
+    
   end
 
   context 'callbacks' do

spec/routing/project_routing_spec.rb

@@ -165,15 +165,19 @@ describe 'project routing' do
   #     edit_project_repository GET    /:project_id/repository/edit(.:format)     projects/repositories#edit
   describe Projects::RepositoriesController, 'routing' do
     it 'to #archive' do
-      expect(get('/gitlab/gitlabhq/repository/archive')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq')
+      expect(get('/gitlab/gitlabhq/repository/master/archive')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', ref: 'master')
     end
 
     it 'to #archive format:zip' do
-      expect(get('/gitlab/gitlabhq/repository/archive.zip')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'zip')
+      expect(get('/gitlab/gitlabhq/repository/master/archive.zip')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'zip', ref: 'master')
     end
 
     it 'to #archive format:tar.bz2' do
-      expect(get('/gitlab/gitlabhq/repository/archive.tar.bz2')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'tar.bz2')
+      expect(get('/gitlab/gitlabhq/repository/master/archive.tar.bz2')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', format: 'tar.bz2', ref: 'master')
+    end
+
+    it 'to #archive with "/" in route' do
+      expect(get('/gitlab/gitlabhq/repository/improve/awesome/archive')).to route_to('projects/repositories#archive', namespace_id: 'gitlab', project_id: 'gitlabhq', ref: 'improve/awesome')
     end
   end
 

spec/services/todo_service_spec.rb

@@ -336,7 +336,7 @@ describe TodoService do
 
     describe '#mark_todos_as_done' do
       it_behaves_like 'updating todos state', :mark_todos_as_done, :pending, :done do
-        let(:collection) { [first_todo, second_todo] }
+        let(:collection) { Todo.all }
       end
     end
 
@@ -348,7 +348,7 @@ describe TodoService do
 
     describe '#mark_todos_as_pending' do
       it_behaves_like 'updating todos state', :mark_todos_as_pending, :done, :pending do
-        let(:collection) { [first_todo, second_todo] }
+        let(:collection) { Todo.all }
       end
     end
 
@@ -910,14 +910,16 @@ describe TodoService do
     it 'marks an array of todos as done' do
       todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
 
-      expect { described_class.new.mark_todos_as_done([todo], john_doe) }
+      todos = TodosFinder.new(john_doe, {}).execute
+      expect { described_class.new.mark_todos_as_done(todos, john_doe) }
         .to change { todo.reload.state }.from('pending').to('done')
     end
 
     it 'returns the ids of updated todos' do # Needed on API
       todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
 
-      expect(described_class.new.mark_todos_as_done([todo], john_doe)).to eq([todo.id])
+      todos = TodosFinder.new(john_doe, {}).execute
+      expect(described_class.new.mark_todos_as_done(todos, john_doe)).to eq([todo.id])
     end
 
     context 'when some of the todos are done already' do
@@ -937,11 +939,32 @@ describe TodoService do
         expect(described_class.new.mark_todos_as_done(Todo.all, john_doe)).to eq([])
       end
     end
+  end
+
+  describe '#mark_todos_as_done_by_ids' do
+    let(:issue) { create(:issue, project: project, author: author, assignees: [john_doe]) }
+    let(:another_issue) { create(:issue, project: project, author: author, assignees: [john_doe]) }
+
+    it 'marks an array of todo ids as done' do
+      todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
+      another_todo = create(:todo, :mentioned, user: john_doe, target: another_issue, project: project)
+
+      expect { described_class.new.mark_todos_as_done_by_ids([todo.id, another_todo.id], john_doe) }
+        .to change { john_doe.todos.done.count }.from(0).to(2)
+    end
+
+    it 'marks a single todo id as done' do
+      todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
+
+      expect { described_class.new.mark_todos_as_done_by_ids(todo.id, john_doe) }
+        .to change { todo.reload.state }.from('pending').to('done')
+    end
 
     it 'caches the number of todos of a user', :use_clean_rails_memory_store_caching do
       create(:todo, :mentioned, user: john_doe, target: issue, project: project)
       todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
-      described_class.new.mark_todos_as_done([todo], john_doe)
+
+      described_class.new.mark_todos_as_done_by_ids(todo, john_doe)
 
       expect_any_instance_of(TodosFinder).not_to receive(:execute)