From a7e67604b3c64a4a9e0cea6e0f9b1fa85d1c30af Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 26 Apr 2017 09:23:22 +0000
Subject: [PATCH] Fix ordering of commits in the network graph.

- We upgraded `rugged` to 0.25.1.1 in !10286 for %9.1

- Prior to this upgrade, the default sort order for commits returned by
  `Gitlab::Git::Repository#find_commits` was `Rugged::SORT_DATE`, which the
  graph relied on.

- While upgrading `rugged`, the MR also changed this default to
  `Rugged::SORT_NONE`, which broke commit ordering in the graph.

- This commit adds an option to `Gitlab::Git::Repository#find_commits` to sort
  by date, and changes the graph builder `Network::Graph` so it explictly
  requests the `:date` sort order
---
 app/models/network/graph.rb            |  3 ++-
 lib/gitlab/git/repository.rb           | 23 ++++++++++++++------
 spec/lib/gitlab/git/repository_spec.rb | 29 ++++++++++++++++++++++++++
 spec/models/network/graph_spec.rb      | 21 +++++++++++++++++++
 4 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/app/models/network/graph.rb b/app/models/network/graph.rb
index 0bbc9451ffd..59737bb6085 100644
--- a/app/models/network/graph.rb
+++ b/app/models/network/graph.rb
@@ -107,7 +107,8 @@ module Network
     def find_commits(skip = 0)
       opts = {
         max_count: self.class.max_count,
-        skip: skip
+        skip: skip,
+        order: :date
       }
 
       opts[:ref] = @commit.id if @filter_ref
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index d7dac9f6149..452dba7971d 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -494,7 +494,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 :date(default) or :topo
+      #     :order is the commits order and allowed value is :none (default), :date, or :topo
+      #        commit ordering types are documented here:
+      #        http://www.rubydoc.info/github/libgit2/rugged/Rugged#SORT_NONE-constant)
       #
       def find_commits(options = {})
         actual_options = options.dup
@@ -522,11 +524,8 @@ module Gitlab
           end
         end
 
-        if actual_options[:order] == :topo
-          walker.sorting(Rugged::SORT_TOPO)
-        else
-          walker.sorting(Rugged::SORT_NONE)
-        end
+        sort_type = rugged_sort_type(actual_options[:order])
+        walker.sorting(sort_type)
 
         commits = []
         offset = actual_options[:skip]
@@ -1273,6 +1272,18 @@ module Gitlab
       def gitaly_ref_client
         @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)
+        @rugged_sort_types ||= {
+          none: Rugged::SORT_NONE,
+          topo: Rugged::SORT_TOPO,
+          date: Rugged::SORT_DATE
+        }
+
+        @rugged_sort_types.fetch(key, Rugged::SORT_NONE)
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 3d6d7292b42..f88653cb1fe 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1031,6 +1031,35 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
   end
 
+  describe '#find_commits' do
+    it 'should return a return a collection of commits' do
+      commits = repository.find_commits
+
+      expect(commits).not_to be_empty
+      expect(commits).to all( be_a_kind_of(Gitlab::Git::Commit) )
+    end
+
+    context 'while applying a sort order based on the `order` option' do
+      it "allows ordering topologically (no parents shown before their children)" do
+        expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_TOPO)
+
+        repository.find_commits(order: :topo)
+      end
+
+      it "allows ordering by date" do
+        expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_DATE)
+
+        repository.find_commits(order: :date)
+      end
+
+      it "applies no sorting by default" do
+        expect_any_instance_of(Rugged::Walker).to receive(:sorting).with(Rugged::SORT_NONE)
+
+        repository.find_commits
+      end
+    end
+  end
+
   describe '#branches with deleted branch' do
     before(:each) do
       ref = double()
diff --git a/spec/models/network/graph_spec.rb b/spec/models/network/graph_spec.rb
index 492c4e01bd8..46b36e11c23 100644
--- a/spec/models/network/graph_spec.rb
+++ b/spec/models/network/graph_spec.rb
@@ -9,4 +9,25 @@ describe Network::Graph, models: true do
 
     expect(graph.notes).to eq( { note_on_commit.commit_id => 1 } )
   end
+
+  describe "#commits" do
+    let(:graph) { described_class.new(project, 'refs/heads/master', project.repository.commit, nil) }
+
+    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
+      # Remove duplicate timestamps because they make it harder to
+      # assert that the commits are sorted as expected.
+      commits = graph.commits.uniq(&:date)
+      sorted_commits = commits.sort_by(&:date).reverse
+
+      expect(commits).not_to be_empty
+      expect(commits.map(&:id)).to eq(sorted_commits.map(&:id))
+    end
+  end
 end