Implement preserve #81
Conversation
| @@ -68,20 +68,16 @@ func ParseResponse(reader io.Reader, writer io.Writer) (*FileInfos, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| message, err = bufferedReader.ReadString('\n') | |||
| if err == io.EOF { | |||
| if bufferedReader.Buffered() == 0 { | |||
There was a problem hiding this comment.
I am a bit lost here what this is supposed to mean. Buffered returns the number of bytes remaining in the read buffer, but as far as I see the buffer should already be empty since we read all its information for parsing the time message. We do not make any Acks in between so we do not expect another message back from the remote. My suggestion is to add more comments to this function to make it more clear why certain checks are necessary and what part of the protocol we are considering at each line in the code.
There was a problem hiding this comment.
wish is a popular ssh custom server. Their implementation allows for sending out the time and permissions in the same message. There is an argument that it's not following the protocol, but being so popular, I thought it would be nice to accommodate and take into consideration the instances in which someone could make a custom ssh implementation.
If you believe we shouldn't accommodate that kind of custom implementation, then I will remove that piece of logic to keep it clean.
There was a problem hiding this comment.
I see, I think you make a good argument for keeping it, but make sure that it does not interfere with implementations that do follow the protocol.
|
Figured out the paths within the Docker and now I'm checking against the file that we're reading. Now the test is more relevant to testing that the feature works. |
|
I see you already made some changes, are those ready for review? If not, could you notify me when they are. Thanks! Kind regards, Bram |
They are ready to review. I think this is close to final form in my opinion. Only thing I would add is perhaps some more testing, but I haven't thought of any new ones. |
| @@ -68,20 +68,16 @@ func ParseResponse(reader io.Reader, writer io.Writer) (*FileInfos, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| message, err = bufferedReader.ReadString('\n') | |||
| if err == io.EOF { | |||
| if bufferedReader.Buffered() == 0 { | |||
There was a problem hiding this comment.
I see, I think you make a good argument for keeping it, but make sure that it does not interfere with implementations that do follow the protocol.
|
This is all I have for now. Next I plan to look into implementing the directory functionality, but I will be off for 2 weeks starting next week |
|
we should be able to also close this one #63 |
|
@bramvdbogaerde when do you plan to merge this pull request? |
|
Merged! Thanks a lot. Note: I squashed your commits into a single one to keep commit history clean in the master branch. |
protocol.go had a bug. Checking if the buffer had more information was not implemented correctly, but it wasn't affecting current functionality. Now that -p is added, I could see the bug. I replaced the check for io.eof with a check if there is any data left buffered.
I added in the configurer the option to have the preserve field set to true through the Preserve method.
To update the signature with a return value, I create a new method linked to client. If there is an error and the user tries to use the FileInfos object, then they will get a null pointer. I think that makes it more obvious that they didn't check the error.
I also added a test for the -p functionality, but I think I might need more to cover it better. I'm not sure exactly how to test that the FileInfo has the right time in atime and mtime, as the file is create in the docker container by cloning the repo, so I don't know if the file information is preserved.
I'm open to suggestions and comments 😄.
I think the tests are lacking and I plan to write more before we merge.