code review 4873057: goinstall: select the tag that is closest to runtime.Version

goinstall: select the tag that is closest to runtime.Version

release.r50 looks for newest tag <= go.r50
weekly.2010-10-10 looks for newest tag <= go.2010-10-10

Implements behavior for hg, git, and bzr.

[adg, 2011-08-18 21:24:23]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[adg, 2011-08-22 01:33:18]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-08-22 01:34:57]

release.r50 looks for newest tag/branch <= go.r50
weekly.2010-10-10 looks for newest tag/branch <= go.2010-10-10

Implements behavior for hg, git, and bzr.
Hg and bzr use tags. Git uses branches.

[dsymonds, 2011-08-22 01:36:50]

On Mon, Aug 22, 2011 at 11:34 AM, Andrew Gerrand <adg@golang.org> wrote:

> release.r50 looks for newest tag/branch <= go.r50
> weekly.2010-10-10 looks for newest tag/branch <= go.2010-10-10
>
> Implements behavior for hg, git, and bzr.
> Hg and bzr use tags. Git uses branches.

Why does git use branches for this? People might have a "weekly"
branch, but they wouldn't have a "go.2010-10-10" branch.

[adg, 2011-08-22 01:44:57]

On 22 August 2011 11:36, David Symonds <dsymonds@golang.org> wrote:
> On Mon, Aug 22, 2011 at 11:34 AM, Andrew Gerrand <adg@golang.org> wrote:
>
>> release.r50 looks for newest tag/branch <= go.r50
>> weekly.2010-10-10 looks for newest tag/branch <= go.2010-10-10
>>
>> Implements behavior for hg, git, and bzr.
>> Hg and bzr use tags. Git uses branches.
>
> Why does git use branches for this? People might have a "weekly"
> branch, but they wouldn't have a "go.2010-10-10" branch.

Fixed as per our discussion.

[dsymonds, 2011-08-22 02:49:47]

The selection logic feels like it needs a test. Can you extract it from that
function and test it?

http://codereview.appspot.com/4873057/diff/12001/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/12001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:291: printf("selecting revision %q\n", pkgTag)
old debugging cruft?

[adg, 2011-08-22 04:36:58]

Added a test.

http://codereview.appspot.com/4873057/diff/12001/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/12001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:291: printf("selecting revision %q\n", pkgTag)
On 2011/08/22 02:49:47, dsymonds wrote:
> old debugging cruft?

Nope, this is verbose logging.

[dsymonds, 2011-08-22 04:43:04]

LGTM

http://codereview.appspot.com/4873057/diff/12003/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/12003/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:274: goVersion = strings.Split(goVersion, " ")[0]
I think you should do this up where you call runtime.Version(). Keep the
knowledge about the particular format of runtime.Version together.

[rsc, 2011-08-23 16:28:28]

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:248: tags := tagListRe.FindAllString(string(b),
-1)
If tags prints

bongo.2011-01-01

then this re is going to pull out 'go.2011-01-01'.
That's probably not a good idea.  The output
should be split to produce the actual tag list,
and then you can filter based on
strings.HasPrefix(tag, "go.") or just pass them all
to selectTag and let it deal.

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:256: return run(dst, nil, v.cmd, v.checkout,
v.updateRevFlag+tag)
This should be v.updateRevFlag, tag.
Adding flag + value is not always reliable.
(It fails when value is empty.)
Tag isn't empty here but if you drop the +
you don't even have to think about it.

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:274: goPrefix := goPrefixRe.FindString(goVersion)
See Rob's blog post.  :-)

if strings.HasPrefix(goVersion, "release.") {
   ... 
}
if strings.HasPrefix(goVersion, "weekly.") {
   ...
}

They're different cases anyway.  You can do weekly
with string comparison, but you can't do that for release,
or else release.r100 is going to break.

[niemeyer, 2011-08-24 00:25:27]

Thanks a lot for pushing this Andrew.

Some minor points:

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:49: tagList       string
Nice cleanups.

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:246: return err
Might be good to capture Stderr and display it if non-empty and err != nil. It
will certainly have useful hints about what failed.

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:280: goVersion = goVersion[len(goPrefix):]
As a minor suggestion, there's a Go prefix and a Go version on both sides of the
comparison, and the non-goPrefix one has a prefix of "go.".  Might be slightly
easier to catch up with the logic in a first glance by using "runVersion" and
"runPrefix" or similar.

