Go: x/tools/godoc/redirect: offer Gerrit/Rietveld CL disambiguation when needed

[bradfitz]

Go's CL numbers as assigned by Gerrit have started to collide with the lower numbers in the sparse set returned by the old Rietveld code review system we used to use.

Our https://golang.org/cl/NNNN handler should render a disambiguation page when the CL exists in both.

/cc @bcmills @dmitshur @andybons @adg

[gopherbot]

Change https://golang.org/cl/150057 mentions this issue: godoc/redirect: improve Rietveld CL heuristic

[aclements]

Is there any way we could knock out the Rietveld CL numbers from Gerrit's numbering space?

[bradfitz]

@aclements, yeah, I had also mused about that this morning with Bryan.

@andybons, could you talk to the Gerrit team about that?

The initial numbers of concern are listed here:
https://go-review.googlesource.com/c/tools/+/150057/5/godoc/redirect/rietveld.go#36

To get numbers higher than 300,000, run:

$ cd $GOROOT
$ git log 7d7c6a9..94151eb | grep "^    https://golang.org/cl/" | perl -npe 's,^\s+https://golang.org/cl/(\d+).*$,$1,;' | sort -n | uniq

[andybons]

@andybons, could you talk to the Gerrit team about that?

Will do.

[andybons]

They can change the number to whatever we want in the future. Where do we want to start at now?

[bradfitz]

The problem is the Rietveld numbers were sparse and got very large. We don't want a dozen digits in our CL numbers. That's more painful than the annoyance cost or occasional collisions.

I suppose we could look for the largest early gap.

[gopherbot]

Change https://golang.org/cl/150577 mentions this issue: [release-branch.go1.11] godoc/redirect: improve Rietveld CL heuristic

[dmitshur]

golang.org has been redeployed with the CL 150057 change applied, so short links like https://golang.org/cl/150057 are correctly pointing to Gerrit CLs now.

[dmitshur]

I believe this issue is now fully resolved by CL 150057, redeploying golang.org, and @andybons taking measures to prevent future Gerrit CL numbers from overlapping with existing Rietveld CLs. We won't need a CL disambiguation page.

Closing to reflect reality (but feel free to re-open if I'm missing something, or there is more follow-up work to be done).

[bradfitz]

and @andybons taking measures to prevent future Gerrit CL numbers from overlapping with existing Rietveld CLs. We won't need a CL disambiguation page.

I'm on that email thread and I haven't gotten any update from the Gerrit team.

Duplicates are still possible. They'll be rare, but possible.

I'd like Gerrit to mark all our old Rietveld numbers in https://go-review.googlesource.com/c/tools/+/150057/5/godoc/redirect/rietveld.go#36 as reserved so they'd never be handed out to us again.

Reopening until we do one of these: a disambiguation page, or modifying Gerrit.

[bcmills]

I'd like Rietveld to mark all the numbers in […] as reserved

Do you mean Gerrit?

[bradfitz]

Yes, fixed.

[bcmills]

Looks like we already missed the window on avoiding collisions:
https://codereview.appspot.com/152078
https://go-review.googlesource.com/c/go/+/152078

[bradfitz]

@bcmills, we can still minimize them going forward if Gerrit folk can reserve those IDs for us.

[dmitshur]

We just hit this again in https://github.com/golang/go/issues/29633 for CL 157099, which exists both in Gerrit and Rietveld. There are 837 more numbers that follow, so we're going to continue hit the issue for a while. There haven't been any new updates on the Gerrit email thread.

Our https://golang.org/cl/NNNN handler should render a disambiguation page when the CL exists in both.

We can query the Gerrit API to figure out if a Rietveld CL is also a Gerrit CL (and cache results). I'll implement this and send a CL.

[gopherbot]

Change https://golang.org/cl/157197 mentions this issue: godoc/redirect: display Gerrit/Rietveld CL disambiguation page when needed

[dmitshur]

@gopherbot Please backport to Go 1.11, so this change can be deployed to golang.org.

[gopherbot]

Backport issue(s) opened: #29645 (for 1.11).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[dmitshur]

Reopening for deploy of golang.org with the fix applied.

[dmitshur]

golang.org has been redeployed with the fix.

For example, https://golang.org/cl/157099 now displays a disambiguation page rather than immediately redirecting to Rietveld CL 157099, since a CL with that number exists in both systems.

[adg]

Nicely done.