code review 9945044: syscall: os.Setenv doesn't set environment variables to...

syscall: os.Setenv doesn't set environment variables to "" on Windows

On Windows, os.Setenv calls syscall.Setenv which calls
sycall.SetEnvironmentVariable. If syscall.Setenv is passed ""
for the value, it converts it to nil before calling
syscall.SetEnvironmentVariable, which then attempts to delete
the environment variable.

Fixes issue 5610.

[peterGo, 2013-06-02 15:33:23]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-06-02 15:40:12]

where's the new test?



On Sun, Jun 2, 2013 at 8:33 AM, <go.peter.90@gmail.com> wrote:

> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
>
https://codereview.appspot.**com/9945044/<https://codereview.appspot.com/9945...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[minux1, 2013-06-02 23:29:42]

this is an API change, and as i'm not convinced the old behavior is
wrong, i think we'd better introduce a Setenv2 for windows, and make
the os package use the new API (we should also clarify in the os.Setenv
documentation).

another way to fix the problem is just ignore the error in os.Setenv when
trying to remove a nonexistent environment variable.

[bradfitz, 2013-06-03 14:10:36]

On Sun, Jun 2, 2013 at 4:29 PM, minux <minux.ma@gmail.com> wrote:

> this is an API change, and as i'm not convinced the old behavior is
> wrong, i think we'd better introduce a Setenv2 for windows, and make
> the os package use the new API (we should also clarify in the os.Setenv
> documentation).
>

I agree.

It's a minor one (an undocumented pkg syscall func), but people might be
relying on it.

[brainman, 2013-06-07 05:17:10]

Regardless what we decide, please add tests, so we all agree on how it is
suppose to work. Thank you.

Alex

[peterGo, 2013-06-08 11:14:43]

On 2013/06/07 05:17:10, brainman wrote:
> Regardless what we decide, please add tests, so we all agree on how it is
> suppose to work. Thank you.
> 
> Alex

[peterGo, 2013-06-08 11:20:35]

Alex,

Brad is way, way ahead of you!

On 2013/06/02 15:40:12, bradfitz wrote:
> where's the new test?

As Brad requested, I wrote tests for the patch and tested the tests. This
uncovered another bug. I've patched that bug too and I'm now writing and testing
tests for it.

Peter

On 2013/06/07 05:17:10, brainman wrote:
> Regardless what we decide, please add tests, so we all agree on how it is
> suppose to work. Thank you.
> 
> Alex

[bradfitz, 2013-06-26 15:28:29]

Q=wait

[gobot, 2013-12-20 16:11:03]

Replacing golang-dev with golang-codereviews.

[bradfitz, 2014-01-16 19:36:15]

Alex, do we care to fix this for Go 1.3?

[brainman, 2014-01-17 03:10:43]

On 2014/01/16 19:36:15, bradfitz wrote:
> Alex, do we care to fix this for Go 1.3?

But, 5610 is closed by https://codereview.appspot.com/10594043. And that is part
of Go 1.2 already.

Alex

[bradfitz, 2014-01-17 17:40:14]

R=close