https://codereview.appspot.com/52950043/diff/1/utils/file.go File utils/file.go (right):
https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode104 utils/file.go:104: func IsDirectory(path string) bool { In light of my comments on MkDirAllForUser, I think this should just be ditched. Some callers will care about symlinks, some won't.
https://codereview.appspot.com/52950043/diff/1/utils/file.go#newcode106 utils/file.go:106: if os.IsNotExist(err) { This is redundant, given the below check.
https://codereview.appspot.com/52950043/diff/1/utils/sudo.go File utils/sudo.go (right):
https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode63 utils/sudo.go:63: if IsDirectory(dir) { How about symlinks?
I think, rather than IsDirectory, use os.Lstat and os.IsNotExist. If there's a file in the path, then the function can just fail when we attempt to mkdir.
IOW, something like:
_, err := os.Lstat(dir) if os.IsNotExist(err) { if err := MkdirAllForUser(filepath.Dir(dir), perm); err != nil { return err } err = MkdirForUser(dir, perm) } if err != nil { return err } return ChownToUser(dir)
https://codereview.appspot.com/52950043/diff/1/utils/sudo.go#newcode79 utils/sudo.go:79: os.RemoveAll(topMostDir) This scares me. Can we please not remove things on failure? What if chown $HOME is a symlink, and chmod fails? Then we try to RemoveAll $HOME.
https://codereview.appspot.com/52950043/
« Back to merge proposal
https:/ /codereview. appspot. com/52950043/ diff/1/ utils/file. go
File utils/file.go (right):
https:/ /codereview. appspot. com/52950043/ diff/1/ utils/file. go#newcode104
utils/file.go:104: func IsDirectory(path string) bool {
In light of my comments on MkDirAllForUser, I think this should just be
ditched. Some callers will care about symlinks, some won't.
https:/ /codereview. appspot. com/52950043/ diff/1/ utils/file. go#newcode106
utils/file.go:106: if os.IsNotExist(err) {
This is redundant, given the below check.
https:/ /codereview. appspot. com/52950043/ diff/1/ utils/sudo. go
File utils/sudo.go (right):
https:/ /codereview. appspot. com/52950043/ diff/1/ utils/sudo. go#newcode63
utils/sudo.go:63: if IsDirectory(dir) {
How about symlinks?
I think, rather than IsDirectory, use os.Lstat and os.IsNotExist. If
there's a file in the path, then the function can just fail when we
attempt to mkdir.
IOW, something like:
_, err := os.Lstat(dir) (filepath. Dir(dir) , perm); err != nil {
if os.IsNotExist(err) {
if err := MkdirAllForUser
return err
}
err = MkdirForUser(dir, perm)
}
if err != nil {
return err
}
return ChownToUser(dir)
https:/ /codereview. appspot. com/52950043/ diff/1/ utils/sudo. go#newcode79 topMostDir)
utils/sudo.go:79: os.RemoveAll(
This scares me. Can we please not remove things on failure? What if
chown $HOME is a symlink, and chmod fails? Then we try to RemoveAll
$HOME.
https:/ /codereview. appspot. com/52950043/