Skip to content
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

WIP: Serve LFS/attachment with http.ServeContent to Support Range-Request #20480

Conversation

6543
Copy link
Member

@6543 6543 commented Jul 25, 2022

take #18448 and finish it ...

The `ServeData` function now can't handle [Range Request](https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests).
Due to the complexity of this part of HTTP protocol, it is difficult to fully implement it and requires a lot of testing.
The official module `http.ServeContent` has this implementation, and we can replace with it directly.
It can handle `If-Match, If-Unmodified-Since, If-None-Match, If-Modified-Since, If-Range` and more, perfectly!

@6543 6543 added the type/enhancement An improvement of existing functionality label Jul 25, 2022
@6543 6543 added this to the 1.18.0 milestone Jul 25, 2022
@6543 6543 added the pr/wip This PR is not ready for review label Jul 25, 2022
func (ct SniffedType) Mime() string {
return strings.Split(ct.contentType, ";")[0]
}

Copy link
Member

@silverwind silverwind Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: this is a better version than I have in #20464, will incorporate there.

Copy link
Member Author

@6543 6543 Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use SplitN so go can skip split after first ; ... just some tiny optimization nits

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@silverwind I'm going to delete the mime stuff as it's unrelated to the pull topic! - so feel free to pick it for your pull :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, for reference, code was:

func (ct SniffedType) Mime() string {
	return strings.Split(ct.contentType, ";")[0]
}

Copy link
Member

@silverwind silverwind Jul 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are talking about micro-optimization, str[:strings.Index(str, ";")] should be even faster than SplitN I think :)

Edit: Done, added it as GetMimeType in #20484.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 25, 2022
Comment on lines +75 to +79
// Mime return the mime
func (ct SniffedType) Mime() string {
return strings.Split(ct.contentType, ";")[0]
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Mime return the mime
func (ct SniffedType) Mime() string {
return strings.Split(ct.contentType, ";")[0]
}

Delete as discussed, it's unused.


func setCommonHeaders(ctx *context.Context, name string, data interface{}) error {
Copy link
Member

@silverwind silverwind Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this additional function is of any benefit. Cache header can be set like in #20484 via httpcache module.

@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022
@lunny lunny modified the milestones: 1.19.0, 1.20.0 Jan 31, 2023
@wxiaoguang
Copy link
Contributor

-> Make repository response support HTTP range request #24592

@wxiaoguang wxiaoguang closed this May 8, 2023
@GiteaBot GiteaBot removed this from the 1.20.0 milestone May 8, 2023
lunny pushed a commit that referenced this pull request May 9, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/wip This PR is not ready for review type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants