500 When viewing commit diffs if gitattributes in main branch uses utf16

Status changed to closed

Changed title: 500 When viewing commit diffs on some repositories after housekeeping (and only after) → 500 When viewing commit diffs if gitattributes in main branch uses utf16

Status changed to reopened

@ravenox Thanks so much the super detailed report! I'm sorry that this had an effect on your evaluation of the product.

@stanhu Who would be best placed to look into this one?

Added gitlab-org/gitlab-ce~~12980 gitlab-org/gitlab-ce~~155949 labels

@ravenox Indeed, thanks for identifying this problem.

I suspect this has to do with !121 (merged). /cc: @yorickpeterse

This is a gitlab_git problem, I'll move the issue there.

Moved from gitlab-org/gitlab-ce#23179

Milestone changed to %8.13

@ravenox For the time being the easiest solution is to encode the file using UTF-8 and getting rid of the UTF-16 BOM at the start of the file.

@stanhu @ravenox

Technically we can support UTF-16 with/without BOM, or basically any other encoding supported by CharlockHolmes. There are however some serious problems with this such as:

Ruby provides no way of automatically figuring out what encoding to use when reading a file. Thus we must use some kind of system where we either read the entire file or a line to determine the encoding.
Reading line by line won't work unless we first apply the encoding to the entire file. Failing to do so will result in Ruby's File#each_line method returning garbage.
Reading the entire file as a whole can have a negative memory impact depending on the size of the file. Wanting to work around this is why we read the file line by line, and I really do not want to change that.
Charlock Holmes can't detect every encoding out there, meaning some more obscure encodings won't work (meaning we're basically providing a half baked solution). To be exact, Charlock Holmes only supports 33 out of the 101 available Ruby encodings

The diff for all of this (excluding the endless piles of tests needed for this) would be:

diff --git a/lib/gitlab_git/attributes.rb b/lib/gitlab_git/attributes.rb
index d007af6..da2d346 100644
--- a/lib/gitlab_git/attributes.rb
+++ b/lib/gitlab_git/attributes.rb
@@ -97,8 +97,15 @@ module Gitlab

         return unless File.exist?(full_path)

-        File.open(full_path, 'r') do |handle|
-          handle.each_line do |line|
+        File.open(full_path, 'rb') do |handle|
+          data = handle.read
+          result = CharlockHolmes::EncodingDetector.detect(data)
+          encoding = result ? result[:ruby_encoding] : 'UTF-8'
+
+          data = data.force_encoding(encoding)
+                     .encode('UTF-8', invalid: :replace, undef: :replace, replace: '')
+
+          data.each_line do |line|
             yield line.strip
           end
         end

This however suffers from all of the problems mentioned above, and there's no way around this that I can think of.

As such I'm inclined to define the following requirement:

The files $GIT_DIR/info/attributes and .gitattributes must be encoded in UTF-8 without a BOM. Any other encoding is not supported.

This may be different from what we required (or not) in the past. I am however not eager to sacrifice performance (remember this code is called for every diff we render on a page) just to support UTF-16 and other encodings, especially when the work around is very easy: just encode the file in UTF-8.

That normal, than gitlab support only UTF8 without BOM .gitattributes, but for good user experience it will be great if this situation will be documented and IMHO gitlab should not crash in this use-case but display some informational message like "Invalid .gitattributes file, check it's encoding and syntax".

P.S. I was already solved this issue with my repos, but it will be great if other users will not waste time for detecting this problem in future.

@yorickpeterse How can we best avoid the Error 500 if there is a non-UTF8 file?

@stanhu The easiest way is to rescue any encoding related errors when parsing the file and simply treat it as an empty file in such a case.

@yorickpeterse Sounds like a good plan.

Mentioned in commit 45580062

Mentioned in merge request !129 (merged)

Status changed to closed by commit 45580062

Mentioned in commit e9cf661b

Documentation is being added in this merge request: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6826

500 When viewing commit diffs if gitattributes in main branch uses utf16

Summary

Steps to reproduce

Expected behavior

Actual behavior

Relevant logs and/or screenshots

Output of checks

Results of GitLab application Check

Results of GitLab environment info

Designs

Child items ...

Activity

Admin message

Admin message

500 When viewing commit diffs if gitattributes in main branch uses utf16

Summary

Steps to reproduce

Expected behavior

Actual behavior

Relevant logs and/or screenshots

Output of checks

Results of GitLab application Check

Results of GitLab environment info

Activity