Sort commit/compare diff files directory first

When viewing merge request changes while ignoring whitespace
changes or viewing changes for a specific commit in a merge
request, they need to be sorted by directory first.

This is to make diffs and each file browser entries consistent
to prevent confusion.

This uses the same logic we use for sorting merge request diff
files so that was extracted into its own class.

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