Optimize SIZE command usage for UploadFile NoCheck AND OverWrite (Issue #858)

[FanDjango]

Please see the tail end of Issue #858

After adding the needed code, I realised that a refactor of UploadFileInternal would optimise the use of the SIZE command not only for the case when NoCheck is requested, but also for the case when OverWrite is active.

Here is what it now looks like if Overwrite is requested (only one SIZE command, previously there were two!):

>         UploadFile("D:\temp\test1", "/home/mike/test1", Overwrite, False, None)
>         FileExists("/home/mike/test1")
Command:  SIZE /home/mike/test1
Response: 550 /home/mike/test1: No such file or directory
>         OpenWrite("/home/mike/test1", Binary)
>         OpenActiveDataStream(PORT, "STOR /home/mike/test1", 0)
Command:  PORT 192,168,1,147,31,146
Response: 200 PORT command successful
Command:  STOR /home/mike/test1
Response: 150 Opening BINARY mode data connection for /home/mike/test1
Response: 226 Transfer complete

And for NoCheck (no SIZE command, previously there was one!):

>         UploadFile("D:\temp\test1", "/home/mike/test1", NoCheck, False, None)
>         OpenWrite("/home/mike/test1", Binary)
Command:  TYPE I
Response: 200 Type set to I
>         OpenActiveDataStream(PORT, "STOR /home/mike/test1", 0)
Command:  PORT 192,168,1,147,31,146
Response: 200 PORT command successful
Command:  STOR /home/mike/test1
Response: 150 Opening BINARY mode data connection for /home/mike/test1
Response: 226 Transfer complete

[robinrodricks]

This looks excellent, However the idea of assuming the remote file length seems suspect to me. Can you elaborate, I mean, it won't mess up any of the upload scenarios, will it?

[FanDjango]

See the above conversation. And ASYNC is still missing, of course.

[robinrodricks]

Ok, you seem to have thought this through. Have you tested the case where we are trying to upload a file but:

Overwrite:

1. Case 1 - The remote file does not exist (full upload)
2. Case 2 - The remote file does exist (so it should overwrite the file, preferably using the delete command)

AddToEnd:

1. Case 1 - The remote file does not exist (full upload)
2. Case 2 - The remote file does exist (so it should add the remaining parts)

Apologies if this is too much over checking on my part, but since we don't have any decent tests for these cases (we probably should create them), I need to check manually.

[FanDjango]

Apologies if this is too much over checking on my part, but since we don't have any decent tests for these cases (we probably should create them), I need to check manually.

No need to apologize.

Overwrite, AddToEnd

Doing some more testing.

[FanDjango]

I actually did find a bug, thanks to doing some testing on AddToEnd. Come on - own up: You tested it already!

However the idea of assuming the remote file length seems suspect to me.

Always trust your instincts: localFileLen = fileData.Length; looks fishy.

This is the point under discussion: Is it harmful to do this SetLength in this case, or not?

Hurts for AddToEnd, it turns out. I goofed.

Edit: What I am trying to achieve is: No unneeded SIZE commands for OverWrite and NoCheck. All others should keep their behaviour.

[robinrodricks]

@FanDjango thanks for the testing, let me know when you are done and I should merge.

[FanDjango]

I am going to rebase and bring all the changes together, then I will go over the changes in detail and also compare sync and async.

[FanDjango]

Ready now.

[robinrodricks]

Its just showing 1 line changed, variable value from 0 to -1, is this it?

[FanDjango]

Yes. Such irony, 😃

[robinrodricks]

Released!

https://www.nuget.org/packages/FluentFTP/41.0.0