diff options
| -rw-r--r-- | CHANGELOG | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 2 | ||||
| -rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 15 | ||||
| -rw-r--r-- | app/controllers/projects/blame_controller.rb | 24 | ||||
| -rw-r--r-- | app/controllers/projects/compare_controller.rb | 2 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 6 | ||||
| -rw-r--r-- | app/models/merge_request_diff.rb | 28 | ||||
| -rw-r--r-- | app/models/project.rb | 5 | ||||
| -rw-r--r-- | app/models/repository.rb | 2 | ||||
| -rw-r--r-- | app/views/projects/blame/show.html.haml | 10 | ||||
| -rw-r--r-- | doc/administration/environment_variables.md | 1 | ||||
| -rw-r--r-- | lib/gitlab/blame.rb | 54 | ||||
| -rw-r--r-- | spec/controllers/blame_controller_spec.rb | 14 | ||||
| -rw-r--r-- | spec/lib/gitlab/blame_spec.rb | 24 | ||||
| -rw-r--r-- | spec/models/build_spec.rb | 2 |
15 files changed, 118 insertions, 73 deletions
diff --git a/CHANGELOG b/CHANGELOG index 145d4c03731..fe0504ec996 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -27,8 +27,10 @@ v 8.4.2 improvement when checking if a repository was empty - Add instrumentation for Gitlab::Git::Repository instance methods so we can track them in Performance Monitoring. + - Correctly highlight MR diff when MR has merge conflicts - Increase contrast between highlighted code comments and inline diff marker - Fix method undefined when using external commit status in builds + - Fix highlighting in blame view. v 8.4.1 - Apply security updates for Rails (4.2.5.1), rails-html-sanitizer (1.0.3), diff --git a/Gemfile.lock b/Gemfile.lock index d2c4ddfba56..ec92964df25 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -725,7 +725,7 @@ GEM activesupport (>= 3.1, < 4.3) select2-rails (3.5.9.3) thor (~> 0.14) - sentry-raven (0.15.3) + sentry-raven (0.15.4) faraday (>= 0.7.6) settingslogic (2.0.9) sexp_processor (4.6.0) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 4cad98b8e98..4c13228fce9 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -21,15 +21,16 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # We only find ourselves here # if the authentication to LDAP was successful. def ldap - @user = Gitlab::LDAP::User.new(oauth) - @user.save if @user.changed? # will also save new users - gl_user = @user.gl_user - gl_user.remember_me = params[:remember_me] if @user.persisted? + ldap_user = Gitlab::LDAP::User.new(oauth) + ldap_user.save if ldap_user.changed? # will also save new users + + @user = ldap_user.gl_user + @user.remember_me = params[:remember_me] if ldap_user.persisted? # Do additional LDAP checks for the user filter and EE features - if @user.allowed? - log_audit_event(gl_user, with: :ldap) - sign_in_and_redirect(gl_user) + if ldap_user.allowed? + log_audit_event(@user, with: :ldap) + sign_in_and_redirect(@user) else flash[:alert] = "Access denied for your LDAP account." redirect_to new_user_session_path diff --git a/app/controllers/projects/blame_controller.rb b/app/controllers/projects/blame_controller.rb index 9ea518e6c85..f576d0be1fc 100644 --- a/app/controllers/projects/blame_controller.rb +++ b/app/controllers/projects/blame_controller.rb @@ -8,28 +8,6 @@ class Projects::BlameController < Projects::ApplicationController def show @blob = @repository.blob_at(@commit.id, @path) - @blame = group_blame_lines - end - - def group_blame_lines - blame = Gitlab::Git::Blame.new(@repository, @commit.id, @path) - - prev_sha = nil - groups = [] - current_group = nil - - blame.each do |commit, line| - if prev_sha && prev_sha == commit.sha - current_group[:lines] << line - else - groups << current_group if current_group.present? - current_group = { commit: commit, lines: [line] } - end - - prev_sha = commit.sha - end - - groups << current_group if current_group.present? - groups + @blame_groups = Gitlab::Blame.new(@blob, @commit).groups end end diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index f8ec76cd4e5..7bbe75b3974 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -21,7 +21,7 @@ class Projects::CompareController < Projects::ApplicationController @commits = Commit.decorate(compare_result.commits, @project) @diffs = compare_result.diffs @commit = @project.commit(head_ref) - @base_commit = @project.commit(base_ref) + @base_commit = @project.merge_base_commit(base_ref, head_ref) @diff_refs = [@base_commit, @commit] @line_notes = [] end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 41dd248d80a..0af60645545 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -183,8 +183,8 @@ class MergeRequest < ActiveRecord::Base def diff_base_commit if merge_request_diff merge_request_diff.base_commit - else - self.target_project.commit(self.target_branch) + elsif source_sha + self.target_project.merge_base_commit(self.source_sha, self.target_branch) end end @@ -489,7 +489,7 @@ class MergeRequest < ActiveRecord::Base end def source_sha - commits.first.sha + last_commit.try(:sha) end def fetch_ref diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index ba0194cd0a6..c95179d6046 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -48,14 +48,11 @@ class MergeRequestDiff < ActiveRecord::Base end def diffs_no_whitespace - # Get latest sha of branch from source project - source_sha = merge_request.source_project.commit(source_branch).sha - compare_result = Gitlab::CompareResult.new( Gitlab::Git::Compare.new( - merge_request.target_project.repository.raw_repository, - merge_request.target_branch, - source_sha, + self.repository.raw_repository, + self.target_branch, + self.source_sha, ), { ignore_whitespace_change: true } ) @diffs_no_whitespace ||= load_diffs(dump_commits(compare_result.diffs)) @@ -83,8 +80,6 @@ class MergeRequestDiff < ActiveRecord::Base @last_commit_short_sha ||= last_commit.short_id end - private - def dump_commits(commits) commits.map(&:to_hash) end @@ -163,7 +158,7 @@ class MergeRequestDiff < ActiveRecord::Base self.st_diffs = new_diffs - self.base_commit_sha = merge_request.target_project.commit(target_branch).try(:sha) + self.base_commit_sha = self.repository.merge_base(self.source_sha, self.target_branch) self.save end @@ -181,7 +176,10 @@ class MergeRequestDiff < ActiveRecord::Base merge_request.target_project.repository end - private + def source_sha + source_commit = merge_request.source_project.commit(source_branch) + source_commit.try(:sha) + end def compare_result @compare_result ||= @@ -189,15 +187,11 @@ class MergeRequestDiff < ActiveRecord::Base # Update ref for merge request merge_request.fetch_ref - # Get latest sha of branch from source project - source_commit = merge_request.source_project.commit(source_branch) - source_sha = source_commit.try(:sha) - Gitlab::CompareResult.new( Gitlab::Git::Compare.new( - merge_request.target_project.repository.raw_repository, - merge_request.target_branch, - source_sha, + self.repository.raw_repository, + self.target_branch, + self.source_sha ) ) end diff --git a/app/models/project.rb b/app/models/project.rb index 4bd51449c25..238932f59a7 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -348,6 +348,11 @@ class Project < ActiveRecord::Base repository.commit(id) end + def merge_base_commit(first_commit_id, second_commit_id) + sha = repository.merge_base(first_commit_id, second_commit_id) + repository.commit(sha) if sha + end + def saved? id && persisted? end diff --git a/app/models/repository.rb b/app/models/repository.rb index 6c1ee4b29cd..130daddd9d1 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -589,6 +589,8 @@ class Repository def merge_base(first_commit_id, second_commit_id) rugged.merge_base(first_commit_id, second_commit_id) + rescue Rugged::ReferenceError + nil end def is_ancestor?(ancestor_id, descendant_id) diff --git a/app/views/projects/blame/show.html.haml b/app/views/projects/blame/show.html.haml index 53dcac78a9f..eb6fbfaffa0 100644 --- a/app/views/projects/blame/show.html.haml +++ b/app/views/projects/blame/show.html.haml @@ -15,12 +15,11 @@ .file-content.blame.code.js-syntax-highlight %table - current_line = 1 - - blame_highlighter = highlighter(@blob.name, @blob.data, nowrap: true) - - @blame.each do |blame_group| + - @blame_groups.each do |blame_group| %tr %td.blame-commit .commit - - commit = Commit.new(blame_group[:commit], @project) + - commit = blame_group[:commit] .commit-row-title %strong = link_to_gfm truncate(commit.title, length: 35), namespace_project_commit_path(@project.namespace, @project, commit.id), class: "cdark" @@ -38,8 +37,7 @@ \ - current_line += line_count %td.lines - %pre{class: 'code highlight'} + %pre.code.highlight %code - blame_group[:lines].each do |line| - :preserve - #{blame_highlighter.highlight(line)} + #{line} diff --git a/doc/administration/environment_variables.md b/doc/administration/environment_variables.md index 42a27dcf6d6..0faef526d43 100644 --- a/doc/administration/environment_variables.md +++ b/doc/administration/environment_variables.md @@ -47,6 +47,7 @@ GITLAB_DATABASE_PORT | 5432 ## Adding more variables We welcome merge requests to make more settings configurable via variables. +Please make changes in the file config/initializers/1_settings.rb Please stick to the naming scheme "GITLAB_#{name 1_settings.rb in upper case}". ## Omnibus configuration diff --git a/lib/gitlab/blame.rb b/lib/gitlab/blame.rb new file mode 100644 index 00000000000..313e6b9fc03 --- /dev/null +++ b/lib/gitlab/blame.rb @@ -0,0 +1,54 @@ +module Gitlab + class Blame + attr_accessor :blob, :commit + + def initialize(blob, commit) + @blob = blob + @commit = commit + end + + def groups(highlight: true) + prev_sha = nil + groups = [] + current_group = nil + + i = 0 + blame.each do |commit, line| + commit = Commit.new(commit, project) + + sha = commit.sha + if prev_sha != sha + groups << current_group if current_group + current_group = { commit: commit, lines: [] } + end + + line = highlighted_lines[i].html_safe if highlight + current_group[:lines] << line + + prev_sha = sha + i += 1 + end + groups << current_group if current_group + + groups + end + + private + + def blame + @blame ||= Gitlab::Git::Blame.new(repository, @commit.id, @blob.path) + end + + def highlighted_lines + @highlighted_lines ||= Gitlab::Highlight.highlight(@blob.name, @blob.data).lines + end + + def project + commit.project + end + + def repository + project.repository + end + end +end diff --git a/spec/controllers/blame_controller_spec.rb b/spec/controllers/blame_controller_spec.rb index 3ad4d5fc0a8..25f06299a29 100644 --- a/spec/controllers/blame_controller_spec.rb +++ b/spec/controllers/blame_controller_spec.rb @@ -24,20 +24,6 @@ describe Projects::BlameController do context "valid file" do let(:id) { 'master/files/ruby/popen.rb' } it { is_expected.to respond_with(:success) } - - it 'groups blames properly' do - blame = assigns(:blame) - # Sanity check a few items - expect(blame.count).to eq(18) - expect(blame[0][:commit].sha).to eq('913c66a37b4a45b9769037c55c2d238bd0942d2e') - expect(blame[0][:lines]).to eq(["require 'fileutils'", "require 'open3'", ""]) - - expect(blame[1][:commit].sha).to eq('874797c3a73b60d2187ed6e2fcabd289ff75171e') - expect(blame[1][:lines]).to eq(["module Popen", " extend self"]) - - expect(blame[-1][:commit].sha).to eq('913c66a37b4a45b9769037c55c2d238bd0942d2e') - expect(blame[-1][:lines]).to eq([" end", "end"]) - end end end end diff --git a/spec/lib/gitlab/blame_spec.rb b/spec/lib/gitlab/blame_spec.rb new file mode 100644 index 00000000000..89245761b6f --- /dev/null +++ b/spec/lib/gitlab/blame_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe Gitlab::Blame, lib: true do + let(:project) { create(:project) } + let(:path) { 'files/ruby/popen.rb' } + let(:commit) { project.commit('master') } + let(:blob) { project.repository.blob_at(commit.id, path) } + + describe "#groups" do + let(:subject) { described_class.new(blob, commit).groups(highlight: false) } + + it 'groups lines properly' do + expect(subject.count).to eq(18) + expect(subject[0][:commit].sha).to eq('913c66a37b4a45b9769037c55c2d238bd0942d2e') + expect(subject[0][:lines]).to eq(["require 'fileutils'", "require 'open3'", ""]) + + expect(subject[1][:commit].sha).to eq('874797c3a73b60d2187ed6e2fcabd289ff75171e') + expect(subject[1][:lines]).to eq(["module Popen", " extend self"]) + + expect(subject[-1][:commit].sha).to eq('913c66a37b4a45b9769037c55c2d238bd0942d2e') + expect(subject[-1][:lines]).to eq([" end", "end"]) + end + end +end diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index d30bc7d0554..606340d87e4 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Ci::Build, models: true do - let(:project) { FactoryGirl.create :empty_project } + let(:project) { FactoryGirl.create :project } let(:commit) { FactoryGirl.create :ci_commit, project: project } let(:build) { FactoryGirl.create :ci_build, commit: commit } |