do not use go get to fetch new repositories

do not use go get to fetch new repositories

"go get" always recursively fetches dependencies,
and this feature cannot be disabled. This means that
when godeps used "go get -d" to fetch a repository,
it could fetch unwanted tip dependencies.

https://code.launchpad.net/~rogpeppe/godeps/004-no-go-get/+merge/258017

(do not edit description out of merge proposal)

[rog, 2015-05-01 11:03:05]

Please take a look.

[mattyw, 2015-05-01 11:26:58]

LGTM

some tests might be useful if you're so inclined.

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode172
godeps.go:172: // We would much prefer to just do:
I think comment would serve better as a go for the function rathen than in the
body.

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode183
godeps.go:183: // Instead, we use code abstracted from the go tool to do the
abstracted or copied?

or inspired by/ in  memory of etc

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode183
godeps.go:183: // Instead, we use code abstracted from the go tool to do the
Also, this comment can stay where it is I think: Here we use code ...

[rog, 2015-05-01 11:32:54]

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode172
godeps.go:172: // We would much prefer to just do:
On 2015/05/01 11:26:58, mattyw wrote:
> I think comment would serve better as a go for the function rathen than in the
> body.

The comment is about the implementation of the function.
Doc comments are usually about what the function does,
not how it's done, so I think this is in the right place.

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode183
godeps.go:183: // Instead, we use code abstracted from the go tool to do the
On 2015/05/01 11:26:58, mattyw wrote:
> Also, this comment can stay where it is I think: Here we use code ...

Ack but I think it's ok as it is.

[rog, 2015-05-01 11:43:14]

On 2015/05/01 11:26:58, mattyw wrote:
> LGTM
> 
> some tests might be useful if you're so inclined.

Tests would of course be great, but I can't think
of decent offline tests that don't involve me
building an entire mock git, mercurial and bzr servers.

I did at least QA it. I did this:

I created two repositories, github.com/rogpeppe/test
and github.com/rogpeppe/test2. The first was a library package
that initially depended on gopkg.in/errgo.v1 only; the second
was a main package that depended on test2 only, and had
a godeps file.

I created a new GOPATH directory, cloned github.com/rogpeppe/test2
into $GOPATH/src/github.com/rogpeppe/test2, then ran
godeps -u on it. It worked OK.

Then I pushed a new commit to github.com/rogpeppe/test that
used a new dependency (gopkg.in/mgo.v2), and followed the
above steps again. The packages were fetched without gopkg.in/mgo.v2,
which was the aim of the whole thing.

I then checked that godeps -u worked correctly on the juju repo
with a fresh checkout. It did.

If anyone can think of something else that might break it, please
give it a go.

[cmars_, 2015-05-01 17:36:23]

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode189
godeps.go:189: rootDir := filepath.Join(buildContext.GOPATH, "src",
filepath.FromSlash(root.Root))
What if $GOPATH is a list of paths?

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode200
godeps.go:200: return err
Might be nice to annotate the cause of this error, to help the user fix their
GOPATH.

[rog, 2015-05-01 19:58:07]

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode189
godeps.go:189: rootDir := filepath.Join(buildContext.GOPATH, "src",
filepath.FromSlash(root.Root))
On 2015/05/01 17:36:23, cmars_ wrote:
> What if $GOPATH is a list of paths?

Yes! This is why we have code reviews. Given the lengths the rest of this piece
of code goes to to deal with multiple GOPATH elements you might have thought I'd
remember that here... Thanks, will fix.

[rog, 2015-05-05 07:58:52]

Please take a look.

[mattyw, 2015-05-05 08:19:39]

https://codereview.appspot.com/230460044/diff/20001/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/20001/godeps.go#newcode189
godeps.go:189: rootDir := filepath.Join(buildContext.GOPATH, "src",
filepath.FromSlash(root.Root))
Isn't this code still wrong for GOPATHS with multiple entries?

[rog, 2015-05-05 08:21:57]

https://codereview.appspot.com/230460044/diff/1/godeps.go
File godeps.go (right):

https://codereview.appspot.com/230460044/diff/1/godeps.go#newcode200
godeps.go:200: return err
On 2015/05/01 17:36:23, cmars_ wrote:
> Might be nice to annotate the cause of this error, to help the user fix their
> GOPATH.

I can't currently think of a decent way of annotating the message
that won't just duplicate the info in the error message we're
getting from MkdirAll, which will include the directory
name and what it's trying to do. Suggestions?

[rog, 2015-05-05 08:25:28]

Please take a look.

[mattyw, 2015-05-05 08:44:54]

LGTM - but I didn't notice the last problem - so get a review from someone else

[martin.hilton, 2015-05-05 08:44:56]

On 2015/05/05 08:25:28, rog wrote:
> Please take a look.

LGTM

[rog, 2015-05-05 09:00:25]

*** Submitted:

do not use go get to fetch new repositories

"go get" always recursively fetches dependencies,
and this feature cannot be disabled. This means that
when godeps used "go get -d" to fetch a repository,
it could fetch unwanted tip dependencies.

R=mattyw, cmars_, martin.hilton
CC=
https://codereview.appspot.com/230460044