Skip to content

Commit

Permalink
[release-branch.go1.11] godoc/redirect: improve Rietveld CL heuristic
Browse files Browse the repository at this point in the history
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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
(cherry picked from commit e77c068)
Reviewed-on: https://go-review.googlesource.com/c/150577
  • Loading branch information
bradfitz committed Nov 21, 2018
1 parent f1c3f97 commit dab65e9
Show file tree
Hide file tree
Showing 3 changed files with 1,110 additions and 4 deletions.
12 changes: 8 additions & 4 deletions godoc/redirect/redirect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions godoc/redirect/redirect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Loading

0 comments on commit dab65e9

Please sign in to comment.