net/http: improve signature of Redirect, NewRequest

In CL https://golang.org/cl/4893043 (6 years ago), a new package named
"url" was created (it is currently known as "net/url"). During that
change, some identifier name collisions were introduced, and two
parameters in net/http were renamed to "urlStr".

Since that time, Go has continued to put high emphasis on the quality
and readability of the documentation. Sometimes, that means making small
sacrifices in the implementation details of a package to ensure that
the godoc reads better, since that's what the majority of users interact
with. See https://golang.org/s/style#named-result-parameters:

> Clarity of docs is always more important than saving a line or two
> in your function.

I think the "urlStr" parameter name is suboptimal for godoc purposes,
and just "url" would be better.

During the review of https://golang.org/cl/4893043, it was also noted
by @rsc that having to rename parameters named "url" was suboptimal:

> It's unfortunate that naming the package url means
> you can't have a parameter or variable named url.

However, at the time, the name of the url package was still being
decided, and uri was an alternative name under consideration.
The reason urlStr was chosen is because it was a lesser evil
compared to naming the url package uri instead:

> Let's not get hung up on URI vs. URL, but I'd like s/uri/urlStr/ even for just
> that the "i" in "uri" looks very similar to the "l" in "url" in many fonts.

> Please let's go with urlStr instead of uri.

Now that we have the Go 1 compatibility guarantee, the name of the
net/url package is fixed. However, it's possible to improve the
signature of Redirect, NewRequest functions in net/http package
for godoc purposes by creating a package global alias to url.Parse,
and renaming urlStr parameter to url in the exported funcs. This CL
does so.

Updates #21077.

Change-Id: Ibcc10e3825863a663e6ad91b6eb47b1862a299a6
Reviewed-on: https://go-review.googlesource.com/49930
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

Author: dmitshur

src/net/http/request.go

@@ -762,7 +762,7 @@ func validMethod(method string) bool {
 // exact value (instead of -1), GetBody is populated (so 307 and 308
 // redirects can replay the body), and Body is set to NoBody if the
 // ContentLength is 0.
-func NewRequest(method, urlStr string, body io.Reader) (*Request, error) {
+func NewRequest(method, url string, body io.Reader) (*Request, error) {
 	if method == "" {
 		// We document that "" means "GET" for Request.Method, and people have
 		// relied on that from NewRequest, so keep that working.
@@ -772,7 +772,7 @@ func NewRequest(method, urlStr string, body io.Reader) (*Request, error) {
 	if !validMethod(method) {
 		return nil, fmt.Errorf("net/http: invalid method %q", method)
 	}
-	u, err := url.Parse(urlStr)
+	u, err := parseURL(url) // Just url.Parse (url is shadowed for godoc).
 	if err != nil {
 		return nil, err
 	}

src/net/http/server.go

@@ -1958,13 +1958,14 @@ func StripPrefix(prefix string, h Handler) Handler {
 	})
 }
 
-// Redirect replies to the request with a redirect to urlStr,
+// Redirect replies to the request with a redirect to url,
 // which may be a path relative to the request path.
 //
 // The provided code should be in the 3xx range and is usually
 // StatusMovedPermanently, StatusFound or StatusSeeOther.
-func Redirect(w ResponseWriter, r *Request, urlStr string, code int) {
-	if u, err := url.Parse(urlStr); err == nil {
+func Redirect(w ResponseWriter, r *Request, url string, code int) {
+	// parseURL is just url.Parse (url is shadowed for godoc).
+	if u, err := parseURL(url); err == nil {
 		// If url was relative, make absolute by
 		// combining with request path.
 		// The browser would probably do this for us,
@@ -1988,39 +1989,43 @@ func Redirect(w ResponseWriter, r *Request, urlStr string, code int) {
 			}
 
 			// no leading http://server
-			if urlStr == "" || urlStr[0] != '/' {
+			if url == "" || url[0] != '/' {
 				// make relative path absolute
 				olddir, _ := path.Split(oldpath)
-				urlStr = olddir + urlStr
+				url = olddir + url
 			}
 
 			var query string
-			if i := strings.Index(urlStr, "?"); i != -1 {
-				urlStr, query = urlStr[:i], urlStr[i:]
+			if i := strings.Index(url, "?"); i != -1 {
+				url, query = url[:i], url[i:]
 			}
 
 			// clean up but preserve trailing slash
-			trailing := strings.HasSuffix(urlStr, "/")
-			urlStr = path.Clean(urlStr)
-			if trailing && !strings.HasSuffix(urlStr, "/") {
-				urlStr += "/"
+			trailing := strings.HasSuffix(url, "/")
+			url = path.Clean(url)
+			if trailing && !strings.HasSuffix(url, "/") {
+				url += "/"
 			}
-			urlStr += query
+			url += query
 		}
 	}
 
-	w.Header().Set("Location", hexEscapeNonASCII(urlStr))
+	w.Header().Set("Location", hexEscapeNonASCII(url))
 	w.WriteHeader(code)
 
 	// RFC 2616 recommends that a short note "SHOULD" be included in the
 	// response because older user agents may not understand 301/307.
 	// Shouldn't send the response for POST or HEAD; that leaves GET.
 	if r.Method == "GET" {
-		note := "<a href=\"" + htmlEscape(urlStr) + "\">" + statusText[code] + "</a>.\n"
+		note := "<a href=\"" + htmlEscape(url) + "\">" + statusText[code] + "</a>.\n"
 		fmt.Fprintln(w, note)
 	}
 }
 
+// parseURL is just url.Parse. It exists only so that url.Parse can be called
+// in places where url is shadowed for godoc. See https://golang.org/cl/49930.
+var parseURL = url.Parse
+
 var htmlReplacer = strings.NewReplacer(
 	"&", "&amp;",
 	"<", "&lt;",