diff options
26 files changed, 339 insertions, 97 deletions
diff --git a/CHANGELOG b/CHANGELOG index 9e5d00c01f8..b6532cafbcd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -8,6 +8,7 @@ v 8.11.0 (unreleased) - Convert switch icon into icon font (ClemMakesApps) - Remove magic comments (`# encoding: UTF-8`) from Ruby files. !5456 (winniehell) - Add support for relative links starting with ./ or / to RelativeLinkFilter (winniehell) + - Ignore URLs starting with // in Markdown links !5677 (winniehell) - Fix CI status icon link underline (ClemMakesApps) - The Repository class is now instrumented - Cache the commit author in RequestStore to avoid extra lookups in PostReceive @@ -64,11 +65,14 @@ v 8.11.0 (unreleased) - Make error pages responsive (Takuya Noguchi) - Fix skip_repo parameter being ignored when destroying a namespace - Change requests_profiles resource constraint to catch virtually any file + - Bump gitlab_git to lazy load compare commits - Reduce number of queries made for merge_requests/:id/diffs - Sensible state specific default sort order for issues and merge requests !5453 (tomb0y) - Fix RequestProfiler::Middleware error when code is reloaded in development - Catch what warden might throw when profiling requests to re-throw it + - Add description to new_issue email and new_merge_request_email in text/plain content type. !5663 (dixpac) - Speed up and reduce memory usage of Commit#repo_changes, Repository#expire_avatar_cache and IrkerWorker + - Add unfold links for Side-by-Side view. !5415 (Tim Masliuchenko) v 8.10.5 (unreleased) @@ -53,7 +53,7 @@ gem 'browser', '~> 2.2' # Extracting information from a git repository # Provide access to Gitlab::Git library -gem 'gitlab_git', '~> 10.4.3' +gem 'gitlab_git', '~> 10.4.5' # LDAP Auth # GitLab fork with several improvements to original library. For full list of changes @@ -326,7 +326,7 @@ group :production do gem 'gitlab_meta', '7.0' end -gem 'newrelic_rpm', '~> 3.14' +gem 'newrelic_rpm', '~> 3.16' gem 'octokit', '~> 4.3.0' diff --git a/Gemfile.lock b/Gemfile.lock index 866f5014847..870f9397b9e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -278,7 +278,7 @@ GEM diff-lcs (~> 1.1) mime-types (>= 1.16, < 3) posix-spawn (~> 0.3) - gitlab_git (10.4.3) + gitlab_git (10.4.5) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) @@ -404,7 +404,7 @@ GEM nested_form (0.3.2) net-ldap (0.12.1) net-ssh (3.0.1) - newrelic_rpm (3.14.1.311) + newrelic_rpm (3.16.0.318) nokogiri (1.6.8) mini_portile2 (~> 2.1.0) pkg-config (~> 1.1.7) @@ -870,7 +870,7 @@ DEPENDENCIES github-linguist (~> 4.7.0) github-markup (~> 1.4) gitlab-flowdock-git-hook (~> 1.0.1) - gitlab_git (~> 10.4.3) + gitlab_git (~> 10.4.5) gitlab_meta (= 7.0) gitlab_omniauth-ldap (~> 1.2.1) gollum-lib (~> 4.2) @@ -902,7 +902,7 @@ DEPENDENCIES mysql2 (~> 0.3.16) nested_form (~> 0.3.2) net-ssh (~> 3.0.1) - newrelic_rpm (~> 3.14) + newrelic_rpm (~> 3.16) nokogiri (~> 1.6.7, >= 1.6.7.2) oauth2 (~> 1.2.0) octokit (~> 4.3.0) diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index 298f3852085..3dd7ceba92f 100644 --- a/app/assets/javascripts/diff.js +++ b/app/assets/javascripts/diff.js @@ -10,7 +10,7 @@ $(document).off('click', '.js-unfold'); $(document).on('click', '.js-unfold', (function(_this) { return function(event) { - var line_number, link, offset, old_line, params, prev_new_line, prev_old_line, ref, ref1, since, target, to, unfold, unfoldBottom; + var line_number, link, file, offset, old_line, params, prev_new_line, prev_old_line, ref, ref1, since, target, to, unfold, unfoldBottom; target = $(event.target); unfoldBottom = target.hasClass('js-unfold-bottom'); unfold = true; @@ -31,14 +31,16 @@ unfold = false; } } - link = target.parents('.diff-file').attr('data-blob-diff-path'); + file = target.parents('.diff-file'); + link = file.data('blob-diff-path'); params = { since: since, to: to, bottom: unfoldBottom, offset: offset, unfold: unfold, - indent: 1 + indent: 1, + view: file.data('view') }; return $.get(link, params, function(response) { return target.parent().replaceWith(response); @@ -48,26 +50,13 @@ } Diff.prototype.lineNumbers = function(line) { - var i, l, len, line_number, line_numbers, lines, results; if (!line.children().length) { return [0, 0]; } - lines = line.children().slice(0, 2); - line_numbers = (function() { - var i, len, results; - results = []; - for (i = 0, len = lines.length; i < len; i++) { - l = lines[i]; - results.push($(l).attr('data-linenumber')); - } - return results; - })(); - results = []; - for (i = 0, len = line_numbers.length; i < len; i++) { - line_number = line_numbers[i]; - results.push(parseInt(line_number)); - } - return results; + + return line.find('.diff-line-num').map(function() { + return parseInt($(this).data('linenumber')); + }); }; return Diff; diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index eda3727a28d..19d051720e9 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -76,6 +76,8 @@ class Projects::BlobController < Projects::ApplicationController end def diff + apply_diff_view_cookie! + @form = UnfoldForm.new(params) @lines = Gitlab::Highlight.highlight_lines(repository, @ref, @path) @lines = @lines[@form.since - 1..@form.to - 1] diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index cc7121b1163..f3c9ea074b4 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -13,12 +13,11 @@ module DiffHelper end def diff_view - diff_views = %w(inline parallel) - - if diff_views.include?(cookies[:diff_view]) - cookies[:diff_view] - else - diff_views.first + @diff_view ||= begin + diff_views = %w(inline parallel) + diff_view = cookies[:diff_view] + diff_view = diff_views.first unless diff_views.include?(diff_view) + diff_view.to_sym end end @@ -33,12 +32,23 @@ module DiffHelper options end - def unfold_bottom_class(bottom) - bottom ? 'js-unfold js-unfold-bottom' : '' - end + def diff_match_line(old_pos, new_pos, text: '', view: :inline, bottom: false) + content = content_tag :td, text, class: "line_content match #{view == :inline ? '' : view}" + cls = ['diff-line-num', 'unfold', 'js-unfold'] + cls << 'js-unfold-bottom' if bottom + + html = '' + if old_pos + html << content_tag(:td, '...', class: cls + ['old_line'], data: { linenumber: old_pos }) + html << content unless view == :inline + end + + if new_pos + html << content_tag(:td, '...', class: cls + ['new_line'], data: { linenumber: new_pos }) + html << content + end - def unfold_class(unfold) - unfold ? 'unfold js-unfold' : '' + html.html_safe end def diff_line_content(line, line_type = nil) @@ -67,11 +77,11 @@ module DiffHelper end def inline_diff_btn - diff_btn('Inline', 'inline', diff_view == 'inline') + diff_btn('Inline', 'inline', diff_view == :inline) end def parallel_diff_btn - diff_btn('Side-by-side', 'parallel', diff_view == 'parallel') + diff_btn('Side-by-side', 'parallel', diff_view == :parallel) end def submodule_link(blob, ref, repository = @repository) @@ -103,7 +113,8 @@ module DiffHelper commit = commit_for_diff(diff_file) { blob_diff_path: namespace_project_blob_diff_path(project.namespace, project, - tree_join(commit.id, diff_file.file_path)) + tree_join(commit.id, diff_file.file_path)), + view: diff_view } end diff --git a/app/models/ability.rb b/app/models/ability.rb index d95a2507199..d9113ffd99a 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -6,6 +6,10 @@ class Ability return [] unless user.is_a?(User) return [] if user.blocked? + abilities_by_subject_class(user: user, subject: subject) + end + + def abilities_by_subject_class(user:, subject:) case subject when CommitStatus then commit_status_abilities(user, subject) when Project then project_abilities(user, subject) diff --git a/app/views/notify/new_issue_email.text.erb b/app/views/notify/new_issue_email.text.erb index fc64c98038b..ca5c2f2688c 100644 --- a/app/views/notify/new_issue_email.text.erb +++ b/app/views/notify/new_issue_email.text.erb @@ -3,3 +3,5 @@ New Issue was created. Issue <%= @issue.iid %>: <%= url_for(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue)) %> Author: <%= @issue.author_name %> Assignee: <%= @issue.assignee_name %> + +<%= @issue.description %> diff --git a/app/views/notify/new_merge_request_email.text.erb b/app/views/notify/new_merge_request_email.text.erb index d4aad8d1862..3c8f178ac77 100644 --- a/app/views/notify/new_merge_request_email.text.erb +++ b/app/views/notify/new_merge_request_email.text.erb @@ -6,3 +6,5 @@ New Merge Request <%= @merge_request.to_reference %> Author: <%= @merge_request.author_name %> Assignee: <%= @merge_request.assignee_name %> +<%= @merge_request.description %> + diff --git a/app/views/projects/blob/diff.html.haml b/app/views/projects/blob/diff.html.haml index 5926d181ba3..a79ae53c780 100644 --- a/app/views/projects/blob/diff.html.haml +++ b/app/views/projects/blob/diff.html.haml @@ -1,20 +1,30 @@ - if @lines.present? + - line_class = diff_view == :inline ? '' : diff_view - if @form.unfold? && @form.since != 1 && !@form.bottom? - %tr.line_holder - = render "projects/diffs/match_line", { line: @match_line, - line_old: @form.since, line_new: @form.since, bottom: false, new_file: false } + %tr.line_holder{ class: line_class } + = diff_match_line @form.since, @form.since, text: @match_line, view: diff_view - @lines.each_with_index do |line, index| - line_new = index + @form.since - line_old = line_new - @form.offset - %tr.line_holder{ id: line_old } - %td.old_line.diff-line-num{ data: { linenumber: line_old } } - = link_to raw(line_old), "##{line_old}" - %td.new_line.diff-line-num{ data: { linenumber: line_old } } - = link_to raw(line_new) , "##{line_old}" - %td.line_content.noteable_line==#{' ' * @form.indent}#{line} + - line_content = capture do + %td.line_content.noteable_line{ class: line_class }==#{' ' * @form.indent}#{line} + %tr.line_holder{ id: line_old, class: line_class } + - case diff_view + - when :inline + %td.old_line.diff-line-num{ data: { linenumber: line_old } } + %a{href: "##{line_old}", data: { linenumber: line_old }} + %td.new_line.diff-line-num{ data: { linenumber: line_new } } + %a{href: "##{line_new}", data: { linenumber: line_new }} + = line_content + - when :parallel + %td.old_line.diff-line-num{data: { linenumber: line_old }} + = link_to raw(line_old), "##{line_old}" + = line_content + %td.new_line.diff-line-num{data: { linenumber: line_new }} + = link_to raw(line_new), "##{line_new}" + = line_content - if @form.unfold? && @form.bottom? && @form.to < @blob.loc - %tr.line_holder{ id: @form.to } - = render "projects/diffs/match_line", { line: @match_line, - line_old: @form.to, line_new: @form.to, bottom: true, new_file: false } + %tr.line_holder{ id: @form.to, class: line_class } + = diff_match_line @form.to, @form.to, text: @match_line, view: diff_view, bottom: true diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index a1b071f130c..d37961c4e40 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -13,7 +13,7 @@ .nothing-here-block.diff-collapsed{data: { diff_for_path: url } } This diff is collapsed. Click to expand it. - elsif diff_file.diff_lines.length > 0 - - if diff_view == 'parallel' + - if diff_view == :parallel = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob - else = render "projects/diffs/text_file", diff_file: diff_file diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index ebaf939f930..62aff36aadd 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -1,6 +1,6 @@ - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true) - diff_files = diffs.diff_files -- if diff_view == 'parallel' +- if diff_view == :parallel - fluid_layout true .content-block.oneline-block.files-changed diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index 4d3af905b58..2d6a370b848 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -4,8 +4,7 @@ %tr.line_holder{ plain ? { class: type} : { class: type, id: line_code } } - case type - when 'match' - = render "projects/diffs/match_line", { line: line.text, - line_old: line.old_pos, line_new: line.new_pos, bottom: false, new_file: diff_file.new_file } + = diff_match_line line.old_pos, line.new_pos, text: line.text - when 'nonewline' %td.old_line.diff-line-num %td.new_line.diff-line-num diff --git a/app/views/projects/diffs/_match_line.html.haml b/app/views/projects/diffs/_match_line.html.haml deleted file mode 100644 index d6dddd97879..00000000000 --- a/app/views/projects/diffs/_match_line.html.haml +++ /dev/null @@ -1,7 +0,0 @@ -%td.old_line.diff-line-num{data: {linenumber: line_old}, - class: [unfold_bottom_class(bottom), unfold_class(!new_file)]} - \... -%td.new_line.diff-line-num{data: {linenumber: line_new}, - class: [unfold_bottom_class(bottom), unfold_class(!new_file)]} - \... -%td.line_content.match= line diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 7f30faa20d8..28aad3f4725 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -1,14 +1,15 @@ / Side-by-side diff view %div.text-file.diff-wrap-lines.code.file-content.js-syntax-highlight{ data: diff_view_data } %table + - last_line = 0 - diff_file.parallel_diff_lines.each do |line| - left = line[:left] - right = line[:right] + - last_line = right.new_pos if right %tr.line_holder.parallel - if left - if left.meta? - %td.old_line.diff-line-num.empty-cell - %td.line_content.parallel.match= left.text + = diff_match_line left.old_pos, nil, text: left.text, view: :parallel - else - left_line_code = diff_file.line_code(left) - left_position = diff_file.position(left) @@ -21,8 +22,7 @@ - if right - if right.meta? - %td.old_line.diff-line-num.empty-cell - %td.line_content.parallel.match= left.text + = diff_match_line nil, right.new_pos, text: left.text, view: :parallel - else - right_line_code = diff_file.line_code(right) - right_position = diff_file.position(right) @@ -37,3 +37,5 @@ - discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file) - if discussion_left || discussion_right = render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right + - if !diff_file.new_file && last_line > 0 + = diff_match_line last_line, last_line, bottom: true, view: :parallel diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 5970b9abf2b..ab5463ba89d 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -15,6 +15,5 @@ - if discussion = render "discussions/diff_discussion", discussion: discussion - - if last_line > 0 - = render "projects/diffs/match_line", { line: "", - line_old: last_line, line_new: last_line, bottom: true, new_file: diff_file.new_file } + - if !diff_file.new_file && last_line > 0 + = diff_match_line last_line, last_line, bottom: true diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 873ed9b59ee..269198adf91 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -2,7 +2,7 @@ - page_description @merge_request.description - page_card_attributes @merge_request.card_attributes -- if diff_view == 'parallel' +- if diff_view == :parallel - fluid_layout true .merge-request{'data-url' => merge_request_path(@merge_request)} diff --git a/db/migrate/20160804150737_add_timestamps_to_members_again.rb b/db/migrate/20160804150737_add_timestamps_to_members_again.rb new file mode 100644 index 00000000000..6691ba57fbb --- /dev/null +++ b/db/migrate/20160804150737_add_timestamps_to_members_again.rb @@ -0,0 +1,21 @@ +# rubocop:disable all +# 20141121133009_add_timestamps_to_members.rb was meant to ensure that all +# rows in the members table had created_at and updated_at set, following an +# error in a previous migration. This failed to set all rows in at least one +# case: https://gitlab.com/gitlab-org/gitlab-ce/issues/20568 +# +# Why this happened is lost in the mists of time, so repeat the SQL query +# without speculation, just in case more than one person was affected. +class AddTimestampsToMembersAgain < ActiveRecord::Migration + DOWNTIME = false + + def up + execute "UPDATE members SET created_at = NOW() WHERE created_at IS NULL" + execute "UPDATE members SET updated_at = NOW() WHERE updated_at IS NULL" + end + + def down + # no change + end + +end diff --git a/db/schema.rb b/db/schema.rb index dc28842758a..71980a6d51f 100644 --- a/db/schema.rb +++ b/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: 20160802010328) do +ActiveRecord::Schema.define(version: 20160804150737) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/doc/development/README.md b/doc/development/README.md index 11aa50b89af..7b5f7ff8ad3 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -31,6 +31,7 @@ - [Rake tasks](rake_tasks.md) for development - [Shell commands](shell_commands.md) in the GitLab codebase - [Sidekiq debugging](sidekiq_debugging.md) +- [What requires downtime?](what_requires_downtime.md) ## Compliance diff --git a/doc/development/what_requires_downtime.md b/doc/development/what_requires_downtime.md new file mode 100644 index 00000000000..abd693cf72d --- /dev/null +++ b/doc/development/what_requires_downtime.md @@ -0,0 +1,153 @@ +# What requires downtime? + +When working with a database certain operations can be performed without taking +GitLab offline, others do require a downtime period. This guide describes +various operations and their impact. + +## Adding Columns + +On PostgreSQL you can safely add a new column to an existing table as long as it +does **not** have a default value. For example, this query would not require +downtime: + +```sql +ALTER TABLE projects ADD COLUMN random_value int; +``` + +Add a column _with_ a default however does require downtime. For example, +consider this query: + +```sql +ALTER TABLE projects ADD COLUMN random_value int DEFAULT 42; +``` + +This requires updating every single row in the `projects` table so that +`random_value` is set to `42` by default. This requires updating all rows and +indexes in a table. This in turn acquires enough locks on the table for it to +effectively block any other queries. + +As of MySQL 5.6 adding a column to a table is still quite an expensive +operation, even when using `ALGORITHM=INPLACE` and `LOCK=NONE`. This means +downtime _may_ be required when modifying large tables as otherwise the +operation could potentially take hours to complete. + +## Dropping Columns + +On PostgreSQL you can safely remove an existing column without the need for +downtime. When you drop a column in PostgreSQL it's not immediately removed, +instead it is simply disabled. The data is removed on the next vacuum run. + +On MySQL this operation requires downtime. + +While database wise dropping a column may be fine on PostgreSQL this operation +still requires downtime because the application code may still be using the +column that was removed. For example, consider the following migration: + +```ruby +class MyMigration < ActiveRecord::Migration + def change + remove_column :projects, :dummy + end +end +``` + +Now imagine that the GitLab instance is running and actively uses the `dummy` +column. If we were to run the migration this would result in the GitLab instance +producing errors whenever it tries to use the `dummy` column. + +As a result of the above downtime _is_ required when removing a column, even +when using PostgreSQL. + +## Changing Column Constraints + +Generally changing column constraints requires checking all rows in the table to +see if they meet the new constraint, unless a constraint is _removed_. For +example, changing a column that previously allowed NULL values to not allow NULL +values requires the database to verify all existing rows. + +The specific behaviour varies a bit between databases but in general the safest +approach is to assume changing constraints requires downtime. + +## Changing Column Types + +This operation requires downtime. + +## Adding Indexes + +Adding indexes is an expensive process that blocks INSERT and UPDATE queries for +the duration. When using PostgreSQL one can work arounds this by using the +`CONCURRENTLY` option: + +```sql +CREATE INDEX CONCURRENTLY index_name ON projects (column_name); +``` + +Migrations can take advantage of this by using the method +`add_concurrent_index`. For example: + +```ruby +class MyMigration < ActiveRecord::Migration + def change + add_concurrent_index :projects, :column_name + end +end +``` + +When running this on PostgreSQL the `CONCURRENTLY` option mentioned above is +used. On MySQL this method produces a regular `CREATE INDEX` query. + +MySQL doesn't really have a workaround for this. Supposedly it _can_ create +indexes without the need for downtime but only for variable width columns. The +details on this are a bit sketchy. Since it's better to be safe than sorry one +should assume that adding indexes requires downtime on MySQL. + +## Dropping Indexes + +Dropping an index does not require downtime on both PostgreSQL and MySQL. + +## Adding Tables + +This operation is safe as there's no code using the table just yet. + +## Dropping Tables + +This operation requires downtime as application code may still be using the +table. + +## Adding Foreign Keys + +Adding foreign keys acquires an exclusive lock on both the source and target +tables in PostgreSQL. This requires downtime as otherwise the entire application +grinds to a halt for the duration of the operation. + +On MySQL this operation also requires downtime _unless_ foreign key checks are +disabled. Because this means checks aren't enforced this is not ideal, as such +one should assume MySQL also requires downtime. + +## Removing Foreign Keys + +This operation should not require downtime on both PostgreSQL and MySQL. + +## Updating Data + +Updating data should generally be safe. The exception to this is data that's +being migrated from one version to another while the application still produces +data in the old version. + +For example, imagine the application writes the string `'dog'` to a column but +it really is meant to write `'cat'` instead. One might think that the following +migration is all that is needed to solve this problem: + +```ruby +class MyMigration < ActiveRecord::Migration + def up + execute("UPDATE some_table SET column = 'cat' WHERE column = 'dog';") + end +end +``` + +Unfortunately this is not enough. Because the application is still running and +using the old value this may result in the table still containing rows where +`column` is set to `dog`, even after the migration finished. + +In these cases downtime _is_ required, even for rarely updated tables. diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 21768c15c17..6bac6011467 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -237,6 +237,15 @@ Feature: Project Merge Requests Then I should see additional file lines @javascript + Scenario: I unfold diff in Side-by-Side view + Given project "Shop" have "Bug NS-05" open merge request with diffs inside + And I visit merge request page "Bug NS-05" + And I click on the Changes tab + And I click Side-by-side Diff tab + And I unfold diff + Then I should see additional file lines + + @javascript Scenario: I show comments on a merge request side-by-side diff with comments in multiple files Given project "Shop" have "Bug NS-05" open merge request with diffs inside And I visit merge request page "Bug NS-05" diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index da848afd48e..a02a54923a5 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -477,6 +477,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps step 'I click Side-by-side Diff tab' do find('a', text: 'Side-by-side').trigger('click') + + # Waits for load + expect(page).to have_css('.parallel') end step 'I should see comments on the side-by-side diff page' do diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb index 5b73fc8fcee..46762d401fb 100644 --- a/lib/banzai/filter/relative_link_filter.rb +++ b/lib/banzai/filter/relative_link_filter.rb @@ -35,6 +35,7 @@ module Banzai def process_link_attr(html_attr) return if html_attr.blank? + return if html_attr.value.start_with?('//') uri = URI(html_attr.value) if uri.relative? && uri.path.present? @@ -92,7 +93,7 @@ module Banzai parts = request_path.split('/') parts.pop if uri_type(request_path) != :tree - path.sub!(%r{^\./}, '') + path.sub!(%r{\A\./}, '') while path.start_with?('../') parts.pop diff --git a/spec/helpers/diff_helper_spec.rb b/spec/helpers/diff_helper_spec.rb index 4949280d641..b6554de1c64 100644 --- a/spec/helpers/diff_helper_spec.rb +++ b/spec/helpers/diff_helper_spec.rb @@ -15,22 +15,22 @@ describe DiffHelper do it 'returns a valid value when cookie is set' do helper.request.cookies[:diff_view] = 'parallel' - expect(helper.diff_view).to eq 'parallel' + expect(helper.diff_view).to eq :parallel end it 'returns a default value when cookie is invalid' do helper.request.cookies[:diff_view] = 'invalid' - expect(helper.diff_view).to eq 'inline' + expect(helper.diff_view).to eq :inline end it 'returns a default value when cookie is nil' do expect(helper.request.cookies).to be_empty - expect(helper.diff_view).to eq 'inline' + expect(helper.diff_view).to eq :inline end end - + describe 'diff_options' do it 'should return no collapse false' do expect(diff_options).to include(no_collapse: false) @@ -59,26 +59,6 @@ describe DiffHelper do end end - describe 'unfold_bottom_class' do - it 'should return empty string when bottom line shouldnt be unfolded' do - expect(unfold_bottom_class(false)).to eq('') - end - - it 'should return js class when bottom lines should be unfolded' do - expect(unfold_bottom_class(true)).to include('js-unfold-bottom') - end - end - - describe 'unfold_class' do - it 'returns empty on false' do - expect(unfold_class(false)).to eq('') - end - - it 'returns a class on true' do - expect(unfold_class(true)).to eq('unfold js-unfold') - end - end - describe '#diff_line_content' do it 'should return non breaking space when line is empty' do expect(diff_line_content(nil)).to eq(' ') @@ -105,4 +85,56 @@ describe DiffHelper do expect(marked_new_line).to be_html_safe end end + + describe "#diff_match_line" do + let(:old_pos) { 40 } + let(:new_pos) { 50 } + let(:text) { 'some_text' } + + it "should generate foldable top match line for inline view with empty text by default" do + output = diff_match_line old_pos, new_pos + + expect(output).to be_html_safe + expect(output).to have_css "td:nth-child(1):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.old_line[data-linenumber='#{old_pos}']", text: '...' + expect(output).to have_css "td:nth-child(2):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.new_line[data-linenumber='#{new_pos}']", text: '...' + expect(output).to have_css 'td:nth-child(3):not(.parallel).line_content.match', text: '' + end + + it "should allow to define text and bottom option" do + output = diff_match_line old_pos, new_pos, text: text, bottom: true + + expect(output).to be_html_safe + expect(output).to have_css "td:nth-child(1).diff-line-num.unfold.js-unfold.js-unfold-bottom.old_line[data-linenumber='#{old_pos}']", text: '...' + expect(output).to have_css "td:nth-child(2).diff-line-num.unfold.js-unfold.js-unfold-bottom.new_line[data-linenumber='#{new_pos}']", text: '...' + expect(output).to have_css 'td:nth-child(3):not(.parallel).line_content.match', text: text + end + + it "should generate match line for parallel view" do + output = diff_match_line old_pos, new_pos, text: text, view: :parallel + + expect(output).to be_html_safe + expect(output).to have_css "td:nth-child(1):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.old_line[data-linenumber='#{old_pos}']", text: '...' + expect(output).to have_css 'td:nth-child(2).line_content.match.parallel', text: text + expect(output).to have_css "td:nth-child(3):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.new_line[data-linenumber='#{new_pos}']", text: '...' + expect(output).to have_css 'td:nth-child(4).line_content.match.parallel', text: text + end + + it "should allow to generate only left match line for parallel view" do + output = diff_match_line old_pos, nil, text: text, view: :parallel + + expect(output).to be_html_safe + expect(output).to have_css "td:nth-child(1):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.old_line[data-linenumber='#{old_pos}']", text: '...' + expect(output).to have_css 'td:nth-child(2).line_content.match.parallel', text: text + expect(output).not_to have_css 'td:nth-child(3)' + end + + it "should allow to generate only right match line for parallel view" do + output = diff_match_line nil, new_pos, text: text, view: :parallel + + expect(output).to be_html_safe + expect(output).to have_css "td:nth-child(1):not(.js-unfold-bottom).diff-line-num.unfold.js-unfold.new_line[data-linenumber='#{new_pos}']", text: '...' + expect(output).to have_css 'td:nth-child(2).line_content.match.parallel', text: text + expect(output).not_to have_css 'td:nth-child(3)' + end + end end diff --git a/spec/lib/banzai/filter/relative_link_filter_spec.rb b/spec/lib/banzai/filter/relative_link_filter_spec.rb index 224baca8030..bda8d2ce38a 100644 --- a/spec/lib/banzai/filter/relative_link_filter_spec.rb +++ b/spec/lib/banzai/filter/relative_link_filter_spec.rb @@ -84,6 +84,11 @@ describe Banzai::Filter::RelativeLinkFilter, lib: true do to eq "/#{project_path}/blob/#{ref}/doc/api/README.md" end + it 'ignores absolute URLs with two leading slashes' do + doc = filter(link('//doc/api/README.md')) + expect(doc.at_css('a')['href']).to eq '//doc/api/README.md' + end + it 'rebuilds relative URL for a file in the repo' do doc = filter(link('doc/api/README.md')) expect(doc.at_css('a')['href']). |