Merge pull request #9999 from opf/fix/40191-git-view-differences-results-in-500-error-in-repository-module

Fix/40191 git view differences results in 500 error in repository module

app/controllers/repositories_controller.rb

@@ -195,26 +195,6 @@ class RepositoriesController < ApplicationController
     end
   end
 
-  def is_entry_text_data?(ent, path)
-    # UTF-16 contains "\x00".
-    # It is very strict that file contains less than 30% of ascii symbols
-    # in non Western Europe.
-    return true if OpenProject::MimeType.is_type?('text', path)
-    # Ruby 1.8.6 has a bug of integer divisions.
-    # http://apidock.com/ruby/v1_8_6_287/String/is_binary_data%3F
-    if ent.respond_to?('is_binary_data?') && ent.is_binary_data? # Ruby 1.8.x and <1.9.2
-      return false
-    elsif ent.respond_to?(:force_encoding) &&
-          (ent.dup.force_encoding('UTF-8') != ent.dup.force_encoding('BINARY')) # Ruby 1.9.2
-      # TODO: need to handle edge cases of non-binary content that isn't UTF-8
-      return false
-    end
-
-    true
-  end
-
-  private :is_entry_text_data?
-
   def annotate
     @entry = @repository.entry(@path, @rev)
 
@@ -259,14 +239,7 @@ class RepositoriesController < ApplicationController
                 type: 'text/x-patch',
                 disposition: 'attachment'
     else
-      @diff_type = params[:type] || User.current.pref[:diff_type] || 'inline'
-      @diff_type = 'inline' unless %w(inline sbs).include?(@diff_type)
-
-      # Save diff type as user preference
-      if User.current.logged? && @diff_type != User.current.pref[:diff_type]
-        User.current.pref[:diff_type] = @diff_type
-        User.current.preference.save
-      end
+      @diff_type = diff_type_persisted
 
       @cache_key = "repositories/diff/#{@repository.id}/" +
                    Digest::MD5.hexdigest("#{@path}-#{@rev}-#{@rev_to}-#{@diff_type}")
@@ -475,7 +448,7 @@ class RepositoriesController < ApplicationController
   def raw_or_to_large_or_non_text(content, path)
     params[:format] == 'raw' ||
       (content.size && content.size > Setting.file_max_size_displayed.to_i.kilobyte) ||
-      !is_entry_text_data?(content, path)
+      !entry_text_data?(content, path)
   end
 
   def send_raw(content, path)
@@ -497,6 +470,30 @@ class RepositoriesController < ApplicationController
     # rails looking for e.g text when .txt is asked for
     render 'entry', formats: [:html]
   end
+
+  def diff_type_persisted
+    preferences = current_user.pref
+
+    diff_type = params[:type] || preferences.diff_type
+    diff_type = 'inline' unless %w(inline sbs).include?(diff_type)
+
+    # Save diff type as user preference
+    if current_user.logged? && diff_type != preferences.diff_type
+      preferences.diff_type = diff_type
+      preferences.save
+    end
+
+    diff_type
+  end
+
+  def entry_text_data?(ent, path)
+    # UTF-16 contains "\x00".
+    # It is very strict that file contains less than 30% of ascii symbols
+    # in non Western Europe.
+    # TODO: need to handle edge cases of non-binary content that isn't UTF-8
+    OpenProject::MimeType.is_type?('text', path) ||
+      ent.dup.force_encoding('UTF-8') == ent.dup.force_encoding('BINARY')
+  end
 end
 
 class Date

app/models/user_preference.rb

@@ -85,6 +85,10 @@ class UserPreference < ApplicationRecord
     comments_sorting == 'desc'
   end
 
+  def diff_type
+    settings.fetch(:diff_type, 'inline')
+  end
+
   def hide_mail
     settings.fetch(:hide_mail, true)
   end

config/schemas/user_preferences.schema.json

@@ -22,6 +22,10 @@
                 "auto_hide_popups": {
                     "type": "boolean"
                 },
+                "diff_type": {
+                  "type": "string",
+                  "enum": ["inline", "sbs"]
+                },
                 "workdays": {
                   "type": "array",
                   "items": {

spec/models/user_preference_spec.rb

@@ -140,6 +140,27 @@ describe UserPreference do
     end
   end
 
+  describe '#diff_type' do
+    it 'can be set and written' do
+      expect(subject.diff_type)
+        .to eql 'inline'
+
+      subject.diff_type = 'sbs'
+
+      expect(subject.diff_type)
+        .to eql 'sbs'
+    end
+
+    context 'with a new pref instance' do
+      subject { described_class.new }
+
+      it 'defaults to `inline`' do
+        expect(subject.diff_type)
+          .to eql 'inline'
+      end
+    end
+  end
+
   describe '#daily_reminders' do
     context 'without reminders being stored' do
       it 'uses the defaults' do