diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index 3c8e7ec73f6dedf53d9719d6bda430476cd8e902..6845fc5e6e67d6c1020554b77a6d89985f58ffe3 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -17,9 +17,17 @@ class Projects::BranchesController < Projects::ApplicationController
end
def create
- @branch = CreateBranchService.new.execute(project, params[:branch_name], params[:ref], current_user)
-
- redirect_to project_tree_path(@project, @branch.name)
+ result = CreateBranchService.new.execute(project,
+ params[:branch_name],
+ params[:ref],
+ current_user)
+ if result[:status] == :success
+ @branch = result[:branch]
+ redirect_to project_tree_path(@project, @branch.name)
+ else
+ @error = result[:message]
+ render action: 'new'
+ end
end
def destroy
diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb
index e03a9f4d66d0802b5f14f16953bf8b3c0aaf1f07..b84c497131a86d437c1b8f421bf59278ec5a20cb 100644
--- a/app/controllers/projects/tags_controller.rb
+++ b/app/controllers/projects/tags_controller.rb
@@ -13,10 +13,15 @@ class Projects::TagsController < Projects::ApplicationController
end
def create
- @tag = CreateTagService.new.execute(@project, params[:tag_name],
- params[:ref], current_user)
-
- redirect_to project_tags_path(@project)
+ result = CreateTagService.new.execute(@project, params[:tag_name],
+ params[:ref], current_user)
+ if result[:status] == :success
+ @tag = result[:tag]
+ redirect_to project_tags_path(@project)
+ else
+ @error = result[:message]
+ render action: 'new'
+ end
end
def destroy
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index 98beeee8354a3769ea7a7961e8e589ece2a933bd..79b8239602e7fb44052ae63272c03b4e70f70e56 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -1,13 +1,38 @@
class CreateBranchService
def execute(project, branch_name, ref, current_user)
+ valid_branch = Gitlab::GitRefValidator.validate(branch_name)
+ if valid_branch == false
+ return error('Branch name invalid')
+ end
+
repository = project.repository
+ existing_branch = repository.find_branch(branch_name)
+ if existing_branch
+ return error('Branch already exists')
+ end
+
repository.add_branch(branch_name, ref)
new_branch = repository.find_branch(branch_name)
if new_branch
Event.create_ref_event(project, current_user, new_branch, 'add')
+ return success(new_branch)
+ else
+ return error('Invalid reference name')
end
+ end
+
+ def error(message)
+ {
+ message: message,
+ status: :error
+ }
+ end
- new_branch
+ def success(branch)
+ {
+ branch: branch,
+ status: :success
+ }
end
end
diff --git a/app/services/create_tag_service.rb b/app/services/create_tag_service.rb
index 9776667740529278503fcd9b375bd95fe93ae775..6869acbe467efe31522a8069d26c6bb4f8a240c1 100644
--- a/app/services/create_tag_service.rb
+++ b/app/services/create_tag_service.rb
@@ -1,13 +1,38 @@
class CreateTagService
def execute(project, tag_name, ref, current_user)
+ valid_tag = Gitlab::GitRefValidator.validate(tag_name)
+ if valid_tag == false
+ return error('Tag name invalid')
+ end
+
repository = project.repository
+ existing_tag = repository.find_tag(tag_name)
+ if existing_tag
+ return error('Tag already exists')
+ end
+
repository.add_tag(tag_name, ref)
new_tag = repository.find_tag(tag_name)
if new_tag
Event.create_ref_event(project, current_user, new_tag, 'add', 'refs/tags')
+ return success(new_tag)
+ else
+ return error('Invalid reference name')
end
+ end
+
+ def error(message)
+ {
+ message: message,
+ status: :error
+ }
+ end
- new_tag
+ def success(branch)
+ {
+ tag: branch,
+ status: :success
+ }
end
end
diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb
index ce2d8093dff90f75b2c90efb5b743b6ed232bbe8..a94dabcdfc0729bae293d23d101923b4d254b73c 100644
--- a/app/services/delete_branch_service.rb
+++ b/app/services/delete_branch_service.rb
@@ -5,21 +5,21 @@ class DeleteBranchService
# No such branch
unless branch
- return error('No such branch')
+ return error('No such branch', 404)
end
if branch_name == repository.root_ref
- return error('Cannot remove HEAD branch')
+ return error('Cannot remove HEAD branch', 405)
end
# Dont allow remove of protected branch
if project.protected_branch?(branch_name)
- return error('Protected branch cant be removed')
+ return error('Protected branch cant be removed', 405)
end
# Dont allow user to remove branch if he is not allowed to push
unless current_user.can?(:push_code, project)
- return error('You dont have push access to repo')
+ return error('You dont have push access to repo', 405)
end
if repository.rm_branch(branch_name)
@@ -30,9 +30,10 @@ class DeleteBranchService
end
end
- def error(message)
+ def error(message, return_code = 400)
{
message: message,
+ return_code: return_code,
state: :error
}
end
diff --git a/app/views/projects/branches/new.html.haml b/app/views/projects/branches/new.html.haml
index 5da2ede2937b44f979c47b7cc686b1dd051c9b91..3f202f7ea6bd738779a76b2f4df8500045e580b8 100644
--- a/app/views/projects/branches/new.html.haml
+++ b/app/views/projects/branches/new.html.haml
@@ -1,3 +1,7 @@
+- if @error
+ .alert.alert-danger
+ %button{ type: "button", class: "close", "data-dismiss" => "alert"} ×
+ = @error
%h3.page-title
%i.icon-code-fork
New branch
@@ -5,11 +9,11 @@
.form-group
= label_tag :branch_name, 'Name for new branch', class: 'control-label'
.col-sm-10
- = text_field_tag :branch_name, nil, placeholder: 'enter new branch name', required: true, tabindex: 1, class: 'form-control'
+ = text_field_tag :branch_name, params[:branch_name], placeholder: 'enter new branch name', required: true, tabindex: 1, class: 'form-control'
.form-group
= label_tag :ref, 'Create from', class: 'control-label'
.col-sm-10
- = text_field_tag :ref, nil, placeholder: 'existing branch name, tag or commit SHA', required: true, tabindex: 2, class: 'form-control'
+ = text_field_tag :ref, params[:ref], placeholder: 'existing branch name, tag or commit SHA', required: true, tabindex: 2, class: 'form-control'
.form-actions
= submit_tag 'Create branch', class: 'btn btn-create', tabindex: 3
= link_to 'Cancel', project_branches_path(@project), class: 'btn btn-cancel'
diff --git a/app/views/projects/tags/new.html.haml b/app/views/projects/tags/new.html.haml
index a9fd97f8915e11284806d55fc0760ebfd604b591..f3a34d37df5ec0c7d73c115f9fdb57b56105c4db 100644
--- a/app/views/projects/tags/new.html.haml
+++ b/app/views/projects/tags/new.html.haml
@@ -1,3 +1,7 @@
+- if @error
+ .alert.alert-danger
+ %button{ type: "button", class: "close", "data-dismiss" => "alert"} ×
+ = @error
%h3.page-title
%i.icon-code-fork
New tag
@@ -5,11 +9,11 @@
.form-group
= label_tag :tag_name, 'Name for new tag', class: 'control-label'
.col-sm-10
- = text_field_tag :tag_name, nil, placeholder: 'v3.0.1', required: true, tabindex: 1, class: 'form-control'
+ = text_field_tag :tag_name, params[:tag_name], placeholder: 'v3.0.1', required: true, tabindex: 1, class: 'form-control'
.form-group
= label_tag :ref, 'Create from', class: 'control-label'
.col-sm-10
- = text_field_tag :ref, nil, placeholder: 'master', required: true, tabindex: 2, class: 'form-control'
+ = text_field_tag :ref, params[:ref], placeholder: 'master', required: true, tabindex: 2, class: 'form-control'
.light Branch name or commit SHA
.form-actions
= submit_tag 'Create tag', class: 'btn btn-create', tabindex: 3
diff --git a/doc/api/branches.md b/doc/api/branches.md
index 31469b6fe97e5736dd3776b7d9114b0d122df6d5..74386615545664a8f9a8f6837607b85e2a358159 100644
--- a/doc/api/branches.md
+++ b/doc/api/branches.md
@@ -196,6 +196,8 @@ Parameters:
}
```
+It return 200 if succeed or 400 if failed with error message explaining reason.
+
## Delete repository branch
```
@@ -207,4 +209,5 @@ Parameters:
- `id` (required) - The ID of a project
- `branch` (required) - The name of the branch
-It return 200 if succeed or 405 if failed with error message explaining reason.
+It return 200 if succeed, 404 if the branch to be deleted does not exist
+or 400 for other reasons. In case of an error, an explaining message is provided.
diff --git a/doc/api/repositories.md b/doc/api/repositories.md
index 1074b78fd735958882415180217ce06e5806a0e8..c9f6a45c3472c9fe327081d247fa252b076a2146 100644
--- a/doc/api/repositories.md
+++ b/doc/api/repositories.md
@@ -71,6 +71,9 @@ Parameters:
]
```
+It returns 200 if the operation succeed. In case of an error,
+405 with an explaining error message is returned.
+
## List repository tree
Get a list of repository files and directories in a project.
diff --git a/features/project/commits/branches.feature b/features/project/commits/branches.feature
index d657bd4951f026fe5e0bb903e77cc0985a650b42..d124cb7eecd8cd01d60861d9e561c9b9adc295d8 100644
--- a/features/project/commits/branches.feature
+++ b/features/project/commits/branches.feature
@@ -15,7 +15,7 @@ Feature: Project Browse branches
Scenario: I create a branch
Given I visit project branches page
And I click new branch link
- When I submit new branch form
+ And I submit new branch form
Then I should see new branch created
@javascript
@@ -23,3 +23,21 @@ Feature: Project Browse branches
Given I visit project branches page
And I click branch 'improve/awesome' delete link
Then I should not see branch 'improve/awesome'
+
+ Scenario: I create a branch with invalid name
+ Given I visit project branches page
+ And I click new branch link
+ And I submit new branch form with invalid name
+ Then I should see new an error that branch is invalid
+
+ Scenario: I create a branch with invalid reference
+ Given I visit project branches page
+ And I click new branch link
+ And I submit new branch form with invalid reference
+ Then I should see new an error that ref is invalid
+
+ Scenario: I create a branch that already exists
+ Given I visit project branches page
+ And I click new branch link
+ And I submit new branch form with branch that already exists
+ Then I should see new an error that branch already exists
diff --git a/features/project/commits/tags.feature b/features/project/commits/tags.feature
index 1ac0f8bfa45ad6f496203763d19078c4fe1c6f53..36c7a6492fffbb193a298ace983075c71c631199 100644
--- a/features/project/commits/tags.feature
+++ b/features/project/commits/tags.feature
@@ -7,5 +7,25 @@ Feature: Project Browse tags
Scenario: I can see all git tags
Then I should see "Shop" all tags list
+ Scenario: I create a tag
+ And I click new tag link
+ And I submit new tag form
+ Then I should see new tag created
+
+ Scenario: I create a tag with invalid name
+ And I click new tag link
+ And I submit new tag form with invalid name
+ Then I should see new an error that tag is invalid
+
+ Scenario: I create a tag with invalid reference
+ And I click new tag link
+ And I submit new tag form with invalid reference
+ Then I should see new an error that tag ref is invalid
+
+ Scenario: I create a tag that already exists
+ And I click new tag link
+ And I submit new tag form with tag that already exists
+ Then I should see new an error that tag already exists
+
# @wip
# Scenario: I can download project by tag
diff --git a/features/steps/project/browse_branches.rb b/features/steps/project/browse_branches.rb
index c00a95a62fd2261a032cb5077583c777acfd92b4..cfc88bdad22ace83581627a8acd20ac459550534 100644
--- a/features/steps/project/browse_branches.rb
+++ b/features/steps/project/browse_branches.rb
@@ -38,10 +38,38 @@ class ProjectBrowseBranches < Spinach::FeatureSteps
click_button 'Create branch'
end
+ step 'I submit new branch form with invalid name' do
+ fill_in 'branch_name', with: '1.0 stable'
+ fill_in 'ref', with: 'master'
+ click_button 'Create branch'
+ end
+
+ step 'I submit new branch form with invalid reference' do
+ fill_in 'branch_name', with: 'foo'
+ fill_in 'ref', with: 'foo'
+ click_button 'Create branch'
+ end
+
+ step 'I submit new branch form with branch that already exists' do
+ fill_in 'branch_name', with: 'master'
+ fill_in 'ref', with: 'master'
+ click_button 'Create branch'
+ end
+
step 'I should see new branch created' do
- within '.tree-ref-holder' do
- page.should have_content 'deploy_keys'
- end
+ page.should have_content 'deploy_keys'
+ end
+
+ step 'I should see new an error that branch is invalid' do
+ page.should have_content 'Branch name invalid'
+ end
+
+ step 'I should see new an error that ref is invalid' do
+ page.should have_content 'Invalid reference name'
+ end
+
+ step 'I should see new an error that branch already exists' do
+ page.should have_content 'Branch already exists'
end
step "I click branch 'improve/awesome' delete link" do
diff --git a/features/steps/project/browse_tags.rb b/features/steps/project/browse_tags.rb
index 7c679911e00678ddf59b266e087d56606c6be6af..64c0c284f6cbd7f736a185dc89d008abdd567b0b 100644
--- a/features/steps/project/browse_tags.rb
+++ b/features/steps/project/browse_tags.rb
@@ -3,8 +3,52 @@ class ProjectBrowseTags < Spinach::FeatureSteps
include SharedProject
include SharedPaths
- Then 'I should see "Shop" all tags list' do
+ step 'I should see "Shop" all tags list' do
page.should have_content "Tags"
page.should have_content "v1.0.0"
end
+
+ step 'I click new tag link' do
+ click_link 'New tag'
+ end
+
+ step 'I submit new tag form' do
+ fill_in 'tag_name', with: 'v7.0'
+ fill_in 'ref', with: 'master'
+ click_button 'Create tag'
+ end
+
+ step 'I submit new tag form with invalid name' do
+ fill_in 'tag_name', with: 'v 1.0'
+ fill_in 'ref', with: 'master'
+ click_button 'Create tag'
+ end
+
+ step 'I submit new tag form with invalid reference' do
+ fill_in 'tag_name', with: 'foo'
+ fill_in 'ref', with: 'foo'
+ click_button 'Create tag'
+ end
+
+ step 'I submit new tag form with tag that already exists' do
+ fill_in 'tag_name', with: 'v1.0.0'
+ fill_in 'ref', with: 'master'
+ click_button 'Create tag'
+ end
+
+ step 'I should see new tag created' do
+ page.should have_content 'v7.0'
+ end
+
+ step 'I should see new an error that tag is invalid' do
+ page.should have_content 'Tag name invalid'
+ end
+
+ step 'I should see new an error that tag ref is invalid' do
+ page.should have_content 'Invalid reference name'
+ end
+
+ step 'I should see new an error that tag already exists' do
+ page.should have_content 'Tag already exists'
+ end
end
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index b32a4aa7bc286d9c1514581fc2f0d71bc4375b15..4db5f61dd28b94b134e17b5f4e50eb2bddd5d16e 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -80,9 +80,17 @@ module API
# POST /projects/:id/repository/branches
post ":id/repository/branches" do
authorize_push_project
- @branch = CreateBranchService.new.execute(user_project, params[:branch_name], params[:ref], current_user)
-
- present @branch, with: Entities::RepoObject, project: user_project
+ result = CreateBranchService.new.execute(user_project,
+ params[:branch_name],
+ params[:ref],
+ current_user)
+ if result[:status] == :success
+ present result[:branch],
+ with: Entities::RepoObject,
+ project: user_project
+ else
+ render_api_error!(result[:message], 400)
+ end
end
# Delete branch
@@ -99,7 +107,7 @@ module API
if result[:state] == :success
true
else
- render_api_error!(result[:message], 405)
+ render_api_error!(result[:message], result[:return_code])
end
end
end
diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb
index 42068bb343d90dbbe72d0e5e2c079286c41a9352..a3773d2c59333676a43737e085116f11760770f5 100644
--- a/lib/api/repositories.rb
+++ b/lib/api/repositories.rb
@@ -36,10 +36,15 @@ module API
# POST /projects/:id/repository/tags
post ':id/repository/tags' do
authorize_push_project
- @tag = CreateTagService.new.execute(user_project, params[:tag_name],
- params[:ref], current_user)
-
- present @tag, with: Entities::RepoObject, project: user_project
+ result = CreateTagService.new.execute(user_project, params[:tag_name],
+ params[:ref], current_user)
+ if result[:status] == :success
+ present result[:tag],
+ with: Entities::RepoObject,
+ project: user_project
+ else
+ render_api_error!(result[:message], 400)
+ end
end
# Get a project repository tree
diff --git a/lib/gitlab/git_ref_validator.rb b/lib/gitlab/git_ref_validator.rb
new file mode 100644
index 0000000000000000000000000000000000000000..13cb08948bb71bad53bc9bddc2a14f09471a7962
--- /dev/null
+++ b/lib/gitlab/git_ref_validator.rb
@@ -0,0 +1,11 @@
+module Gitlab
+ module GitRefValidator
+ extend self
+ # Validates a given name against the git reference specification
+ #
+ # Returns true for a valid reference name, false otherwise
+ def validate(ref_name)
+ system *%W(git check-ref-format refs/#{ref_name})
+ end
+ end
+end
diff --git a/spec/lib/git_ref_validator_spec.rb b/spec/lib/git_ref_validator_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..b2469c18395edab10b10724374ade915e0c0d9cc
--- /dev/null
+++ b/spec/lib/git_ref_validator_spec.rb
@@ -0,0 +1,20 @@
+require 'spec_helper'
+
+describe Gitlab::GitRefValidator do
+ it { Gitlab::GitRefValidator.validate('feature/new').should be_true }
+ it { Gitlab::GitRefValidator.validate('implement_@all').should be_true }
+ it { Gitlab::GitRefValidator.validate('my_new_feature').should be_true }
+ it { Gitlab::GitRefValidator.validate('#1').should be_true }
+ it { Gitlab::GitRefValidator.validate('feature/~new/').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature/^new/').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature/:new/').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature/?new/').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature/*new/').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature/[new/').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature/new/').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature/new.').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature\@{').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature\new').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature//new').should be_false }
+ it { Gitlab::GitRefValidator.validate('feature new').should be_false }
+end
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index f3d7ca2ed21fe7caf8f17f0e81027e4cd14f17ec..e7f91c5e46e34c286f71aa3a85f6efda00d84ae4 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -94,22 +94,50 @@ describe API::API, api: true do
describe "POST /projects/:id/repository/branches" do
it "should create a new branch" do
post api("/projects/#{project.id}/repository/branches", user),
- branch_name: branch_name,
- ref: branch_sha
+ branch_name: 'feature1',
+ ref: branch_sha
response.status.should == 201
- json_response['name'].should == branch_name
+ json_response['name'].should == 'feature1'
json_response['commit']['id'].should == branch_sha
end
it "should deny for user without push access" do
post api("/projects/#{project.id}/repository/branches", user2),
- branch_name: branch_name,
- ref: branch_sha
-
+ branch_name: branch_name,
+ ref: branch_sha
response.status.should == 403
end
+
+ it 'should return 400 if branch name is invalid' do
+ post api("/projects/#{project.id}/repository/branches", user),
+ branch_name: 'new design',
+ ref: branch_sha
+ response.status.should == 400
+ json_response['message'].should == 'Branch name invalid'
+ end
+
+ it 'should return 400 if branch already exists' do
+ post api("/projects/#{project.id}/repository/branches", user),
+ branch_name: 'new_design1',
+ ref: branch_sha
+ response.status.should == 201
+
+ post api("/projects/#{project.id}/repository/branches", user),
+ branch_name: 'new_design1',
+ ref: branch_sha
+ response.status.should == 400
+ json_response['message'].should == 'Branch already exists'
+ end
+
+ it 'should return 400 if ref name is invalid' do
+ post api("/projects/#{project.id}/repository/branches", user),
+ branch_name: 'new_design3',
+ ref: 'foo'
+ response.status.should == 400
+ json_response['message'].should == 'Invalid reference name'
+ end
end
describe "DELETE /projects/:id/repository/branches/:branch" do
@@ -120,6 +148,11 @@ describe API::API, api: true do
response.status.should == 200
end
+ it 'should return 404 if branch not exists' do
+ delete api("/projects/#{project.id}/repository/branches/foobar", user)
+ response.status.should == 404
+ end
+
it "should remove protected branch" do
project.protected_branches.create(name: branch_name)
delete api("/projects/#{project.id}/repository/branches/#{branch_name}", user)
diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb
index f8603e11a043035e0b086f266e3dfbf9c67fee00..ffcdbc4255ee9e986856d32eebd4f1fec1375657 100644
--- a/spec/requests/api/repositories_spec.rb
+++ b/spec/requests/api/repositories_spec.rb
@@ -25,20 +25,46 @@ describe API::API, api: true do
describe 'POST /projects/:id/repository/tags' do
it 'should create a new tag' do
post api("/projects/#{project.id}/repository/tags", user),
- tag_name: 'v1.0.0',
+ tag_name: 'v2.0.0',
ref: 'master'
-
response.status.should == 201
- json_response['name'].should == 'v1.0.0'
+ json_response['name'].should == 'v2.0.0'
end
it 'should deny for user without push access' do
post api("/projects/#{project.id}/repository/tags", user2),
tag_name: 'v1.0.0',
ref: '621491c677087aa243f165eab467bfdfbee00be1'
-
response.status.should == 403
end
+
+ it 'should return 400 if tag name is invalid' do
+ post api("/projects/#{project.id}/repository/tags", user),
+ tag_name: 'v 1.0.0',
+ ref: 'master'
+ response.status.should == 400
+ json_response['message'].should == 'Tag name invalid'
+ end
+
+ it 'should return 400 if tag already exists' do
+ post api("/projects/#{project.id}/repository/tags", user),
+ tag_name: 'v8.0.0',
+ ref: 'master'
+ response.status.should == 201
+ post api("/projects/#{project.id}/repository/tags", user),
+ tag_name: 'v8.0.0',
+ ref: 'master'
+ response.status.should == 400
+ json_response['message'].should == 'Tag already exists'
+ end
+
+ it 'should return 400 if ref name is invalid' do
+ post api("/projects/#{project.id}/repository/tags", user),
+ tag_name: 'mytag',
+ ref: 'foo'
+ response.status.should == 400
+ json_response['message'].should == 'Invalid reference name'
+ end
end
describe "GET /projects/:id/repository/tree" do