Added client methods CreateDirectory() and RemoveDirectory()

[claudiofelber]

Hi Dave

I added two methods CreateDirectory() and RemoveDirectory() to your excellent SFTP library. Maybe you want to include my changes.

Regards,
Claudio

[davecheney]

Hi Claudio.

Thank you very much for your contribution.

Here are some things if like to see.

1. Rename CreateDirectory to Mkdir. While this is less clear, it is more in keeping with the os interface and if/when Go gets a FileSystem package, possibly something like kr/fs, it will be less churn.
2. Is it possible to merge Remove and RemoveDirectory? I can see how to do it without issuing a stat on the path first, or possibly after, if the remove fails (because path was a directory)

My goal is to keep the surface area of the api as small as possible. This gives us a better chance of being compatible with tr other FileSystem interfaces.

On 23 Nov 2013, at 10:43, the Felber notifications@github.com wrote:

Hi Dave

I added two methods CreateDirectory() and RemoveDirectory() to your excellent SFTP library. Maybe you want to include my changes.

Regards,
Claudio

You can merge this Pull Request by running

git pull https://github.com/claudiofelber/sftp master
Or view, comment on, or merge it at:

#4

Commit Summary

Added client methods CreateDirectory() and RemoveDirectory()
File Changes

M client.go (62)
Patch Links:

https://github.com/pkg/sftp/pull/4.patch
https://github.com/pkg/sftp/pull/4.diff

[claudiofelber]

Hi Dave

I understand your motivation to want to stay compatible with the other file system interfaces. Actually I myself tried to delete a directory with Remove() first. That's how I became aware that the directory functions had not yet been implemented. So...

CreateDirectory() → Mkdir()
No problem with that

RemoveDirectory() → Remove()
This one's a litte bit more complicated. There is no possibility to find out whether a path stands for a directory or a file without first communicating with the server. So there is some overhead involved (do we want that?). The two possibilities are (as pointed out by yourself):

1. Use SSH_FXP_LSTAT to check whether the path identifies a directory or a file, then act accordingly (SSH_FXP_REMOVE or SSH_FXP_RMDIR).
2. Start with SSH_FXP_REMOVE. If SSH_FX_FAILURE is returned, try again with SSH_FXP_RMDIR.

Method 1 is safe but slow. Method 2 is slow only when a directory is being removed. Trying SSH_FXP_RMDIR for every error returned by SSH_FXP_REMOVE is probably wrong because we do not want to return error codes from SSH_FXP_RMDIR when the path actually specified a file, so I recommend to go with SSH_FX_FAILURE only.

I have implemented and committed method 2. What do you think?

Regards,
Claudio

[davecheney]

Thanks, I agree with option 2.

I'll review the diff when I get back home this eventing

Thanks again

On 24 Nov 2013, at 0:07, Claudio Felber notifications@github.com wrote:

Hi Dave

I understand your motivation to want to stay compatible with the other file system interfaces. Actually I myself tried to delete a directory with Remove() first. That's how I became aware that the directory functions had not yet been implemented. So...

CreateDirectory() → Mkdir()
No problem with that

RemoveDirectory() → Remove()
This one's a litte bit more complicated. There is no possibility to find out whether a path stands for a directory or a file without first communicating with the server. So there is some overhead involved (do we want that?). The two possibilities are (as pointed out by yourself):

Use SSH_FXP_LSTAT to check whether the path identifies a directory or a file, then act accordingly (SSH_FXP_REMOVE or SSH_FXP_RMDIR).
Start with SSH_FXP_REMOVE. If SSH_FX_FAILURE is returned, try again with SSH_FXP_RMDIR.
Method 1 is safe but slow. Method 2 is slow only when a directory is being removed. Trying SSH_FXP_RMDIR for every error returned by SSH_FXP_REMOVE is probably wrong because we do not want to return error codes from SSH_FXP_RMDIR when the path actually specified a file, so I recommend to go with SSH_FX_FAILURE only.

I have implemented and committed method 2. What do you think?

Regards,
Claudio

—
Reply to this email directly or view it on GitHub.

[hochhaus]

Dave, do you have time to provide an update on this review?

[davecheney]

Oh my god, have I really left this open so long.

I am so sorry, this is unacceptable.

Next 24 hours, I promise.

On Sat, Mar 1, 2014 at 11:20 AM, hochhaus notifications@github.com wrote:

Dave, do you have time to provide an update on this review?

Reply to this email directly or view it on GitHubhttps://github.com//pull/4#issuecomment-36408583
.

[davecheney]

no need for the parens, this isn't C

[davecheney]

LGTM. One small nit.

I've given you contrib on the repo, please land this when you've addressed the review points.

Thanks again

Dave