code review 13410045: os/exec: change windows LookPath so it works like cmd.exe

os/exec: change windows LookPath so it works like cmd.exe

Fixes issue 6224

[brainman, 2013-09-05 02:19:27]

Hello golang-dev@googlegroups.com,

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

[frichter, 2013-09-06 01:28:00]

Alex, 
hasExt() looks broken, for example, it will return true for `.\dir.path\file`

Instead, how about:

func hasExt(path string) bool {
	for i := len(path) - 1; i >= 0 && !os.IsPathSeparator(path[i]); i-- {
		if path[i] == '.' {
			return true
		}
	}
	return false
}

based on existing filepath.Ext() 
https://code.google.com/p/go/source/browse/src/pkg/path/filepath/path.go#212

The rest of the logic in findExecutable() looks fine.

[brainman, 2013-09-06 02:41:30]

On 2013/09/06 01:28:00, frichter wrote:
> hasExt() looks broken, ...

Good catch. It is. I spent too much time on the test, not the fix :-).

> Instead, how about:

I fixed my version. I think my version is simpler.

Alex

[brainman, 2013-09-06 02:41:46]

Hello golang-dev@googlegroups.com, hcwfrichter@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2013-09-06 02:43:52]

On 2013/09/06 02:41:30, brainman wrote:
> 
> I fixed my version. ...

I also added new test to catch errors in hasExt().

Alex

[frichter, 2013-09-06 17:42:29]

New version LGTM

On Thursday, September 5, 2013 10:43:53 PM UTC-4, brainman wrote:
>
> On 2013/09/06 02:41:30, brainman wrote: 
>
> > I fixed my version. ... 
>
> I also added new test to catch errors in hasExt(). 
>
> Alex 
>
> https://codereview.appspot.com/13410045/ 
>

[bradfitz, 2013-09-06 19:16:02]

LGTM

Test seems a little complicated, but maybe it's necessary.

https://codereview.appspot.com/13410045/diff/10001/src/pkg/os/exec/lp_windows...
File src/pkg/os/exec/lp_windows_test.go (right):

https://codereview.appspot.com/13410045/diff/10001/src/pkg/os/exec/lp_windows...
src/pkg/os/exec/lp_windows_test.go:326: lookpathExe := buildExe(t, lookpathSrc,
tmp, "lookpath")
why can't the parent test process just call LookPath?

[brainman, 2013-09-08 07:45:05]

https://codereview.appspot.com/13410045/diff/10001/src/pkg/os/exec/lp_windows...
File src/pkg/os/exec/lp_windows_test.go (right):

https://codereview.appspot.com/13410045/diff/10001/src/pkg/os/exec/lp_windows...
src/pkg/os/exec/lp_windows_test.go:326: lookpathExe := buildExe(t, lookpathSrc,
tmp, "lookpath")
On 2013/09/06 19:16:02, bradfitz wrote:
> why can't the parent test process just call LookPath?

It can be implemented that way. But, we need to change current directory and
PATH and PATHEXT environment variables to test LookPath, while calling external
program at the same time. It is easy to shoot myself in the foot this way. And
since we are calling external program anyway, what is to make another call. I
think my current code is simpler and shorter too.

But if you insist, I will try and change the test. Please, let me know.

[bradfitz, 2013-09-08 14:34:41]

It's okay as-is. Maybe skip that test in short mode if it's slow.
 On Sep 8, 2013 12:45 AM, <alex.brainman@gmail.com> wrote:

>
> https://codereview.appspot.**com/13410045/diff/10001/src/**
>
pkg/os/exec/lp_windows_test.go<https://codereview.appspot.com/13410045/diff/10001/src/pkg/os/exec/lp_windows_test.go>
> File src/pkg/os/exec/lp_windows_**test.go (right):
>
> https://codereview.appspot.**com/13410045/diff/10001/src/**
>
pkg/os/exec/lp_windows_test.**go#newcode326<https://codereview.appspot.com/13410045/diff/10001/src/pkg/os/exec/lp_windows_test.go#newcode326>
> src/pkg/os/exec/lp_windows_**test.go:326: lookpathExe := buildExe(t,
> lookpathSrc, tmp, "lookpath")
> On 2013/09/06 19:16:02, bradfitz wrote:
>
>> why can't the parent test process just call LookPath?
>>
>
> It can be implemented that way. But, we need to change current directory
> and PATH and PATHEXT environment variables to test LookPath, while
> calling external program at the same time. It is easy to shoot myself in
> the foot this way. And since we are calling external program anyway,
> what is to make another call. I think my current code is simpler and
> shorter too.
>
> But if you insist, I will try and change the test. Please, let me know.
>
>
https://codereview.appspot.**com/13410045/<https://codereview.appspot.com/134...
>

[brainman, 2013-09-10 04:50:38]

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

os/exec: change windows LookPath so it works like cmd.exe

Fixes issue 6224

R=golang-dev, hcwfrichter, bradfitz
CC=golang-dev
https://codereview.appspot.com/13410045

[brainman, 2013-09-10 04:51:37]

On 2013/09/08 14:34:41, bradfitz wrote:
> ... Maybe skip that test in short mode if it's slow.
> 

It is bit slow - it takes 3-4 seconds on my system. But I would like it running
as often as possible for now. It is important part of the system - I don't want
new breakage to go unnoticed. We can move it off the -short list later.

Alex