From 897da534fa7ab3b2280bfea05c5da4b57e3b8612 Mon Sep 17 00:00:00 2001 From: randx Date: Wed, 4 Apr 2012 07:39:04 +0300 Subject: [PATCH] Fix issues with push 600+ commits. refactored, improved push events --- app/models/event/push_trait.rb | 17 +++++--- app/models/project/hooks_trait.rb | 55 ++++++++++++++++++++------ app/views/commits/compare.html.haml | 2 +- app/views/dashboard/index.html.haml | 2 +- app/views/events/_event_push.html.haml | 8 ++-- app/workers/post_receive.rb | 9 +---- spec/models/project_hooks_spec.rb | 16 ++++---- spec/models/project_spec.rb | 4 +- spec/workers/post_receive_spec.rb | 4 +- 9 files changed, 72 insertions(+), 45 deletions(-) diff --git a/app/models/event/push_trait.rb b/app/models/event/push_trait.rb index a3bd50be..f12d50bd 100644 --- a/app/models/event/push_trait.rb +++ b/app/models/event/push_trait.rb @@ -49,11 +49,14 @@ module Event::PushTrait def tag_name @tag_name ||= data[:ref].gsub("refs/tags/", "") end - + + # Max 20 commits from push DESC def commits - @commits ||= data[:commits].map do |commit| - project.commit(commit["id"]) - end + @commits ||= data[:commits].map { |commit| project.commit(commit[:id]) }.reverse + end + + def commits_count + data[:total_commits_count] || commits.count || 0 end def ref_type @@ -71,13 +74,15 @@ module Event::PushTrait end def parent_commit - commits.first.prev_commit + project.commit(commit_from) rescue => ex nil end def last_commit - commits.last + project.commit(commit_to) + rescue => ex + nil end def push_with_commits? diff --git a/app/models/project/hooks_trait.rb b/app/models/project/hooks_trait.rb index fc32cf4c..c3a4f613 100644 --- a/app/models/project/hooks_trait.rb +++ b/app/models/project/hooks_trait.rb @@ -1,7 +1,7 @@ module Project::HooksTrait as_trait do - def observe_push(oldrev, newrev, ref, author_key_id) - data = web_hook_data(oldrev, newrev, ref, author_key_id) + def observe_push(oldrev, newrev, ref, user) + data = post_receive_data(oldrev, newrev, ref, user) Event.create( :project => self, @@ -11,10 +11,9 @@ module Project::HooksTrait ) end - def update_merge_requests(oldrev, newrev, ref, author_key_id) + def update_merge_requests(oldrev, newrev, ref, user) return true unless ref =~ /heads/ branch_name = ref.gsub("refs/heads/", "") - user = Key.find_by_identifier(author_key_id).user c_ids = self.commits_between(oldrev, newrev).map(&:id) # Update code for merge requests @@ -29,36 +28,48 @@ module Project::HooksTrait true end - def execute_web_hooks(oldrev, newrev, ref, author_key_id) + def execute_web_hooks(oldrev, newrev, ref, user) ref_parts = ref.split('/') # Return if this is not a push to a branch (e.g. new commits) return if ref_parts[1] !~ /heads/ || oldrev == "00000000000000000000000000000000" - data = web_hook_data(oldrev, newrev, ref, author_key_id) + data = post_receive_data(oldrev, newrev, ref, user) web_hooks.each { |web_hook| web_hook.execute(data) } end - def web_hook_data(oldrev, newrev, ref, author_key_id) - key = Key.find_by_identifier(author_key_id) + def post_receive_data(oldrev, newrev, ref, user) + + push_commits = commits_between(oldrev, newrev) + + # Total commits count + push_commits_count = push_commits.size + + # Get latest 20 commits ASC + push_commits_limited = push_commits.last(20) + + # Hash to be passed as post_receive_data data = { before: oldrev, after: newrev, ref: ref, - user_id: key.user.id, - user_name: key.user_name, + user_id: user.id, + user_name: user.name, repository: { name: name, url: web_url, description: description, homepage: web_url, - private: private? }, - commits: [] + commits: [], + total_commits_count: push_commits_count } - commits_between(oldrev, newrev).each do |commit| + # For perfomance purposes maximum 20 latest commits + # will be passed as post receive hook data. + # + push_commits_limited.each do |commit| data[:commits] << { id: commit.id, message: commit.safe_message, @@ -73,5 +84,23 @@ module Project::HooksTrait data end + + + # This method will be called after each post receive + # and only if autor_key_id present in gitlab. + # All callbacks for post receive should be placed here + # + def trigger_post_receive(oldrev, newrev, ref, author_key_id) + user = Key.find_by_identifier(author_key_id).user + + # Create push event + self.observe_push(oldrev, newrev, ref, user) + + # Close merged MR + self.update_merge_requests(oldrev, newrev, ref, user) + + # Execute web hooks + self.execute_web_hooks(oldrev, newrev, ref, user) + end end end diff --git a/app/views/commits/compare.html.haml b/app/views/commits/compare.html.haml index a4b07e29..493b47ca 100644 --- a/app/views/commits/compare.html.haml +++ b/app/views/commits/compare.html.haml @@ -24,7 +24,7 @@ - unless @commits.empty? - %h4 Commits + %h4 Commits (#{@commits.count}) %ul.unstyled= render @commits - unless @diffs.empty? diff --git a/app/views/dashboard/index.html.haml b/app/views/dashboard/index.html.haml index cd81724c..a33a7d77 100644 --- a/app/views/dashboard/index.html.haml +++ b/app/views/dashboard/index.html.haml @@ -47,7 +47,7 @@ - if @last_push && @last_push.valid_push? .padded.prepend-top-20 %h5 - %small Latest push was to the #{@last_push.branch_name} branch of #{@last_push.project.name}: + %small Latest push was to the #{@last_push.ref_name} #{@last_push.ref_type} of #{@last_push.project.name}: %ul.unstyled= render @last_push - if @merge_requests.any? diff --git a/app/views/events/_event_push.html.haml b/app/views/events/_event_push.html.haml index 4c6102d0..a94dc653 100644 --- a/app/views/events/_event_push.html.haml +++ b/app/views/events/_event_push.html.haml @@ -11,17 +11,17 @@ ago. - if event.push_with_commits? - - if event.commits.count > 1 + - if event.commits_count > 1 = link_to compare_project_commits_path(event.project, :from => event.parent_commit.id, :to => event.last_commit.id) do - %strong #{event.commits.first.id[0..7]}...#{event.last_commit.id[0..7]} + %strong #{event.parent_commit.id[0..7]}...#{event.last_commit.id[0..7]} - project = event.project %ul.unstyled.event_commits - - if event.commits.size > 3 + - if event.commits_count > 3 - event.commits[0...2].each do |commit| = render "events/commit", :commit => commit, :project => project %li %br - \... and #{event.commits.size - 2} more commits + \... and #{event.commits_count - 2} more commits - else - event.commits.each do |commit| = render "events/commit", :commit => commit, :project => project diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index 28216ec3..0ab9079c 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -8,13 +8,6 @@ class PostReceive # Ignore push from non-gitlab users return false unless Key.find_by_identifier(author_key_id) - # Create push event - project.observe_push(oldrev, newrev, ref, author_key_id) - - # Close merged MR - project.update_merge_requests(oldrev, newrev, ref, author_key_id) - - # Execute web hooks - project.execute_web_hooks(oldrev, newrev, ref, author_key_id) + project.trigger_post_receive(oldrev, newrev, ref, author_key_id) end end diff --git a/spec/models/project_hooks_spec.rb b/spec/models/project_hooks_spec.rb index 5a03b01f..9e2a9df2 100644 --- a/spec/models/project_hooks_spec.rb +++ b/spec/models/project_hooks_spec.rb @@ -4,19 +4,20 @@ describe Project, "Hooks" do let(:project) { Factory :project } before do @key = Factory :key, :user => project.owner + @user = @key.user @key_id = @key.identifier end describe "Post Receive Event" do it "should create push event" do oldrev, newrev, ref = '00000000000000000000000000000000', 'newrev', 'refs/heads/master' - project.observe_push(oldrev, newrev, ref, @key_id) + project.observe_push(oldrev, newrev, ref, @user) event = Event.last event.should_not be_nil event.project.should == project event.action.should == Event::Pushed - event.data == project.web_hook_data(oldrev, newrev, ref, @key_id) + event.data == project.post_receive_data(oldrev, newrev, ref, @user) end end @@ -24,7 +25,7 @@ describe Project, "Hooks" do context "with no web hooks" do it "raises no errors" do lambda { - project.execute_web_hooks('oldrev', 'newrev', 'ref', @key_id) + project.execute_web_hooks('oldrev', 'newrev', 'ref', @user) }.should_not raise_error end end @@ -40,7 +41,7 @@ describe Project, "Hooks" do @webhook.should_receive(:execute).once @webhook_2.should_receive(:execute).once - project.execute_web_hooks('oldrev', 'newrev', 'refs/heads/master', @key_id) + project.execute_web_hooks('oldrev', 'newrev', 'refs/heads/master', @user) end end @@ -52,12 +53,12 @@ describe Project, "Hooks" do it "when pushing a branch for the first time" do @webhook.should_not_receive(:execute) - project.execute_web_hooks('00000000000000000000000000000000', 'newrev', 'refs/heads/master', @key_id) + project.execute_web_hooks('00000000000000000000000000000000', 'newrev', 'refs/heads/master', @user) end it "when pushing tags" do @webhook.should_not_receive(:execute) - project.execute_web_hooks('oldrev', 'newrev', 'refs/tags/v1.0.0', @key_id) + project.execute_web_hooks('oldrev', 'newrev', 'refs/tags/v1.0.0', @user) end end @@ -73,7 +74,7 @@ describe Project, "Hooks" do # Fill nil/empty attributes project.description = "This is a description" - @data = project.web_hook_data(@oldrev, @newrev, @ref, @key_id) + @data = project.post_receive_data(@oldrev, @newrev, @ref, @user) end subject { @data } @@ -91,7 +92,6 @@ describe Project, "Hooks" do it { should include(url: project.web_url) } it { should include(description: project.description) } it { should include(homepage: project.web_url) } - it { should include(private: project.private?) } end context "with commits" do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 1f8133e7..6285a852 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -175,7 +175,7 @@ describe Project do it "should close merge request if last commit from source branch was pushed to target branch" do @merge_request.reloaded_commits @merge_request.last_commit.id.should == "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a" - project.update_merge_requests("8716fc78f3c65bbf7bcf7b574febd583bc5d2812", "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a", "refs/heads/stable", @key.identifier) + project.update_merge_requests("8716fc78f3c65bbf7bcf7b574febd583bc5d2812", "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a", "refs/heads/stable", @key.user) @merge_request.reload @merge_request.merged.should be_true @merge_request.closed.should be_true @@ -183,7 +183,7 @@ describe Project do it "should update merge request commits with new one if pushed to source branch" do @merge_request.last_commit.should == nil - project.update_merge_requests("8716fc78f3c65bbf7bcf7b574febd583bc5d2812", "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a", "refs/heads/master", @key.identifier) + project.update_merge_requests("8716fc78f3c65bbf7bcf7b574febd583bc5d2812", "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a", "refs/heads/master", @key.user) @merge_request.reload @merge_request.last_commit.id.should == "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a" end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 6b833d72..c70b2f6c 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -29,14 +29,14 @@ describe PostReceive do it "asks the project to execute web hooks" do Project.stub(find_by_path: project) - project.should_receive(:execute_web_hooks).with('sha-old', 'sha-new', 'refs/heads/master', key_id) + project.should_receive(:execute_web_hooks).with('sha-old', 'sha-new', 'refs/heads/master', project.owner) PostReceive.perform(project.path, 'sha-old', 'sha-new', 'refs/heads/master', key_id) end it "asks the project to observe push/create event data" do Project.stub(find_by_path: project) - project.should_receive(:observe_push).with('sha-old', 'sha-new', 'refs/heads/master', key_id) + project.should_receive(:observe_push).with('sha-old', 'sha-new', 'refs/heads/master', project.owner) PostReceive.perform(project.path, 'sha-old', 'sha-new', 'refs/heads/master', key_id) end