code review 2068041: Initial LookPath unix/windows separation.

Initial LookPath unix/windows separation.

	There were no code changes to the unix version.
	I believe I tested most of the path variations
	on windows.

	Note, this CL supersedes CL 1942050; those
        changes are rolled in to this CL. I'll close
	CL 1942050.

[Joe Poirier, 2010-08-28 05:48:51]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[Joe Poirier, 2010-08-28 06:04:52]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[Joe Poirier, 2010-08-29 16:07:25]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-08-30 06:41:59]

http://codereview.appspot.com/2068041/diff/10001/11003
File src/pkg/exec/exec_test.go (right):

http://codereview.appspot.com/2068041/diff/10001/11003#newcode16
src/pkg/exec/exec_test.go:16: // system folder can be called rather than form
MinGW/Msys?
Not sure.

http://codereview.appspot.com/2068041/diff/10001/11003#newcode17
src/pkg/exec/exec_test.go:17: // But how?
Don't know.

http://codereview.appspot.com/2068041/diff/10001/11003#newcode20
src/pkg/exec/exec_test.go:20: // on windows - what's the status of that
functionality?
Wei have implemented it in his CL. But I had problems with it, because
OpenProcess() failed for me couple of times. It seems to me, it doesn't work if
child process finishes and exit the system before we call OpenProcess(). I've
spoken to Russ about it and he offered to extend os.ForkExec interface
(http://code.google.com/p/go/issues/detail?id=1004) to allow us pass
CreateProcess() handle as well as PID from syscall to os to exec.

http://codereview.appspot.com/2068041/diff/10001/11003#newcode22
src/pkg/exec/exec_test.go:22: // something happens after Run. echo is just a
script file in
You need the rest of the code.
I also found that it works from "DOS shell", but msys bin folder is not on the
PATH of "DOS shell". On the other hand it is in the PATH of "msys shell", but
our Run command doesn't work from "msys shell" (cat.exe crashes). Not sure what
we're going to do about it yet. But one thing at the time.

http://codereview.appspot.com/2068041/diff/10001/11003#newcode25
src/pkg/exec/exec_test.go:25: //func TestRunPing(t *testing.T) {
Why is it commented? If you don't use it, get rid of it.

http://codereview.appspot.com/2068041/diff/10001/11005
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/10001/11005#newcode23
src/pkg/exec/lp_windows.go:23: // executable.
It is hard coded in pkg/os/stat_windows.go/^setFileInfo(). I would not worry
about that. Don't need any of those comments.

http://codereview.appspot.com/2068041/diff/10001/11005#newcode49
src/pkg/exec/lp_windows.go:49: }
You should be consistent (one way or the other):
f := file + e
return f, chkStat(f)

[Joe Poirier, 2010-08-30 07:48:50]

Jus replies to Alex's comments.

http://codereview.appspot.com/2068041/diff/10001/11003
File src/pkg/exec/exec_test.go (right):

http://codereview.appspot.com/2068041/diff/10001/11003#newcode17
src/pkg/exec/exec_test.go:17: // But how?
On 2010/08/30 06:41:59, brainman wrote:
> Don't know.

There are quite a few package tests that are unix'ish only. Maybe something as
simple as a platform check in the gotest script. Eg if windows looks for test
file names with windows in them and if otherwise test all files without windows
in the file name.

http://codereview.appspot.com/2068041/diff/10001/11003#newcode22
src/pkg/exec/exec_test.go:22: // something happens after Run. echo is just a
script file in
On 2010/08/30 06:41:59, brainman wrote:
> You need the rest of the code.
> I also found that it works from "DOS shell", but msys bin folder is not on
the
> PATH of "DOS shell". On the other hand it is in the PATH of "msys shell", but
> our Run command doesn't work from "msys shell" (cat.exe crashes). Not sure
what
> we're going to do about it yet. But one thing at the time.

From a Go user's perspective on windows, ie someone developing Go code on
windows, supporting Go via the standard windows shell should be the main
focus/priority for sure. As time permits, non-standard environment issues, like
Msys' shell, can be dealt with. 

I'd assume that if someone is installing Msys that they'd know they need to add
the bin folder to the system PATH.

