lfsapi: support for http.<url>.extraHeader #2159
Conversation
lfsapi/client.go
Outdated
| var copy http.Header = make(http.Header) | ||
| for k, vs := range req.Header { | ||
| copy[k] = vs | ||
| } |
There was a problem hiding this comment.
Why bother making a copy? Can't you just modify the req headers in place?
Also, why are you using the longer var syntax to init the var, instead of copy := make(http.Header)?
There was a problem hiding this comment.
Why bother making a copy? Can't you just modify the req headers in place?
Good question. I think it would use less memory to modify the headers in place (though not much). My thought with making the copy was to localize mutating the request to outside of this function so that this could be more easily tested/changed in the future. I'm not tied at all to this implementation, so if you feel strongly, I'm happy to change it.
I pushed d4aec73 to clarify the name of this method, and 961456e to remove the extraneous var decl. I prefer those for initializing values that are going to change immediately, but I don't mind either way.
lfsapi/client.go
Outdated
|
|
||
| func (c *Client) extraHeaders(u *url.URL) map[string][]string { | ||
| if c.uc == nil { | ||
| c.uc = config.NewURLConfig(c.GitEnv()) |
There was a problem hiding this comment.
I'd probably move this to lfsapi.NewClient, or at least add a mutex.
There was a problem hiding this comment.
Good call. I had this here originally to prevent a case during testing where the *lfsapi.Client was initialized as &lfsapi.Client{} (or new(lfsapi.Client)) and uc was un-initialized. I moved this initialization to the constructor (since the value returned by c.GitEnv() is private and doesn't mutate), and taught *config.URLConfig to respond to method calls against the nil receiver.
lfsapi/client.go
Outdated
| m := make(map[string][]string, len(hdrs)) | ||
|
|
||
| for _, hdr := range hdrs { | ||
| parts := strings.SplitN(hdr, ": ", 2) |
There was a problem hiding this comment.
Relying on just a single space after the : seems kind of fragile. What do you think about splitting on :, and then trimming any whitespace in the value?
There was a problem hiding this comment.
Great call. Made this change in 442dc02.
|
@technoweenie thanks for the review! I addressed your comments, and I think this should be ready to merge. |
| @@ -76,6 +77,37 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) { | |||
| return res, c.handleResponse(res) | |||
| } | |||
|
|
|||
| func (c *Client) extraHeadersFor(req *http.Request) http.Header { | |||
| copy := make(http.Header, len(req.Header)) | |||
There was a problem hiding this comment.
You could even get the values from c.extraHeaders() to get a more accurate initial size :)
This pull request resolves the proposal made in #1500 by implementing support for
http.<url>.extraHeader.This pull request is based off of #2158, and that should be merged before this one. In the meantime, here's an inter-diff: url-config-prefix...lfsapi-extra-headers.
When making any HTTP(S) request from the lfsapi package, LFS will first look for matching
http.{url}.extraHeaderconfig entries, whereurlis the request URL. LFS currently adds one extra header to each request it makes, which is theUser-Agent:header. Thelfsapipackage looks forextraHeadersbefore adding the User-Agent header, so it cannot be overridden.As per #2152 & #2154, multiple extraHeaders can be added to a particular request. The following configuration is valid:
would send a request with the headers equal to:
Closes: #1500.
/cc @git-lfs/core #1500