Skip to content

Don't convert buffer to string #7115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 22, 2019
Merged

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 3, 2019

should fix #6236

@codecov-io
Copy link

codecov-io commented Jun 3, 2019

Codecov Report

Merging #7115 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7115      +/-   ##
==========================================
- Coverage   41.46%   41.46%   -0.01%     
==========================================
  Files         478      478              
  Lines       63966    63966              
==========================================
- Hits        26526    26525       -1     
- Misses      33984    33986       +2     
+ Partials     3456     3455       -1
Impacted Files Coverage Δ
models/issue_comment.go 47.56% <0%> (ø) ⬆️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
models/repo_list.go 72.08% <0%> (-1.02%) ⬇️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09ca391...c2fb205. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 3, 2019
@zeripath
Copy link
Contributor

zeripath commented Jun 3, 2019

I'm not certain that this is enough.

We're still going to be casting these bytes as a string to store them in the database which will cause the same problem. I think Patch needs to be a []byte not string.

however, how do you display a []byte - well most often this is by casting to string or attempting to decode through detection. The trouble is detecting the encoding from such a small sample is going to be very difficult - you really need the rest of the file as context, and it's perfectly reasonable for a file to change encoding.

As per usual file encoding is difficult and complex.

The most complete answer probably involves:

  • Storing the bytes only.
  • Sending the bytes to the client - and or best guess re-encoding.
  • Telling the client the detected encodings of the two sides of the diff
  • Allowing the client to override this by requesting the actual bytes and then they re-encode.
  • Displaying broken characters in some obvious way - without blowing up.

@lunny
Copy link
Member Author

lunny commented Jun 3, 2019

We could detect the encoding of this file but not the patch and convert it to UTF8 before store it in database.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 7, 2019
@lafriks
Copy link
Member

lafriks commented Jun 7, 2019

Converting patch to utf-8 would be best option and would solve issue on storing into database

@stale
Copy link

stale bot commented Aug 7, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Aug 7, 2019
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Aug 7, 2019
@stale stale bot removed the issue/stale label Aug 7, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 7, 2019
@lunny lunny merged commit 111d31d into go-gitea:master Aug 22, 2019
@lunny lunny deleted the lunny/improve_code branch August 22, 2019 04:52
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal server error when adding review comment on file with ascii special characters
6 participants