code review 67280043: exec: explicitely mention Cmd.Wait() has to be called e...

os/exec: explicitely mention Cmd.Wait() has to be called eventually

[patrick2, 2014-02-22 11:22:00]

Hello golang-codereviews@googlegroups.com,

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

[minux1, 2014-02-23 00:17:26]

please use os/exec instead of just exec in the description.

LGTM. Will wait for until Monday to submit it.

R: +r for the wording.

[patrick2, 2014-02-23 09:22:51]

On 2014/02/23 00:17:26, minux wrote:
> please use os/exec instead of just exec in the description.
> 
> LGTM. Will wait for until Monday to submit it.
> 
> R: +r for the wording.

I fixed the description.

[r, 2014-02-23 15:52:47]

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

https://codereview.appspot.com/67280043/diff/60001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:242: // Wait has to be called eventually to release
associated resources.
"eventually" makes me think, "when?" plus adverbs are sloppy.

The Wait method will return the exit code and release associated resources once
the command exits.

[patrick2, 2014-02-23 16:58:31]

PTAL

(BTW, I have another similar one liner here:
https://codereview.appspot.com/64770043/ )

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

https://codereview.appspot.com/67280043/diff/60001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:242: // Wait has to be called eventually to release
associated resources.
On 2014/02/23 15:52:47, r wrote:
> "eventually" makes me think, "when?" plus adverbs are sloppy.
> 
> The Wait method will return the exit code and release associated resources
once
> the command exits.

Done.

[r, 2014-02-23 17:42:05]

LGTM

[r, 2014-02-23 17:43:47]

Please sign the CLA as described at the bottom of
http://golang.org/doc/contribute.html

[patrick2, 2014-02-23 18:05:34]

On 2014/02/23 17:43:47, r wrote:
> Please sign the CLA as described at the bottom of
> http://golang.org/doc/contribute.html

Done. I hoped the one I signed for libphonenumber would be enough, I suppose
this is per project?

[r, 2014-02-23 20:52:00]

It's not per project, but the search is by e-mail address so maybe you
used a different one there.

-rob


On Sun, Feb 23, 2014 at 10:05 AM,  <patrick@mezard.eu> wrote:
> On 2014/02/23 17:43:47, r wrote:
>>
>> Please sign the CLA as described at the bottom of
>> http://golang.org/doc/contribute.html
>
>
> Done. I hoped the one I signed for libphonenumber would be enough, I
> suppose this is per project?
>
> https://codereview.appspot.com/67280043/

[r, 2014-02-23 20:53:04]

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

os/exec: explicitly mention Cmd.Wait() has to be called eventually

LGTM=minux.ma, r
R=golang-codereviews, minux.ma, r
CC=golang-codereviews
https://codereview.appspot.com/67280043

Committer: Rob Pike <r@golang.org>

[gobot, 2014-02-23 21:14:10]

This CL appears to have broken the darwin-amd64 builder.

[dave_cheney.net, 2014-02-23 21:15:50]

not really, old darwin bug

--- FAIL: TestPipeLookPathLeak (0.05 seconds)
	exec_test.go:228: leaked 5 fds; want ~0
FAIL
FAIL	os/exec	0.794s



On Mon, Feb 24, 2014 at 8:14 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the darwin-amd64 builder.
>
>
> https://codereview.appspot.com/67280043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>