code review 59580044: os/exec: fix Command with relative paths

os/exec: fix Command with relative paths

Command was (and is) documented like:
"If name contains no path separators, Command uses LookPath to
resolve the path to a complete name if possible. Otherwise it
uses name directly."

But that wasn't true. It always did LookPath, and then
set a sticky error that the user couldn't unset.
And then if cmd.Dir was changed, Start would still fail
due to the earlier sticky error being set.

This keeps LookPath in the same place as before (so no user
visible changes in cmd.Path after Command), but only does
it when the documentation says it will happen.

Also, clarify the docs about a relative Dir path.

No change in any existing behavior, except using Command
is now possible with relative paths. Previously it only
worked if you built the *Cmd by hand.

Fixes Issue 7228

[bradfitz, 2014-02-03 20:25:11]

Hello iant (cc: adg, golang-codereviews@googlegroups.com),

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

[iant, 2014-02-03 21:27:17]

LGTM

[bradfitz, 2014-02-03 21:32:17]

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

os/exec: fix Command with relative paths

Command was (and is) documented like:
"If name contains no path separators, Command uses LookPath to
resolve the path to a complete name if possible. Otherwise it
uses name directly."

But that wasn't true. It always did LookPath, and then
set a sticky error that the user couldn't unset.
And then if cmd.Dir was changed, Start would still fail
due to the earlier sticky error being set.

This keeps LookPath in the same place as before (so no user
visible changes in cmd.Path after Command), but only does
it when the documentation says it will happen.

Also, clarify the docs about a relative Dir path.

No change in any existing behavior, except using Command
is now possible with relative paths. Previously it only
worked if you built the *Cmd by hand.

Fixes Issue 7228

LGTM=iant
R=iant
CC=adg, golang-codereviews
https://codereview.appspot.com/59580044

[brainman, 2014-02-03 23:43:19]

https://codereview.appspot.com/59580044/diff/40001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/59580044/diff/40001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:124: 
This won't work on windows. Windows path have both \ and /, and, more
importantly, they might have drive letter, like d:test.exe (it means run
test.exe from whatever current directory on drive d: is).

Perhaps, you should just do

return filepath.Base(s) != s

[bradfitz, 2014-02-04 04:05:26]

What doesn't work? I see chunk mismatch now.

I think it should work fine on Windows. The test passes.

https://codereview.appspot.com/59580044/diff/40001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/59580044/diff/40001/src/
pkg/os/exec/exec.go#newcode124
src/pkg/os/exec/exec.go:124:
This won't work on windows. Windows path have both \ and /, and, more
importantly, they might have drive letter, like d:test.exe (it means run
test.exe from whatever current directory on drive d: is).

Perhaps, you should just do

return filepath.Base(s) != s

https://codereview.appspot.com/59580044/

[brainman, 2014-02-04 04:59:21]

On 2014/02/04 04:05:26, bradfitz wrote:
> What doesn't work? ...
> 

Like I said earlier, your containsPathSeparator (it should really be called
isRelativePath or similar instead) function is wrong on windows. On windows path
can contain no \, but still be a "relative" path. For example:
- hello\world (use of \ to separate file names in the path is ok on windows)
- d:hello.txt (this is hello.txt file in current directory of d: drive)
Probably many others that I cannot think of.

I would just use:

return filepath.Base(s) != s

to test if path is "relative". Maybe you have better suggestion.

> ... The test passes.

Well. We don't have appropriate test.

I'm happy to send a change, if you agree. Don't know about adding more tests.

Alex

[adg, 2014-02-04 09:55:44]

LGTM

[adg, 2014-02-04 09:55:54]

On 4 February 2014 05:59, <alex.brainman@gmail.com> wrote:

> I would just use:
>
> return filepath.Base(s) != s
>
> to test if path is "relative". Maybe you have better suggestion.
>

That sounds OK to me.

[iant, 2014-02-04 14:19:20]

On Mon, Feb 3, 2014 at 8:59 PM,  <alex.brainman@gmail.com> wrote:
> On 2014/02/04 04:05:26, bradfitz wrote:
>>
>> What doesn't work? ...
>
>
>
> Like I said earlier, your containsPathSeparator (it should really be
> called isRelativePath or similar instead) function is wrong on windows.
> On windows path can contain no \, but still be a "relative" path. For
> example:
> - hello\world (use of \ to separate file names in the path is ok on
> windows)

As far as I can see, Brad's patch, now checked in, does handle this
case correctly.  The code calls os.IsPathSeparator, which on Windows
returns true for both '/' and '\\'.


> - d:hello.txt (this is hello.txt file in current directory of d: drive)

I agree that this case is not handled correctly on Windows.  On the
other hand, looking at the code, I don't think it worked correctly
before, either.


> Probably many others that I cannot think of.

I'm not sure I agree with that.  Paths aren't that complicated, even
on Windows.

A different issue on Windows is that LookPath would try appending
various suffixes (.com, .exe, .bat, .cmd), and that no longer happens
for relative paths.  I don't know what should happen there.


> I would just use:
>
> return filepath.Base(s) != s
>
> to test if path is "relative".

Seems worth a try.


> Well. We don't have appropriate test.
>
> I'm happy to send a change, if you agree. Don't know about adding more
> tests.

Brad's CL has a nice example of a test of executing a relative path.

Ian

