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