Handle Pathname in download/upload

[Tietew]

Net::SFTP::Operations::Download and Upload try to know local/remote path is an IO or not using respond_to?(:write).

But Pathname#write exists.
Therefore, passing Pathname object to downloading/uploading causes confusing exception.

This PR convert pathnames to string using #to_path.

When downloading

Net::SFTP.start(*credentials) do |sftp|
  sftp.download!('/path/to/remote', Pathname.new('/path/to/local'))
end

fails with: (message depends on file content)

.../net-sftp-2.1.2/lib/net/sftp/operations/download.rb:339:in `write': "\x80" from ASCII-8BIT to UTF-8 (Encoding::UndefinedConversionError)

Because Pathname#write opens file without binary flag (external encoding is UTF-8), and IO#write trys to convert data to UTF-8.

When uploading

Net::SFTP.start(*credentials) do |sftp|
  sftp.upload!(Pathname.new('/path/to/local'), '/path/to/remote')
end

fails with:

.../net-sftp-2.1.2/lib/net/sftp/operations/upload.rb:360:in `write_next_chunk': undefined method `pos' for #<Pathname:***> (NoMethodError)

[Tietew]

#33 seems to be same issue.

[mfazekas]

@Tietew thanks for the PR! I'm a bit concerned that adding a couple more respond_to? could introduce more issues. Like File objects do have .to_path methods, so passing File to those methods will reopen the files.

A less intrusive change would be detecting Pathname-s and error-ing saying that Pathname-s are not supported, then it'll be the user's responsibility to call .to_path on pathnames.

What do you think?!

[Tietew]

@mfazekas OMG! Thanks to pointing out.

Another plan: check for #open and call:

index 8e7275e..fe2c317 100644
--- a/lib/net/sftp/operations/download.rb
+++ b/lib/net/sftp/operations/download.rb
@@ -311,7 +311,14 @@ module Net; module SFTP; module Operations
         raise StatusException.new(response, "open #{entry.remote}") unless resp
onse.ok?

         entry.handle = response[:handle]
-        entry.sink = entry.local.respond_to?(:write) ? entry.local : ::File.open(entry.local, "wb")
+        entry.sink =
+          if entry.local.respond_to?(:open)
+            entry.local.open("wb")
+          elsif entry.local.respond_to?(:write)
+            entry.local
+          else
+            ::File.open(entry.local, "wb")
+          end
         entry.offset = 0

         download_next_chunk(entry)

cf. https://ruby-doc.org/stdlib-2.7.0/libdoc/pathname/rdoc/Pathname.html#method-i-open

Choices:

1. raise "unexpected Pathname" if local.is_a?(Pathname)
2. local = local.to_path if local.is_a?(Pathname)
3. See above.

[Tietew]

I found Tempfile is open-able...
https://ruby-doc.org/stdlib-2.7.0/libdoc/tempfile/rdoc/Tempfile.html#method-i-open

Following code will crash:

tmp = Tempfile.new
sftp.download!('/path/to/remote', tmp)
# => ArgumentError: wrong number of arguments (given 1, expected 0)

Is No.2 the best solution?

[Tietew]

I asked ruby-list ML this issue.
http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-list/50874

[mfazekas]

@Tietew sorry for the late answer, yes let's go with 2.) that sound the least intrusive.
local = local.to_path if local.is_a?(Pathname)

[til]

Hi 👋🏾 ! I recently ran into an error when passing a Pathname to upload and found this open PR. I've locally tested to rebase it on the latest master and change the implementation to Option 2.) and all tests passed. @Tietew would you mind if I create a new PR based on yours, or do you maybe want to update yours yourself?

net-sftp is cool, Pathname is cool, it would be even cooler if both worked together 😄 .