Added special character handling when copying directories from UNIX to Windows #178
Conversation
There was a problem hiding this comment.
I added some remarks.
It would also be great if you could add a unit test for ScpClient.Download(string, DirectoryInfo), but - too be honest - that method is not easy to test. You may get inspiration from the ScpClientTest_Upload_FileInfoAndPath_Success test (or it may scare you away forever).
| /// Enabling this will remove the following special characters from filenames when copying whole directories: | ||
| /// " < > | : * ? \ / | ||
| /// </summary> | ||
| public bool UnixToWindows { get; set; } |
There was a problem hiding this comment.
Do we need this extra property, or can we always enable this (or only when running on Windows)?
In that case, we should add a remark for this to ScpClient.Download(string, DirectoryInfo).
There was a problem hiding this comment.
No, not particularly. It was just my way of making sure any users knew that the filenames would be changed because they had to specifically request it, with a log and summary note it shouldn't be necessary.
I'd be torn on whether to always apply the change. Personally I don't see why there'd ever be a good reason to keep special characters that can break things in cross-system setups. But then maintaining original file names where possible might be preferred for some people.
Should be possible to just apply for Windows environments.
Also that test file looks...interesting, I'll take a look in the next few days when I get a chance.
There was a problem hiding this comment.
If we'd be downloading a single file, I'd advise against "fixing" the name and would instruct the user to specify a destination file without these special characters.
In this case,we're recursively downloading a bunch of files, so either we allow an extensible file name mapping mechanism or ensure it "just works".
Why should we only apply it for Windows? I know it's very unlikely to have a character that is valid on some OS but not on unix,, but for consistency we may just as well always apply it.
Note that there's small risk that - by replacing or removing invalid characters - two distinct files on the remote server may end up having the same file name locally.
|
|
||
| private static string SanitizeUnixFilenameForWindows(string fileName) | ||
| { | ||
| fileName = Path.GetInvalidFileNameChars().Aggregate(fileName, (current, c) => current.Replace(c.ToString(), string.Empty)); |
There was a problem hiding this comment.
I'm not sure If I should seek treatment for this, but I'm beginning to get aversion to Linq.
Also please log a message using DiagnosticAbstraction.Log(string) when at least one character needs to be "sanitized".
There was a problem hiding this comment.
All hail the mighty Linq!
I think that was a resharper replacement, I'm just used to seeing Linq everywhere. Imagine that will go when I add the logging.
#177
Changes I'm using locally. Not sure about your preferred style, so I haven't added it to constructors or anything for now.