From 7044bcf6f19562f6d3aa4f6b5b5b7cf4176df3ac Mon Sep 17 00:00:00 2001 From: Ciro Santilli Date: Tue, 4 Nov 2014 11:14:05 +0100 Subject: [PATCH] Factor File.join to join ids path pairs It is not semantically correct in those cases since that uses the separator of the current system, but the GitLab frontend is system agnostic, e.g., should in theory work in Windows where the path separator is \ without front-end changes. --- app/controllers/projects/blob_controller.rb | 3 ++- app/controllers/projects/new_tree_controller.rb | 8 ++++++-- app/controllers/projects/tree_controller.rb | 3 ++- app/helpers/tree_helper.rb | 5 ++--- features/steps/shared/paths.rb | 9 ++++++--- lib/extracts_path.rb | 14 +++++++++++--- .../security/project/internal_access_spec.rb | 5 ++++- .../security/project/private_access_spec.rb | 5 ++++- .../security/project/public_access_spec.rb | 5 ++++- 9 files changed, 41 insertions(+), 16 deletions(-) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 2412800c493..4bcf30a282a 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -47,7 +47,8 @@ class Projects::BlobController < Projects::ApplicationController if @blob @blob elsif tree.entries.any? - redirect_to project_tree_path(@project, File.join(@ref, @path)) and return + redirect_to project_tree_path(@project, ExtractsPath.join(@ref, @path)) + return else return not_found! end diff --git a/app/controllers/projects/new_tree_controller.rb b/app/controllers/projects/new_tree_controller.rb index ffba706b2f6..011bc1d2251 100644 --- a/app/controllers/projects/new_tree_controller.rb +++ b/app/controllers/projects/new_tree_controller.rb @@ -6,12 +6,16 @@ class Projects::NewTreeController < Projects::BaseTreeController end def update - file_path = File.join(@path, File.basename(params[:file_name])) + file_path = File.basename(params[:file_name]) + if @path != '' + file_path = File.join(@path, file_path) + end result = Files::CreateService.new(@project, current_user, params, @ref, file_path).execute if result[:status] == :success flash[:notice] = "Your changes have been successfully committed" - redirect_to project_blob_path(@project, File.join(@ref, file_path)) + redirect_to project_blob_path(@project, + ExtractsPath.join(@ref, file_path)) else flash[:alert] = result[:message] render :show diff --git a/app/controllers/projects/tree_controller.rb b/app/controllers/projects/tree_controller.rb index 4d033b36848..6c9be7a0225 100644 --- a/app/controllers/projects/tree_controller.rb +++ b/app/controllers/projects/tree_controller.rb @@ -4,7 +4,8 @@ class Projects::TreeController < Projects::BaseTreeController if tree.entries.empty? if @repository.blob_at(@commit.id, @path) - redirect_to project_blob_path(@project, File.join(@ref, @path)) and return + redirect_to project_blob_path(@project, ExtractsPath.join(@ref, @path)) + return else return not_found! end diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb index 8e209498323..24407d425e1 100644 --- a/app/helpers/tree_helper.rb +++ b/app/helpers/tree_helper.rb @@ -48,9 +48,8 @@ module TreeHelper "file_#{hexdigest(content.name)}" end - # Simple shortcut to File.join - def tree_join(*args) - File.join(*args) + def tree_join(ref, path) + ExtractsPath.join(ref, path) end def allowed_tree_edit? diff --git a/features/steps/shared/paths.rb b/features/steps/shared/paths.rb index 5f292255ce1..79f625dfa15 100644 --- a/features/steps/shared/paths.rb +++ b/features/steps/shared/paths.rb @@ -258,11 +258,14 @@ module SharedPaths end step 'I visit blob file from repo' do - visit project_blob_path(@project, File.join(sample_commit.id, sample_blob.path)) + visit project_blob_path( + @project, + ExtractsPath.join(sample_commit.id, sample_blob.path) + ) end step 'I visit ".gitignore" file in repo' do - visit project_blob_path(@project, File.join(root_ref, '.gitignore')) + visit project_blob_path(@project, ExtractsPath.join(root_ref, '.gitignore')) end step 'I am on the new file page' do @@ -271,7 +274,7 @@ module SharedPaths step 'I am on the ".gitignore" edit file page' do current_path.should eq(project_edit_tree_path( - @project, File.join(root_ref, '.gitignore'))) + @project, ExtractsPath.join(root_ref, '.gitignore'))) end step 'I visit project source page for "6d39438"' do diff --git a/lib/extracts_path.rb b/lib/extracts_path.rb index e51cb30bdd9..1603c5db282 100644 --- a/lib/extracts_path.rb +++ b/lib/extracts_path.rb @@ -12,6 +12,12 @@ module ExtractsPath end end + class << self + def join(ref, path) + ref + SEPARATOR + path + end + end + # Given a string containing both a Git tree-ish, such as a branch or tag, and # a filesystem path joined by forward slashes, attempts to separate the two. # @@ -58,10 +64,10 @@ module ExtractsPath # branches and tags # Append a trailing slash if we only get a ref and no file path - id += '/' unless id.ends_with?('/') + id += SEPARATOR unless id.ends_with?(SEPARATOR) valid_refs = @project.repository.ref_names - valid_refs.select! { |v| id.start_with?("#{v}/") } + valid_refs.select! { |v| id.start_with?("#{v}#{SEPARATOR}") } if valid_refs.length != 1 # No exact ref match, so just try our best @@ -122,9 +128,11 @@ module ExtractsPath private + SEPARATOR = '/' + def get_id id = params[:id] || params[:ref] - id += "/" + params[:path] unless params[:path].blank? + id = self.class.join(id, params[:path]) unless params[:path].blank? id end end diff --git a/spec/features/security/project/internal_access_spec.rb b/spec/features/security/project/internal_access_spec.rb index 598d554a946..fecaec2dc91 100644 --- a/spec/features/security/project/internal_access_spec.rb +++ b/spec/features/security/project/internal_access_spec.rb @@ -91,7 +91,10 @@ describe "Internal Project Access", feature: true do before do commit = project.repository.commit path = '.gitignore' - @blob_path = project_blob_path(project, File.join(commit.id, path)) + @blob_path = project_blob_path( + project, + ExtractsPath.join(commit.id, path) + ) end it { @blob_path.should be_allowed_for master } diff --git a/spec/features/security/project/private_access_spec.rb b/spec/features/security/project/private_access_spec.rb index b1d4c79e05b..a15a57b3081 100644 --- a/spec/features/security/project/private_access_spec.rb +++ b/spec/features/security/project/private_access_spec.rb @@ -91,7 +91,10 @@ describe "Private Project Access", feature: true do before do commit = project.repository.commit path = '.gitignore' - @blob_path = project_blob_path(project, File.join(commit.id, path)) + @blob_path = project_blob_path( + project, + ExtractsPath.join(commit.id, path) + ) end it { @blob_path.should be_allowed_for master } diff --git a/spec/features/security/project/public_access_spec.rb b/spec/features/security/project/public_access_spec.rb index a4c8a2be25a..ec77bd3e105 100644 --- a/spec/features/security/project/public_access_spec.rb +++ b/spec/features/security/project/public_access_spec.rb @@ -96,7 +96,10 @@ describe "Public Project Access", feature: true do before do commit = project.repository.commit path = '.gitignore' - @blob_path = project_blob_path(project, File.join(commit.id, path)) + @blob_path = project_blob_path( + project, + ExtractsPath.join(commit.id, path) + ) end it { @blob_path.should be_allowed_for master } -- GitLab