http://codereview.appspot.com/2068041/diff/10001/11003#newcode25
src/pkg/exec/exec_test.go:25: //func TestRunPing(t *testing.T) {
On 2010/08/30 06:41:59, brainman wrote:
> Why is it commented? If you don't use it, get rid of it.

Left over from testing via windows shell. I'll remove it.

http://codereview.appspot.com/2068041/diff/10001/11005
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/10001/11005#newcode23
src/pkg/exec/lp_windows.go:23: // executable.
On 2010/08/30 06:41:59, brainman wrote:
> It is hard coded in pkg/os/stat_windows.go/^setFileInfo(). I would not worry
> about that. Don't need any of those comments.

Okay, I'll remove the comments.

http://codereview.appspot.com/2068041/diff/10001/11005#newcode49
src/pkg/exec/lp_windows.go:49: }
On 2010/08/30 06:41:59, brainman wrote:
> You should be consistent (one way or the other):
> f := file + e
> return f, chkStat(f)

If "if chkStat(f)" returns false the code must continue at the top of the "for"
loop.

[brainman, 2010-08-30 08:02:02]

http://codereview.appspot.com/2068041/diff/10001/11003
File src/pkg/exec/exec_test.go (right):

http://codereview.appspot.com/2068041/diff/10001/11003#newcode17
src/pkg/exec/exec_test.go:17: // But how?
On 2010/08/30 07:48:50, Joe Poirier wrote:
> There are quite a few package tests that are unix'ish only. Maybe something as
> simple as a platform check in the gotest script. ...

I would rather have one single test that works everywhere. Unless we hit brick
wall, it is.

http://codereview.appspot.com/2068041/diff/10001/11003#newcode22
src/pkg/exec/exec_test.go:22: // something happens after Run. echo is just a
script file in
On 2010/08/30 07:48:50, Joe Poirier wrote:
> From a Go user's perspective on windows, ...

We're talking about tests here. If you're running go tests, I expect you to have
mingw.  None of make, sh, echo, cat and so on come as part of windows.

http://codereview.appspot.com/2068041/diff/10001/11005
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/10001/11005#newcode49
src/pkg/exec/lp_windows.go:49: }
On 2010/08/30 07:48:50, Joe Poirier wrote:
> If "if chkStat(f)" returns false the code must continue at the top of the
"for"
> loop. 

Missed that - you're right, I'm wrong. But shouldn't previous chkStat failure
continue with the loop, then?

[Joe Poirier, 2010-08-30 09:00:31]

http://codereview.appspot.com/2068041/diff/10001/11003
File src/pkg/exec/exec_test.go (right):

http://codereview.appspot.com/2068041/diff/10001/11003#newcode22
src/pkg/exec/exec_test.go:22: // something happens after Run. echo is just a
script file in
On 2010/08/30 08:02:02, brainman wrote:
> On 2010/08/30 07:48:50, Joe Poirier wrote:
> > From a Go user's perspective on windows, ...
> 
> We're talking about tests here. If you're running go tests, I expect you to
have
> mingw.  None of make, sh, echo, cat and so on come as part of windows.

True. I forgot about the context that tests are being run in. 

Actually, mingw includes just the compiler suite, it's msys that has the unix
utilities.

At some point, it would be nice not to have to install mingw and msys and just
have a package (a simple folder) that doesn't require installation and that
includes gcc and the unix utilities. 
  
That would require the _MINGW compile switches be changed to _WINDOWS but that's
about all. It'd be a straight forward process to copy the unix utility
executables and GGC in to a single folder, then just set the path to the
executables, call sh.exe from cmd.exe and compile.

http://codereview.appspot.com/2068041/diff/10001/11005
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/10001/11005#newcode49
src/pkg/exec/lp_windows.go:49: }
On 2010/08/30 08:02:02, brainman wrote:
> On 2010/08/30 07:48:50, Joe Poirier wrote:
> > If "if chkStat(f)" returns false the code must continue at the top of the
> "for"
> > loop. 
> 
> Missed that - you're right, I'm wrong. But shouldn't previous chkStat failure
> continue with the loop, then?

No. 

If the file being checked has an extension we're done in the current function
and it's okay to return - the file and the status from chkStat.

The code is re-factored a bit. There could be two separate for loops, the first
one to check if the file has an extension, then a second one (if the first one
fails) checking if a file with an appended extension exists. In either case both
loops would need to range through the PATHEXT variables.