[adg, 2011-08-24 05:53:24]

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:246: return err
On 2011/08/24 00:25:27, niemeyer wrote:
> Might be good to capture Stderr and display it if non-empty and err != nil. It
> will certainly have useful hints about what failed.

Done.

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:248: tags := tagListRe.FindAllString(string(b),
-1)
On 2011/08/23 16:28:28, rsc wrote:
> If tags prints
> 
> bongo.2011-01-01
> 
> then this re is going to pull out 'go.2011-01-01'.
> That's probably not a good idea.  The output
> should be split to produce the actual tag list,
> and then you can filter based on
> strings.HasPrefix(tag, "go.") or just pass them all
> to selectTag and let it deal.
> 

Done.

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:256: return run(dst, nil, v.cmd, v.checkout,
v.updateRevFlag+tag)
On 2011/08/23 16:28:28, rsc wrote:
> This should be v.updateRevFlag, tag.
> Adding flag + value is not always reliable.
> (It fails when value is empty.)
> Tag isn't empty here but if you drop the +
> you don't even have to think about it.

I did this because in most cases (except bzr) the flag is empty. The rev is
never empty.

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:274: goPrefix := goPrefixRe.FindString(goVersion)
On 2011/08/23 16:28:28, rsc wrote:
> See Rob's blog post.  :-)
> 
> if strings.HasPrefix(goVersion, "release.") {
>    ... 
> }
> if strings.HasPrefix(goVersion, "weekly.") {
>    ...
> }
> 
> They're different cases anyway.  You can do weekly
> with string comparison, but you can't do that for release,
> or else release.r100 is going to break.

Done.

http://codereview.appspot.com/4873057/diff/15002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:280: goVersion = goVersion[len(goPrefix):]
I changed a lot of variable names.

[adg, 2011-08-24 06:34:31]

Hello dsymonds@golang.org, rsc@golang.org, n13m3y3r@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-08-24 06:35:00]

Added documentation.

[dsymonds, 2011-08-26 05:30:09]

LGTM

I haven't studied it closely, so wait for someone else's LGTM too.

http://codereview.appspot.com/4873057/diff/11002/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/11002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:278: if strings.HasPrefix(goVersion, "release.r")
{
pull this, and the weekly prefix out into constants.

http://codereview.appspot.com/4873057/diff/11002/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:298: if strings.HasPrefix(goVersion, "weekly.20")
{
I'd probably lean towards just "weekly.". I know we won't be using this in 89
years' time, but it feels weird to chop the date up like this.

[adg, 2011-08-26 05:38:12]

On 26 August 2011 15:30,  <dsymonds@golang.org> wrote:
> http://codereview.appspot.com/4873057/diff/11002/src/cmd/goinstall/download.go
> File src/cmd/goinstall/download.go (right):
>
>
http://codereview.appspot.com/4873057/diff/11002/src/cmd/goinstall/download.g...
> src/cmd/goinstall/download.go:278: if strings.HasPrefix(goVersion,
> "release.r") {
> pull this, and the weekly prefix out into constants.

Done.

>
http://codereview.appspot.com/4873057/diff/11002/src/cmd/goinstall/download.g...
> src/cmd/goinstall/download.go:298: if strings.HasPrefix(goVersion,
> "weekly.20") {
> I'd probably lean towards just "weekly.". I know we won't be using this
> in 89 years' time, but it feels weird to chop the date up like this.

It has to be at least "weekly.2" to distinguish between "go.2" and
"go.r". I'd prefer to leave it as-is.

[niemeyer, 2011-08-26 14:46:30]

LGTM, with only trivial suggestions.

In the future, I think we'll want to do something similar to what David proposed
in the mailing list, but in a single direction: the weekly release can safely
and correctly fall back to a stable release that is more recent than the latest
weekly tag found in the repository. Stable releases should never fall back to a
weekly.

This sounds like a good task for another CL, though.  This is a great
improvement as is already.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/doc.go
File src/cmd/goinstall/doc.go (right):

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/doc.go#new...
src/cmd/goinstall/doc.go:97: When downloading or updating, goinstall looks for a
tag named "go.X" that
s/named "go.X"/with the "go." prefix/?

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:294: if tf <= v && (tf > matchf || matchf == 0) {
tf > matchf should cover the matchf == 0 properly as well.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:299: const wPrefix = "weekly.20"
This has a y2.1k bug. ;-)

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:307: if t[len(p):] <= v && (t > match || match ==
"") {
Same thing.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/tag_test.go
File src/cmd/goinstall/tag_test.go (right):

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/tag_test.g...
src/cmd/goinstall/tag_test.go:63: {"", ""},
Nice set of tests.

[rsc, 2011-08-26 15:07:09]

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:274: // selectTag returns the closest matching tag
for a given version.
add line
// Closest means the latest one that is not after the current release.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:281: v, err :=
strconv.Atof32(goVersion[len(rPrefix):])
In general, unless there are extenuating circumstances (like memory pressure),
use float64.  There aren't any such circumstances here.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:294: if tf <= v && (tf > matchf || matchf == 0) {
if matchf < tf && tf <= v

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:299: const wPrefix = "weekly.20"
I'm uncomfortable with this heuristic.
We get to define the rules, so let's define a different rule:

go.r59 for releases
go.weekly.2011-01-01 for weeklies

It's okay if weekly tags are a little hard to spell.
This gives us much more flexibility in the future
if we want to add some other kind of go.foo.

[adg, 2011-08-28 22:59:54]

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/doc.go
File src/cmd/goinstall/doc.go (right):

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/doc.go#new...
src/cmd/goinstall/doc.go:97: When downloading or updating, goinstall looks for a
tag named "go.X" that
On 2011/08/26 14:46:30, niemeyer wrote:
> s/named "go.X"/with the "go." prefix/?

Done.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.go
File src/cmd/goinstall/download.go (right):

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:274: // selectTag returns the closest matching tag
for a given version.
On 2011/08/26 15:07:09, rsc wrote:
> add line
> // Closest means the latest one that is not after the current release.

Done.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:281: v, err :=
strconv.Atof32(goVersion[len(rPrefix):])
On 2011/08/26 15:07:09, rsc wrote:
> In general, unless there are extenuating circumstances (like memory pressure),
> use float64.  There aren't any such circumstances here.

Done.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:294: if tf <= v && (tf > matchf || matchf == 0) {
On 2011/08/26 14:46:30, niemeyer wrote:
> tf > matchf should cover the matchf == 0 properly as well.

Done.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:294: if tf <= v && (tf > matchf || matchf == 0) {
On 2011/08/26 15:07:09, rsc wrote:
> if matchf < tf && tf <= v 

Done.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:299: const wPrefix = "weekly.20"
On 2011/08/26 15:07:09, rsc wrote:
> I'm uncomfortable with this heuristic.
> We get to define the rules, so let's define a different rule:
> 
> go.r59 for releases
> go.weekly.2011-01-01 for weeklies
> 
> It's okay if weekly tags are a little hard to spell.
> This gives us much more flexibility in the future
> if we want to add some other kind of go.foo.

Done.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:299: const wPrefix = "weekly.20"
On 2011/08/26 14:46:30, niemeyer wrote:
> This has a y2.1k bug. ;-)

Fixed a per Russ' suggestion.

http://codereview.appspot.com/4873057/diff/26001/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:307: if t[len(p):] <= v && (t > match || match ==
"") {
On 2011/08/26 14:46:30, niemeyer wrote:
> Same thing.

Done.

[rsc, 2011-08-29 13:12:13]

LGTM

[adg, 2011-08-29 23:37:30]

*** Submitted as http://code.google.com/p/go/source/detail?r=db63f3a1f992 ***

goinstall: select the tag that is closest to runtime.Version

release.r50 looks for newest tag <= go.r50
weekly.2010-10-10 looks for newest tag <= go.2010-10-10

Implements behavior for hg, git, and bzr.

R=dsymonds, rsc, n13m3y3r
CC=golang-dev
http://codereview.appspot.com/4873057