code review 87890043: undo CL 87300043 / 1dc800571456

undo CL 87300043 / 1dc800571456

This breaks "go get -d repo/path/...".

««« original CL description
cmd/go: do not miss an error if import path contains "cmd/something"

Fixes issue 7638

LGTM=rsc
R=rsc
CC=golang-codereviews
https://codereview.appspot.com/87300043
»»»

[adg, 2014-04-15 00:17:21]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2014-04-15 00:18:58]

LGTM

but re-open that old bug afterwards and tag it Go1.3 if it should be fixed
otherwise



On Mon, Apr 14, 2014 at 5:17 PM, <adg@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> undo CL 87300043 / 1dc800571456
>
> This breaks "go get -d repo/path/...".
>
> ««« original CL description
> cmd/go: do not miss an error if import path contains "cmd/something"
>
> Fixes issue 7638
>
> LGTM=rsc
> R=rsc
> CC=golang-codereviews
> https://codereview.appspot.com/87300043
> »»»
>
> Please review this at https://codereview.appspot.com/87890043/
>
> Affected files (+0, -4 lines):
>   M src/cmd/go/get.go
>
>
> Index: src/cmd/go/get.go
> ===================================================================
> --- a/src/cmd/go/get.go
> +++ b/src/cmd/go/get.go
> @@ -143,10 +143,6 @@
>  // for the package named by the argument.
>  func download(arg string, stk *importStack, getTestDeps bool) {
>         p := loadPackage(arg, stk)
> -       if p.Error != nil {
> -               errorf("%s", p.Error)
> -               return
> -       }
>
>         // There's nothing to do if this is a package in the standard
> library.
>         if p.Standard {
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[adg, 2014-04-15 00:20:11]

*** Submitted as https://code.google.com/p/go/source/detail?r=3f29d0979cce ***

undo CL 87300043 / 1dc800571456

This breaks "go get -d repo/path/...".

««« original CL description
cmd/go: do not miss an error if import path contains "cmd/something"

Fixes issue 7638

LGTM=rsc
R=rsc
CC=golang-codereviews
https://codereview.appspot.com/87300043
»»»

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/87890043

[dfc, 2014-04-15 00:26:20]

LGTM. Oops.

[gobot, 2014-04-15 01:29:51]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/7f1b3d19d9bf7524a0b99437449fdccc914ed3c8

[rsc, 2014-04-16 02:07:27]

I think this change was correct. What did 'go get -d' do?

[rsc, 2014-04-16 02:08:00]

FWIW, if you are going to roll back a change, it would be polite to CC the
reviewer and original author.

[adg, 2014-04-16 02:35:13]

On 16 April 2014 12:08, <rsc@golang.org> wrote:

> FWIW, if you are going to roll back a change, it would be polite to CC
> the reviewer and original author.


My apologies. I updated the issue, but didn't notice at the time that
neither you nor atom are cc'd on it.

https://code.google.com/p/go/issues/detail?id=7638

[rsc, 2014-04-16 02:37:16]

Thanks, and sorry for the confusion. 'hg undo' should probably prepopulate
the CC list but that's not completely trivial from the hg log. If it pulled
the metadata direct from codereview.appspot.com that could work.

[adg, 2014-04-16 02:41:21]

On 16 April 2014 12:07, <rsc@golang.org> wrote:

> I think this change was correct. What did 'go get -d' do?


The builders do
 go get -d code.google.com/p/go.blog/...
to fetch the repo.

This change caused that command to error with:


package code.google.com/p/go.blog/...: cannot find package
"code.google.com/p/go.blog/..." in any of:
	/Volumes/ramdisk/gobuilder/darwin-386-fa4c16e1888b/go/src/pkg/code.google.com/p/go.blog/...
(from $GOROOT)
	/Volumes/ramdisk/gobuilder/darwin-386-fa4c16e1888b/src/code.google.com/p/go.blog/...
(from $GOPATH)

Which would be correct for "install", "test", etc, but when using "go
get" it is ok if the repo does not exist locally.

If you insist this change is correct, then we need to actively decide
to retire "go get repo/..."

Andrew

[rsc, 2014-04-16 04:32:05]

I think the change is correct because loadPackage might return any number of
errors, and only this one error about ... should be suppressed. It's not that we
have to retire ..., just that we have to figure out which errors we really want
to display here. Maybe loadPackage should return a different error in this case,
or maybe no error, or maybe the caller should check for it. I'll take a look
after gophercon.