Skip to content

Fix empty file download #1506

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 1 commit into from
Apr 20, 2017
Merged

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Apr 17, 2017

No description provided.

@lunny lunny added this to the 1.2.0 milestone Apr 18, 2017
@lunny lunny added the type/bug label Apr 18, 2017
@lunny
Copy link
Member

lunny commented Apr 18, 2017

So this will let you download an empty file?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 18, 2017
@lafriks
Copy link
Member Author

lafriks commented Apr 18, 2017

This will return corectly empty file when viewing in Raw mode. Currently for empty files it did return 1024 zero byte file

@strk
Copy link
Member

strk commented Apr 18, 2017

It looks to me that better error handling should be performed for the value of n.
What happens, for example, if n is negative ? Is that possible at all ?
Anyway, the change LGTM, although I'd be much much happier if it came with unit tests for various combinations of download files (including unreadable files, for checking error handling)

@strk
Copy link
Member

strk commented Apr 18, 2017

Another problem with that code (current as well as proposed): it looks like it would truncate files bigger than 1024 bytes before sending them to client. Also, reading all files in memory before sending them to http socket doesn't seem to be the best way to do this. Just in case you feel like further improving this contribution @lafriks

@tboerger tboerger 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 Apr 18, 2017
@lafriks
Copy link
Member Author

lafriks commented Apr 19, 2017

No it reads in memory only first 1024 bytes to detect file type (text/image/pdf/other) later in code it streams remaining bytes directly from reader

@lunny
Copy link
Member

lunny commented Apr 20, 2017

LGTM

@lunny
Copy link
Member

lunny commented Apr 20, 2017

let L-G-T-M work

@tboerger tboerger 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 Apr 20, 2017
@lunny lunny merged commit bb14c97 into go-gitea:master Apr 20, 2017
@lafriks lafriks deleted the fix_empty_file_download branch May 7, 2017 16:43
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

4 participants