code review 5343042: goinstall: support googlecode subrepos and add repo mat...

goinstall: support googlecode subrepos and add repo match tests
goinstall: don't hit network unless a checkout or update is required

[adg, 2011-11-04 09:29:50]

Hello golang-dev@googlegroups.com,

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

[rsc, 2011-11-07 16:29:49]

This looks fine but is inconsistent with the existing URLs.
I think we need to break with the past and convert from
foo.googlecode.com/hg to code.google.com/p/foo,
but we need a plan for doing that.

I'd like to understand that plan before pushing this change.
It is obviously a sweeping change.

It should not be too hard to do a gofix for the conversion
now that imports get sorted.

[rog, 2011-11-07 18:25:15]

On 7 November 2011 16:29,  <rsc@golang.org> wrote:
> This looks fine but is inconsistent with the existing URLs.
> I think we need to break with the past and convert from
> foo.googlecode.com/hg to code.google.com/p/foo,
> but we need a plan for doing that.

+1

when about to goinstall code from google code, i often
end up looking in the Makefile to remind myself
of the correct import path - something which
isn't always possible, especially with the upcoming
demise of Makefiles.

it always bothered me that there were two names for
a package in google code.

it's just a pity about that "p".

[rsc, 2011-11-07 18:37:04]

On Mon, Nov 7, 2011 at 13:25, roger peppe <rogpeppe@gmail.com> wrote:
> it's just a pity about that "p".

not entirely.  it's good to have the import path be a
url you can actually visit, and having the /p/ means
that you could goinstall personal clones using the /r/ paths.

[rog, 2011-11-07 18:41:56]

On 7 November 2011 18:37, Russ Cox <rsc@golang.org> wrote:
> On Mon, Nov 7, 2011 at 13:25, roger peppe <rogpeppe@gmail.com> wrote:
>> it's just a pity about that "p".
>
> not entirely.  it's good to have the import path be a
> url you can actually visit

i agree with that - my complaint was really that google code uses
the "p", not that goinstall would be wrong in following that convention.

> , and having the /p/ means
> that you could goinstall personal clones using the /r/ paths.

i ddn't know about /r/ paths. /r/ just seems to redirect to /p/
for me.

[rsc, 2011-11-07 18:49:50]

The links at http://code.google.com/p/go/source/clones are all r paths.

[adg, 2011-11-07 20:34:52]

Agree. I'll write something up.

Also I have been ruminating on this over the weekend. This code requires a
net connection to build- not just install- packages from google code, bit
bucket, and other aribtrary sites. That's because goinstall uses the remote
site to figure out which vcs to use. I have a plan to have goinstall check
the local repo first, if it exists. It's a bit of refactoring, but it'll be
cleaner in the end.

However, I'd like to get this change in for now. We can deal with the path
change in another cl.

On Tuesday, 8 November 2011,  <rsc@golang.org> wrote:
> This looks fine but is inconsistent with the existing URLs.
> I think we need to break with the past and convert from
> foo.googlecode.com/hg to code.google.com/p/foo,
> but we need a plan for doing that.
>
> I'd like to understand that plan before pushing this change.
> It is obviously a sweeping change.
>
> It should not be too hard to do a gofix for the conversion
> now that imports get sorted.
>
> http://codereview.appspot.com/5343042/
>

[rsc, 2011-11-07 21:53:38]

I want to understand the plan for the path change
before we commit this CL.  (The plan does not have
to be implemented in this CL, but we should agree
about what it is.)

Russ

[adg, 2011-11-17 22:54:39]

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

Please take another look.

[adg, 2011-11-17 22:56:35]

This includes a refactoring so that goinstall won't hit the network
unless a checkout or update is required.

[rsc, 2011-11-21 13:45:03]

LGTM

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

http://codereview.appspot.com/5343042/diff/15009/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:140: getRepo func(repo string) (RemoteRepo, error)
s/getR/r/
s/getG/g
s/getB/b
s/getL/l/

http://codereview.appspot.com/5343042/diff/15009/src/cmd/goinstall/download.g...
src/cmd/goinstall/download.go:176: // IsCheckedOut returns whether the repo root
inside srcDir contains a
s/returns/reports/

[adg, 2011-11-21 20:10:36]

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

goinstall: support googlecode subrepos and add repo match tests
goinstall: don't hit network unless a checkout or update is required

R=rsc, rogpeppe
CC=golang-dev
http://codereview.appspot.com/5343042