Sort merge request diff files directory first

Each file browser and the diffs we show under the "Changes"
tab need to be consistent to prevent confusion.

Whenever we create a merge request diff, we create and store
`MergeRequestDiffFile` records and we keep the `relative_order`
of each file.

In order for the new sorting order to persist, we will sort the
diff files on:

- when a `MergeRequestDiff` is created
- when we load merge request diffs

Then we persist/update the `relative_order` of each file.

app/models/merge_request_diff.rb

@@ -358,6 +358,7 @@ class MergeRequestDiff < ApplicationRecord
       if comparison
         comparison.diffs_in_batch(batch_page, batch_size, diff_options: diff_options)
       else
+        reorder_diff_files!
         diffs_in_batch_collection(batch_page, batch_size, diff_options: diff_options)
       end
     end
@@ -371,6 +372,7 @@ class MergeRequestDiff < ApplicationRecord
       if comparison
         comparison.diffs(diff_options)
       else
+        reorder_diff_files!
         diffs_collection(diff_options)
       end
     end
@@ -565,7 +567,7 @@ class MergeRequestDiff < ApplicationRecord
   end
 
   def build_merge_request_diff_files(diffs)
-    diffs.map.with_index do |diff, index|
+    sort_diffs(diffs).map.with_index do |diff, index|
       diff_hash = diff.to_hash.merge(
         binary: false,
         merge_request_diff_id: self.id,
@@ -678,6 +680,7 @@ class MergeRequestDiff < ApplicationRecord
 
         rows = build_merge_request_diff_files(diff_collection)
         create_merge_request_diff_files(rows)
+        new_attributes[:sorted] = true
         self.class.uncached { merge_request_diff_files.reset }
       end
 
@@ -719,6 +722,59 @@ class MergeRequestDiff < ApplicationRecord
       repo.keep_around(start_commit_sha, head_commit_sha, base_commit_sha)
     end
   end
+
+  def reorder_diff_files!
+    return unless sort_diffs?
+    return if sorted? || merge_request_diff_files.empty?
+
+    diff_files = sort_diffs(merge_request_diff_files)
+
+    diff_files.each_with_index do |diff_file, index|
+      diff_file.relative_order = index
+    end
+
+    transaction do
+      # The `merge_request_diff_files` table doesn't have an `id` column so
+      # we cannot use `Gitlab::Database::BulkUpdate`.
+      MergeRequestDiffFile.where(merge_request_diff_id: id).delete_all
+      MergeRequestDiffFile.bulk_insert!(diff_files)
+      update_column(:sorted, true)
+    end
+  end
+
+  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)
+  end
+
+  def sort_diffs?
+    Feature.enabled?(:sort_diffs, project, default_enabled: false)
+  end
 end
 
 MergeRequestDiff.prepend_if_ee('EE::MergeRequestDiff')

app/serializers/diff_file_base_entity.rb

@@ -118,7 +118,7 @@ class DiffFileBaseEntity < Grape::Entity
     strong_memoize(:submodule_links) do
       next unless diff_file.submodule?
 
-      options[:submodule_links].for(diff_file.blob, diff_file.content_sha, diff_file)
+      options[:submodule_links]&.for(diff_file.blob, diff_file.content_sha, diff_file)
     end
   end
 

changelogs/unreleased/26552-sort-diff-files.yml

+---
+title: Sort merge request diff files directory first
+merge_request: 49118
+author:
+type: changed

config/feature_flags/development/sort_diffs.yml

+---
+name: sort_diffs
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49118
+rollout_issue_url:
+milestone: '13.7'
+type: development
+group: group::code review
+default_enabled: false

db/migrate/20201202133606_add_sorted_to_merge_request_diffs.rb

+# frozen_string_literal: true
+
+class AddSortedToMergeRequestDiffs < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :merge_request_diffs, :sorted, :boolean, null: false, default: false
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :merge_request_diffs, :sorted
+    end
+  end
+end

db/schema_migrations/20201202133606

+83c7e30abb0c8f4e11faa648a4a509029aafa3230e64fe7b14d63f0a39df05ad
\ No newline at end of file

db/structure.sql

@@ -13787,6 +13787,7 @@ CREATE TABLE merge_request_diffs (
     external_diff_store integer DEFAULT 1,
     stored_externally boolean,
     files_count smallint,
+    sorted boolean DEFAULT false NOT NULL,
     CONSTRAINT check_93ee616ac9 CHECK ((external_diff_store IS NOT NULL))
 );
 

lib/gitlab/git/diff_collection.rb

@@ -66,6 +66,12 @@ module Gitlab
         @iterator = nil
       end
 
+      def sort(&block)
+        @array = @array.sort(&block)
+
+        self
+      end
+
       def empty?
         any? # Make sure the iterator has been exercised
         @empty

spec/features/merge_request/user_comments_on_diff_spec.rb

@@ -31,7 +31,7 @@ RSpec.describe 'User comments on a diff', :js do
             click_button('Add comment now')
           end
 
-          page.within('.diff-files-holder > div:nth-child(3)') do
+          page.within('.diff-files-holder > div:nth-child(6)') do
             expect(page).to have_content('Line is wrong')
 
             find('.js-diff-more-actions').click
@@ -53,7 +53,7 @@ RSpec.describe 'User comments on a diff', :js do
 
           wait_for_requests
 
-          page.within('.diff-files-holder > div:nth-child(2) .note-body > .note-text') do
+          page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do
             expect(page).to have_content('Line is correct')
           end
 
@@ -67,7 +67,7 @@ RSpec.describe 'User comments on a diff', :js do
           wait_for_requests
 
           # Hide the comment.
-          page.within('.diff-files-holder > div:nth-child(3)') do
+          page.within('.diff-files-holder > div:nth-child(6)') do
             find('.js-diff-more-actions').click
             click_button 'Hide comments on this file'
 
@@ -76,22 +76,22 @@ RSpec.describe 'User comments on a diff', :js do
 
           # At this moment a user should see only one comment.
           # The other one should be hidden.
-          page.within('.diff-files-holder > div:nth-child(2) .note-body > .note-text') do
+          page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do
             expect(page).to have_content('Line is correct')
           end
 
           # Show the comment.
-          page.within('.diff-files-holder > div:nth-child(3)') do
+          page.within('.diff-files-holder > div:nth-child(6)') do
             find('.js-diff-more-actions').click
             click_button 'Show comments on this file'
           end
 
           # Now both the comments should be shown.
-          page.within('.diff-files-holder > div:nth-child(3) .note-body > .note-text') do
+          page.within('.diff-files-holder > div:nth-child(6) .note-body > .note-text') do
             expect(page).to have_content('Line is wrong')
           end
 
-          page.within('.diff-files-holder > div:nth-child(2) .note-body > .note-text') do
+          page.within('.diff-files-holder > div:nth-child(5) .note-body > .note-text') do
             expect(page).to have_content('Line is correct')
           end
 
@@ -102,11 +102,11 @@ RSpec.describe 'User comments on a diff', :js do
 
           wait_for_requests
 
-          page.within('.diff-files-holder > div:nth-child(3) .parallel .note-body > .note-text') do
+          page.within('.diff-files-holder > div:nth-child(6) .parallel .note-body > .note-text') do
             expect(page).to have_content('Line is wrong')
           end
 
-          page.within('.diff-files-holder > div:nth-child(2) .parallel .note-body > .note-text') do
+          page.within('.diff-files-holder > div:nth-child(5) .parallel .note-body > .note-text') do
             expect(page).to have_content('Line is correct')
           end
         end
@@ -204,7 +204,7 @@ RSpec.describe 'User comments on a diff', :js do
         click_button('Add comment now')
       end
 
-      page.within('.diff-file:nth-of-type(5) .discussion .note') do
+      page.within('.diff-file:nth-of-type(1) .discussion .note') do
         find('.js-note-edit').click
 
         page.within('.current-note-edit-form') do
@@ -215,7 +215,7 @@ RSpec.describe 'User comments on a diff', :js do
         expect(page).not_to have_button('Save comment', disabled: true)
       end
 
-      page.within('.diff-file:nth-of-type(5) .discussion .note') do
+      page.within('.diff-file:nth-of-type(1) .discussion .note') do
         expect(page).to have_content('Typo, please fix').and have_no_content('Line is wrong')
       end
     end
@@ -234,7 +234,7 @@ RSpec.describe 'User comments on a diff', :js do
         expect(page).to have_content('1')
       end
 
-      page.within('.diff-file:nth-of-type(5) .discussion .note') do
+      page.within('.diff-file:nth-of-type(1) .discussion .note') do
         find('.more-actions').click
         find('.more-actions .dropdown-menu li', match: :first)
 

spec/features/merge_request/user_posts_diff_notes_spec.rb

@@ -74,7 +74,10 @@ RSpec.describe 'Merge request > User posts diff notes', :js do
 
     context 'with an unfolded line' do
       before do
-        find('.js-unfold', match: :first).click
+        page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
+          find('.js-unfold', match: :first).click
+        end
+
         wait_for_requests
       end
 
@@ -137,7 +140,10 @@ RSpec.describe 'Merge request > User posts diff notes', :js do
 
     context 'with an unfolded line' do
       before do
-        find('.js-unfold', match: :first).click
+        page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
+          find('.js-unfold', match: :first).click
+        end
+
         wait_for_requests
       end
 

spec/features/merge_request/user_views_diffs_file_by_file_spec.rb

@@ -23,12 +23,12 @@ RSpec.describe 'User views diffs file-by-file', :js do
   it 'shows diffs file-by-file' do
     page.within('#diffs') do
       expect(page).to have_selector('.file-holder', count: 1)
-      expect(page).to have_selector('.diff-file .file-title', text: '.DS_Store')
+      expect(page).to have_selector('.diff-file .file-title', text: 'files/ruby/popen.rb')
 
       find('.page-link.next-page-item').click
 
       expect(page).to have_selector('.file-holder', count: 1)
-      expect(page).to have_selector('.diff-file .file-title', text: '.gitignore')
+      expect(page).to have_selector('.diff-file .file-title', text: 'files/ruby/regex.rb')
     end
   end
 end

spec/features/merge_request/user_views_diffs_spec.rb

@@ -22,8 +22,8 @@ RSpec.describe 'User views diffs', :js do
     it 'unfolds diffs upwards' do
       first('.js-unfold').click
 
-      page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
-        expect(find('.text-file')).to have_content('.bundle')
+      page.within('.file-holder[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd"]') do
+        expect(find('.text-file')).to have_content('fileutils')
         expect(page).to have_selector('.new_line [data-linenumber="1"]', count: 1)
       end
     end

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

@@ -130,6 +130,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
     end
 
     let(:diffable) { merge_request.merge_request_diff }
+    let(:batch_page) { 2 }
     let(:stub_path) { '.gitignore' }
 
     subject do

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -219,6 +219,7 @@ MergeRequestDiff:
 - start_commit_sha
 - commits_count
 - files_count
+- sorted
 MergeRequestDiffCommit:
 - merge_request_diff_id
 - relative_order

spec/models/merge_request_diff_spec.rb

@@ -419,7 +419,7 @@ RSpec.describe MergeRequestDiff do
 
       context 'when persisted files available' do
         it 'returns paginated diffs' do
-          diffs = diff_with_commits.diffs_in_batch(1, 10, diff_options: {})
+          diffs = diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options)
 
           expect(diffs).to be_a(Gitlab::Diff::FileCollection::MergeRequestDiffBatch)
           expect(diffs.diff_files.size).to eq(10)
@@ -427,6 +427,150 @@ RSpec.describe MergeRequestDiff do
                                               next_page: 2,
                                               total_pages: 2)
         end
+
+        it 'sorts diff files directory first' do
+          diff_with_commits.update!(sorted: false) # Mark as unsorted so it'll re-order
+
+          expect(diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options).diff_file_paths).to eq([
+            'bar/branch-test.txt',
+            'custom-highlighting/test.gitlab-custom',
+            'encoding/iso8859.txt',
+            'files/images/wm.svg',
+            'files/js/commit.coffee',
+            'files/lfs/lfs_object.iso',
+            'files/ruby/popen.rb',
+            'files/ruby/regex.rb',
+            'files/.DS_Store',
+            'files/whitespace'
+          ])
+        end
+
+        context 'when sort_diffs feature flag is disabled' do
+          before do
+            stub_feature_flags(sort_diffs: false)
+          end
+
+          it 'does not sort diff files directory first' do
+            expect(diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options).diff_file_paths).to eq([
+              '.DS_Store',
+              '.gitattributes',
+              '.gitignore',
+              '.gitmodules',
+              'CHANGELOG',
+              'README',
+              'bar/branch-test.txt',
+              'custom-highlighting/test.gitlab-custom',
+              'encoding/iso8859.txt',
+              'files/.DS_Store'
+            ])
+          end
+        end
+      end
+    end
+
+    describe '#diffs' do
+      let(:diff_options) { {} }
+
+      shared_examples_for 'fetching full diffs' do
+        it 'returns diffs from repository comparison' do
+          expect_next_instance_of(Compare) do |comparison|
+            expect(comparison).to receive(:diffs)
+              .with(diff_options)
+              .and_call_original
+          end
+
+          diff_with_commits.diffs(diff_options)
+        end
+
+        it 'returns a Gitlab::Diff::FileCollection::Compare with full diffs' do
+          diffs = diff_with_commits.diffs(diff_options)
+
+          expect(diffs).to be_a(Gitlab::Diff::FileCollection::Compare)
+          expect(diffs.diff_files.size).to eq(20)
+        end
+      end
+
+      context 'when no persisted files available' do
+        before do
+          diff_with_commits.clean!
+        end
+
+        it_behaves_like 'fetching full diffs'
+      end
+
+      context 'when diff_options include ignore_whitespace_change' do
+        it_behaves_like 'fetching full diffs' do
+          let(:diff_options) do
+            { ignore_whitespace_change: true }
+          end
+        end
+      end
+
+      context 'when persisted files available' do
+        it 'returns diffs' do
+          diffs = diff_with_commits.diffs(diff_options)
+
+          expect(diffs).to be_a(Gitlab::Diff::FileCollection::MergeRequestDiff)
+          expect(diffs.diff_files.size).to eq(20)
+        end
+
+        it 'sorts diff files directory first' do
+          diff_with_commits.update!(sorted: false) # Mark as unsorted so it'll re-order
+
+          expect(diff_with_commits.diffs(diff_options).diff_file_paths).to eq([
+            'bar/branch-test.txt',
+            'custom-highlighting/test.gitlab-custom',
+            'encoding/iso8859.txt',
+            'files/images/wm.svg',
+            'files/js/commit.coffee',
+            'files/lfs/lfs_object.iso',
+            'files/ruby/popen.rb',
+            'files/ruby/regex.rb',
+            'files/.DS_Store',
+            'files/whitespace',
+            'foo/bar/.gitkeep',
+            'with space/README.md',
+            '.DS_Store',
+            '.gitattributes',
+            '.gitignore',
+            '.gitmodules',
+            'CHANGELOG',
+            'README',
+            'gitlab-grack',
+            'gitlab-shell'
+          ])
+        end
+
+        context 'when sort_diffs feature flag is disabled' do
+          before do
+            stub_feature_flags(sort_diffs: false)
+          end
+
+          it 'does not sort diff files directory first' do
+            expect(diff_with_commits.diffs(diff_options).diff_file_paths).to eq([
+              '.DS_Store',
+              '.gitattributes',
+              '.gitignore',
+              '.gitmodules',
+              'CHANGELOG',
+              'README',
+              'bar/branch-test.txt',
+              'custom-highlighting/test.gitlab-custom',
+              'encoding/iso8859.txt',
+              'files/.DS_Store',
+              'files/images/wm.svg',
+              'files/js/commit.coffee',
+              'files/lfs/lfs_object.iso',
+              'files/ruby/popen.rb',
+              'files/ruby/regex.rb',
+              'files/whitespace',
+              'foo/bar/.gitkeep',
+              'gitlab-grack',
+              'gitlab-shell',
+              'with space/README.md'
+            ])
+          end
+        end
       end
     end
 
@@ -505,6 +649,68 @@ RSpec.describe MergeRequestDiff do
         expect(mr_diff.empty?).to be_truthy
       end
 
+      it 'persists diff files sorted directory first' do
+        mr_diff = create(:merge_request).merge_request_diff
+        diff_files_paths = mr_diff.merge_request_diff_files.map { |file| file.new_path.presence || file.old_path }
+
+        expect(diff_files_paths).to eq([
+          'bar/branch-test.txt',
+          'custom-highlighting/test.gitlab-custom',
+          'encoding/iso8859.txt',
+          'files/images/wm.svg',
+          'files/js/commit.coffee',
+          'files/lfs/lfs_object.iso',
+          'files/ruby/popen.rb',
+          'files/ruby/regex.rb',
+          'files/.DS_Store',
+          'files/whitespace',
+          'foo/bar/.gitkeep',
+          'with space/README.md',
+          '.DS_Store',
+          '.gitattributes',
+          '.gitignore',
+          '.gitmodules',
+          'CHANGELOG',
+          'README',
+          'gitlab-grack',
+          'gitlab-shell'
+        ])
+      end
+
+      context 'when sort_diffs feature flag is disabled' do
+        before do
+          stub_feature_flags(sort_diffs: false)
+        end
+
+        it 'persists diff files unsorted by directory first' do
+          mr_diff = create(:merge_request).merge_request_diff
+          diff_files_paths = mr_diff.merge_request_diff_files.map { |file| file.new_path.presence || file.old_path }
+
+          expect(diff_files_paths).to eq([
+            '.DS_Store',
+            '.gitattributes',
+            '.gitignore',
+            '.gitmodules',
+            'CHANGELOG',
+            'README',
+            'bar/branch-test.txt',
+            'custom-highlighting/test.gitlab-custom',
+            'encoding/iso8859.txt',
+            'files/.DS_Store',
+            'files/images/wm.svg',
+            'files/js/commit.coffee',
+            'files/lfs/lfs_object.iso',
+            'files/ruby/popen.rb',
+            'files/ruby/regex.rb',
+            'files/whitespace',
+            'foo/bar/.gitkeep',
+            'gitlab-grack',
+            'gitlab-shell',
+            'with space/README.md'
+          ])
+        end
+      end
+
       it 'expands collapsed diffs before saving' do
         mr_diff = create(:merge_request, source_branch: 'expand-collapse-lines', target_branch: 'master').merge_request_diff
         diff_file = mr_diff.merge_request_diff_files.find_by(new_path: 'expand-collapse/file-5.txt')