-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
http: false User-Agent in DumpRequest #2272
Labels
Comments
Accepted, but this is mostly a documentation problem more than a bug. Request.Write is about what you'd write if you were sending this request to another host as a client, not about preserving the exact bytes you got in. (hence the HTTP/1.1 version, for example) The blank host is actually required for HTTP/1.1, if I recall correctly. The User-Agent perhaps should be added by the http.Client, rather than DumpRequest. I'll think about that part. Owner changed to @bradfitz. Status changed to Accepted. |
Oh, right, this is about DumpRequest more than Request.Write. I think I've used DumpRequest once ever, but looking at the docs now, and rsc's comment #3, it's unclear what this should mean: // DumpRequest returns the wire representation of req Does that mean the incoming wire representation (what we just got in a Server Handler?) or the outgoing representation (what a client would send?) DumpRequest already takes one somewhat ugly bool parameter. It'd be really gross to add a second. Arguably the biggest feature of DumpRequest is its Request.Body saving/cloning. Otherwise you could just use Request.Write. So I assume that this function was intended to show the *outgoing* wire representation. Should we rename it to DumpClientRequest and add a DumpServerRequest? Should we keep the "body bool" parameter or make the client do that? Perhaps exposing drainBody as an *http.DumpBodySaver type? Seems like too much package noise and too much abstraction for a debug function. I'm happy to do whatever if people have opinions. |
This issue was closed by revision 6b6cb72. Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The text was updated successfully, but these errors were encountered: