Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape spaces in file names before calling ls in FTP.exists #64

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ryanmcfall
Copy link

This fixes a bug in which filenames containing spaces are uploaded to the server every time that a sync operation is initiated. The bug occurs because, at least on the ftp server that I am using, the ls command invokes the callback with a data argument whose length property is zero, therefore indicating to ftp-sync that the file does not exist on the server.

This should potentially be considered a bug in jsftp rather than ftp-sync, so perhaps it should be reported there instead; I'll leave that decision to you.

@timburgess
Copy link
Owner

Thanks for this and awesome to see contributions. With changes in core.js, I want to review and test these pretty thoroughly before bringing them into the codebase.

@ryanmcfall
Copy link
Author

Tim:

Note that I was unaware that committing to the forked version of the repository after submitting a pull request would result in those changes being part of the pull request (I thought it was static).

So the original pull request is just the commit with SHA 1feb3d0, which only changes FTP.js

The subsequent commits eliminate the checking of file sizes on the server altogether, by recording (in .ftpsync_settings) the local time when a sync was last completed, and then comparing file modification dates to that date in order to determine what needs to be uploaded.

This change seems to be working for me, but more eyes on it and additional testing would certainly be good.

@timburgess
Copy link
Owner

OK. I have cherry-picked 1feb3d0 into the master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants