Merge branch '30973-network-graph-sorted-by-date-and-topo' into 'master'

changelogs/unreleased/30973-network-graph-sorted-by-date-and-topo.yml

+---
+title: Sort the network graph both by commit date and topographically
+merge_request: 11057
+author:

lib/gitlab/git/repository.rb

@@ -486,8 +486,9 @@ module Gitlab
       #     :contains is the commit contained by the refs from which to begin (SHA1 or name)
       #     :max_count is the maximum number of commits to fetch
       #     :skip is the number of commits to skip
-      #     :order is the commits order and allowed value is :none (default), :date, or :topo
-      #        commit ordering types are documented here:
+      #     :order is the commits order and allowed value is :none (default), :date,
+      #        :topo, or any combination of them (in an array). Commit ordering types
+      #        are documented here:
       #        http://www.rubydoc.info/github/libgit2/rugged/Rugged#SORT_NONE-constant)
       #
       def find_commits(options = {})
@@ -1265,16 +1266,30 @@ module Gitlab
         @gitaly_ref_client ||= Gitlab::GitalyClient::Ref.new(self)
       end
 
-      # Returns the `Rugged` sorting type constant for a given
-      # sort type key. Valid keys are `:none`, `:topo`, and `:date`
-      def rugged_sort_type(key)
+      def gitaly_commit_client
+        @gitaly_commit_client ||= Gitlab::GitalyClient::Commit.new(self)
+      end
+
+      def gitaly_migrate(method, &block)
+        Gitlab::GitalyClient.migrate(method, &block)
+      rescue GRPC::NotFound => e
+        raise NoRepository.new(e)
+      rescue GRPC::BadStatus => e
+        raise CommandError.new(e)
+      end
+
+      # Returns the `Rugged` sorting type constant for one or more given
+      # sort types. Valid keys are `:none`, `:topo`, and `:date`, or an array
+      # containing more than one of them. `:date` uses a combination of date and
+      # topological sorting to closer mimic git's native ordering.
+      def rugged_sort_type(sort_type)
         @rugged_sort_types ||= {
           none: Rugged::SORT_NONE,
           topo: Rugged::SORT_TOPO,
-          date: Rugged::SORT_DATE
+          date: Rugged::SORT_DATE | Rugged::SORT_TOPO
         }
 
-        @rugged_sort_types.fetch(key, Rugged::SORT_NONE)
+        @rugged_sort_types.fetch(sort_type, Rugged::SORT_NONE)
       end
     end
   end

spec/lib/gitlab/git/repository_spec.rb

@@ -1044,7 +1044,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
       end
 
       it "allows ordering by date" do
-        expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_DATE)
+        expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_DATE | Rugged::SORT_TOPO)
 
         repository.find_commits(order: :date)
       end

spec/models/network/graph_spec.rb

@@ -10,17 +10,17 @@ describe Network::Graph, models: true do
     expect(graph.notes).to eq( { note_on_commit.commit_id => 1 } )
   end
 
-  describe "#commits" do
+  describe '#commits' do
     let(:graph) { described_class.new(project, 'refs/heads/master', project.repository.commit, nil) }
 
-    it "returns a list of commits" do
+    it 'returns a list of commits' do
       commits = graph.commits
 
       expect(commits).not_to be_empty
       expect(commits).to all( be_kind_of(Network::Commit) )
     end
 
-    it "sorts the commits by commit date (descending)" do
+    it 'it the commits by commit date (descending)' do
       # Remove duplicate timestamps because they make it harder to
       # assert that the commits are sorted as expected.
       commits = graph.commits.uniq(&:date)
@@ -29,5 +29,20 @@ describe Network::Graph, models: true do
       expect(commits).not_to be_empty
       expect(commits.map(&:id)).to eq(sorted_commits.map(&:id))
     end
+
+    it 'sorts children before parents for commits with the same timestamp' do
+      commits_by_time = graph.commits.group_by(&:date)
+
+      commits_by_time.each do |time, commits|
+        commit_ids = commits.map(&:id)
+
+        commits.each_with_index do |commit, index|
+          parent_indexes = commit.parent_ids.map { |parent_id| commit_ids.find_index(parent_id) }.compact
+
+          # All parents of the current commit should appear after it
+          expect(parent_indexes).to all( be > index )
+        end
+      end
+    end
   end
 end