Fix issues with push 600+ commits. refactored, improved push events

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? 

app/models/project/hooks_trait.rb

 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

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?

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?

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

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

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

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

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