Make tilde expansion work on Windows #27
Conversation
gabriel-samfira
commented
Aug 27, 2014
gabriel-samfira
commented
- use Home() to return users home on Windows as well
- only match strings starting with tilde
- fix tests on Windows
gabriel-samfira
force-pushed
the
testing
branch
3 times, most recently
from
4a2c3a9
to
b1f9629
Compare
| ) | ||
|
|
||
| // UserHomeDir returns the home directory for the specified user, or the | ||
| // home directory for the current user if the specified user is empty. | ||
| func UserHomeDir(userName string) (homeDir string, err error) { | ||
| var u *user.User | ||
| var u string | ||
| if userName == "" { | ||
| // TODO (wallyworld) - fix tests on Windows |
There was a problem hiding this comment.
This whole comment seems out of place now, since the code part it was refering to just dissapeared, could you please take a look at it and make sure its still valid? it does not seem to me that we are doing anything out of the ordinary, moreover I dont see a UserHomeDir defined in windows speciffic code, also the code below could be outside the else (but that is just me being picky)
There was a problem hiding this comment.
UserHomeDir() is not really platform specific. We need to avoid user.Current() on windows as well now, so we can patch it during tests. That's why I removed that bit. The comment is stil relevant as we still use Home() instead of user.Current().Home. I can remove the TODO, but as of yet, the tests are still not fully working on Windows. :)
The getHomeDir() function does have windows specific stuff to get the home dir of a different user. Unfortunately, Go cannot get the home directory of a different user using user.Lookup(). So I had to get it from the registry.
There was a problem hiding this comment.
the else can be removed without a problem at this point.
gabriel-samfira
force-pushed
the
testing
branch
from
b1f9629
to
02edd30
Compare
| @@ -113,6 +107,8 @@ func AtomicWriteFileAndChange(filename string, contents []byte, change func(*os. | |||
| defer func() { | |||
| if err != nil { | |||
| // Don't leave the temp file lying around on error. | |||
| // Close the file before removing | |||
There was a problem hiding this comment.
If I get this right, this is due to the limitations of windows regarding file handlers, please add a comment to prevent anyone to ever remove this.
There was a problem hiding this comment.
quite right. Done
gabriel-samfira
force-pushed
the
testing
branch
2 times, most recently
from
4ea8835
to
befaec0
Compare
| @@ -103,7 +106,7 @@ var atomicWriteFileTests = []struct { | |||
| summary: "atomic file write and change", | |||
| change: func(filename string, contents []byte) error { | |||
| chmodChange := func(f *os.File) error { | |||
| return f.Chmod(0700) | |||
| return os.Chmod(f.Name(), 0700) | |||
There was a problem hiding this comment.
I dont understand the reason behind this change.
There was a problem hiding this comment.
FileMod.Chmod() is not implemented on Windows unfortunately. os.Chmod() is. Will comment :)
gabriel-samfira
force-pushed
the
testing
branch
2 times, most recently
from
1cdb347
to
b6e9e02
Compare
|
|
||
| func getHomeFromRegistry(sid string) (string, error) { | ||
| var h syscall.Handle | ||
| keyPath := fmt.Sprintf("Software\\Microsoft\\Windows NT\\CurrentVersion\\ProfileList\\%s", sid) |
There was a problem hiding this comment.
This seems rather hardcodded, this is always NT? I do believe that since XP every windows is NT based but I am not sure so better ask
There was a problem hiding this comment.
if its old enough not to have this, we probably should not support it :). The agent will probably only support windows 2008 and above.
There was a problem hiding this comment.
A small comment for non windows folks would be nice ;)
gabriel-samfira
force-pushed
the
testing
branch
3 times, most recently
from
f95b2ef
to
fb2aa54
Compare
|
Ok, with the new changes LGTM and now you need to get approval to my lgtm from @davecheney |
| @@ -13,6 +15,8 @@ const ( | |||
| movefile_write_through = 0x8 | |||
| ) | |||
|
|
|||
| var noSuchUser = "No mapping between account names and security IDs was done." | |||
There was a problem hiding this comment.
Looks different to our usual messages. Maybe better "no mapping between accound names and security id".
There was a problem hiding this comment.
this error is returned by the operating system. I just match the error here.
|
LGTM, only minor comments. |
gabriel-samfira
force-pushed
the
testing
branch
from
fb2aa54
to
5108dcf
Compare
gabriel-samfira
force-pushed
the
testing
branch
from
5108dcf
to
7b9bf21
Compare
|
LGTM, too |
|
|
Make tilde expansion work on Windows
