Commit 9a73b634 authored by Sean McGivern's avatar Sean McGivern

Add table for files in merge request diffs

This adds an ID-less table containing one row per file, per merge request
diff. It has a column for each attribute on Gitlab::Git::Diff that is serialised
currently, with the advantage that we can easily query the attributes of this
new table.

It does not migrate existing data, so we have fallback code when the legacy
st_diffs column is present instead. For a merge request diff to be valid, it
should have at most one of:

* Rows in this new table, with the correct merge_request_diff_id.
* A non-NULL st_diffs column.

It may have neither, if the diff is empty.
parent 352a9ed5
...@@ -10,6 +10,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -10,6 +10,7 @@ class MergeRequestDiff < ActiveRecord::Base
VALID_CLASSES = [Hash, Rugged::Patch, Rugged::Diff::Delta].freeze VALID_CLASSES = [Hash, Rugged::Patch, Rugged::Diff::Delta].freeze
belongs_to :merge_request belongs_to :merge_request
has_many :merge_request_diff_files, -> { order(:merge_request_diff_id, :relative_order) }
serialize :st_commits # rubocop:disable Cop/ActiverecordSerialize serialize :st_commits # rubocop:disable Cop/ActiverecordSerialize
serialize :st_diffs # rubocop:disable Cop/ActiverecordSerialize serialize :st_diffs # rubocop:disable Cop/ActiverecordSerialize
...@@ -91,7 +92,7 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -91,7 +92,7 @@ class MergeRequestDiff < ActiveRecord::Base
head_commit_sha).diffs(options) head_commit_sha).diffs(options)
else else
@raw_diffs ||= {} @raw_diffs ||= {}
@raw_diffs[options] ||= load_diffs(st_diffs, options) @raw_diffs[options] ||= load_diffs(options)
end end
end end
...@@ -253,14 +254,22 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -253,14 +254,22 @@ class MergeRequestDiff < ActiveRecord::Base
update_columns_serialized(new_attributes) update_columns_serialized(new_attributes)
end end
def dump_diffs(diffs) def create_merge_request_diff_files(diffs)
if diffs.respond_to?(:map) rows = diffs.map.with_index do |diff, index|
diffs.map(&:to_hash) diff.to_hash.merge(
merge_request_diff_id: self.id,
relative_order: index
)
end end
Gitlab::Database.bulk_insert('merge_request_diff_files', rows)
end end
def load_diffs(raw, options) def load_diffs(options)
if valid_raw_diff?(raw) return Gitlab::Git::DiffCollection.new([]) unless diffs_from_database
raw = diffs_from_database
if paths = options[:paths] if paths = options[:paths]
raw = raw.select do |diff| raw = raw.select do |diff|
paths.include?(diff[:old_path]) || paths.include?(diff[:new_path]) paths.include?(diff[:old_path]) || paths.include?(diff[:new_path])
...@@ -268,8 +277,20 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -268,8 +277,20 @@ class MergeRequestDiff < ActiveRecord::Base
end end
Gitlab::Git::DiffCollection.new(raw, options) Gitlab::Git::DiffCollection.new(raw, options)
else end
Gitlab::Git::DiffCollection.new([])
def diffs_from_database
return @diffs_from_database if defined?(@diffs_from_database)
@diffs_from_database =
if st_diffs.present?
if valid_raw_diff?(st_diffs)
st_diffs
end
elsif merge_request_diff_files.present?
merge_request_diff_files
.as_json(only: Gitlab::Git::Diff::SERIALIZE_KEYS)
.map(&:with_indifferent_access)
end end
end end
...@@ -285,11 +306,10 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -285,11 +306,10 @@ class MergeRequestDiff < ActiveRecord::Base
new_attributes[:real_size] = diff_collection.real_size new_attributes[:real_size] = diff_collection.real_size
if diff_collection.any? if diff_collection.any?
new_diffs = dump_diffs(diff_collection)
new_attributes[:state] = :collected new_attributes[:state] = :collected
end
new_attributes[:st_diffs] = new_diffs || [] create_merge_request_diff_files(diff_collection)
end
# Set our state to 'overflow' to make the #empty? and #collected? # Set our state to 'overflow' to make the #empty? and #collected?
# methods (generated by StateMachine) return false. # methods (generated by StateMachine) return false.
......
class MergeRequestDiffFile < ActiveRecord::Base
include Gitlab::EncodingHelper
belongs_to :merge_request_diff
def utf8_diff
return '' if diff.blank?
encode_utf8(diff) if diff.respond_to?(:encoding)
end
end
class CreateMergeRequestDiffFiles < ActiveRecord::Migration
DOWNTIME = false
disable_ddl_transaction!
def change
create_table :merge_request_diff_files, id: false do |t|
t.belongs_to :merge_request_diff, null: false, foreign_key: { on_delete: :cascade }
t.integer :relative_order, null: false
t.boolean :new_file, null: false
t.boolean :renamed_file, null: false
t.boolean :deleted_file, null: false
t.boolean :too_large, null: false
t.string :a_mode, null: false
t.string :b_mode, null: false
t.text :new_path, null: false
t.text :old_path, null: false
t.text :diff, null: false
t.index [:merge_request_diff_id, :relative_order], name: 'index_merge_request_diff_files_on_mr_diff_id_and_order', unique: true
end
end
end
require_relative 'merge_request_diff_file_limits_to_mysql'
class MergeRequestDiffFileLimitsToMysql < ActiveRecord::Migration
DOWNTIME = false
def up
return unless Gitlab::Database.mysql?
change_column :merge_request_diff_files, :diff, :text, limit: 2147483647
end
def down
end
end
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20170609183112) do ActiveRecord::Schema.define(version: 20170614115405) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -692,6 +692,22 @@ ActiveRecord::Schema.define(version: 20170609183112) do ...@@ -692,6 +692,22 @@ ActiveRecord::Schema.define(version: 20170609183112) do
add_index "members", ["source_id", "source_type"], name: "index_members_on_source_id_and_source_type", using: :btree add_index "members", ["source_id", "source_type"], name: "index_members_on_source_id_and_source_type", using: :btree
add_index "members", ["user_id"], name: "index_members_on_user_id", using: :btree add_index "members", ["user_id"], name: "index_members_on_user_id", using: :btree
create_table "merge_request_diff_files", id: false, force: :cascade do |t|
t.integer "merge_request_diff_id", null: false
t.integer "relative_order", null: false
t.boolean "new_file", null: false
t.boolean "renamed_file", null: false
t.boolean "deleted_file", null: false
t.boolean "too_large", null: false
t.string "a_mode", null: false
t.string "b_mode", null: false
t.text "new_path", null: false
t.text "old_path", null: false
t.text "diff", null: false
end
add_index "merge_request_diff_files", ["merge_request_diff_id", "relative_order"], name: "index_merge_request_diff_files_on_mr_diff_id_and_order", unique: true, using: :btree
create_table "merge_request_diffs", force: :cascade do |t| create_table "merge_request_diffs", force: :cascade do |t|
t.string "state" t.string "state"
t.text "st_commits" t.text "st_commits"
...@@ -1530,6 +1546,7 @@ ActiveRecord::Schema.define(version: 20170609183112) do ...@@ -1530,6 +1546,7 @@ ActiveRecord::Schema.define(version: 20170609183112) do
add_foreign_key "labels", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "labels", "namespaces", column: "group_id", on_delete: :cascade
add_foreign_key "lists", "boards" add_foreign_key "lists", "boards"
add_foreign_key "lists", "labels" add_foreign_key "lists", "labels"
add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade
add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade
add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade
add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade add_foreign_key "merge_requests_closing_issues", "issues", on_delete: :cascade
......
...@@ -27,7 +27,8 @@ with all their related data and be moved into a new GitLab instance. ...@@ -27,7 +27,8 @@ with all their related data and be moved into a new GitLab instance.
| GitLab version | Import/Export version | | GitLab version | Import/Export version |
| -------- | -------- | | -------- | -------- |
| 9.2.0 to current | 0.1.7 | | 9.4.0 to current | 0.1.8 |
| 9.2.0 | 0.1.7 |
| 8.17.0 | 0.1.6 | | 8.17.0 | 0.1.6 |
| 8.13.0 | 0.1.5 | | 8.13.0 | 0.1.5 |
| 8.12.0 | 0.1.4 | | 8.12.0 | 0.1.4 |
......
...@@ -83,6 +83,22 @@ module Gitlab ...@@ -83,6 +83,22 @@ module Gitlab
end end
end end
def self.bulk_insert(table, rows)
return if rows.empty?
keys = rows.first.keys
columns = keys.map { |key| connection.quote_column_name(key) }
tuples = rows.map do |row|
row.values_at(*keys).map { |value| connection.quote(value) }
end
connection.execute <<-EOF.strip_heredoc
INSERT INTO #{table} (#{columns.join(', ')})
VALUES #{tuples.map { |tuple| "(#{tuple.join(', ')})" }.join(', ')}
EOF
end
# pool_size - The size of the DB pool. # pool_size - The size of the DB pool.
# host - An optional host name to use instead of the default one. # host - An optional host name to use instead of the default one.
def self.create_connection_pool(pool_size, host = nil) def self.create_connection_pool(pool_size, host = nil)
......
...@@ -16,11 +16,11 @@ module Gitlab ...@@ -16,11 +16,11 @@ module Gitlab
alias_method :renamed_file?, :renamed_file alias_method :renamed_file?, :renamed_file
attr_accessor :expanded attr_accessor :expanded
attr_writer :too_large
alias_method :expanded?, :expanded alias_method :expanded?, :expanded
# We need this accessor because of `to_hash` and `init_from_hash` SERIALIZE_KEYS = %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large).freeze
attr_accessor :too_large
class << self class << self
# The maximum size of a diff to display. # The maximum size of a diff to display.
...@@ -231,16 +231,10 @@ module Gitlab ...@@ -231,16 +231,10 @@ module Gitlab
end end
end end
def serialize_keys
@serialize_keys ||= %i(diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large)
end
def to_hash def to_hash
hash = {} hash = {}
keys = serialize_keys SERIALIZE_KEYS.each do |key|
keys.each do |key|
hash[key] = send(key) hash[key] = send(key)
end end
...@@ -267,6 +261,9 @@ module Gitlab ...@@ -267,6 +261,9 @@ module Gitlab
end end
end end
# This is used by `to_hash` and `init_from_hash`.
alias_method :too_large, :too_large?
def too_large! def too_large!
@diff = '' @diff = ''
@line_count = 0 @line_count = 0
...@@ -315,7 +312,7 @@ module Gitlab ...@@ -315,7 +312,7 @@ module Gitlab
def init_from_hash(hash) def init_from_hash(hash)
raw_diff = hash.symbolize_keys raw_diff = hash.symbolize_keys
serialize_keys.each do |key| SERIALIZE_KEYS.each do |key|
send(:"#{key}=", raw_diff[key.to_sym]) send(:"#{key}=", raw_diff[key.to_sym])
end end
end end
......
...@@ -3,7 +3,7 @@ module Gitlab ...@@ -3,7 +3,7 @@ module Gitlab
extend self extend self
# For every version update, the version history in import_export.md has to be kept up to date. # For every version update, the version history in import_export.md has to be kept up to date.
VERSION = '0.1.7'.freeze VERSION = '0.1.8'.freeze
FILENAME_LIMIT = 50 FILENAME_LIMIT = 50
def export_path(relative_path:) def export_path(relative_path:)
......
...@@ -26,7 +26,8 @@ project_tree: ...@@ -26,7 +26,8 @@ project_tree:
- notes: - notes:
- :author - :author
- :events - :events
- :merge_request_diff - merge_request_diff:
- :merge_request_diff_files
- :events - :events
- :timelogs - :timelogs
- label_links: - label_links:
...@@ -92,6 +93,8 @@ excluded_attributes: ...@@ -92,6 +93,8 @@ excluded_attributes:
- :expired_at - :expired_at
merge_request_diff: merge_request_diff:
- :st_diffs - :st_diffs
merge_request_diff_files:
- :diff
issues: issues:
- :milestone_id - :milestone_id
merge_requests: merge_requests:
...@@ -113,6 +116,8 @@ methods: ...@@ -113,6 +116,8 @@ methods:
- :type - :type
merge_request_diff: merge_request_diff:
- :utf8_st_diffs - :utf8_st_diffs
merge_request_diff_files:
- :utf8_diff
merge_requests: merge_requests:
- :diff_head_sha - :diff_head_sha
project: project:
......
...@@ -83,7 +83,9 @@ module Gitlab ...@@ -83,7 +83,9 @@ module Gitlab
# +value+ existing model to be included in the hash # +value+ existing model to be included in the hash
# +json_config_hash+ the original hash containing the root model # +json_config_hash+ the original hash containing the root model
def add_model_value(current_key, value, json_config_hash) def add_model_value(current_key, value, json_config_hash)
@attributes_finder.parse(value) { |hash| value = { value => hash } } @attributes_finder.parse(value) do |hash|
value = { value => hash } unless value.is_a?(Hash)
end
add_to_array(current_key, json_config_hash, value) add_to_array(current_key, json_config_hash, value)
end end
......
...@@ -71,6 +71,7 @@ module Gitlab ...@@ -71,6 +71,7 @@ module Gitlab
@relation_hash['data'].deep_symbolize_keys! if @relation_name == :events && @relation_hash['data'] @relation_hash['data'].deep_symbolize_keys! if @relation_name == :events && @relation_hash['data']
set_st_diff_commits if @relation_name == :merge_request_diff set_st_diff_commits if @relation_name == :merge_request_diff
set_diff if @relation_name == :merge_request_diff_files
end end
def update_user_references def update_user_references
...@@ -202,6 +203,10 @@ module Gitlab ...@@ -202,6 +203,10 @@ module Gitlab
HashUtil.deep_symbolize_array_with_date!(@relation_hash['st_commits']) HashUtil.deep_symbolize_array_with_date!(@relation_hash['st_commits'])
end end
def set_diff
@relation_hash['diff'] = @relation_hash.delete('utf8_diff')
end
def existing_or_new_object def existing_or_new_object
# Only find existing records to avoid mapping tables such as milestones # Only find existing records to avoid mapping tables such as milestones
# Otherwise always create the record, skipping the extra SELECT clause. # Otherwise always create the record, skipping the extra SELECT clause.
......
require Rails.root.join('db/migrate/limits_to_mysql') require Rails.root.join('db/migrate/limits_to_mysql')
require Rails.root.join('db/migrate/markdown_cache_limits_to_mysql') require Rails.root.join('db/migrate/markdown_cache_limits_to_mysql')
require Rails.root.join('db/migrate/merge_request_diff_file_limits_to_mysql')
desc "GitLab | Add limits to strings in mysql database" desc "GitLab | Add limits to strings in mysql database"
task add_limits_mysql: :environment do task add_limits_mysql: :environment do
puts "Adding limits to schema.rb for mysql" puts "Adding limits to schema.rb for mysql"
LimitsToMysql.new.up LimitsToMysql.new.up
MarkdownCacheLimitsToMysql.new.up MarkdownCacheLimitsToMysql.new.up
MergeRequestDiffFileLimitsToMysql.new.up
end end
...@@ -129,6 +129,55 @@ describe Gitlab::Database, lib: true do ...@@ -129,6 +129,55 @@ describe Gitlab::Database, lib: true do
end end
end end
describe '.bulk_insert' do
before do
allow(described_class).to receive(:connection).and_return(connection)
allow(connection).to receive(:quote_column_name, &:itself)
allow(connection).to receive(:quote, &:itself)
allow(connection).to receive(:execute)
end
let(:connection) { double(:connection) }
let(:rows) do
[
{ a: 1, b: 2, c: 3 },
{ c: 6, a: 4, b: 5 }
]
end
it 'does nothing with empty rows' do
expect(connection).not_to receive(:execute)
described_class.bulk_insert('test', [])
end
it 'uses the ordering from the first row' do
expect(connection).to receive(:execute) do |sql|
expect(sql).to include('(1, 2, 3)')
expect(sql).to include('(4, 5, 6)')
end
described_class.bulk_insert('test', rows)
end
it 'quotes column names' do
expect(connection).to receive(:quote_column_name).with(:a)
expect(connection).to receive(:quote_column_name).with(:b)
expect(connection).to receive(:quote_column_name).with(:c)
described_class.bulk_insert('test', rows)
end
it 'quotes values' do
1.upto(6) do |i|
expect(connection).to receive(:quote).with(i)
end
described_class.bulk_insert('test', rows)
end
end
describe '.create_connection_pool' do describe '.create_connection_pool' do
it 'creates a new connection pool with specific pool size' do it 'creates a new connection pool with specific pool size' do
pool = described_class.create_connection_pool(5) pool = described_class.create_connection_pool(5)
......
...@@ -90,7 +90,7 @@ EOT ...@@ -90,7 +90,7 @@ EOT
let(:diff) { described_class.new(@rugged_diff) } let(:diff) { described_class.new(@rugged_diff) }
it 'initializes the diff' do it 'initializes the diff' do
expect(diff.to_hash).to eq(@raw_diff_hash.merge(too_large: nil)) expect(diff.to_hash).to eq(@raw_diff_hash)
end end
it 'does not prune the diff' do it 'does not prune the diff' do
......
...@@ -88,6 +88,9 @@ merge_requests: ...@@ -88,6 +88,9 @@ merge_requests:
- head_pipeline - head_pipeline
merge_request_diff: merge_request_diff:
- merge_request - merge_request
- merge_request_diff_files
merge_request_diff_files:
- merge_request_diff
pipelines: pipelines:
- project - project
- user - user
......
...@@ -2821,9 +2821,11 @@ ...@@ -2821,9 +2821,11 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "committer_email": "dmitriy.zaporozhets@gmail.com"
} }
], ],
"utf8_st_diffs": [ "merge_request_diff_files": [
{ {
"diff": "Binary files a/.DS_Store and /dev/null differ\n", "merge_request_diff_id": 27,
"relative_order": 0,
"utf8_diff": "Binary files a/.DS_Store and /dev/null differ\n",
"new_path": ".DS_Store", "new_path": ".DS_Store",
"old_path": ".DS_Store", "old_path": ".DS_Store",
"a_mode": "100644", "a_mode": "100644",
...@@ -2834,7 +2836,9 @@ ...@@ -2834,7 +2836,9 @@
"too_large": false "too_large": false
}, },
{ {
"diff": "--- a/.gitignore\n+++ b/.gitignore\n@@ -17,3 +17,4 @@ rerun.txt\n pickle-email-*.html\n .project\n config/initializers/secret_token.rb\n+.DS_Store\n", "merge_request_diff_id": 27,
"relative_order": 1,
"utf8_diff": "--- a/.gitignore\n+++ b/.gitignore\n@@ -17,3 +17,4 @@ rerun.txt\n pickle-email-*.html\n .project\n config/initializers/secret_token.rb\n+.DS_Store\n",
"new_path": ".gitignore", "new_path": ".gitignore",
"old_path": ".gitignore", "old_path": ".gitignore",
"a_mode": "100644", "a_mode": "100644",
...@@ -2845,7 +2849,9 @@ ...@@ -2845,7 +2849,9 @@
"too_large": false "too_large": false
}, },
{ {
"diff": "--- a/.gitmodules\n+++ b/.gitmodules\n@@ -1,3 +1,9 @@\n [submodule \"six\"]\n \tpath = six\n \turl = git://github.com/randx/six.git\n+[submodule \"gitlab-shell\"]\n+\tpath = gitlab-shell\n+\turl = https://github.com/gitlabhq/gitlab-shell.git\n+[submodule \"gitlab-grack\"]\n+\tpath = gitlab-grack\n+\turl = https://gitlab.com/gitlab-org/gitlab-grack.git\n", "merge_request_diff_id": 27,
"relative_order": 2,
"utf8_diff": "--- a/.gitmodules\n+++ b/.gitmodules\n@@ -1,3 +1,9 @@\n [submodule \"six\"]\n \tpath = six\n \turl = git://github.com/randx/six.git\n+[submodule \"gitlab-shell\"]\n+\tpath = gitlab-shell\n+\turl = https://github.com/gitlabhq/gitlab-shell.git\n+[submodule \"gitlab-grack\"]\n+\tpath = gitlab-grack\n+\turl = https://gitlab.com/gitlab-org/gitlab-grack.git\n",
"new_path": ".gitmodules", "new_path": ".gitmodules",
"old_path": ".gitmodules", "old_path": ".gitmodules",
"a_mode": "100644", "a_mode": "100644",
...@@ -2856,7 +2862,9 @@ ...@@ -2856,7 +2862,9 @@
"too_large": false "too_large": false
}, },
{ {
"diff": "Binary files a/files/.DS_Store and /dev/null differ\n", "merge_request_diff_id": 27,
"relative_order": 3,
"utf8_diff": "Binary files a/files/.DS_Store and /dev/null differ\n",
"new_path": "files/.DS_Store", "new_path": "files/.DS_Store",
"old_path": "files/.DS_Store", "old_path": "files/.DS_Store",
"a_mode": "100644", "a_mode": "100644",
...@@ -2867,7 +2875,9 @@ ...@@ -2867,7 +2875,9 @@
"too_large": false "too_large": false
}, },
{ {
"diff": "--- /dev/null\n+++ b/files/ruby/feature.rb\n@@ -0,0 +1,4 @@\n+# This file was changed in feature branch\n+# We put different code here to make merge conflict\n+class Conflict\n+end\n", "merge_request_diff_id": 27,
"relative_order": 4,
"utf8_diff": "--- /dev/null\n+++ b/files/ruby/feature.rb\n@@ -0,0 +1,4 @@\n+# This file was changed in feature branch\n+# We put different code here to make merge conflict\n+class Conflict\n+end\n",
"new_path": "files/ruby/feature.rb", "new_path": "files/ruby/feature.rb",
"old_path": "files/ruby/feature.rb", "old_path": "files/ruby/feature.rb",
"a_mode": "0", "a_mode": "0",
...@@ -2878,7 +2888,9 @@ ...@@ -2878,7 +2888,9 @@
"too_large": false "too_large": false
}, },
{ {
"diff": "--- a/files/ruby/popen.rb\n+++ b/files/ruby/popen.rb\n@@ -6,12 +6,18 @@ module Popen\n \n def popen(cmd, path=nil)\n unless cmd.is_a?(Array)\n- raise \"System commands must be given as an array of strings\"\n+ raise RuntimeError, \"System commands must be given as an array of strings\"\n end\n \n path ||= Dir.pwd\n- vars = { \"PWD\" =\u003e path }\n- options = { chdir: path }\n+\n+ vars = {\n+ \"PWD\" =\u003e path\n+ }\n+\n+ options = {\n+ chdir: path\n+ }\n \n unless File.directory?(path)\n FileUtils.mkdir_p(path)\n@@ -19,6 +25,7 @@ module Popen\n \n @cmd_output = \"\"\n @cmd_status = 0\n+\n Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|\n @cmd_output \u003c\u003c stdout.read\n @cmd_output \u003c\u003c stderr.read\n", "merge_request_diff_id": 27,
"relative_order": 5,
"utf8_diff": "--- a/files/ruby/popen.rb\n+++ b/files/ruby/popen.rb\n@@ -6,12 +6,18 @@ module Popen\n \n def popen(cmd, path=nil)\n unless cmd.is_a?(Array)\n- raise \"System commands must be given as an array of strings\"\n+ raise RuntimeError, \"System commands must be given as an array of strings\"\n end\n \n path ||= Dir.pwd\n- vars = { \"PWD\" =\u003e path }\n- options = { chdir: path }\n+\n+ vars = {\n+ \"PWD\" =\u003e path\n+ }\n+\n+ options = {\n+ chdir: path\n+ }\n \n unless File.directory?(path)\n FileUtils.mkdir_p(path)\n@@ -19,6 +25,7 @@ module Popen\n \n @cmd_output = \"\"\n @cmd_status = 0\n+\n Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|\n @cmd_output \u003c\u003c stdout.read\n @cmd_output \u003c\u003c stderr.read\n",
"new_path": "files/ruby/popen.rb", "new_path": "files/ruby/popen.rb",
"old_path": "files/ruby/popen.rb", "old_path": "files/ruby/popen.rb",
"a_mode": "100644", "a_mode": "100644",
...@@ -2889,7 +2901,9 @@ ...@@ -2889,7 +2901,9 @@
"too_large": false "too_large": false
}, },
{ {
"diff": "--- a/files/ruby/regex.rb\n+++ b/files/ruby/regex.rb\n@@ -19,14 +19,12 @@ module Gitlab\n end\n \n def archive_formats_regex\n- #|zip|tar| tar.gz | tar.bz2 |\n- /(zip|tar|tar\\.gz|tgz|gz|tar\\.bz2|tbz|tbz2|tb2|bz2)/\n+ /(zip|tar|7z|tar\\.gz|tgz|gz|tar\\.bz2|tbz|tbz2|tb2|bz2)/\n end\n \n def git_reference_regex\n # Valid git ref regex, see:\n # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html\n-\n %r{\n (?!\n (?# doesn't begins with)\n", "merge_request_diff_id": 27,
"relative_order": 6,
"utf8_diff": "--- a/files/ruby/regex.rb\n+++ b/files/ruby/regex.rb\n@@ -19,14 +19,12 @@ module Gitlab\n end\n \n def archive_formats_regex\n- #|zip|tar| tar.gz | tar.bz2 |\n- /(zip|tar|tar\\.gz|tgz|gz|tar\\.bz2|tbz|tbz2|tb2|bz2)/\n+ /(zip|tar|7z|tar\\.gz|tgz|gz|tar\\.bz2|tbz|tbz2|tb2|bz2)/\n end\n \n def git_reference_regex\n # Valid git ref regex, see:\n # https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html\n-\n %r{\n (?!\n (?# doesn't begins with)\n",
"new_path": "files/ruby/regex.rb", "new_path": "files/ruby/regex.rb",
"old_path": "files/ruby/regex.rb", "old_path": "files/ruby/regex.rb",
"a_mode": "100644", "a_mode": "100644",
...@@ -2900,7 +2914,9 @@ ...@@ -2900,7 +2914,9 @@
"too_large": false "too_large": false
}, },
{ {
"diff": "--- /dev/null\n+++ b/gitlab-grack\n@@ -0,0 +1 @@\n+Subproject commit 645f6c4c82fd3f5e06f67134450a570b795e55a6\n", "merge_request_diff_id": 27,
"relative_order": 7,
"utf8_diff": "--- /dev/null\n+++ b/gitlab-grack\n@@ -0,0 +1 @@\n+Subproject commit 645f6c4c82fd3f5e06f67134450a570b795e55a6\n",
"new_path": "gitlab-grack", "new_path": "gitlab-grack",
"old_path": "gitlab-grack", "old_path": "gitlab-grack",
"a_mode": "0", "a_mode": "0",
...@@ -2911,7 +2927,9 @@ ...@@ -2911,7 +2927,9 @@
"too_large": false "too_large": false
}, },
{ {
"diff": "--- /dev/null\n+++ b/gitlab-shell\n@@ -0,0 +1 @@\n+Subproject commit 79bceae69cb5750d6567b223597999bfa91cb3b9\n", "merge_request_diff_id": 27,
"relative_order": 8,
"utf8_diff": "--- /dev/null\n+++ b/gitlab-shell\n@@ -0,0 +1 @@\n+Subproject commit 79bceae69cb5750d6567b223597999bfa91cb3b9\n",
"new_path": "gitlab-shell", "new_path": "gitlab-shell",
"old_path": "gitlab-shell", "old_path": "gitlab-shell",
"a_mode": "0", "a_mode": "0",
......
...@@ -86,8 +86,13 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do ...@@ -86,8 +86,13 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
it 'has the correct data for merge request st_diffs' do it 'has the correct data for merge request st_diffs' do
# makes sure we are renaming the custom method +utf8_st_diffs+ into +st_diffs+ # makes sure we are renaming the custom method +utf8_st_diffs+ into +st_diffs+
# one MergeRequestDiff uses the new format, where st_diffs is expected to be nil
expect(MergeRequestDiff.where.not(st_diffs: nil).count).to eq(9) expect(MergeRequestDiff.where.not(st_diffs: nil).count).to eq(8)
end
it 'has the correct data for merge request diff files' do
expect(MergeRequestDiffFile.where.not(diff: nil).count).to eq(9)
end end
it 'has the correct time for merge request st_commits' do it 'has the correct time for merge request st_commits' do
......
...@@ -83,6 +83,10 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do ...@@ -83,6 +83,10 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
expect(saved_project_json['merge_requests'].first['merge_request_diff']['utf8_st_diffs']).not_to be_nil expect(saved_project_json['merge_requests'].first['merge_request_diff']['utf8_st_diffs']).not_to be_nil
end end
it 'has merge request diff files' do
expect(saved_project_json['merge_requests'].first['merge_request_diff']['merge_request_diff_files']).not_to be_empty
end
it 'has merge requests comments' do it 'has merge requests comments' do
expect(saved_project_json['merge_requests'].first['notes']).not_to be_empty expect(saved_project_json['merge_requests'].first['notes']).not_to be_empty
end end
...@@ -145,6 +149,12 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do ...@@ -145,6 +149,12 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
expect(project_tree_saver.save).to be true expect(project_tree_saver.save).to be true
end end
it 'does not complain about non UTF-8 characters in MR diff files' do
ActiveRecord::Base.connection.execute("UPDATE merge_request_diff_files SET diff = '---\n- :diff: !binary |-\n LS0tIC9kZXYvbnVsbAorKysgYi9pbWFnZXMvbnVjb3IucGRmCkBAIC0wLDAg\n KzEsMTY3OSBAQAorJVBERi0xLjUNJeLjz9MNCisxIDAgb2JqDTw8L01ldGFk\n YXR'")
expect(project_tree_saver.save).to be true
end
context 'group members' do context 'group members' do
let(:user2) { create(:user, email: 'group@member.com') } let(:user2) { create(:user, email: 'group@member.com') }
let(:member_emails) do let(:member_emails) do
......
...@@ -172,6 +172,17 @@ MergeRequestDiff: ...@@ -172,6 +172,17 @@ MergeRequestDiff:
- real_size - real_size
- head_commit_sha - head_commit_sha
- start_commit_sha - start_commit_sha
MergeRequestDiffFile:
- merge_request_diff_id
- relative_order
- new_file
- renamed_file
- deleted_file
- new_path
- old_path
- a_mode
- b_mode
- too_large
Ci::Pipeline: Ci::Pipeline:
- id - id
- project_id - project_id
......
require 'rails_helper'
describe MergeRequestDiffFile, type: :model do
describe '#utf8_diff' do
it 'does not raise error when a hash value is in binary' do
subject.diff = "\x05\x00\x68\x65\x6c\x6c\x6f"
expect { subject.utf8_diff }.not_to raise_error
end
end
end
...@@ -37,7 +37,7 @@ describe MergeRequestDiff, models: true do ...@@ -37,7 +37,7 @@ describe MergeRequestDiff, models: true do
context 'when the raw diffs are empty' do context 'when the raw diffs are empty' do
before do before do
mr_diff.update_attributes(st_diffs: '') MergeRequestDiffFile.delete_all(merge_request_diff_id: mr_diff.id)
end end
it 'returns an empty DiffCollection' do it 'returns an empty DiffCollection' do
...@@ -48,6 +48,7 @@ describe MergeRequestDiff, models: true do ...@@ -48,6 +48,7 @@ describe MergeRequestDiff, models: true do
context 'when the raw diffs have invalid content' do context 'when the raw diffs have invalid content' do
before do before do
MergeRequestDiffFile.delete_all(merge_request_diff_id: mr_diff.id)
mr_diff.update_attributes(st_diffs: ["--broken-diff"]) mr_diff.update_attributes(st_diffs: ["--broken-diff"])
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment