Fix encoding error in MR diffs

3 files changed, 52 insertions, 14 deletions

diff --git a/app/models/merge_request_diff_file.rb b/app/models/merge_request_diff_file.rb
index 01ee82ae398..a532c1e6356 100644
--- a/app/models/merge_request_diff_file.rb
+++ b/app/models/merge_request_diff_file.rb
@@ -17,7 +17,7 @@ class MergeRequestDiffFile < ApplicationRecord
       if merge_request_diff&.stored_externally?
         merge_request_diff.opening_external_diff do |file|
           file.seek(external_diff_offset)
-          file.read(external_diff_size)
+          force_encode_utf8(file.read(external_diff_size))
         end
       else
         super
diff --git a/changelogs/unreleased/61841-fix-encoding-error-in-mr-diffs.yml b/changelogs/unreleased/61841-fix-encoding-error-in-mr-diffs.yml
new file mode 100644
index 00000000000..8f689d89671
--- /dev/null
+++ b/changelogs/unreleased/61841-fix-encoding-error-in-mr-diffs.yml
@@ -0,0 +1,5 @@
+---
+title: Fix encoding error in MR diffs when using external diffs
+merge_request: 32862
+author: Hiroyuki Sato
+type: fixed
diff --git a/spec/models/merge_request_diff_file_spec.rb b/spec/models/merge_request_diff_file_spec.rb
index 97b30bb8607..84f9c9d06ba 100644
--- a/spec/models/merge_request_diff_file_spec.rb
+++ b/spec/models/merge_request_diff_file_spec.rb
@@ -4,26 +4,59 @@ require 'spec_helper'
 
 describe MergeRequestDiffFile do
   describe '#diff' do
-    let(:unpacked) { 'unpacked' }
-    let(:packed) { [unpacked].pack('m0') }
+    context 'when diff is not stored' do
+      let(:unpacked) { 'unpacked' }
+      let(:packed) { [unpacked].pack('m0') }
 
-    before do
-      subject.diff = packed
-    end
-
-    context 'when the diff is marked as binary' do
       before do
-        subject.binary = true
+        subject.diff = packed
+      end
+
+      context 'when the diff is marked as binary' do
+        before do
+          subject.binary = true
+        end
+
+        it 'unpacks from base 64' do
+          expect(subject.diff).to eq(unpacked)
+        end
+      end
+
+      context 'when the diff is not marked as binary' do
+        it 'returns the raw diff' do
+          expect(subject.diff).to eq(packed)
+        end
       end
+    end
 
-      it 'unpacks from base 64' do
-        expect(subject.diff).to eq(unpacked)
+    context 'when diff is stored in DB' do
+      let(:file) { create(:merge_request).merge_request_diff.merge_request_diff_files.first }
+
+      it 'returns UTF-8 string' do
+        expect(file.diff.encoding).to eq Encoding::UTF_8
       end
     end
 
-    context 'when the diff is not marked as binary' do
-      it 'returns the raw diff' do
-        expect(subject.diff).to eq(packed)
+    context 'when diff is stored in external storage' do
+      let(:file) { create(:merge_request).merge_request_diff.merge_request_diff_files.first }
+      let(:test_dir) { 'tmp/tests/external-diffs' }
+
+      around do |example|
+        FileUtils.mkdir_p(test_dir)
+
+        begin
+          example.run
+        ensure
+          FileUtils.rm_rf(test_dir)
+        end
+      end
+
+      before do
+        stub_external_diffs_setting(enabled: true, storage_path: test_dir)
+      end
+
+      it 'returns UTF-8 string' do
+        expect(file.diff.encoding).to eq Encoding::UTF_8
       end
     end
   end