Merge branch '26552-sort-compare-commit' into 'master'

Sort commit/compare diff files directory first

See merge request gitlab-org/gitlab!49136

app/models/merge_request_diff.rb

@@ -745,31 +745,7 @@ class MergeRequestDiff < ApplicationRecord
   def sort_diffs(diffs)
     return diffs unless sort_diffs?
 
-    diffs.sort do |a, b|
-      compare_path_parts(path_parts(a), path_parts(b))
-    end
-  end
-
-  def path_parts(diff)
-    (diff.new_path.presence || diff.old_path).split(::File::SEPARATOR)
-  end
-
-  # Used for sorting the file paths by:
-  # 1. Directory name
-  # 2. Depth
-  # 3. File name
-  def compare_path_parts(a_parts, b_parts)
-    a_part = a_parts.shift
-    b_part = b_parts.shift
-
-    return 1 if a_parts.size < b_parts.size && a_parts.empty?
-    return -1 if a_parts.size > b_parts.size && b_parts.empty?
-
-    comparison = a_part <=> b_part
-
-    return comparison unless comparison == 0
-
-    compare_path_parts(a_parts, b_parts)
+    Gitlab::Diff::FileCollectionSorter.new(diffs).sort
   end
 
   def sort_diffs?

app/serializers/diffs_metadata_entity.rb

@@ -2,7 +2,9 @@
 
 class DiffsMetadataEntity < DiffsEntity
   unexpose :diff_files
-  expose :raw_diff_files, as: :diff_files, using: DiffFileMetadataEntity
+  expose :diff_files, using: DiffFileMetadataEntity do |diffs, _|
+    diffs.raw_diff_files(sorted: true)
+  end
 
   expose :conflict_resolution_path do |_, options|
     presenter(options[:merge_request]).conflict_resolution_path

app/serializers/paginated_diff_entity.rb

@@ -13,7 +13,7 @@ class PaginatedDiffEntity < Grape::Entity
     submodule_links = Gitlab::SubmoduleLinks.new(merge_request.project.repository)
 
     DiffFileEntity.represent(
-      diffs.diff_files,
+      diffs.diff_files(sorted: true),
       options.merge(
         submodule_links: submodule_links,
         code_navigation_path: code_navigation_path(diffs),

changelogs/unreleased/26552-sort-compare-commit.yml

+---
+title: Sort commit/compare diff files directory first
+merge_request: 49136
+author:
+type: changed

lib/gitlab/diff/file_collection/base.rb

@@ -30,12 +30,16 @@ module Gitlab
           @diffs ||= diffable.raw_diffs(diff_options)
         end
 
-        def diff_files
-          raw_diff_files
+        def diff_files(sorted: false)
+          raw_diff_files(sorted: sorted)
         end
 
-        def raw_diff_files
-          @raw_diff_files ||= diffs.decorate! { |diff| decorate_diff!(diff) }
+        def raw_diff_files(sorted: false)
+          strong_memoize(:"raw_diff_files_#{sorted}") do
+            collection = diffs.decorate! { |diff| decorate_diff!(diff) }
+            collection = sort_diffs(collection) if sorted
+            collection
+          end
         end
 
         def diff_file_paths
@@ -111,6 +115,12 @@ module Gitlab
                                  fallback_diff_refs: fallback_diff_refs,
                                  stats: stats)
         end
+
+        def sort_diffs(diffs)
+          return diffs unless Feature.enabled?(:sort_diffs, project, default_enabled: false)
+
+          Gitlab::Diff::FileCollectionSorter.new(diffs).sort
+        end
       end
     end
   end

lib/gitlab/diff/file_collection/merge_request_diff_base.rb

@@ -16,7 +16,7 @@ module Gitlab
             fallback_diff_refs: merge_request_diff.fallback_diff_refs)
         end
 
-        def diff_files
+        def diff_files(sorted: false)
           strong_memoize(:diff_files) do
             diff_files = super
 
@@ -26,6 +26,12 @@ module Gitlab
           end
         end
 
+        def raw_diff_files(sorted: false)
+          # We force `sorted` to `false` as we don't need to sort the diffs when
+          # dealing with `MergeRequestDiff` since we sort its files on create.
+          super(sorted: false)
+        end
+
         override :write_cache
         def write_cache
           highlight_cache.write_if_empty

lib/gitlab/diff/file_collection_sorter.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Diff
+    class FileCollectionSorter
+      attr_reader :diffs
+
+      def initialize(diffs)
+        @diffs = diffs
+      end
+
+      def sort
+        diffs.sort do |a, b|
+          compare_path_parts(path_parts(a), path_parts(b))
+        end
+      end
+
+      private
+
+      def path_parts(diff)
+        (diff.new_path.presence || diff.old_path).split(::File::SEPARATOR)
+      end
+
+      # Used for sorting the file paths by:
+      # 1. Directory name
+      # 2. Depth
+      # 3. File name
+      def compare_path_parts(a_parts, b_parts)
+        a_part = a_parts.shift
+        b_part = b_parts.shift
+
+        return 1 if a_parts.size < b_parts.size && a_parts.empty?
+        return -1 if a_parts.size > b_parts.size && b_parts.empty?
+
+        comparison = a_part <=> b_part
+
+        return comparison unless comparison == 0
+
+        compare_path_parts(a_parts, b_parts)
+      end
+    end
+  end
+end

