code review 6457075: cmd/go: skipping relative paths on go test -i ./...

cmd/go: skipping relative paths on go test -i ./...

Fixes issue 3896.

[fss, 2012-08-02 23:59:41]

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

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

[r, 2012-08-03 00:01:41]

http://codereview.appspot.com/6457075/diff/7001/src/cmd/go/test.go
File src/cmd/go/test.go (right):

http://codereview.appspot.com/6457075/diff/7001/src/cmd/go/test.go#newcode280
src/cmd/go/test.go:280: if !strings.HasPrefix(path, ".") {
It's a small point but a path like ".something/foo" shouldn't be rejected here.

[fss, 2012-08-03 00:11:52]

On 2012/08/03 00:01:41, r wrote:
> http://codereview.appspot.com/6457075/diff/7001/src/cmd/go/test.go
> File src/cmd/go/test.go (right):
> 
> http://codereview.appspot.com/6457075/diff/7001/src/cmd/go/test.go#newcode280
> src/cmd/go/test.go:280: if !strings.HasPrefix(path, ".") {
> It's a small point but a path like ".something/foo" shouldn't be rejected
here.

Hmm, I see. May I check these cases below?

"." and ".." (exact values)
"./" and "../" (prefixes)

Do you see any other restriction?

[r, 2012-08-03 00:12:55]

To do what the CL claims it's doing, yes.

-rob

[r, 2012-08-03 00:13:12]

By "yes" I mean, that's all you need to check.

-rob

[fss, 2012-08-03 00:24:16]

Please take another look.

I was not sure if I should break that condition in multiple lines.

http://codereview.appspot.com/6457075/diff/7001/src/cmd/go/test.go
File src/cmd/go/test.go (right):

http://codereview.appspot.com/6457075/diff/7001/src/cmd/go/test.go#newcode280
src/cmd/go/test.go:280: if !strings.HasPrefix(path, ".") {
On 2012/08/03 00:01:41, r wrote:
> It's a small point but a path like ".something/foo" shouldn't be rejected
here.

Done.

[r, 2012-08-03 04:11:34]

http://codereview.appspot.com/6457075/diff/11001/src/cmd/go/test.go
File src/cmd/go/test.go (right):

http://codereview.appspot.com/6457075/diff/11001/src/cmd/go/test.go#newcode280
src/cmd/go/test.go:280: if path == "." || path == ".." ||
strings.HasPrefix(path, "./") || strings.HasPrefix(path, "../") {
make this test a function. it's ugly like this. i'd call it isRelativeImport.
i'm a little surprised path/filepath doesn't already have something to do this
and maybe it should, but not now. making it a separate function here will make
it easier to catch later.

if isRelativeImport(path) {
  all = append(all, path)
}

[fss, 2012-08-03 12:26:01]

PTAL.

About path/filepath having a IsRelativePath, I don't think this function would
solve the problem. `launchpad.net/gocheck`, for example, is a relative path in
the file system, but an absolute import path in Go.

http://codereview.appspot.com/6457075/diff/11001/src/cmd/go/test.go
File src/cmd/go/test.go (right):

http://codereview.appspot.com/6457075/diff/11001/src/cmd/go/test.go#newcode280
src/cmd/go/test.go:280: if path == "." || path == ".." ||
strings.HasPrefix(path, "./") || strings.HasPrefix(path, "../") {
On 2012/08/03 04:11:35, r wrote:
> make this test a function. it's ugly like this. i'd call it isRelativeImport.
> i'm a little surprised path/filepath doesn't already have something to do this
> and maybe it should, but not now. making it a separate function here will make
> it easier to catch later.
> 
> if isRelativeImport(path) {
>   all = append(all, path)
> }

Done.

[rsc, 2012-08-03 19:47:22]

http://codereview.appspot.com/6457075/diff/3002/src/cmd/go/test.go
File src/cmd/go/test.go (right):

http://codereview.appspot.com/6457075/diff/3002/src/cmd/go/test.go#newcode206
src/cmd/go/test.go:206: func isRelativeImport(path string) bool {
Can delete this and use build.IsLocalImport.

[fss, 2012-08-04 02:31:21]

PTAL

http://codereview.appspot.com/6457075/diff/3002/src/cmd/go/test.go
File src/cmd/go/test.go (right):

http://codereview.appspot.com/6457075/diff/3002/src/cmd/go/test.go#newcode206
src/cmd/go/test.go:206: func isRelativeImport(path string) bool {
On 2012/08/03 19:47:22, rsc wrote:
> Can delete this and use build.IsLocalImport.

Done.

[fss, 2012-08-06 21:05:22]

ping

[fss, 2012-08-14 16:36:27]

ping?

[r, 2012-08-15 14:25:56]

LGTM

[r, 2012-08-15 14:33:04]

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

cmd/go: skipping relative paths on go test -i ./...

Fixes issue 3896.

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

Committer: Rob Pike <r@golang.org>