code review 4370048: gotest: handle \r\n returned by gomake on Windows (fixe...

gotest: handle \r\n returned by gomake on Windows (fixes build)

[brainman, 2011-04-08 11:22:50]

Hello golang-dev@googlegroups.com (cc: Joe Poirier),

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

[brainman, 2011-04-08 11:46:21]

Please see latest patch set. Test for empty string added.

Alex

[peterGo, 2011-04-08 13:04:56]

http://codereview.appspot.com/4370048/diff/5001/src/cmd/gotest/gotest.go
File src/cmd/gotest/gotest.go (right):

http://codereview.appspot.com/4370048/diff/5001/src/cmd/gotest/gotest.go#newc...
src/cmd/gotest/gotest.go:241: s = s[:len(s)-1]
In addition to trimming right '\n' and '\r'+'\n', aren't you also trimming right
'\r'. Is this by accident or design?

[rsc1, 2011-04-08 13:14:15]

LGTM

>
http://codereview.appspot.com/4370048/diff/5001/src/cmd/gotest/gotest.go#newc...
> src/cmd/gotest/gotest.go:241: s = s[:len(s)-1]
> In addition to trimming right '\n' and '\r'+'\n', aren't you also
> trimming right '\r'. Is this by accident or design?

Either way, LGTM.

[brainman, 2011-04-08 13:47:16]

> src/cmd/gotest/gotest.go:241: s = s[:len(s)-1]
> In addition to trimming right '\n' and '\r'+'\n', aren't you also trimming
right
> '\r'. Is this by accident or design?

It is by accident. I haven't used system that use \r as line delimiter yet. Do
you have a suggestion on how to change the code?

[brainman, 2011-04-08 13:54:05]

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

gotest: handle \r\n returned by gomake on Windows (fixes build)

R=golang-dev, peterGo, rsc1
CC=Joe Poirier, golang-dev
http://codereview.appspot.com/4370048

[rsc1, 2011-04-08 14:05:48]

> It is by accident. I haven't used system that use \r as line delimiter
> yet. Do you have a suggestion on how to change the code?

Just a happy accident that the code will work when
someone ports it to Mac OS 9.

[r, 2011-04-08 14:36:44]

http://codereview.appspot.com/4370048/diff/8001/src/cmd/gotest/gotest.go
File src/cmd/gotest/gotest.go (right):

http://codereview.appspot.com/4370048/diff/8001/src/cmd/gotest/gotest.go#newc...
src/cmd/gotest/gotest.go:246: return s
this doesn't get the error case right. need to rewrite
if strings.HasSuffix(s, "\r\n") { // Probably Windows.
  s = s[:len(s)-2]
} else if strings.HasSuffix(s, "\n") {
  s = s[:len(s)-1]
}
if len(s) == 0 {
  Fatalf(...)
}
return s