[bradfitz, 2014-02-04 16:09:46]

On Tue, Feb 4, 2014 at 6:19 AM, Ian Lance Taylor <iant@golang.org> wrote:

> On Mon, Feb 3, 2014 at 8:59 PM,  <alex.brainman@gmail.com> wrote:
> > On 2014/02/04 04:05:26, bradfitz wrote:
> >>
> >> What doesn't work? ...
> >
> >
> >
> > Like I said earlier, your containsPathSeparator (it should really be
> > called isRelativePath or similar instead) function is wrong on windows.
> > On windows path can contain no \, but still be a "relative" path. For
> > example:
> > - hello\world (use of \ to separate file names in the path is ok on
> > windows)
>
> As far as I can see, Brad's patch, now checked in, does handle this
> case correctly.  The code calls os.IsPathSeparator, which on Windows
> returns true for both '/' and '\\'.
>
>
> > - d:hello.txt (this is hello.txt file in current directory of d: drive)
>
> I agree that this case is not handled correctly on Windows.  On the
> other hand, looking at the code, I don't think it worked correctly
> before, either.
>

This feels contrived... doesn't seem like something people would ever do?
But patches welcome.


>
>
> > Probably many others that I cannot think of.
>
> I'm not sure I agree with that.  Paths aren't that complicated, even
> on Windows.
>
> A different issue on Windows is that LookPath would try appending
> various suffixes (.com, .exe, .bat, .cmd), and that no longer happens
> for relative paths.  I don't know what should happen there.
>

It should still happen?


> > I would just use:
> >
> > return filepath.Base(s) != s
> >
> > to test if path is "relative".
>
> Seems worth a try.
>

filepath.Base?  filepath.Base("/foo/bar/baz") is "baz", which !=, but it's
not relative.

"foo.exe" is relative (if relative means !filepath.IsAbs) but we still want
to use LookPath on it.

I don't follow.

[iant, 2014-02-04 17:28:48]

On Tue, Feb 4, 2014 at 8:09 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
> On Tue, Feb 4, 2014 at 6:19 AM, Ian Lance Taylor <iant@golang.org> wrote:
>>
>> A different issue on Windows is that LookPath would try appending
>> various suffixes (.com, .exe, .bat, .cmd), and that no longer happens
>> for relative paths.  I don't know what should happen there.
>
>
> It should still happen?

Are you asking whether it should happen?

For an argument like a/b, I think that previously exec.Command would
look for a/b.bat, etc.  I think that it no longer does.


>> > I would just use:
>> >
>> > return filepath.Base(s) != s
>> >
>> > to test if path is "relative".
>>
>> Seems worth a try.
>
>
> filepath.Base?  filepath.Base("/foo/bar/baz") is "baz", which !=, but it's
> not relative.
>
> "foo.exe" is relative (if relative means !filepath.IsAbs) but we still want
> to use LookPath on it.
>
> I don't follow.

You're right of course that it needs to also check filepath.IsAbs.

"foo.exe" is not a relative path by the standard of the current code.
Base("foo.exe") will return "foo.exe".  Maybe I'm confused about what
you mean.

Ian

[brainman, 2014-02-05 06:18:46]

Replies to everyone follow ...

On 2014/02/04 14:19:20, iant wrote:
> ...  The code calls os.IsPathSeparator, which on Windows
> returns true for both '/' and '\\'.

My bad, I have forgotten that os.IsPathSeparator does do that.

> > - d:hello.txt (this is hello.txt file in current directory of d: drive)
> 
> I agree that this case is not handled correctly on Windows.  On the
> other hand, looking at the code, I don't think it worked correctly
> before, either.

You are, probably, right.

> > Probably many others that I cannot think of.
> 
> I'm not sure I agree with that.  Paths aren't that complicated, even
> on Windows.

No comment. :-)

> A different issue on Windows is that LookPath would try appending
> various suffixes (.com, .exe, .bat, .cmd), and that no longer happens
> for relative paths.  I don't know what should happen there.

windows os.StartProcess will do that. It calls Windows CreateProcess API, which
will do that. I think ...

> Brad's CL has a nice example of a test of executing a relative path.

Yes, but if PC has only dirve c: (and many PCs do), we won't be able to test my
d:hello.txt scenario. Everything else I can think of is covered by Brad's test.


On 2014/02/04 16:09:46, bradfitz wrote:
> 
> This feels contrived... doesn't seem like something people would ever do?

Sure they do. People always find ways to break my program.

> But patches welcome.

Sure. I will send a change.

> 
> filepath.Base?  filepath.Base("/foo/bar/baz") is "baz", which !=, but it's
> not relative.

You are right. I have forgotten about Absolute paths are OK here too. I guess,
the function cannot be called isRelativePath as I suggested. But the expression
filepath.Base(s) != s is still looks good for me.


On 2014/02/04 17:28:48, iant wrote:
> 
> For an argument like a/b, I think that previously exec.Command would
> look for a/b.bat, etc.  I think that it no longer does.

Like I said above, Windows CreateProcess will do that anyway.

I will think about this little more, and I will send a new CL for review.

Thank you all for your input.

Alex

[brainman, 2014-02-06 05:28:31]

On 2014/02/05 06:18:46, brainman wrote:
> 
> ... I will send a new CL for review.
> 

Please review https://codereview.appspot.com/59740050/

Alex