fly sftp get /path/to/base.txt should create base.txt locally
#2280
Conversation
It was creating /path/to/base.txt locally and it is counter-intuitive. In fact, sftp shell's get command used the remote file's basename even before this change.
| @@ -509,6 +516,10 @@ func (sc *sftpContext) get(args ...string) error { | |||
| } else { | |||
| sc.out("wrote %d bytes", bytes) | |||
| } | |||
| err = f.Sync() | |||
| if err != nil { | |||
| sc.out("failed to sync %s: %s", localFile, err) | |||
There was a problem hiding this comment.
Can we give the user more information about what happened and what to do about it?
There was a problem hiding this comment.
The err part should have what happened. E.g. "no space left on device" if this downloads 2GB file and the local host doesn't have the space.
"What to do about" is difficult. This error is not coming from Fly's platform. So we don't have much concrete suggestions.
There was a problem hiding this comment.
All good.
I'd say these error messages are very normal. We tell the user "wrote %d bytes" which is technically true even if it can seem weird if the sync to disk then goes wrong. I was just wondering if we could very easily do better than "normal," while you've got a PR going already. I wonder this about most of our error messages. ;)
|
I would guess not in this case, but is this change going to break anything for existing users? |
Yes and some might depend this behavior. That said, at least three people were confused about and |
|
I agree with your previous answer for various reasons; this was a belt-and-braces check. |
It was creating /path/to/base.txt locally and it is counter-intuitive.
In fact, sftp shell's get command used the remote file's basename even before this change.