sftp server implementation #46
Conversation
…d the ATTRs field, it has a name in it.
|
@marksheahan this looks awesome! Thank you for this. I'll have a read through the diff later today, could you please fix the small build error. Ta |
| @@ -312,6 +336,25 @@ func (c *Client) ReadLink(p string) (string, error) { | |||
| } | |||
| } | |||
|
|
|||
| // Symlink creates a symbolic link at 'newname', pointing at target 'oldname' | |||
There was a problem hiding this comment.
This should be called Link, for compatibility with the os package, http://godoc.org/os#Link
You can do this in a followup if you like.
There was a problem hiding this comment.
os.Link() creates a hard link, not a soft link. This name was chosen for consistency with os.Symlink:
https://golang.org/pkg/os/#Symlink
There was a problem hiding this comment.
My apologies, this was my mistake
On 8 Sep 2015, at 14:11, Mark Sheahan notifications@github.com wrote:
In client.go:
@@ -312,6 +336,25 @@ func (c *Client) ReadLink(p string) (string, error) {
}
}+// Symlink creates a symbolic link at 'newname', pointing at target 'oldname'
os.Link() creates a hard link, not a soft link. This name was chosen for consistency with os.Symlink:
https://golang.org/pkg/os/#Symlink—
Reply to this email directly or view it on GitHub.
|
Thanks. A bunch more comments and nits. To get this landed as quickly as possible I want to encourage you to pull out anything that is controversial and handle it as a followup. |
|
Working on deleting the user/ dir, and the other items. Thanks for the quick review turnaround! |
| .*.swo | ||
| .*.swp | ||
|
|
||
| testdata/ |
There was a problem hiding this comment.
the go tool ignores testdata, please remove this line
if something is writing into testdata/ let's fix that in a followup
|
LGTM. I've sent you an invitation to collaborate on the sftp repository, when you're happy, please submit this. Let's tackle the other changes that I asked to be deferred in follow up PRs. Thanks again for this big improvement, this was a large hole in the package. |
|
Just got nervous before committing, wrote a few more tests and found a bug. I'll add a recursive put and get test; once those pass I'll be happy with it. |
|
Cool. We can always land more pull requests after this one, you don't need On Wed, Sep 9, 2015 at 10:11 AM, Mark Sheahan notifications@github.com
|
| .*.swo | ||
| .*.swp | ||
|
|
||
| server_standalone/server_standalone |
There was a problem hiding this comment.
you can probably delete this as well, this would only happen if you did go build in the server_standalone directory
There was a problem hiding this comment.
Not that .gitignore files need to be filled with thousands of lines of junk. Assumedly including 'server_standalone/server_standalone' is useful during development?
There was a problem hiding this comment.
Yes, I did my integration tests that way. I probably could have used go install but it's just habit...
There was a problem hiding this comment.
It's your call, doesn't really matter much each way, i'm just finding
things to nit pick while you land your change.
On Wed, Sep 9, 2015 at 3:06 PM, Mark Sheahan notifications@github.com
wrote:
In .gitignore #46 (comment):
@@ -0,0 +1,5 @@
+..swo
+..swp
+
+server_standalone/server_standaloneYes, I did my integration tests that way. I probably could have used go
install but it's just habit...—
Reply to this email directly or view it on GitHub
https://github.com/pkg/sftp/pull/46/files#r39007718.
Mostly tested through integration tests, by using the client package and comparing behavior to the openssh sftp server.
Part of the server implementation of readdir involves implementing unix group lookup; gid -> groupname conversion for 'ls -l' style output. This is the user/ directory, which is os/user with this patchset applied: https://codereview.appspot.com/13454043