Merge branch '30973-fix-network-graph-ordering' into 'master'

Fix ordering of commits in the network graph.

Closes #30973

See merge request !10936

5 files changed, 73 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/changelogs/unreleased/30973-fix-network-graph-ordering.yml b/changelogs/unreleased/30973-fix-network-graph-ordering.yml
new file mode 100644
index 00000000000..420ec107842
--- /dev/null
+++ b/changelogs/unreleased/30973-fix-network-graph-ordering.yml
@@ -0,0 +1,4 @@
+---
+title: Fix ordering of commits in the network graph
+merge_request: 10936
+author:
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 41ab73abb56..dd47e7166c4 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -486,7 +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 :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
@@ -514,11 +516,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]
@@ -1265,6 +1264,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 690f604db5e..02ac337ebb6 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1028,6 +1028,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