various improvements / potential fix for issue #8 #191
Conversation
Make runLs return the recommended format in stubs, even if it does not contain all information Removed unused parameter
Fixed runLs tests on Windows
…s to 0 on non *nix systems
server_test.go
Outdated
|
|
||
| const ( | ||
| TYPE_DIRECTORY = "d" | ||
| TYPE_FILE = "[^d]" |
There was a problem hiding this comment.
don't use ALL_CAPS in Go names; use CamelCase
server_test.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| TYPE_DIRECTORY = "d" |
There was a problem hiding this comment.
don't use ALL_CAPS in Go names; use CamelCase
request-server.go
Outdated
| } // all paths are absolute | ||
|
|
||
| cleaned_path := filepath.Clean(path) | ||
| cleaned_path := filepath.ToSlash(filepath.Clean(path)) |
There was a problem hiding this comment.
don't use underscores in Go names; var cleaned_path should be cleanedPath
There was a problem hiding this comment.
don't create a new variable, overwrite path so that the unclean version cannot be used by mistake.
|
Reviewing... |
request-server.go
Outdated
| if !filepath.IsAbs(path) { | ||
| path = "/" + path | ||
| // prevent double slash (e.g. on windows, / paths are not absolute) | ||
| path = "/" + strings.TrimPrefix(path, "/") |
There was a problem hiding this comment.
filepath.Clean may be more appropriate here
request-server.go
Outdated
| } // all paths are absolute | ||
|
|
||
| cleaned_path := filepath.Clean(path) | ||
| cleaned_path := filepath.ToSlash(filepath.Clean(path)) |
There was a problem hiding this comment.
don't create a new variable, overwrite path so that the unclean version cannot be used by mistake.
request.go
Outdated
| // NewRequest creates a new Request object. | ||
| func NewRequest(method, path string) Request { | ||
| request := Request{Method: method, Filepath: filepath.Clean(path)} | ||
| request := Request{Method: method, Filepath: filepath.ToSlash(filepath.Clean(path))} |
There was a problem hiding this comment.
This filepath.ToSlash(filepath.Clean() logic appears too many times.
Please break it into a helper function and add a unit test for all the appropriate cases; "/", "//", "\", "/a/", "\a/", etc
request.go
Outdated
| func NewRequest(method, path string) Request { | ||
| request := Request{Method: method, Filepath: filepath.Clean(path)} | ||
| request := Request{Method: method, Filepath: filepath.ToSlash(filepath.Clean(path))} | ||
| request.packets = make(chan packet_data, sftpServerWorkerCount) |
There was a problem hiding this comment.
This is the line that is in conflict below. The conflict is due to my making sftpServerWorkerCount public (capitalizing it). It is easy to resolve.
|
|
||
| // mod time (len 12, e.g. Aug 9 19:46) | ||
| got = result[43:55] | ||
| layout := "Jan 2 15:04" |
There was a problem hiding this comment.
Date stat display flips over to the year at a certain point (6 months I think). The LICENSE file on my system did this and this test failed. The format can be "Jan 2 15:04" or "Jan 2 2006" depending on this. One fix would be to check both and only error if neither parses.
Splitted cleanPath into cleanPacketPath and cleanPath for better handling of slashes in file paths Added test for cleanPath func Removed code duplication => filepath.ToSlash(filepath.Clean(...)) => cleanPath(...) Fixed tests for runLs to match year or time Renamed constants to fit hound rules
|
|
||
| const ( | ||
| sftpServerWorkerCount = 8 | ||
| SftpServerWorkerCount = 8 |
There was a problem hiding this comment.
exported const SftpServerWorkerCount should have comment (or a comment on this block) or be unexported
|
|
||
| const ( | ||
| sftpServerWorkerCount = 8 | ||
| SftpServerWorkerCount = 8 |
There was a problem hiding this comment.
exported const SftpServerWorkerCount should have comment (or a comment on this block) or be unexported
|
All mentioned things should be fixed now... |
|
@davecheney What do you think? There are a few extraneous things but they are all improvements, so I'm OK with them. I'm ready to merge it unless you have more you'd like to see. |
4 participants
On windows systems the directory separator is a backslash (). This leads to misbehavior of the library in some cases. I tried to fix this issues without changing to much code.
According to RFC https://tools.ietf.org/html/draft-ietf-secsh-filexfer-02, SFTP always uses slashes.
Other improvements: