Merge pull request #3169 from hiroponz/refactor-network-graph

Refactor network graph

app/assets/javascripts/branch-graph.js

@@ -117,8 +117,7 @@
       // Draw lines
       for (var j = 0, jj = this.commits[i].parents.length; j < jj; j++) {
         c = this.preparedCommits[this.commits[i].parents[j][0]];
-        ps = this.commits[i].parent_spaces[j];
+        ps = this.commits[i].parents[j][1];
-        if (c) {
         var cx = offsetX + 20 * c.time
           , cy = offsetY + 10 * c.space
           , psy = offsetY + 10 * ps;
@@ -171,7 +170,6 @@
           });
         }
       }
-      }
       
       if (this.commits[i].refs) {
         this.appendLabel(x, y, this.commits[i].refs);

app/controllers/graph_controller.rb

@@ -8,24 +8,21 @@ class GraphController < ProjectResourceController
   before_filter :require_non_empty_project
 
   def show
-    if params.has_key?(:q) && params[:q].blank?
+    if params.has_key?(:q)
+      if params[:q].blank?
         redirect_to project_graph_path(@project, params[:id])
         return
       end
 
-    if params.has_key?(:q)
       @q = params[:q]
       @commit = @project.repository.commit(@q) || @commit
     end
 
     respond_to do |format|
       format.html
+
       format.json do
-        graph = Graph::JsonBuilder.new(project, @ref, @commit)
+        @graph = Network::Graph.new(project, @ref, @commit)
-        graph.commits.each do |c|
-          c.icon = gravatar_icon(c.author.email)
-        end
-        render :json => graph.to_json
       end
     end
   end

app/helpers/graph_helper.rb

@@ -0,0 +1,10 @@
+module GraphHelper
+  def join_with_space(ary)
+    ary.collect{|r|r.name}.join(" ") unless ary.nil?
+  end
+
+  def parents_zip_spaces(parents, parent_spaces)
+    ids = parents.map { |p| p.id }
+    ids.zip(parent_spaces)
+  end
+end

app/models/graph/commit.rb

@@ -1,59 +0,0 @@
-require "grit"
-
-module Graph
-  class Commit
-    include ActionView::Helpers::TagHelper
-
-    attr_accessor :time, :spaces, :refs, :parent_spaces, :icon
-
-    def initialize(commit)
-      @_commit = commit
-      @time = -1
-      @spaces = []
-      @parent_spaces = []
-    end
-
-    def method_missing(m, *args, &block)
-      @_commit.send(m, *args, &block)
-    end
-
-    def to_graph_hash
-      h = {}
-      h[:parents] = self.parents.collect do |p|
-        [p.id,0,0]
-      end
-      h[:author]  = {
-        name: author.name,
-        email: author.email,
-        icon: icon
-      }
-      h[:time]    = time
-      h[:space]   = spaces.first
-      h[:parent_spaces]   = parent_spaces
-      h[:refs]    = refs.collect{|r|r.name}.join(" ") unless refs.nil?
-      h[:id]      = sha
-      h[:date]    = date
-      h[:message] = message
-      h
-    end
-
-    def add_refs(ref_cache, repo)
-      if ref_cache.empty?
-        repo.refs.each do |ref|
-          ref_cache[ref.commit.id] ||= []
-          ref_cache[ref.commit.id] << ref
-        end
-      end
-      @refs = ref_cache[@_commit.id] if ref_cache.include?(@_commit.id)
-      @refs ||= []
-    end
-
-    def space
-      if @spaces.size > 0
-        @spaces.first
-      else
-        0
-      end
-    end
-  end
-end

app/models/graph/json_builder.rb

@@ -1,291 +0,0 @@
-require "grit"
-
-module Graph
-  class JsonBuilder
-    attr_accessor :days, :commits, :ref_cache, :repo
-
-    def self.max_count
-      @max_count ||= 650
-    end
-
-    def initialize project, ref, commit
-      @project = project
-      @ref = ref
-      @commit = commit
-      @repo = project.repo
-      @ref_cache = {}
-
-      @commits = collect_commits
-      @days = index_commits
-    end
-
-    def to_json(*args)
-      {
-        days: @days.compact.map { |d| [d.day, d.strftime("%b")] },
-        commits: @commits.map(&:to_graph_hash)
-      }.to_json(*args)
-    end
-
-  protected
-
-    # Get commits from repository
-    #
-    def collect_commits
-
-      @commits = Grit::Commit.find_all(repo, nil, {date_order: true, max_count: self.class.max_count, skip: to_commit}).dup
-
-      # Decorate with app/models/commit.rb
-      @commits.map! { |commit| Commit.new(commit) }
-
-      # Decorate with lib/gitlab/graph/commit.rb
-      @commits.map! { |commit| Graph::Commit.new(commit) }
-
-      # add refs to each commit
-      @commits.each { |commit| commit.add_refs(ref_cache, repo) }
-
-      @commits
-    end
-
-    # Method is adding time and space on the
-    # list of commits. As well as returns date list
-    # corelated with time set on commits.
-    #
-    # @param [Array<Graph::Commit>] commits to index
-    #
-    # @return [Array<TimeDate>] list of commit dates corelated with time on commits
-    def index_commits
-      days, times = [], []
-      map = {}
-
-      commits.reverse.each_with_index do |c,i|
-        c.time = i
-        days[i] = c.committed_date
-        map[c.id] = c
-        times[i] = c
-      end
-
-      @_reserved = {}
-      days.each_index do |i|
-        @_reserved[i] = []
-      end
-
-      commits_sort_by_ref.each do |commit|
-        if map.include? commit.id then
-          place_chain(map[commit.id], map)
-        end
-      end
-
-      # find parent spaces for not overlap lines
-      times.each do |c|
-        c.parent_spaces.concat(find_free_parent_spaces(c, map, times))
-      end
-
-      days
-    end
-
-    # Skip count that the target commit is displayed in center.
-    def to_commit
-      commits = Grit::Commit.find_all(repo, nil, {date_order: true})
-      commit_index = commits.index do |c|
-        c.id == @commit.id
-      end
-
-      if commit_index && (self.class.max_count / 2 < commit_index) then
-        # get max index that commit is displayed in the center.
-        commit_index - self.class.max_count / 2
-      else
-        0
-      end
-    end
-
-    def commits_sort_by_ref
-      commits.sort do |a,b|
-        if include_ref?(a)
-          -1
-        elsif include_ref?(b)
-          1
-        else
-          b.committed_date <=> a.committed_date
-        end
-      end
-    end
-
-    def include_ref?(commit)
-      heads = commit.refs.select do |ref|
-        ref.is_a?(Grit::Head) or ref.is_a?(Grit::Remote) or ref.is_a?(Grit::Tag)
-      end
-
-      heads.map! do |head|
-        head.name
-      end
-
-      heads.include?(@ref)
-    end
-
-    def find_free_parent_spaces(commit, map, times)
-      spaces = []
-
-      commit.parents.each do |p|
-        if map.include?(p.id) then
-          parent = map[p.id]
-
-          range = if commit.time < parent.time then
-                    commit.time..parent.time
-                  else
-                    parent.time..commit.time
-                  end
-
-          space = if commit.space >= parent.space then
-                    find_free_parent_space(range, parent.space, -1, commit.space, times)
-                  else
-                    find_free_parent_space(range, commit.space, -1, parent.space, times)
-                  end
-
-          mark_reserved(range, space)
-          spaces << space
-        end
-      end
-
-      spaces
-    end
-
-    def find_free_parent_space(range, space_base, space_step, space_default, times)
-      if is_overlap?(range, times, space_default) then
-        find_free_space(range, space_step, space_base, space_default)
-      else
-        space_default
-      end
-    end
-
-    def is_overlap?(range, times, overlap_space)
-      range.each do |i|
-        if i != range.first &&
-          i != range.last &&
-          times[i].spaces.include?(overlap_space) then
-
-          return true;
-        end
-      end
-
-      false
-    end
-
-    # Add space mark on commit and its parents
-    #
-    # @param [Graph::Commit] the commit object.
-    # @param [Hash<String,Graph::Commit>] map of commits
-    def place_chain(commit, map, parent_time = nil)
-      leaves = take_left_leaves(commit, map)
-      if leaves.empty?
-        return
-      end
-
-      time_range = leaves.last.time..leaves.first.time
-      space_base = get_space_base(leaves, map)
-      space = find_free_space(time_range, 2, space_base)
-      leaves.each do |l|
-        l.spaces << space
-        # Also add space to parent
-        l.parents.each do |p|
-          if map.include?(p.id)
-            parent = map[p.id]
-            if parent.space > 0
-              parent.spaces << space
-            end
-          end
-        end
-      end
-
-      # and mark it as reserved
-      min_time = leaves.last.time
-      parents = leaves.last.parents.collect
-      parents.each do |p|
-        if map.include? p.id
-          parent = map[p.id]
-          if parent.time < min_time
-            min_time = parent.time
-          end
-        end
-      end
-
-      if parent_time.nil?
-        max_time = leaves.first.time
-      else
-        max_time = parent_time - 1
-      end
-      mark_reserved(min_time..max_time, space)
-
-      # Visit branching chains
-      leaves.each do |l|
-        parents = l.parents.collect.select{|p| map.include? p.id and map[p.id].space.zero?}
-        for p in parents
-          place_chain(map[p.id], map, l.time)
-        end
-      end
-    end
-
-    def get_space_base(leaves, map)
-      space_base = 1
-      if leaves.last.parents.size > 0
-        first_parent = leaves.last.parents.first
-        if map.include?(first_parent.id)
-          first_p = map[first_parent.id]
-          if first_p.space > 0
-            space_base = first_p.space
-          end
-        end
-      end
-      space_base
-    end
-
-    def mark_reserved(time_range, space)
-      for day in time_range
-        @_reserved[day].push(space)
-      end
-    end
-
-    def find_free_space(time_range, space_step, space_base = 1, space_default = nil)
-      space_default ||= space_base
-
-      reserved = []
-      for day in time_range
-        reserved += @_reserved[day]
-      end
-      reserved.uniq!
-
-      space = space_default
-      while reserved.include?(space) do
-        space += space_step
-        if space < space_base then
-          space_step *= -1
-          space = space_base + space_step
-        end
-      end
-
-      space
-    end
-
-    # Takes most left subtree branch of commits
-    # which don't have space mark yet.
-    #
-    # @param [Graph::Commit] the commit object.
-    # @param [Hash<String,Graph::Commit>] map of commits
-    #
-    # @return [Array<Graph::Commit>] list of branch commits
-    def take_left_leaves(commit, map)
-      leaves = []
-      leaves.push(commit) if commit.space.zero?
-
-      while true
-        return leaves if commit.parents.count.zero?
-        return leaves unless map.include? commit.parents.first.id
-
-        commit = map[commit.parents.first.id]
-
-        return leaves unless commit.space.zero?
-
-        leaves.push(commit)
-      end
-    end
-  end
-end

app/models/network/commit.rb

@@ -0,0 +1,39 @@
+require "grit"
+
+module Network
+  class Commit
+    include ActionView::Helpers::TagHelper
+
+    attr_reader :refs
+    attr_accessor :time, :spaces, :parent_spaces
+
+    def initialize(raw_commit, refs)
+      @commit = ::Commit.new(raw_commit)
+      @time = -1
+      @spaces = []
+      @parent_spaces = []
+      @refs = refs || []
+    end
+
+    def method_missing(m, *args, &block)
+      @commit.send(m, *args, &block)
+    end
+
+    def space
+      if @spaces.size > 0
+        @spaces.first
+      else
+        0
+      end
+    end
+
+    def parents(map)
+      @commit.parents.map do |p|
+        if map.include?(p.id)
+          map[p.id]
+        end
+      end
+      .compact
+    end
+  end
+end

app/models/network/graph.rb

@@ -0,0 +1,276 @@
+require "grit"
+
+module Network
+  class Graph
+    attr_reader :days, :commits, :map
+
+    def self.max_count
+      @max_count ||= 650
+    end
+
+    def initialize project, ref, commit
+      @project = project
+      @ref = ref
+      @commit = commit
+      @repo = project.repo
+
+      @commits = collect_commits
+      @days = index_commits
+    end
+
+    protected
+
+    # Get commits from repository
+    #
+    def collect_commits
+      refs_cache = build_refs_cache
+
+      Grit::Commit.find_all(
+        @repo,
+        nil,
+        {
+          date_order: true,
+          max_count: self.class.max_count,
+          skip: count_to_display_commit_in_center
+        }
+      )
+      .map do |commit|
+          # Decorate with app/model/network/commit.rb
+          Network::Commit.new(commit, refs_cache[commit.id])
+      end
+    end
+
+    # Method is adding time and space on the
+    # list of commits. As well as returns date list
+    # corelated with time set on commits.
+    #
+    # @return [Array<TimeDate>] list of commit dates corelated with time on commits
+    def index_commits
+      days = []
+      @map = {}
+
+      @commits.reverse.each_with_index do |c,i|
+        c.time = i
+        days[i] = c.committed_date
+        @map[c.id] = c
+      end
+
+      @reserved = {}
+      days.each_index do |i|
+        @reserved[i] = []
+      end
+
+      commits_sort_by_ref.each do |commit|
+        place_chain(commit)
+      end
+
+      # find parent spaces for not overlap lines
+      @commits.each do |c|
+        c.parent_spaces.concat(find_free_parent_spaces(c))
+      end
+
+      days
+    end
+
+    # Skip count that the target commit is displayed in center.
+    def count_to_display_commit_in_center
+      commit_index = Grit::Commit.find_all(@repo, nil, {date_order: true}).index do |c|
+        c.id == @commit.id
+      end
+
+      if commit_index && (self.class.max_count / 2 < commit_index) then
+        # get max index that commit is displayed in the center.
+        commit_index - self.class.max_count / 2
+      else
+        0
+      end
+    end
+
+    def commits_sort_by_ref
+      @commits.sort do |a,b|
+        if include_ref?(a)
+          -1
+        elsif include_ref?(b)
+          1
+        else
+          b.committed_date <=> a.committed_date
+        end
+      end
+    end
+
+    def include_ref?(commit)
+      heads = commit.refs.select do |ref|
+        ref.is_a?(Grit::Head) or ref.is_a?(Grit::Remote) or ref.is_a?(Grit::Tag)
+      end
+
+      heads.map! do |head|
+        head.name
+      end
+
+      heads.include?(@ref)
+    end
+
+    def find_free_parent_spaces(commit)
+      spaces = []
+
+      commit.parents(@map).each do |parent|
+        range = if commit.time < parent.time then
+                  commit.time..parent.time
+                else
+                  parent.time..commit.time
+                end
+
+        space = if commit.space >= parent.space then
+                  find_free_parent_space(range, parent.space, -1, commit.space)
+                else
+                  find_free_parent_space(range, commit.space, -1, parent.space)
+                end
+
+        mark_reserved(range, space)
+        spaces << space
+      end
+
+      spaces
+    end
+
+    def find_free_parent_space(range, space_base, space_step, space_default)
+      if is_overlap?(range, space_default) then
+        find_free_space(range, space_step, space_base, space_default)
+      else
+        space_default
+      end
+    end
+
+    def is_overlap?(range, overlap_space)
+      range.each do |i|
+        if i != range.first &&
+          i != range.last &&
+          @commits[reversed_index(i)].spaces.include?(overlap_space) then
+
+          return true;
+        end
+      end
+
+      false
+    end
+
+    # Add space mark on commit and its parents
+    #
+    # @param [::Commit] the commit object.
+    def place_chain(commit, parent_time = nil)
+      leaves = take_left_leaves(commit)
+      if leaves.empty?
+        return
+      end
+
+      time_range = leaves.last.time..leaves.first.time
+      space_base = get_space_base(leaves)
+      space = find_free_space(time_range, 2, space_base)
+      leaves.each do |l|
+        l.spaces << space
+        # Also add space to parent
+        l.parents(@map).each do |parent|
+          if parent.space > 0
+            parent.spaces << space
+          end
+        end
+      end
+
+      # and mark it as reserved
+      min_time = leaves.last.time
+      leaves.last.parents(@map).each do |parent|
+        if parent.time < min_time
+          min_time = parent.time
+        end
+      end
+
+      if parent_time.nil?
+        max_time = leaves.first.time
+      else
+        max_time = parent_time - 1
+      end
+      mark_reserved(min_time..max_time, space)
+
+      # Visit branching chains
+      leaves.each do |l|
+        parents = l.parents(@map).select{|p| p.space.zero?}
+        for p in parents
+          place_chain(p, l.time)
+        end
+      end
+    end
+
+    def get_space_base(leaves)
+      space_base = 1
+      parents = leaves.last.parents(@map)
+      if parents.size > 0
+        if parents.first.space > 0
+          space_base = parents.first.space
+        end
+      end
+      space_base
+    end
+
+    def mark_reserved(time_range, space)
+      for day in time_range
+        @reserved[day].push(space)
+      end
+    end
+
+    def find_free_space(time_range, space_step, space_base = 1, space_default = nil)
+      space_default ||= space_base
+
+      reserved = []
+      for day in time_range
+        reserved += @reserved[day]
+      end
+      reserved.uniq!
+
+      space = space_default
+      while reserved.include?(space) do
+        space += space_step
+        if space < space_base then
+          space_step *= -1
+          space = space_base + space_step
+        end
+      end
+
+      space
+    end
+
+    # Takes most left subtree branch of commits
+    # which don't have space mark yet.
+    #
+    # @param [::Commit] the commit object.
+    #
+    # @return [Array<Network::Commit>] list of branch commits
+    def take_left_leaves(raw_commit)
+      commit = @map[raw_commit.id]
+      leaves = []
+      leaves.push(commit) if commit.space.zero?
+
+      while true
+        return leaves if commit.parents(@map).count.zero?
+
+        commit = commit.parents(@map).first
+
+        return leaves unless commit.space.zero?
+
+        leaves.push(commit)
+      end
+    end
+
+    def build_refs_cache
+      refs_cache = {}
+      @repo.refs.each do |ref|
+        refs_cache[ref.commit.id] = [] unless refs_cache.include?(ref.commit.id)
+        refs_cache[ref.commit.id] << ref
+      end
+      refs_cache
+    end
+
+    def reversed_index(index)
+      -index - 1
+    end
+  end
+end

app/views/graph/show.json.erb

@@ -0,0 +1,23 @@
+<% self.formats = ["html"] %>
+
+<%= raw(
+  {
+    days: @graph.days.compact.map { |d| [d.day, d.strftime("%b")] },
+    commits: @graph.commits.map do |c|
+      {
+        parents: parents_zip_spaces(c.parents(@graph.map), c.parent_spaces),
+        author: {
+          name: c.author.name,
+          email: c.author.email,
+          icon: gravatar_icon(c.author.email, 20)
+        },
+        time: c.time,
+        space: c.spaces.first,
+        refs: join_with_space(c.refs),
+        id: c.sha,
+        date: c.date,
+        message: c.message,
+      }
+    end
+  }.to_json
+) %>

features/steps/project/project_network_graph.rb

@@ -8,8 +8,8 @@ class ProjectNetworkGraph < Spinach::FeatureSteps
   end
 
   When 'I visit project "Shop" network page' do
-    # Stub Graph::JsonBuilder max_size to speed up test (10 commits vs. 650)
+    # Stub Graph max_size to speed up test (10 commits vs. 650)
-    Graph::JsonBuilder.stub(max_count: 10)
+    Network::Graph.stub(max_count: 10)
 
     project = Project.find_by_name("Shop")
     visit project_graph_path(project, "master")
@@ -25,7 +25,7 @@ class ProjectNetworkGraph < Spinach::FeatureSteps
     end
   end
 
-  And 'I switch ref to "stable"' do
+  When 'I switch ref to "stable"' do
     page.select 'stable', :from => 'ref'
     sleep 2
   end
@@ -40,7 +40,7 @@ class ProjectNetworkGraph < Spinach::FeatureSteps
     end
   end
 
-  And 'I looking for a commit by SHA of "v2.1.0"' do
+  When 'I looking for a commit by SHA of "v2.1.0"' do
     within ".content .search" do
       fill_in 'q', :with => '98d6492'
       find('button').click

features/steps/shared/paths.rb

@@ -142,8 +142,8 @@ module SharedPaths
   end
 
   Given "I visit my project's network page" do
-    # Stub Graph::JsonBuilder max_size to speed up test (10 commits vs. 650)
+    # Stub Graph max_size to speed up test (10 commits vs. 650)
-    Graph::JsonBuilder.stub(max_count: 10)
+    Network::Graph.stub(max_count: 10)
 
     visit project_graph_path(@project, root_ref)
   end