|
|
|
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.
Patch Set 1 #Patch Set 2 : diff -r c0c4cf119f9b https://code.google.com/p/go/ #Patch Set 3 : diff -r c0c4cf119f9b https://code.google.com/p/go/ #Patch Set 4 : diff -r c0c4cf119f9b https://code.google.com/p/go/ #Total messages: 13
|
peterGo
|
12 years, 7 months ago (2013年06月02日 15:31:53 UTC) #1 |
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
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... > . > > >
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.
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.
Regardless what we decide, please add tests, so we all agree on how it is suppose to work. Thank you. Alex
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
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
Q=wait
Replacing golang-dev with golang-codereviews.
Alex, do we care to fix this for Go 1.3?
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