Set original file content type on basic upload. #3137
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
calavera
force-pushed
the
set_upload_content_type
branch
from
e8d13e3
to
ac8a7ef
Compare
ttaylorr
reviewed
Jul 23, 2018
tq/basic_upload.go
Outdated
| @@ -74,6 +72,25 @@ func (a *basicUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progres | |||
| } | |||
| defer f.Close() | |||
|
|
|||
| if len(req.Header.Get("Content-Type")) == 0 { | |||
There was a problem hiding this comment.
Could this be extracted into its own method? I can foresee changes like adding a configuration option to disable this, changing the default content type, and etc.
Maybe something like:
func setContentTypeFor(req *http.Request, r io.Reader) error { ... }
tq/basic_upload.go
Outdated
| buffer := make([]byte, 512) | ||
| n, err := f.Read(buffer) | ||
| if err != nil && err != io.EOF { | ||
| return errors.Wrap(err, "basic upload") |
There was a problem hiding this comment.
I think this wrap description could be more informative. Perhaps something like:
return errors.Wrap(err, "content type detect")
tq/basic_upload.go
Outdated
|
|
||
| contentType := http.DetectContentType(buffer[:n]) | ||
| if _, err := f.Seek(0, 0); err != nil { | ||
| return errors.Wrap(err, "basic upload") |
There was a problem hiding this comment.
I have the same thought about this error wrap as I do the above, maybe replacing "content type detect" with "content type rewind" instead.
This change tries to detect the content type for a file when uploading to the final store instead of always use `octet-stream`. It ignores this behavior only if the response from the LFS server set an explicit content type. Signed-off-by: David Calavera <david.calavera@gmail.com>
Signed-off-by: David Calavera <david.calavera@gmail.com>
calavera
force-pushed
the
set_upload_content_type
branch
from
ac8a7ef
to
62f6b6d
Compare
|
Thanks for the review @ttaylorr, I made those changes. |
ttaylorr
approved these changes
Jul 23, 2018
ttaylorr
added a commit
that referenced
this pull request
Jul 31, 2018
For the majority of Git LFS's lifetime, Git LFS objects have been
uploaded via HTTP and contained the header:
Content-Type: application/octet-stream
In [1], this changed and we began attempting to guess an appropriate
Content-Type header based on the first 512 bytes of the file being
uploaded.
This breaks some hosting platforms' implementation of this API, so let's
offer a way to disable this guessing to allow users to upload objects
again on affected implementations.
[1]: a173629 (Merge pull request #3137 from
calavera/set_upload_content_type, 2018-07-23)
swills
pushed a commit
to swills/gitlabhq
that referenced
this pull request
Aug 1, 2018
git-lfs 2.5.0 now sets the Content-Type header instead of hard-coding it to application/octet-stream: git-lfs/git-lfs#3137 To avoid this issue, we explicitly tell the client to use application/octet-stream. Closes #49752
jirutka
pushed a commit
to jirutka/gitlab-foss
that referenced
this pull request
Aug 17, 2018
git-lfs 2.5.0 now sets the Content-Type header instead of hard-coding it to application/octet-stream: git-lfs/git-lfs#3137 To avoid this issue, we explicitly tell the client to use application/octet-stream. Closes #49752 Upstream-Issue: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20923
jirutka
pushed a commit
to jirutka/gitlab-foss
that referenced
this pull request
Aug 17, 2018
git-lfs 2.5.0 now sets the Content-Type header instead of hard-coding it to application/octet-stream: git-lfs/git-lfs#3137 To avoid this issue, we explicitly tell the client to use application/octet-stream. Closes #49752 Upstream-Issue: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20923
rcoup
added a commit
to rcoup/giftless
that referenced
this pull request
Apr 6, 2022
git-lfs as of v2.5.0 [1] detects the file Content-Type and provides it during PUT requests to the storage when using the basic transfer. If we don't include the same `Content-Type` value in the S3 signature generation, then S3 rejects it during upload. Unfortunately git-lfs doesn't pass the `Content-Type` value to the Batch request, so it can't be included in S3 signature generation. And while setting `lfs.contenttype=false` in the client config forces sending `application/octet-stream` [2], S3 isn't happy with that either. If the Batch response upload action includes a `Content-Type` header then git-lfs will always use that for the PUT request, so always return `application/octet-stream`, and also use it for signature generation. [1] git-lfs/git-lfs#3137 [2] git-lfs/git-lfs#3163
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change tries to detect the content type for a file when uploading
to the final store instead of always use
octet-stream. It ignoresthis behavior only if the response from the LFS server set an explicit
content type.
After reading the documentation, I'm not very clear about why the
content type is always octet-stream, so let me know if I missed
something obvious.
I've used this change with a repository using GitHub LFS and everything
seems to be working as expected. But I can also add tests as needed if
you point me in the right direction.
Signed-off-by: David Calavera david.calavera@gmail.com