[Joe Poirier, 2010-08-30 09:22:53]

Hello brainman, rsc, vcc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-08-30 20:41:22]

http://codereview.appspot.com/2068041/diff/19001/20005
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/19001/20005#newcode25
src/pkg/exec/lp_windows.go:25: exts := strings.Split(os.Getenv("PATHEXT"), ";",
-1)
Is this guaranteed to be set?

http://codereview.appspot.com/2068041/diff/19001/20005#newcode36
src/pkg/exec/lp_windows.go:36: if (lf > x) && (strings.ToLower(file[lf-x:]) ==
e) {
drop unnecessary parens on both sides of &&
Actually this looks like

if strings.HasSuffix(strings.ToLower(file), e) {

Is it?

http://codereview.appspot.com/2068041/diff/19001/20005#newcode39
src/pkg/exec/lp_windows.go:39: // no ext, append one
Should this be a second loop through the extensions?

If file is "x.exe" and PATHEXT says "bat;exe", the current
code will prefer "x.exe.bat" to "x.exe".

http://codereview.appspot.com/2068041/diff/19001/20005#newcode52
src/pkg/exec/lp_windows.go:52: if (strings.Index(file, "\\") >= 0) ||
(strings.Index(file, "/") >= 0) {
drop unnecessary parens on both sides of ||

http://codereview.appspot.com/2068041/diff/19001/20005#newcode52
src/pkg/exec/lp_windows.go:52: if (strings.Index(file, "\\") >= 0) ||
(strings.Index(file, "/") >= 0) {
I'd be inclined to use `` instead of "" throughout this file.
Then you can say `\`.

[Joe Poirier, 2010-08-30 23:15:33]

http://codereview.appspot.com/2068041/diff/19001/20005
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/19001/20005#newcode25
src/pkg/exec/lp_windows.go:25: exts := strings.Split(os.Getenv("PATHEXT"), ";",
-1)
On 2010/08/30 20:41:23, rsc1 wrote:
> Is this guaranteed to be set?
> 

Yes

http://codereview.appspot.com/2068041/diff/19001/20005#newcode36
src/pkg/exec/lp_windows.go:36: if (lf > x) && (strings.ToLower(file[lf-x:]) ==
e) {
On 2010/08/30 20:41:23, rsc1 wrote:
> drop unnecessary parens on both sides of &&
> Actually this looks like
> 
> if strings.HasSuffix(strings.ToLower(file), e) {
> 
> Is it?  

Done.

http://codereview.appspot.com/2068041/diff/19001/20005#newcode39
src/pkg/exec/lp_windows.go:39: // no ext, append one
On 2010/08/30 20:41:23, rsc1 wrote:
> Should this be a second loop through the extensions?
> 
> If file is "x.exe" and PATHEXT says "bat;exe", the current
> code will prefer "x.exe.bat" to "x.exe".

Done.

http://codereview.appspot.com/2068041/diff/19001/20005#newcode52
src/pkg/exec/lp_windows.go:52: if (strings.Index(file, "\\") >= 0) ||
(strings.Index(file, "/") >= 0) {
On 2010/08/30 20:41:23, rsc1 wrote:
> drop unnecessary parens on both sides of ||
> 

Done.

http://codereview.appspot.com/2068041/diff/19001/20005#newcode52
src/pkg/exec/lp_windows.go:52: if (strings.Index(file, "\\") >= 0) ||
(strings.Index(file, "/") >= 0) {
On 2010/08/30 20:41:23, rsc1 wrote:
> I'd be inclined to use `` instead of "" throughout this file.
> Then you can say `\`.
> 

Done.

[Joe Poirier, 2010-08-30 23:15:55]

Hello brainman, rsc, vcc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-08-30 23:30:42]

Looking pretty good.  Few more small things.

http://codereview.appspot.com/2068041/diff/15002/24005
File src/pkg/exec/lp_unix.go (right):

http://codereview.appspot.com/2068041/diff/15002/24005#newcode26
src/pkg/exec/lp_unix.go:26: // TODO(rsc): Does LookPath belong in os instead?
please delete this comment

http://codereview.appspot.com/2068041/diff/15002/24006
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/15002/24006#newcode19
src/pkg/exec/lp_windows.go:19: //return d.IsRegular() && d.Permission()&0111 !=
0
delete

http://codereview.appspot.com/2068041/diff/15002/24006#newcode23
src/pkg/exec/lp_windows.go:23: func canExec(file string) (string, bool) {
Please inline this function in LookPath.
When you do I think you'll find that you can
compute exts just once rather than once per PATH entry.

http://codereview.appspot.com/2068041/diff/15002/24006#newcode46
src/pkg/exec/lp_windows.go:46: // LookPath searches for an executable binary
named file
delete doc comment: the one on the other
LookPath is good enough, and then there 
aren't two to maintain

[Joe Poirier, 2010-08-31 00:17:49]

http://codereview.appspot.com/2068041/diff/15002/24005
File src/pkg/exec/lp_unix.go (right):

http://codereview.appspot.com/2068041/diff/15002/24005#newcode26
src/pkg/exec/lp_unix.go:26: // TODO(rsc): Does LookPath belong in os instead?
On 2010/08/30 23:30:42, rsc1 wrote:
> please delete this comment
> 

Done.

http://codereview.appspot.com/2068041/diff/15002/24006
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/15002/24006#newcode19
src/pkg/exec/lp_windows.go:19: //return d.IsRegular() && d.Permission()&0111 !=
0
On 2010/08/30 23:30:42, rsc1 wrote:
> delete
> 

Done.

http://codereview.appspot.com/2068041/diff/15002/24006#newcode23
src/pkg/exec/lp_windows.go:23: func canExec(file string) (string, bool) {
On 2010/08/30 23:30:42, rsc1 wrote:
> Please inline this function in LookPath.
> When you do I think you'll find that you can
> compute exts just once rather than once per PATH entry.
> 

Done

Note, changed:
    if strings.Index(e, `.`) == -1 {
to
    if e[0:1] != `.` {

http://codereview.appspot.com/2068041/diff/15002/24006#newcode46
src/pkg/exec/lp_windows.go:46: // LookPath searches for an executable binary
named file
On 2010/08/30 23:30:42, rsc1 wrote:
> delete doc comment: the one on the other
> LookPath is good enough, and then there 
> aren't two to maintain
> 

Done.

[Joe Poirier, 2010-08-31 00:18:05]

Hello brainman, rsc, vcc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-08-31 00:33:40]

Almost there.
Have to avoid slicing e[0:1] when e is empty,
and might as well check the byte instead of
a string compare.

http://codereview.appspot.com/2068041/diff/9002/16006
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/9002/16006#newcode42
src/pkg/exec/lp_windows.go:42: if e[0:1] != `.` {
If len(e) < 1 || e[0] != '.' {

[Joe Poirier, 2010-08-31 00:57:46]

> Almost there.
:-]

> Have to avoid slicing e[0:1] when e is empty,
Yup

> and might as well check the byte instead of
> a string compare.
but type uint8 != string for e[0] != '.'

-joe

[rsc, 2010-08-31 01:06:09]

>> and might as well check the byte instead of
>> a string compare.
> but type uint8 != string for e[0] != '.'

those are single quotes not backquotes: ' not `

[Joe Poirier, 2010-08-31 01:30:10]

http://codereview.appspot.com/2068041/diff/9002/16006
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/9002/16006#newcode22
src/pkg/exec/lp_windows.go:22: func canExec(file string, exts []string) (string,
bool) {

Moving ToLower out of the loop
	   f := strings.ToLower(file)
	   for _, e := range exts {
		   if strings.HasSuffix(f, e) {
			   return file, chkStat(file)
		   }
	   }

http://codereview.appspot.com/2068041/diff/9002/16006#newcode42
src/pkg/exec/lp_windows.go:42: if e[0:1] != `.` {
On 2010/08/31 00:33:40, rsc1 wrote:
> If len(e) < 1 || e[0] != '.' {
> 
Okay, changing e[0] != '.' 

I just checked, there's no need to do if len(e) < 1 because windows strips
whitespace and redundant semicolons from PATHEXT when os.Getenv("PATHEXT") is
called

E.g if PATHEXT is: .COM;.EXE; ;;;; .VBS;;.EXE;VBE;   DDD;
windows returns: ".COM;.EXE;.VBS;.EXE;VBE;DDD"

[Joe Poirier, 2010-08-31 01:31:57]

Hello brainman, rsc, vcc (cc: golang-dev@googlegroups.com),

Please take another look.

[Joe Poirier, 2010-08-31 02:14:35]

On Mon, Aug 30, 2010 at 8:30 PM,  <jdpoirier@gmail.com> wrote:
>
> http://codereview.appspot.com/2068041/diff/9002/16006
> File src/pkg/exec/lp_windows.go (right):
>
> http://codereview.appspot.com/2068041/diff/9002/16006#newcode22
> src/pkg/exec/lp_windows.go:22: func canExec(file string, exts []string)
> (string, bool) {
>
> Moving ToLower out of the loop
>           f := strings.ToLower(file)
>           for _, e := range exts {
>                   if strings.HasSuffix(f, e) {
>                           return file, chkStat(file)
>                   }
>           }
>
> http://codereview.appspot.com/2068041/diff/9002/16006#newcode42
> src/pkg/exec/lp_windows.go:42: if e[0:1] != `.` {
> On 2010/08/31 00:33:40, rsc1 wrote:
>>
>> If len(e) < 1 || e[0] != '.' {
>
> Okay, changing e[0] != '.'
>
> I just checked, there's no need to do if len(e) < 1 because windows
> strips whitespace and redundant semicolons from PATHEXT when
> os.Getenv("PATHEXT") is called
>
> E.g if PATHEXT is: .COM;.EXE; ;;;; .VBS;;.EXE;VBE;   DDD;
> windows returns: ".COM;.EXE;.VBS;.EXE;VBE;DDD"

FYI
I realized I'd forgotten to start a new shell when running make test and
after deleting and modifying PATHEXT. So after double checking I /will/
need to add the checks to the code after all.

-joe

[Joe Poirier, 2010-08-31 04:03:25]

A user can delete the PATH and PATHEXT variables from the environment
properties, so the code checks if they're empty. 

Re-tested different scenarios correctly, ie closing and restarting the cmd shell
each time the environment variables were modified/deleted.

[Joe Poirier, 2010-08-31 04:04:19]

Hello brainman, rsc, vcc (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-08-31 07:29:17]

Feel free to ignore my suggestions.

http://codereview.appspot.com/2068041/diff/39001/40005
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/39001/40005#newcode23
src/pkg/exec/lp_windows.go:23: if len(exts) < 1 {
if len(exts) = 0 {

http://codereview.appspot.com/2068041/diff/39001/40005#newcode36
src/pkg/exec/lp_windows.go:36: }
if f := file + e; chkStat(f) {
  return f, true
}

http://codereview.appspot.com/2068041/diff/39001/40005#newcode47
src/pkg/exec/lp_windows.go:47: if len(e) < 1 || e[0] != '.' {
if len(e) = 0 || e[0] != '.' {

http://codereview.appspot.com/2068041/diff/39001/40005#newcode56
src/pkg/exec/lp_windows.go:56: }
if f, ok := canExec(file, exts); ok {
  return f, nil
}

http://codereview.appspot.com/2068041/diff/39001/40005#newcode60
src/pkg/exec/lp_windows.go:60: if len(pathenv) < 1 {
if len(pathenv) = 0 {

http://codereview.appspot.com/2068041/diff/39001/40005#newcode64
src/pkg/exec/lp_windows.go:64: }

if f, ok := canExec(`.\`+file, exts); ok {
  return f, nil
}

http://codereview.appspot.com/2068041/diff/39001/40005#newcode70
src/pkg/exec/lp_windows.go:70: }
if f, ok := canExec(dir+`\`+file, exts); ok {
  return f, nil
}

[Joe Poirier, 2010-08-31 07:53:59]

It's nice being able to put the assignment and conditional check on the
same line but I find it's a hindrance when debugging using print statements.

I'm impartial between  if len(exts) < 1  and  if len(exts) == 0
I'll change the code to the latter if that's deemed the convention.

-joe


On Tue, Aug 31, 2010 at 2:29 AM,  <alex.brainman@gmail.com> wrote:
> Feel free to ignore my suggestions.
>
>
> http://codereview.appspot.com/2068041/diff/39001/40005
> File src/pkg/exec/lp_windows.go (right):
>
> http://codereview.appspot.com/2068041/diff/39001/40005#newcode23
> src/pkg/exec/lp_windows.go:23: if len(exts) < 1 {
> if len(exts) = 0 {
>
> http://codereview.appspot.com/2068041/diff/39001/40005#newcode36
> src/pkg/exec/lp_windows.go:36: }
> if f := file + e; chkStat(f) {
>  return f, true
> }
>
> http://codereview.appspot.com/2068041/diff/39001/40005#newcode47
> src/pkg/exec/lp_windows.go:47: if len(e) < 1 || e[0] != '.' {
> if len(e) = 0 || e[0] != '.' {
>
> http://codereview.appspot.com/2068041/diff/39001/40005#newcode56
> src/pkg/exec/lp_windows.go:56: }
> if f, ok := canExec(file, exts); ok {
>  return f, nil
> }
>
> http://codereview.appspot.com/2068041/diff/39001/40005#newcode60
> src/pkg/exec/lp_windows.go:60: if len(pathenv) < 1 {
> if len(pathenv) = 0 {
>
> http://codereview.appspot.com/2068041/diff/39001/40005#newcode64
> src/pkg/exec/lp_windows.go:64: }
>
> if f, ok := canExec(`.\`+file, exts); ok {
>  return f, nil
> }
>
> http://codereview.appspot.com/2068041/diff/39001/40005#newcode70
> src/pkg/exec/lp_windows.go:70: }
> if f, ok := canExec(dir+`\`+file, exts); ok {
>  return f, nil
> }
>
> http://codereview.appspot.com/2068041/
>

[brainman, 2010-08-31 07:59:15]

> It's nice being able to put the assignment and conditional check on the
> same line but I find it's a hindrance when debugging using print statements.
> 

Brings noise level down. For my simple mind anyway <g>.

> I'm impartial between  if len(exts) < 1  and  if len(exts) == 0

When I see "< 1", I start thinking 0? -1? 0.5?

> ... if that's deemed the convention.

I don't think it is a convention.

Alex

[Joe Poirier, 2010-08-31 08:27:57]

On Tue, Aug 31, 2010 at 2:59 AM,  <alex.brainman@gmail.com> wrote:
>> It's nice being able to put the assignment and conditional check on
>
> the
>>
>> same line but I find it's a hindrance when debugging using print
>
> statements.
>
>
> Brings noise level down. For my simple mind anyway <g>.

It's just the opposite for me, but then my brain is still thinking
in terms of C and respecting sequence points.

>
>> I'm impartial between  if len(exts) < 1  and  if len(exts) == 0
>
> When I see "< 1", I start thinking 0? -1? 0.5?
>

I know len() returns a type int so for this code I don't really see a
difference between < 1 and == 0.

[rsc, 2010-08-31 11:44:52]

> I know len() returns a type int so for this code I don't really see a
> difference between < 1 and == 0.

If you were checking the second byte of the string you'd use

if len(x) < 2 || x[1] != '.'

That's why I suggested

if len(x) < 1 || x[0] != '.'

but it doesn't matter.  You could even say if x == "" || x[0] != '.'.
They're all the same as far as efficiency is concerned.

Russ

[Joe Poirier, 2010-08-31 13:57:19]

>
> If you were checking the second byte of the string you'd use
>
> if len(x) < 2 || x[1] != '.'
>
> That's why I suggested
>
> if len(x) < 1 || x[0] != '.'
>
> but it doesn't matter.  You could even say if x == "" || x[0] != '.'.
> They're all the same as far as efficiency is concerned.

Actually, I like x == "" over the length check.

I'm curious, was the suggestion to use the back tics for `\` to avoid
having to escape the backslash and then use them throughout just
to be consistent?

Also, is return "" preferred over return `` ?

-joe

[rsc, 2010-09-09 17:59:51]

Joe - still planning to finish this?

Russ

[Joe Poirier, 2010-09-09 20:43:23]

On Thu, Sep 9, 2010 at 12:59 PM, Russ Cox <rsc@golang.org> wrote:
> Joe - still planning to finish this?
>

Yes.

I guess it needs a little polish and some more testing; it should be finished
by tomorrow.

-joe

[Joe Poirier, 2010-09-10 07:17:58]

I'll upload the code in a bit.

BTW, the exec package tests still fail/crash. I guess 
it has something to do with a PIPE problem?

The code passed the following test cases against 
a9511f1368c8+ tip

*the PATHEXT variable can be deleted by the user 
via system properties but the default extensions 
remain, as does the environment variable.

-------------------------------------------------------
Test A. With PATHEXT and PATH environment variables
Test B. With PATHEXT environment variable but without 
        PATH environment variable
Test C. With PATHEXT and PATH environment variables but
        with multiple separators, spaces, bogus paths

Test A, B, and C:
-----------------
- Path included in the string sent to LookPath
  path && file && extension
  path && file && no extension
  bogus path && file && extension
  bogus path && file && no extension
  path && bogus file && extension
  path && bogus file && no extension
  path && file && bogus extension
  bogus path && bogus file && extension
  bogus path && bogus file && no extension
  path && bogus file && bogus extension
  bogus path && bogus file && bogus extension
  spaces in path && file && extension
  spaces in path && file && no extension

- No path in the string sent to LookPath
  file && extension
  file && no extension
  bogus file && extension
  bogus file && no extension
  file && bogus extension
  bogus file && bogus extension
  spaces in path && file && extension
  spaces in path && file && no extension

[Joe Poirier, 2010-09-10 07:30:45]

Hello brainman, rsc, vcc (cc: golang-dev@googlegroups.com),

Please take another look.

[vcc, 2010-09-10 17:11:04]

2010/9/10 <jdpoirier@gmail.com>

>
> BTW, the exec package tests still fail/crash. I guess
> it has something to do with a PIPE problem?
>  <http://codereview.appspot.com/2068041/>
>

a workaround: change $GOROOT/bin/gotest last line to:
cmd /c $O.out "$@"

exec tests should PASS, this tests run in dos shell is ok, can't run in msys
shell.

Wei guangjing


> http://codereview.appspot.com/2068041/
>

[Joe Poirier, 2010-09-11 04:45:37]

PTAL

Although I added the mingw bin folder to the system path
for gcc, the godashboard builds will continue to fail 
because of the existing LookPath code.

-joe

[rsc1, 2010-09-11 04:50:36]

looks good except for the two comments

http://codereview.appspot.com/2068041/diff/45001/src/pkg/exec/lp_unix.go
File src/pkg/exec/lp_unix.go (right):

http://codereview.appspot.com/2068041/diff/45001/src/pkg/exec/lp_unix.go#newc...
src/pkg/exec/lp_unix.go:5: // Unix environment variables.
wrong comment

http://codereview.appspot.com/2068041/diff/45001/src/pkg/exec/lp_windows.go
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/45001/src/pkg/exec/lp_windows.go#n...
src/pkg/exec/lp_windows.go:5: // Windows environment variables.
wrong comment

[Joe Poirier, 2010-09-11 05:19:51]

http://codereview.appspot.com/2068041/diff/45001/src/pkg/exec/lp_unix.go
File src/pkg/exec/lp_unix.go (right):

http://codereview.appspot.com/2068041/diff/45001/src/pkg/exec/lp_unix.go#newc...
src/pkg/exec/lp_unix.go:5: // Unix environment variables.
On 2010/09/11 04:50:36, rsc1 wrote:
> wrong comment
> 

Done.

http://codereview.appspot.com/2068041/diff/45001/src/pkg/exec/lp_windows.go
File src/pkg/exec/lp_windows.go (right):

http://codereview.appspot.com/2068041/diff/45001/src/pkg/exec/lp_windows.go#n...
src/pkg/exec/lp_windows.go:5: // Windows environment variables.
On 2010/09/11 04:50:36, rsc1 wrote:
> wrong comment
> 

Done.

[Joe Poirier, 2010-09-11 05:20:04]

Hello brainman, rsc, vcc, rsc1 (cc: golang-dev@googlegroups.com),

Please take another look.

[vcc, 2010-09-11 05:56:18]

LGTM

On 2010/09/11 05:20:04, Joe Poirier wrote:
> Hello brainman, rsc, vcc, rsc1 (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

[brainman, 2010-09-12 02:32:36]

LGTM

[rsc1, 2010-09-12 03:21:52]

LGTM

[brainman, 2010-09-12 07:38:40]

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

exec.LookPath() unix/windows separation

R=brainman, rsc, vcc, rsc1
CC=golang-dev
http://codereview.appspot.com/2068041

Committer: Alex Brainman <alex.brainman@gmail.com>