From 36738897bf06937056285b0eeabef13928d4d704 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 4 Sep 2012 23:57:39 -0400 Subject: [PATCH 1/6] Add specs for Project#discover_default_branch --- spec/roles/repository_spec.rb | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/spec/roles/repository_spec.rb b/spec/roles/repository_spec.rb index 62aecc1341f..579425d75b4 100644 --- a/spec/roles/repository_spec.rb +++ b/spec/roles/repository_spec.rb @@ -19,4 +19,30 @@ describe Project, "Repository" do project.should_not be_empty_repo end end + + describe "#discover_default_branch" do + let(:master) { double(name: 'master') } + let(:stable) { double(name: 'stable') } + + it "returns 'master' when master exists" do + project.should_receive(:heads).and_return([stable, master]) + project.discover_default_branch.should == 'master' + end + + it "returns non-master when master exists but default branch is set to something else" do + project.default_branch = 'stable' + project.should_receive(:heads).and_return([stable, master]) + project.discover_default_branch.should == 'stable' + end + + it "returns a non-master branch when only one exists" do + project.should_receive(:heads).and_return([stable]) + project.discover_default_branch.should == 'stable' + end + + it "returns nil when no branch exists" do + project.should_receive(:heads).and_return([]) + project.discover_default_branch.should be_nil + end + end end From f06d98e907f71dc9b3a4a2da0db1c096b6c07024 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 5 Sep 2012 01:00:07 -0400 Subject: [PATCH 2/6] Add SetDefaultBranchDefaultToNil migration default_branch now defaults to nil, not 'master'. It will be set after the first push by discover_default_branch. --- app/models/project.rb | 2 +- ...043334_set_default_branch_default_to_nil.rb | 12 ++++++++++++ db/schema.rb | 18 +++++++++--------- 3 files changed, 22 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20120905043334_set_default_branch_default_to_nil.rb diff --git a/app/models/project.rb b/app/models/project.rb index a7735a42137..fc18ad55528 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -187,7 +187,7 @@ end # private_flag :boolean(1) default(TRUE), not null # code :string(255) # owner_id :integer(4) -# default_branch :string(255) default("master"), not null +# default_branch :string(255) # issues_enabled :boolean(1) default(TRUE), not null # wall_enabled :boolean(1) default(TRUE), not null # merge_requests_enabled :boolean(1) default(TRUE), not null diff --git a/db/migrate/20120905043334_set_default_branch_default_to_nil.rb b/db/migrate/20120905043334_set_default_branch_default_to_nil.rb new file mode 100644 index 00000000000..f5956fe8751 --- /dev/null +++ b/db/migrate/20120905043334_set_default_branch_default_to_nil.rb @@ -0,0 +1,12 @@ +class SetDefaultBranchDefaultToNil < ActiveRecord::Migration + def up + # Set the default_branch to allow nil, and default it to nil + change_column_null(:projects, :default_branch, true) + change_column_default(:projects, :default_branch, nil) + end + + def down + change_column_null(:projects, :default_branch, false) + change_column_default(:projects, :default_branch, 'master') + end +end diff --git a/db/schema.rb b/db/schema.rb index 46461e44aad..00bb55234af 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20120729131232) do +ActiveRecord::Schema.define(:version => 20120905043334) do create_table "events", :force => true do |t| t.string "target_type" @@ -98,16 +98,16 @@ ActiveRecord::Schema.define(:version => 20120729131232) do t.string "name" t.string "path" t.text "description" - t.datetime "created_at", :null => false - t.datetime "updated_at", :null => false - t.boolean "private_flag", :default => true, :null => false + t.datetime "created_at", :null => false + t.datetime "updated_at", :null => false + t.boolean "private_flag", :default => true, :null => false t.string "code" t.integer "owner_id" - t.string "default_branch", :default => "master", :null => false - t.boolean "issues_enabled", :default => true, :null => false - t.boolean "wall_enabled", :default => true, :null => false - t.boolean "merge_requests_enabled", :default => true, :null => false - t.boolean "wiki_enabled", :default => true, :null => false + t.string "default_branch" + t.boolean "issues_enabled", :default => true, :null => false + t.boolean "wall_enabled", :default => true, :null => false + t.boolean "merge_requests_enabled", :default => true, :null => false + t.boolean "wiki_enabled", :default => true, :null => false end create_table "protected_branches", :force => true do |t| From 443e23e61a10f34e9eccc3b6e91803a0996f5050 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 5 Sep 2012 01:01:20 -0400 Subject: [PATCH 3/6] Add Repository#discover_default_branch and add it to PushObserver --- app/roles/push_observer.rb | 16 ++++++++++++---- app/roles/repository.rb | 24 +++++++++++++++++++++--- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/app/roles/push_observer.rb b/app/roles/push_observer.rb index 1067404d5af..947ed42345d 100644 --- a/app/roles/push_observer.rb +++ b/app/roles/push_observer.rb @@ -1,3 +1,6 @@ +# Includes methods for handling Git Push events +# +# Triggered by PostReceive job module PushObserver def observe_push(oldrev, newrev, ref, user) data = post_receive_data(oldrev, newrev, ref, user) @@ -84,11 +87,10 @@ module PushObserver data end - - # This method will be called after each post receive - # and only if user present in gitlab. - # All callbacks for post receive should be placed here + # This method will be called after each post receive and only if the provided + # user is present in GitLab. # + # All callbacks for post receive should be placed here. def trigger_post_receive(oldrev, newrev, ref, user) # Create push event self.observe_push(oldrev, newrev, ref, user) @@ -101,5 +103,11 @@ module PushObserver # Create satellite self.satellite.create unless self.satellite.exists? + + # Discover the default branch, but only if it hasn't already been set to + # something else + if default_branch.nil? + update_attributes(default_branch: discover_default_branch) + end end end diff --git a/app/roles/repository.rb b/app/roles/repository.rb index 5f6c35414e1..a77de4ad5f0 100644 --- a/app/roles/repository.rb +++ b/app/roles/repository.rb @@ -94,6 +94,24 @@ module Repository end.sort_by(&:name) end + # Discovers the default branch based on the repository's available branches + # + # - If no branches are present, returns nil + # - If one branch is present, returns its name + # - If two or more branches are present, returns the one that has a name + # matching root_ref (default_branch or 'master' if default_branch is nil) + def discover_default_branch + branches = heads.collect(&:name) + + if branches.length == 0 + nil + elsif branches.length == 1 + branches.first + else + branches.select { |v| v == root_ref }.first + end + end + def has_commits? !!commit end @@ -102,7 +120,7 @@ module Repository default_branch || "master" end - def root_ref? branch + def root_ref?(branch) root_ref == branch end @@ -111,7 +129,7 @@ module Repository # Already packed repo archives stored at # app_root/tmp/repositories/project_name/project_name-commit-id.tag.gz # - def archive_repo ref + def archive_repo(ref) ref = ref || self.root_ref commit = self.commit(ref) return nil unless commit @@ -138,6 +156,6 @@ module Repository end def http_url_to_repo - http_url = [Gitlab.config.url, "/", path, ".git"].join() + http_url = [Gitlab.config.url, "/", path, ".git"].join('') end end From 861a51488a44eac143ac4f44802fdf05fbd8f4c1 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 5 Sep 2012 01:02:30 -0400 Subject: [PATCH 4/6] Add specs for a couple more Repository methods used by discover_default_branch --- spec/roles/repository_spec.rb | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/roles/repository_spec.rb b/spec/roles/repository_spec.rb index 579425d75b4..0fda57a3e27 100644 --- a/spec/roles/repository_spec.rb +++ b/spec/roles/repository_spec.rb @@ -45,4 +45,28 @@ describe Project, "Repository" do project.discover_default_branch.should be_nil end end + + describe "#root_ref" do + it "returns default_branch when set" do + project.default_branch = 'stable' + project.root_ref.should == 'stable' + end + + it "returns 'master' when default_branch is nil" do + project.default_branch = nil + project.root_ref.should == 'master' + end + end + + describe "#root_ref?" do + it "returns true when branch is root_ref" do + project.default_branch = 'stable' + project.root_ref?('stable').should be_true + end + + it "returns false when branch is not root_ref" do + project.default_branch = nil + project.root_ref?('stable').should be_false + end + end end From a9f275bc201e666b9f26768aa228aca8250d5a94 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 5 Sep 2012 01:12:44 -0400 Subject: [PATCH 5/6] Fix load_refs in ApplicationController after default_branch change As a last resort it was calling a method that didn't exist. Woops! --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d53b23bb246..dae2e906256 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -124,7 +124,7 @@ class ApplicationController < ActionController::Base if params[:ref].blank? @branch = params[:branch].blank? ? nil : params[:branch] @tag = params[:tag].blank? ? nil : params[:tag] - @ref = @branch || @tag || @project.try(:default_branch) || Repository.default_ref + @ref = @branch || @tag || @project.try(:default_branch) || 'master' else @ref = params[:ref] end From 5e1c63d3f0f0729a1d9d9e19c64b2fef65cc30fb Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 5 Sep 2012 01:13:41 -0400 Subject: [PATCH 6/6] Move load_refs out of ApplicationController and into CommitsController That was the only place it was used. --- app/controllers/application_controller.rb | 10 ---------- app/controllers/commits_controller.rb | 14 +++++++++++++- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dae2e906256..7e53b8fe5ff 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -120,16 +120,6 @@ class ApplicationController < ActionController::Base end end - def load_refs - if params[:ref].blank? - @branch = params[:branch].blank? ? nil : params[:branch] - @tag = params[:tag].blank? ? nil : params[:tag] - @ref = @branch || @tag || @project.try(:default_branch) || 'master' - else - @ref = params[:ref] - end - end - def render_404 render file: File.join(Rails.root, "public", "404"), layout: false, status: "404" end diff --git a/app/controllers/commits_controller.rb b/app/controllers/commits_controller.rb index 717912d9e92..5e10a1b6ee7 100644 --- a/app/controllers/commits_controller.rb +++ b/app/controllers/commits_controller.rb @@ -59,7 +59,7 @@ class CommitsController < ApplicationController def patch @commit = project.commit(params[:id]) - + send_data( @commit.to_patch, type: "text/plain", @@ -67,4 +67,16 @@ class CommitsController < ApplicationController filename: (@commit.id.to_s + ".patch") ) end + + protected + + def load_refs + if params[:ref].blank? + @branch = params[:branch].blank? ? nil : params[:branch] + @tag = params[:tag].blank? ? nil : params[:tag] + @ref = @branch || @tag || @project.try(:default_branch) || 'master' + else + @ref = params[:ref] + end + end end