From a530e9da3580e05a171b3ba0a3f4408afc000b10 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 24 Feb 2017 09:32:10 -0600 Subject: [PATCH] Address review --- app/models/repository.rb | 4 ++-- app/services/files/multi_service.rb | 12 +++++----- lib/gitlab/git/index.rb | 36 +++++++++++++++++------------ spec/lib/gitlab/git/index_spec.rb | 10 ++++++++ spec/models/repository_spec.rb | 4 ++-- 5 files changed, 41 insertions(+), 25 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 73256df0591..c3fceddb9ca 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -753,7 +753,7 @@ class Repository author_email: nil, author_name: nil, start_branch_name: nil, start_project: project) - entry = tree_entry_at(start_branch_name || branch_name, path) + entry = start_project.repository.tree_entry_at(start_branch_name || branch_name, path) if entry if entry[:type] == :blob raise Gitlab::Git::Repository::InvalidBlobName.new( @@ -865,7 +865,7 @@ class Repository end actions.each do |options| - index.__send__(options.delete(:action), options) + index.public_send(options.delete(:action), options) end options = { diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb index 809fa32eca5..2c89f6e7f92 100644 --- a/app/services/files/multi_service.rb +++ b/app/services/files/multi_service.rb @@ -22,6 +22,12 @@ module Files def validate super params[:actions].each_with_index do |action, index| + if ACTIONS.include?(action[:action].to_s) + action[:action] = action[:action].to_sym + else + raise_error("Unknown action type `#{action[:action]}`.") + end + unless action[:file_path].present? raise_error("You must specify a file_path.") end @@ -32,12 +38,6 @@ module Files regex_check(action[:file_path]) regex_check(action[:previous_path]) if action[:previous_path] - if ACTIONS.include?(action[:action].to_s) - action[:action] = action[:action].to_sym - else - raise_error("Unknown action type `#{action[:action]}`.") - end - if project.empty_repo? && action[:action] != :create raise_error("No files to #{action[:action]}.") end diff --git a/lib/gitlab/git/index.rb b/lib/gitlab/git/index.rb index 69a7a513281..546696e4bd1 100644 --- a/lib/gitlab/git/index.rb +++ b/lib/gitlab/git/index.rb @@ -10,20 +10,20 @@ module Gitlab @raw_index = repository.rugged.index end - delegate :read_tree, to: :raw_index + delegate :read_tree, :get, to: :raw_index def write_tree raw_index.write_tree(repository.rugged) end - def get(*args) - raw_index.get(*args) + def dir_exists?(path) + raw_index.find { |entry| entry[:path].start_with?("#{path}/") } end def create(options) - normalize_options!(options) + options = normalize_options(options) - file_entry = raw_index.get(options[:file_path]) + file_entry = get(options[:file_path]) if file_entry raise Gitlab::Git::Repository::InvalidBlobName.new("Filename already exists") end @@ -32,13 +32,17 @@ module Gitlab end def create_dir(options) - normalize_options!(options) + options = normalize_options(options) - file_entry = raw_index.get(options[:file_path]) + file_entry = get(options[:file_path]) if file_entry raise Gitlab::Git::Repository::InvalidBlobName.new("Directory already exists as a file") end + if dir_exists?(options[:file_path]) + raise Gitlab::Git::Repository::InvalidBlobName.new("Directory already exists") + end + options = options.dup options[:file_path] += '/.gitkeep' options[:content] = '' @@ -47,9 +51,9 @@ module Gitlab end def update(options) - normalize_options!(options) + options = normalize_options(options) - file_entry = raw_index.get(options[:file_path]) + file_entry = get(options[:file_path]) unless file_entry raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist") end @@ -58,9 +62,9 @@ module Gitlab end def move(options) - normalize_options!(options) + options = normalize_options(options) - file_entry = raw_index.get(options[:previous_path]) + file_entry = get(options[:previous_path]) unless file_entry raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist") end @@ -71,9 +75,9 @@ module Gitlab end def delete(options) - normalize_options!(options) + options = normalize_options(options) - file_entry = raw_index.get(options[:file_path]) + file_entry = get(options[:file_path]) unless file_entry raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist") end @@ -83,13 +87,15 @@ module Gitlab private - def normalize_options!(options) + def normalize_options(options) + options = options.dup options[:file_path] = normalize_path(options[:file_path]) if options[:file_path] options[:previous_path] = normalize_path(options[:previous_path]) if options[:previous_path] + options end def normalize_path(path) - pathname = Gitlab::Git::PathHelper.normalize_path(path) + pathname = Gitlab::Git::PathHelper.normalize_path(path.dup) if pathname.each_filename.include?('..') raise Gitlab::Git::Repository::InvalidBlobName.new('Invalid path') diff --git a/spec/lib/gitlab/git/index_spec.rb b/spec/lib/gitlab/git/index_spec.rb index 91ffbd138ce..d0c7ca60ddc 100644 --- a/spec/lib/gitlab/git/index_spec.rb +++ b/spec/lib/gitlab/git/index_spec.rb @@ -92,6 +92,16 @@ describe Gitlab::Git::Index, seed_helper: true do expect { index.create_dir(options) }.to raise_error('Directory already exists as a file') end end + + context 'when a directory at that path exists' do + before do + options[:file_path] = 'files/executables' + end + + it 'raises an error' do + expect { index.create_dir(options) }.to raise_error('Directory already exists') + end + end end describe '#update' do diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 58943fd7252..ae203fada12 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -291,7 +291,7 @@ describe Repository, models: true do end end - describe "#commit_dir" do + describe "#create_dir" do it "commits a change that creates a new directory" do expect do repository.create_dir(user, 'newdir', @@ -424,7 +424,7 @@ describe Repository, models: true do end end - describe "#remove_file" do + describe "#delete_file" do it 'removes file successfully' do expect do repository.delete_file(user, 'README',