godoc/redirect: improve Rietveld CL heuristic

Go's CL numbers as assigned by Gerrit have started to collide with the lower numbers in the sparse set of CL numbers as returned by our old code review system (Rietveld). The old heuristic no longer works now that Gerrit CL numbers have reached 150000. Instead, include a map of the low Rietveld CL numbers where we might overlap and bump the threshold heuristic up. Updates golang/go#28836 Change-Id: Ice719ab807ce3922b885a800ac873cdbf165a8f7 Reviewed-on: https://go-review.googlesource.com/c/150057 Run-TryBot: Brad Fitzpatrick <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
oggodoc · Nov 16, 2018 · 7cca380 · 7cca380
1 parent 2165053
commit 7cca380
Show file tree

Hide file tree

Showing 3 changed files with 1,110 additions and 4 deletions.
diff --git a/redirect/redirect.go b/redirect/redirect.go
@@ -115,7 +115,7 @@ var redirects = map[string]string{
 	"/tour":  "http://tour.golang.org",
 	"/wiki":  "https://github.com/golang/go/wiki",
 
-	"/doc/articles/c_go_cgo.html": "/blog/c-go-cgo",
+	"/doc/articles/c_go_cgo.html":                    "/blog/c-go-cgo",
 	"/doc/articles/concurrency_patterns.html":        "/blog/go-concurrency-patterns-timing-out-and",
 	"/doc/articles/defer_panic_recover.html":         "/blog/defer-panic-and-recover",
 	"/doc/articles/error_handling.html":              "/blog/error-handling-and-go",
@@ -191,9 +191,13 @@ func clHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	target := ""
-	// the first CL in rietveld is about 152046, so only treat the id as
-	// a rietveld CL if it is larger than 150000.
-	if n, err := strconv.Atoi(id); err == nil && n > 150000 {
+
+	if n, err := strconv.Atoi(id); err == nil && isRietveldCL(n) {
+		// TODO: Issue 28836: if this Rietveld CL happens to
+		// also be a Gerrit CL, render a disambiguation HTML
+		// page with two links instead. We'll need to make an
+		// RPC (to maintner?) to figure that out. For now just
+		// redirect to rietveld.
 		target = "https://codereview.appspot.com/" + id
 	} else {
 		target = "https://go-review.googlesource.com/" + id

diff --git a/redirect/redirect_test.go b/redirect/redirect_test.go
@@ -62,6 +62,15 @@ func TestRedirects(t *testing.T) {
 		"/cl/1/":         {302, "https://go-review.googlesource.com/1"},
 		"/cl/267120043":  {302, "https://codereview.appspot.com/267120043"},
 		"/cl/267120043/": {302, "https://codereview.appspot.com/267120043"},
+
+		// Verify that we're using the Rietveld CL table:
+		"/cl/152046": {302, "https://codereview.appspot.com/152046"},
+		"/cl/152047": {302, "https://go-review.googlesource.com/152047"},
+		"/cl/152048": {302, "https://codereview.appspot.com/152048"},
+
+		// And verify we're using the the "bigEnoughAssumeRietveld" value:
+		"/cl/299999": {302, "https://go-review.googlesource.com/299999"},
+		"/cl/300000": {302, "https://codereview.appspot.com/300000"},
 	}
 
 	mux := http.NewServeMux()