spec/lib/gitlab/diff/file_collection/commit_spec.rb

@@ -4,17 +4,75 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Diff::FileCollection::Commit do
   let(:project) { create(:project, :repository) }
+  let(:diffable) { project.commit }
 
-  it_behaves_like 'diff statistics' do
-    let(:collection_default_args) do
-      { diff_options: {} }
-    end
+  let(:collection_default_args) do
+    { diff_options: {} }
+  end
 
-    let(:diffable) { project.commit }
+  it_behaves_like 'diff statistics' do
     let(:stub_path) { 'bar/branch-test.txt' }
   end
 
-  it_behaves_like 'unfoldable diff' do
-    let(:diffable) { project.commit }
+  it_behaves_like 'unfoldable diff'
+
+  it_behaves_like 'sortable diff files' do
+    let(:diffable) { project.commit('913c66a') }
+
+    let(:unsorted_diff_files_paths) do
+      [
+        '.DS_Store',
+        'CHANGELOG',
+        'MAINTENANCE.md',
+        'PROCESS.md',
+        'VERSION',
+        'encoding/feature-1.txt',
+        'encoding/feature-2.txt',
+        'encoding/hotfix-1.txt',
+        'encoding/hotfix-2.txt',
+        'encoding/russian.rb',
+        'encoding/test.txt',
+        'encoding/テスト.txt',
+        'encoding/テスト.xls',
+        'files/.DS_Store',
+        'files/html/500.html',
+        'files/images/logo-black.png',
+        'files/images/logo-white.png',
+        'files/js/application.js',
+        'files/js/commit.js.coffee',
+        'files/markdown/ruby-style-guide.md',
+        'files/ruby/popen.rb',
+        'files/ruby/regex.rb',
+        'files/ruby/version_info.rb'
+      ]
+    end
+
+    let(:sorted_diff_files_paths) do
+      [
+        'encoding/feature-1.txt',
+        'encoding/feature-2.txt',
+        'encoding/hotfix-1.txt',
+        'encoding/hotfix-2.txt',
+        'encoding/russian.rb',
+        'encoding/test.txt',
+        'encoding/テスト.txt',
+        'encoding/テスト.xls',
+        'files/html/500.html',
+        'files/images/logo-black.png',
+        'files/images/logo-white.png',
+        'files/js/application.js',
+        'files/js/commit.js.coffee',
+        'files/markdown/ruby-style-guide.md',
+        'files/ruby/popen.rb',
+        'files/ruby/regex.rb',
+        'files/ruby/version_info.rb',
+        'files/.DS_Store',
+        '.DS_Store',
+        'CHANGELOG',
+        'MAINTENANCE.md',
+        'PROCESS.md',
+        'VERSION'
+      ]
+    end
   end
 end

spec/lib/gitlab/diff/file_collection/compare_spec.rb

@@ -27,4 +27,43 @@ RSpec.describe Gitlab::Diff::FileCollection::Compare do
     let(:diffable) { Compare.new(raw_compare, project) }
     let(:stub_path) { '.gitignore' }
   end
+
+  it_behaves_like 'sortable diff files' do
+    let(:diffable) { Compare.new(raw_compare, project) }
+    let(:collection_default_args) do
+      {
+        project: diffable.project,
+        diff_options: {},
+        diff_refs: diffable.diff_refs
+      }
+    end
+
+    let(:unsorted_diff_files_paths) do
+      [
+        '.DS_Store',
+        '.gitignore',
+        '.gitmodules',
+        'Gemfile.zip',
+        'files/.DS_Store',
+        'files/ruby/popen.rb',
+        'files/ruby/regex.rb',
+        'files/ruby/version_info.rb',
+        'gitlab-shell'
+      ]
+    end
+
+    let(:sorted_diff_files_paths) do
+      [
+        'files/ruby/popen.rb',
+        'files/ruby/regex.rb',
+        'files/ruby/version_info.rb',
+        'files/.DS_Store',
+        '.DS_Store',
+        '.gitignore',
+        '.gitmodules',
+        'Gemfile.zip',
+        'gitlab-shell'
+      ]
+    end
+  end
 end

spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb

@@ -144,4 +144,18 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
   it_behaves_like 'cacheable diff collection' do
     let(:cacheable_files_count) { batch_size }
   end
+
+  it_behaves_like 'unsortable diff files' do
+    let(:diffable) { merge_request.merge_request_diff }
+    let(:collection_default_args) do
+      { diff_options: {} }
+    end
+
+    subject do
+      described_class.new(merge_request.merge_request_diff,
+                          batch_page,
+                          batch_size,
+                          **collection_default_args)
+    end
+  end
 end

spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb

@@ -54,4 +54,11 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiff do
   it 'returns a valid instance of a DiffCollection' do
     expect(diff_files).to be_a(Gitlab::Git::DiffCollection)
   end
+
+  it_behaves_like 'unsortable diff files' do
+    let(:diffable) { merge_request.merge_request_diff }
+    let(:collection_default_args) do
+      { diff_options: {} }
+    end
+  end
 end

spec/lib/gitlab/diff/file_collection_sorter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Diff::FileCollectionSorter do
+  let(:diffs) do
+    [
+      double(new_path: '.dir/test', old_path: '.dir/test'),
+      double(new_path: '', old_path: '.file'),
+      double(new_path: '1-folder/A-file.ext', old_path: '1-folder/A-file.ext'),
+      double(new_path: nil, old_path: '1-folder/M-file.ext'),
+      double(new_path: '1-folder/Z-file.ext', old_path: '1-folder/Z-file.ext'),
+      double(new_path: '', old_path: '1-folder/nested/A-file.ext'),
+      double(new_path: '1-folder/nested/M-file.ext', old_path: '1-folder/nested/M-file.ext'),
+      double(new_path: nil, old_path: '1-folder/nested/Z-file.ext'),
+      double(new_path: '2-folder/A-file.ext', old_path: '2-folder/A-file.ext'),
+      double(new_path: '', old_path: '2-folder/M-file.ext'),
+      double(new_path: '2-folder/Z-file.ext', old_path: '2-folder/Z-file.ext'),
+      double(new_path: nil, old_path: '2-folder/nested/A-file.ext'),
+      double(new_path: 'A-file.ext', old_path: 'A-file.ext'),
+      double(new_path: '', old_path: 'M-file.ext'),
+      double(new_path: 'Z-file.ext', old_path: 'Z-file.ext')
+    ]
+  end
+
+  subject { described_class.new(diffs) }
+
+  describe '#sort' do
+    let(:sorted_files_paths) { subject.sort.map { |file| file.new_path.presence || file.old_path } }
+
+    it 'returns list sorted directory first' do
+      expect(sorted_files_paths).to eq([
+        '.dir/test',
+        '1-folder/nested/A-file.ext',
+        '1-folder/nested/M-file.ext',
+        '1-folder/nested/Z-file.ext',
+        '1-folder/A-file.ext',
+        '1-folder/M-file.ext',
+        '1-folder/Z-file.ext',
+        '2-folder/nested/A-file.ext',
+        '2-folder/A-file.ext',
+        '2-folder/M-file.ext',
+        '2-folder/Z-file.ext',
+        '.file',
+        'A-file.ext',
+        'M-file.ext',
+        'Z-file.ext'
+      ])
+    end
+  end
+end

spec/support/shared_examples/lib/gitlab/diff_file_collections_shared_examples.rb

@@ -143,3 +143,55 @@ RSpec.shared_examples 'cacheable diff collection' do
     end
   end
 end
+
+shared_examples_for 'sortable diff files' do
+  subject { described_class.new(diffable, **collection_default_args) }
+
+  describe '#raw_diff_files' do
+    let(:raw_diff_files_paths) do
+      subject.raw_diff_files(sorted: sorted).map { |file| file.new_path.presence || file.old_path }
+    end
+
+    context 'when sorted is false (default)' do
+      let(:sorted) { false }
+
+      it 'returns unsorted diff files' do
+        expect(raw_diff_files_paths).to eq(unsorted_diff_files_paths)
+      end
+    end
+
+    context 'when sorted is true' do
+      let(:sorted) { true }
+
+      it 'returns sorted diff files' do
+        expect(raw_diff_files_paths).to eq(sorted_diff_files_paths)
+      end
+
+      context 'when sort_diffs feature flag is disabled' do
+        before do
+          stub_feature_flags(sort_diffs: false)
+        end
+
+        it 'returns unsorted diff files' do
+          expect(raw_diff_files_paths).to eq(unsorted_diff_files_paths)
+        end
+      end
+    end
+  end
+end
+
+shared_examples_for 'unsortable diff files' do
+  subject { described_class.new(diffable, **collection_default_args) }
+
+  describe '#raw_diff_files' do
+    it 'does not call Gitlab::Diff::FileCollectionSorter even when sorted is true' do
+      # Ensure that diffable is created before expectation to ensure that we are
+      # not calling it from `FileCollectionSorter` from `#raw_diff_files`.
+      diffable
+
+      expect(Gitlab::Diff::FileCollectionSorter).not_to receive(:new)
+
+      subject.raw_diff_files(sorted: true)
+    end
+  end
+end