Skip to content

Conversation

@nvier
Copy link
Contributor

@nvier nvier commented Apr 1, 2012

Force diff line to UTF-8 encoding to prevent 'incompatible character encodings' error while rendering text file diff in compare view.

…encodings' error while rendering text file diff in compare view.
@dzaporozhets
Copy link
Contributor

@nvier
Copy link
Contributor Author

nvier commented Apr 3, 2012

Sounds legit!

But, there is already forcing an encoding on Line 61 for full_line variable.

  full_line = html_escape(line.gsub(/\n/, '')).force_encoding("UTF-8")

Shouldn't be Line 70 then:

    - yield(line, type, nil, nil, nil)
    + yield(full_line, type, nil, nil, nil)

From the other side GitLab's part of code is not the only one fails on messing with encodings, I've been patching Grit as well and described the patch here.

@dzaporozhets
Copy link
Contributor

@denisnovikov

- yield(line, type, nil, nil, nil)
+ yield(full_line, type, nil, nil, nil)

i think you are right

@nvier
Copy link
Contributor Author

nvier commented Apr 3, 2012

Alright, let me test it on known-not-to-render commits and I will re-create pull request.

@dzaporozhets
Copy link
Contributor

ok

@nvier
Copy link
Contributor Author

nvier commented Apr 3, 2012

So I successfully made GitLab not throwing errors regarding incompatible character encodings with the following commit and pull request.

However applying only this patch doesn't fix completely the issue I faced with Grit messing encodings. Now for my "bad" commit I have:

  1. Commit view completely empty (which means Grit fail on Commit.diffs method).
  2. Compare view between offending commit with previous works like a charm.

So the question now is: should GitLab request data from Grit for each separate commit as it does in compare view or should we escalate this issue to Grit maintainer(s)?

For now I'm sticking with patched Grit, but would like for sure to hear thoughts.

@nvier nvier closed this Apr 3, 2012
@brodock
Copy link
Member

brodock commented May 5, 2012

The solution commented here: #725 - https://github.com/gitlabhq/gitlabhq/issues/725#issuecomment-5519854 may be a better way to solve it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants