Code review - Issue 1699050: code review 1699050: Added Bazaar+Launchpad support in goinstallhttps://codereview.appspot.com/2010-07-01T06:33:52+00:00rietveld
Message from unknown
2010-06-26T04:21:52+00:00niemeyerurn:md5:68ecaf1961141d1702c7926ce86ff080
Message from unknown
2010-06-26T17:29:38+00:00niemeyerurn:md5:cf7fa499170756ccfa564012fde85ec3
Message from n13m3y3r@gmail.com
2010-06-26T17:29:53+00:00niemeyerurn:md5:2a47759ab0a7af46497cd7640d578a39
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
I'd like you to review this change.
Message from rsc@google.com
2010-06-28T21:17:44+00:00rsc1urn:md5:3350658f552a87929c9ea0ce0a4b1803
looks pretty good; thanks
http://codereview.appspot.com/1699050/diff/2001/3001
File src/cmd/goinstall/doc.go (right):
http://codereview.appspot.com/1699050/diff/2001/3001#newcode45
src/cmd/goinstall/doc.go:45: Launchpad
wrong indentation
http://codereview.appspot.com/1699050/diff/2001/3002
File src/cmd/goinstall/download.go (right):
http://codereview.appspot.com/1699050/diff/2001/3002#newcode37
src/cmd/goinstall/download.go:37: var launchpad = regexp.MustCompile(`^(launchpad\.net/([a-z0-9A-Z_.\-]+(/[a-z0-9A-Z_.\-]+)?|~[a-z0-9A-Z_.\-]+/(\+junk|[a-z0-9A-Z_.\-]+)/[a-z0-9A-Z_.\-]+))(/[a-z0-9A-Z_.\-/]+)?$`)
Really? +junk ?
http://codereview.appspot.com/1699050/diff/2001/3002#newcode187
src/cmd/goinstall/download.go:187: // XXX What is the expected behavior with svn here? This seems
// NOTE(n13m3y3r@gmail.com): What is the expected behavior with svn here?
// "svn log -l1 release" doesn't make sense in this context and will probably fail.
Message from n13m3y3r@gmail.com
2010-06-28T21:46:30+00:00niemeyerurn:md5:92965c6d6eaba2510f46cabc436d8b45
Thanks for the review. Changes addressed.
http://codereview.appspot.com/1699050/diff/2001/3001
File src/cmd/goinstall/doc.go (right):
http://codereview.appspot.com/1699050/diff/2001/3001#newcode45
src/cmd/goinstall/doc.go:45: Launchpad
On 2010/06/28 21:17:44, rsc1 wrote:
> wrong indentation
>
Done.
http://codereview.appspot.com/1699050/diff/2001/3002
File src/cmd/goinstall/download.go (right):
http://codereview.appspot.com/1699050/diff/2001/3002#newcode37
src/cmd/goinstall/download.go:37: var launchpad = regexp.MustCompile(`^(launchpad\.net/([a-z0-9A-Z_.\-]+(/[a-z0-9A-Z_.\-]+)?|~[a-z0-9A-Z_.\-]+/(\+junk|[a-z0-9A-Z_.\-]+)/[a-z0-9A-Z_.\-]+))(/[a-z0-9A-Z_.\-/]+)?$`)
> Really? +junk ?
Yeah, I know it sounds weird. The background of the naming is that this discourages people from using this unless it's indeed something temporary. Users can push branches such as "~myself/project/branch-name" to any project, without special authorization from the project itself, and this what's encouraged as it enhances organization.
http://codereview.appspot.com/1699050/diff/2001/3002#newcode187
src/cmd/goinstall/download.go:187: // XXX What is the expected behavior with svn here? This seems
Done. I've just used my actual email instead.
Message from unknown
2010-06-28T21:48:09+00:00niemeyerurn:md5:4dbb1987490bfb35880cdaf885d17d8b
Message from n13m3y3r@gmail.com
2010-06-28T21:48:20+00:00niemeyerurn:md5:7d5e0013e3c6d5e4e34ecc0a27a21df8
Hello rsc (cc: golang-dev@googlegroups.com),
Please take another look.
Message from rsc@google.com
2010-06-30T21:51:27+00:00rsc1urn:md5:f4240c3fd1d81bca863d67e76903c235
LGTM
Could you please complete the CLA as described at
http://golang.org/doc/contribute.html#copyright ?
Thanks.
Message from gustavo@niemeyer.net
2010-06-30T22:24:15+00:00gustavo_niemeyer.neturn:md5:434d2cf36a2c41e11bd51611ecb1bdaf
> Could you please complete the CLA as described at
> http://golang.org/doc/contribute.html#copyright ?
I've already done that before submitting the change last week. You
should have it as "gustavo@niemeyer.net", which is the email I
actually use personally. I used it to login with hg too, but I guess
it has translated it to my Gmail account which is unfortunately a bit
cryptic.
--
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter
Message from rsc@golang.org
2010-07-01T00:19:01+00:00rscurn:md5:f7a932941fc572bc6b70705098452f5b
On Wed, Jun 30, 2010 at 15:23, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
>> Could you please complete the CLA as described at
>> http://golang.org/doc/contribute.html#copyright ?
>
> I've already done that before submitting the change last week. You
> should have it as "gustavo@niemeyer.net", which is the email I
> actually use personally. I used it to login with hg too, but I guess
> it has translated it to my Gmail account which is unfortunately a bit
> cryptic.
Ah, I see it. Sorry about that.
The Mercurial plugin requires that the email address
we get from code review (the gmail one) be listed
in the CONTRIBUTORS file, because there's an
automated check that any code added to the repository
is from someone who has been okayed as a contributor.
We've had a few people in this situation before.
There are three things we could do.
2. Add you to the file with both addresses, so that
the check will find the gmail address, but with the
non-gmail address marked as the preferred one
for use in the check-in logs.
1. Cut the association between your gmail and
your non-gmail address. You'd then have to create
a separate account+password for the non-gmail
address, and use that to log in to code review.
2. Add you to the CONTRIBUTORS file using your
gmail address. Checkins will use the gmail address too.
The non-gmail address would go unused.
3. Add you to the CONTRIBUTORS file with both addresses.
The Mercurial plugin would find you via the gmail address
but there would be a notation on the line to say that checkins
should use the non-gmail address.
It really boils down to which address you want in the
Mercurial logs and how much you care about not
exposing the gmail address.
Let me know which you'd prefer.
Thanks, and sorry for the trouble.
Russ
Message from gustavo@niemeyer.net
2010-07-01T01:40:48+00:00gustavo_niemeyer.neturn:md5:5569a2f94017cfbc2a6ef66f709423e6
> 3. Add you to the CONTRIBUTORS file with both addresses.
> The Mercurial plugin would find you via the gmail address
> but there would be a notation on the line to say that checkins
> should use the non-gmail address.
This sounds like the best option. I can't cut the association with
the Gmail account because I actually use my personal email to receive
and deliver from there, so this would be the only other choice which
would keep the real email around. If that works, it certainly looks
good for me.
> It really boils down to which address you want in the
> Mercurial logs and how much you care about not
> exposing the gmail address.
>
> Let me know which you'd prefer.
>
> Thanks, and sorry for the trouble.
No problem, and thanks for your help with the check in
--
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/blog
http://niemeyer.net/twitter
Message from rsc@golang.org
2010-07-01T06:33:52+00:00rscurn:md5:0144f4af767238072491bc23813f1248
*** Submitted as http://code.google.com/p/go/source/detail?r=3db9a916a3fe ***
goinstall: support for Bazaar+Launchpad
With these changes, goinstall is now able to use branches
maintained with Bazaar located in Launchpad.
Project aliases such as /project and /project/series are
supported in addition to specific user or team branches
such as /~user/project/branch. Temporary branches under
the +junk special project are also supported.
As a curious side effect, since Launchpad is able to import
code from other locations, they can be indirectly
accessible too if desired.
R=rsc
CC=golang-dev
http://codereview.appspot.com/1699050
Committer: Russ Cox <rsc@golang.org>