Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ce into ce-upstream

CHANGELOG

@@ -22,6 +22,7 @@ v 8.5.0 (unreleased)
   - Fix visibility level text in admin area (Zeger-Jan van de Weg)
   - Warn admin during OAuth of granting admin rights (Zeger-Jan van de Weg)
   - Update the ExternalIssue regex pattern (Blake Hitchcock)
+  - Remember user's inline/side-by-side diff view preference in a cookie (Kirill Katsnelson)
   - Optimized performance of finding issues to be closed by a merge request
   - Revert "Add IP check against DNSBLs at account sign-up"
   - Fix API to keep request parameters in Link header (Michael Potthoff)
@@ -35,6 +36,7 @@ v 8.5.0 (unreleased)
   - Fixed logo animation on Safari (Roman Rott)
   - Hide remove source branch button when the MR is merged but new commits are pushed (Zeger-Jan van de Weg)
   - In seach autocomplete show only groups and projects you are member of
+  - Fix: init.d script not working on OS X
 
 v 8.4.4
   - Update omniauth-saml gem to 1.4.2

CONTRIBUTING.md

@@ -15,10 +15,10 @@
       - [Regression issues](#regression-issues)
   - [Merge requests](#merge-requests)
       - [Merge request guidelines](#merge-request-guidelines)
+      - [Merge request description format](#merge-request-description-format)
+      - [Contribution acceptance criteria](#contribution-acceptance-criteria)
   - [Changes for Stable Releases](#changes-for-stable-releases)
   - [Definition of done](#definition-of-done)
-  - [Merge request description format](#merge-request-description-format)
-  - [Contribution acceptance criteria](#contribution-acceptance-criteria)
   - [Style guides](#style-guides)
   - [Code of conduct](#code-of-conduct)
 
@@ -260,15 +260,17 @@ request is as follows:
 1. Add your changes to the [CHANGELOG](CHANGELOG)
 1. If you are changing the README, some documentation or other things which
    have no effect on the tests, add `[ci skip]` somewhere in the commit message
+   and make sure to read the [documentation styleguide][doc-styleguide]
 1. If you have multiple commits please combine them into one commit by
    [squashing them][git-squash]
 1. Push the commit(s) to your fork
 1. Submit a merge request (MR) to the master branch
 1. The MR title should describe the change you want to make
 1. The MR description should give a motive for your change and the method you
-   used to achieve it
+   used to achieve it, see the [merge request description format]
+   (#merge-request-description-format)
 1. If the MR changes the UI it should include before and after screenshots
-1. If the MR changes CSS classes please include the list of affected pages
+1. If the MR changes CSS classes please include the list of affected pages,
    `grep css-class ./app -R`
 1. Link any relevant [issues][ce-tracker] in the merge request description and
    leave a comment on them with a link back to the MR
@@ -301,7 +303,55 @@ For examples of feedback on merge requests please look at already
 request feel free to mention one of the Merge Marshalls of the [core team][].
 Please ensure that your merge request meets the contribution acceptance criteria.
 
-When having your code reviewed and when reviewing merge requests please take the [thoughtbot code review guidelines](https://github.com/thoughtbot/guides/tree/master/code-review) into account.
+When having your code reviewed and when reviewing merge requests please take the
+[thoughtbot code review guidelines](https://github.com/thoughtbot/guides/tree/master/code-review)
+into account.
+
+### Merge request description format
+
+Please submit merge requests using the following template in the merge request
+description area. Copy-paste it to retain the markdown format.
+
+```
+## What does this MR do?
+
+## Are there points in the code the reviewer needs to double check?
+
+## Why was this MR needed?
+
+## What are the relevant issue numbers?
+
+## Screenshots (if relevant)
+```
+
+### Contribution acceptance criteria
+
+1. The change is as small as possible
+1. Include proper tests and make all tests pass (unless it contains a test
+   exposing a bug in existing code)
+1. If you suspect a failing CI build is unrelated to your contribution, you may
+   try and restart the failing CI job or ask a developer to fix the
+   aforementioned failing test
+1. Your MR initially contains a single commit (please use `git rebase -i` to
+   squash commits)
+1. Your changes can merge without problems (if not please merge `master`, never
+   rebase commits pushed to the remote server)
+1. Does not break any existing functionality
+1. Fixes one specific issue or implements one specific feature (do not combine
+   things, send separate merge requests if needed)
+1. Migrations should do only one thing (e.g., either create a table, move data
+   to a new table or remove an old table) to aid retrying on failure
+1. Keeps the GitLab code base clean and well structured
+1. Contains functionality we think other users will benefit from too
+1. Doesn't add configuration options since they complicate future changes
+1. Changes after submitting the merge request should be in separate commits
+   (no squashing). If necessary, you will be asked to squash when the review is
+   over, before merging.
+1. It conforms to the [style guides](#style-guides) and the following:
+    - If your change touches a line that does not follow the style, modify the
+      entire line to follow it. This prevents linting tools from generating warnings.
+    - Don't touch neighbouring lines. As an exception, automatic mass
+      refactoring modifications may leave style non-compliant.
 
 ## Changes for Stable Releases
 
@@ -324,7 +374,7 @@ the feature you contribute through all of these steps.
 1. Description explaining the relevancy (see following item)
 1. Working and clean code that is commented where needed
 1. Unit and integration tests that pass on the CI server
-1. Documented in the /doc directory
+1. [Documented][doc-styleguide] in the /doc directory
 1. Changelog entry added
 1. Reviewed and any concerns are addressed
 1. Merged by the project lead
@@ -345,43 +395,6 @@ merge request:
 1. Test suite https://gitlab.com/gitlab-org/gitlab-ce/blob/master/scripts/prepare_build.sh
 1. Omnibus package creator https://gitlab.com/gitlab-org/omnibus-gitlab
 
-## Merge request description format
-
-1. What does this MR do?
-1. Are there points in the code the reviewer needs to double check?
-1. Why was this MR needed?
-1. What are the relevant issue numbers?
-1. Screenshots (if relevant)
-
-## Contribution acceptance criteria
-
-1. The change is as small as possible (see the above paragraph for details)
-1. Include proper tests and make all tests pass (unless it contains a test
-   exposing a bug in existing code)
-1. If you suspect a failing CI build is unrelated to your contribution, you may
-   try and restart the failing CI job or ask a developer to fix the
-   aforementioned failing test
-1. Your MR initially contains a single commit (please use `git rebase -i` to
-   squash commits)
-1. Your changes can merge without problems (if not please merge `master`, never
-   rebase commits pushed to the remote server)
-1. Does not break any existing functionality
-1. Fixes one specific issue or implements one specific feature (do not combine
-   things, send separate merge requests if needed)
-1. Migrations should do only one thing (eg: either create a table, move data to
-   a new table or remove an old table) to aid retrying on failure
-1. Keeps the GitLab code base clean and well structured
-1. Contains functionality we think other users will benefit from too
-1. Doesn't add configuration options since they complicate future changes
-1. Changes after submitting the merge request should be in separate commits
-   (no squashing). If necessary, you will be asked to squash when the review is
-   over, before merging.
-1. It conforms to the following style guides:
-    * If your change touches a line that does not follow the style, modify the
-      entire line to follow it. This prevents linting tools from generating warnings.
-    * Don't touch neighbouring lines. As an exception, automatic mass
-      refactoring modifications may leave style non-compliant.
-
 ## Style guides
 
 1.  [Ruby](https://github.com/bbatsov/ruby-style-guide).
@@ -396,7 +409,7 @@ merge request:
     contributors to enhance security
 1.  [Database Migrations](doc/development/migration_style_guide.md)
 1.  [Markdown](http://www.cirosantilli.com/markdown-styleguide)
-1.  [Documentation styleguide](doc/development/doc_styleguide.md)
+1.  [Documentation styleguide][doc-styleguide]
 1.  Interface text should be written subjectively instead of objectively. It
     should be the GitLab core team addressing a person. It should be written in
     present time and never use past tense (has been/was). For example instead
@@ -437,7 +450,7 @@ reported by emailing `contact@gitlab.com`.
 This Code of Conduct is adapted from the [Contributor Covenant][], version 1.1.0,
 available at [http://contributor-covenant.org/version/1/1/0/](http://contributor-covenant.org/version/1/1/0/).
 
-[core team]: https://about.gitlab.com/core-team/
+[core-team]: https://about.gitlab.com/core-team/
 [getting help page]: https://about.gitlab.com/getting-help/
 [Codetriage]: http://www.codetriage.com/gitlabhq/gitlabhq
 [up-for-grabs]: https://gitlab.com/gitlab-org/gitlab-ce/issues?label_name=up-for-grabs
@@ -458,3 +471,4 @@ available at [http://contributor-covenant.org/version/1/1/0/](http://contributor
 [Contributor Covenant]: http://contributor-covenant.org
 [rss-source]: https://github.com/bbatsov/ruby-style-guide/blob/master/README.md#source-code-layout
 [rss-naming]: https://github.com/bbatsov/ruby-style-guide/blob/master/README.md#naming
+[doc-styleguide]: doc/development/doc_styleguide.md "Documentation styleguide"

app/controllers/projects/application_controller.rb

@@ -28,6 +28,11 @@ class Projects::ApplicationController < ApplicationController
 
   private
 
+  def apply_diff_view_cookie!
+    view = params[:view] || cookies[:diff_view]
+    cookies.permanent[:diff_view] = params[:view] = view if view
+  end
+
   def builds_enabled
     return render_404 unless @project.builds_enabled?
   end

app/controllers/projects/commit_controller.rb

@@ -13,6 +13,8 @@ class Projects::CommitController < Projects::ApplicationController
   def show
     return git_not_found! unless @commit
 
+    apply_diff_view_cookie!
+
     @line_notes = commit.notes.inline
     @note = @project.build_commit_note(commit)
     @notes = commit.notes.not_inline.fresh

app/controllers/projects/merge_requests_controller.rb

@@ -57,6 +57,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def diffs
+    apply_diff_view_cookie!
+
     @commit = @merge_request.last_commit
     @base_commit = @merge_request.diff_base_commit
 

doc/install/requirements.md

@@ -66,7 +66,7 @@ If you have enough RAM memory and a recent CPU the speed of GitLab is mainly lim
 You need at least 2GB of addressable memory (RAM + swap) to install and use GitLab!
 With less memory GitLab will give strange errors during the reconfigure run and 500 errors during usage.
 
-- 512MB RAM + 1.5GB of swap is the absolute minimum but we strongly **advise against** this amount of memory. See the unicorn worker section below for more advise.
+- 512MB RAM + 1.5GB of swap is the absolute minimum but we strongly **advise against** this amount of memory. See the unicorn worker section below for more advice.
 - 1GB RAM + 1GB swap supports up to 100 users but it will be slow
 - **2GB RAM** is the **recommended** memory size and supports up to 100 users
 - 4GB RAM supports up to 1,000 users

lib/backup/manager.rb

 module Backup
   class Manager
     def pack
+      # Make sure there is a connection
+      ActiveRecord::Base.connection.reconnect!
+
       # saving additional informations
       s = {}
       s[:db_version]         = "#{ActiveRecord::Migrator.current_version}"

lib/gitlab/current_settings.rb

@@ -7,8 +7,8 @@ module Gitlab
         settings = nil
 
         if connect_to_db?
-          settings = ApplicationSetting.current
-          settings ||= ApplicationSetting.create_from_defaults unless ActiveRecord::Migrator.needs_migration?
+          settings = ::ApplicationSetting.current
+          settings ||= ::ApplicationSetting.create_from_defaults unless ActiveRecord::Migrator.needs_migration?
         end
 
         settings || fake_application_settings

lib/support/init.d/gitlab

@@ -38,7 +38,7 @@ web_server_pid_path="$pid_path/unicorn.pid"
 sidekiq_pid_path="$pid_path/sidekiq.pid"
 mail_room_enabled=false
 mail_room_pid_path="$pid_path/mail_room.pid"
-gitlab_workhorse_dir=$(cd $app_root/../gitlab-workhorse && pwd)
+gitlab_workhorse_dir=$(cd $app_root/../gitlab-workhorse 2> /dev/null && pwd)
 gitlab_workhorse_pid_path="$pid_path/gitlab-workhorse.pid"
 gitlab_workhorse_options="-listenUmask 0 -listenNetwork unix -listenAddr $socket_path/gitlab-workhorse.socket -authBackend http://127.0.0.1:8080 -authSocket $rails_socket -documentRoot $app_root/public"
 gitlab_workhorse_log="$app_root/log/gitlab-workhorse.log"
@@ -49,7 +49,7 @@ test -f /etc/default/gitlab && . /etc/default/gitlab
 
 # Switch to the app_user if it is not he/she who is running the script.
 if [ `whoami` != "$app_user" ]; then
-  eval su - "$app_user" -s $shell_path -c $(echo \")$0 "$@"$(echo \"); exit;
+  eval su - "$app_user" -c $(echo \")$shell_path -l -c \'$0 "$@"\'$(echo \"); exit;
 fi
 
 # Switch to the gitlab path, exit on failure.

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -188,7 +188,7 @@ describe Projects::MergeRequestsController do
         expect(response).to render_template('diffs')
       end
     end
-    
+
     context 'as json' do
       it 'renders the diffs template to a string' do
         go format: 'json'
@@ -199,6 +199,32 @@ describe Projects::MergeRequestsController do
     end
   end
 
+  describe 'GET diffs with view' do
+    def go(extra_params = {})
+      params = {
+        namespace_id: project.namespace.to_param,
+        project_id:   project.to_param,
+        id:           merge_request.iid
+      }
+
+      get :diffs, params.merge(extra_params)
+    end
+
+    it 'saves the preferred diff view in a cookie' do
+      go view: 'parallel'
+
+      expect(response.cookies['diff_view']).to eq('parallel')
+    end
+
+    it 'assigns :view param based on cookie' do
+      request.cookies['diff_view'] = 'parallel'
+
+      go
+
+      expect(controller.params[:view]).to eq 'parallel'
+    end
+  end
+
   describe 'GET commits' do
     def go(format: 'html')
       get :commits,

spec/factories/ci/builds.rb

-# == Schema Information
-#
-# Table name: builds
-#
-#  id                 :integer          not null, primary key
-#  project_id         :integer
-#  status             :string(255)
-#  finished_at        :datetime
-#  trace              :text
-#  created_at         :datetime
-#  updated_at         :datetime
-#  started_at         :datetime
-#  runner_id          :integer
-#  commit_id          :integer
-#  coverage           :float
-#  commands           :text
-#  job_id             :integer
-#  name               :string(255)
-#  deploy             :boolean          default(FALSE)
-#  options            :text
-#  allow_failure      :boolean          default(FALSE), not null
-#  stage              :string(255)
-#  trigger_request_id :integer
-#
-
-# Read about factories at https://github.com/thoughtbot/factory_girl
-
 FactoryGirl.define do
   factory :ci_build, class: Ci::Build do
     name 'test'
@@ -65,5 +38,20 @@ FactoryGirl.define do
         build.trace = 'BUILD TRACE'
       end
     end
+
+    trait :artifacts do
+      after(:create) do |build, _|
+        build.artifacts_file =
+          fixture_file_upload(Rails.root +
+                              'spec/fixtures/ci_build_artifacts.zip',
+                              'application/zip')
+
+        build.artifacts_metadata =
+          fixture_file_upload(Rails.root +
+                              'spec/fixtures/ci_build_artifacts_metadata.gz',
+                              'application/x-gzip')
+        build.save!
+      end
+    end
   end
 end

spec/requests/ci/api/builds_spec.rb

@@ -151,8 +151,8 @@ describe Ci::API::API do
     context "Artifacts" do
       let(:file_upload) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
       let(:file_upload2) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/gif') }
-      let(:commit) { FactoryGirl.create(:ci_commit, project: project) }
-      let(:build) { FactoryGirl.create(:ci_build, commit: commit, runner_id: runner.id) }
+      let(:commit) { create(:ci_commit, project: project) }
+      let(:build) { create(:ci_build, commit: commit, runner_id: runner.id) }
       let(:authorize_url) { ci_api("/builds/#{build.id}/artifacts/authorize") }
       let(:post_url) { ci_api("/builds/#{build.id}/artifacts") }
       let(:delete_url) { ci_api("/builds/#{build.id}/artifacts") }
@@ -160,12 +160,10 @@ describe Ci::API::API do
       let(:headers) { { "GitLab-Workhorse" => "1.0" } }
       let(:headers_with_token) { headers.merge(Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token) }
 
+      before { build.run! }
+
       describe "POST /builds/:id/artifacts/authorize" do
         context "should authorize posting artifact to running build" do
-          before do
-            build.run!
-          end
-
           it "using token as parameter" do
             post authorize_url, { token: build.token }, headers
             expect(response.status).to eq(200)
@@ -180,10 +178,6 @@ describe Ci::API::API do
         end
 
         context "should fail to post too large artifact" do
-          before do
-            build.run!
-          end
-
           it "using token as parameter" do
             stub_application_setting(max_artifacts_size: 0)
             post authorize_url, { token: build.token, filesize: 100 }, headers
@@ -197,8 +191,8 @@ describe Ci::API::API do
           end
         end
 
-        context "should get denied" do
-          it do
+        context 'token is invalid' do
+          it 'should respond with forbidden'do
             post authorize_url, { token: 'invalid', filesize: 100 }
             expect(response.status).to eq(403)
           end
@@ -206,17 +200,13 @@ describe Ci::API::API do
       end
 
       describe "POST /builds/:id/artifacts" do
-        context "Disable sanitizer" do
+        context "disable sanitizer" do
           before do
             # by configuring this path we allow to pass temp file from any path
             allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/')
           end
 
           context "should post artifact to running build" do
-            before do
-              build.run!
-            end
-
             it "uses regual file post" do
               upload_artifacts(file_upload, headers_with_token, false)
               expect(response.status).to eq(201)
@@ -244,10 +234,7 @@ describe Ci::API::API do
             let(:stored_artifacts_file) { build.reload.artifacts_file.file }
             let(:stored_metadata_file) { build.reload.artifacts_metadata.file }
 
-            before do
-              build.run!
-              post(post_url, post_data, headers_with_token)
-            end
+            before { post(post_url, post_data, headers_with_token) }
 
             context 'post data accelerated by workhorse is correct' do
               let(:post_data) do
@@ -257,11 +244,8 @@ describe Ci::API::API do
                   'metadata.name' => metadata.original_filename }
               end
 
-              it 'responds with valid status' do
-                expect(response.status).to eq(201)
-              end
-
               it 'stores artifacts and artifacts metadata' do
+                expect(response.status).to eq(201)
                 expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename)
                 expect(stored_metadata_file.original_filename).to eq(metadata.original_filename)
               end
@@ -282,56 +266,42 @@ describe Ci::API::API do
             end
           end
 
-
-          context "should fail to post too large artifact" do
-            before do
-              build.run!
-            end
-
-            it do
+          context "artifacts file is too large" do
+            it "should fail to post too large artifact" do
               stub_application_setting(max_artifacts_size: 0)
               upload_artifacts(file_upload, headers_with_token)
               expect(response.status).to eq(413)
             end
           end
 
-          context "should fail to post artifacts without file" do
-            before do
-              build.run!
-            end
-
-            it do
+          context "artifacts post request does not contain file" do
+            it "should fail to post artifacts without file" do
               post post_url, {}, headers_with_token
               expect(response.status).to eq(400)
             end
           end
 
-          context "should fail to post artifacts without GitLab-Workhorse" do
-            before do
-              build.run!
-            end
-
-            it do
+          context 'GitLab Workhorse is not configured' do
+            it "should fail to post artifacts without GitLab-Workhorse" do
               post post_url, { token: build.token }, {}
               expect(response.status).to eq(403)
             end
           end
         end
 
-        context "should fail to post artifacts for outside of tmp path" do
+        context "artifacts are being stored outside of tmp path" do
           before do
             # by configuring this path we allow to pass file from @tmpdir only
             # but all temporary files are stored in system tmp directory
             @tmpdir = Dir.mktmpdir
             allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir)
-            build.run!
           end
 
           after do
             FileUtils.remove_entry @tmpdir
           end
 
-          it do
+          it "should fail to post artifacts for outside of tmp path" do
             upload_artifacts(file_upload, headers_with_token)
             expect(response.status).to eq(400)
           end
@@ -349,33 +319,37 @@ describe Ci::API::API do
         end
       end
 
-      describe "DELETE /builds/:id/artifacts" do
-        before do
-          build.run!
-          post delete_url, token: build.token, file: file_upload
-        end
+      describe 'DELETE /builds/:id/artifacts' do
+        let(:build) { create(:ci_build, :artifacts) }
+        before { delete delete_url, token: build.token }
 
-        it "should delete artifact build" do
-          build.success
-          delete delete_url, token: build.token
+        it 'should remove build artifacts' do
           expect(response.status).to eq(200)
+          expect(build.artifacts_file.exists?).to be_falsy
+          expect(build.artifacts_metadata.exists?).to be_falsy
         end
       end
 
-      describe "GET /builds/:id/artifacts" do
-        before do
-          build.run!
-        end
+      describe 'GET /builds/:id/artifacts' do
+        before { get get_url, token: build.token }
 
-        it "should download artifact" do
-          build.update_attributes(artifacts_file: file_upload)
-          get get_url, token: build.token
-          expect(response.status).to eq(200)
+        context 'build has artifacts' do
+          let(:build) { create(:ci_build, :artifacts) }
+          let(:download_headers) do
+            { 'Content-Transfer-Encoding'=>'binary',
+              'Content-Disposition'=>'attachment; filename=ci_build_artifacts.zip' }
+          end
+
+          it 'should download artifact' do
+            expect(response.status).to eq(200)
+            expect(response.headers).to include download_headers
+          end
         end
 
-        it "should fail to download if no artifact uploaded" do
-          get get_url, token: build.token
-          expect(response.status).to eq(404)
+        context 'build does not has artifacts' do
+          it 'should respond with not found' do
+            expect(response.status).to eq(404)
+          end
         end
       end
     end