Code review comment for lp:~thumper/juju-core/juju-ssh-dir-as-user

On 2014/01/16 03:09:31, axw wrote:
> 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.

LGTM pending changes

https://codereview.appspot.com/52950043/