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

Add code to escape non-escaped spaces in arg paths #47

Merged
merged 2 commits into from Aug 29, 2016
Merged

Add code to escape non-escaped spaces in arg paths #47

merged 2 commits into from Aug 29, 2016

Conversation

kohlmannj
Copy link
Contributor

Based on jedrichards/grunt-rsync#51, I've added code to rsyncwrapper which automatically escapes any space characters in file paths within rsyncwrapper's arguments.

I've also added some rudimentary tests to verify that:

  • Copying from a single source path with space characters does not error, etc.
  • Copying from a single source path with spaces to a destination with spaces does not error, etc.
  • Passing in source and destination strings with already-escaped space character results in an identical rsync command

This is only partial test coverage, but there is now at least one test failure when removing the new space-escaping code, so I believe it's a reasonable first pass.

Based on jedrichards/grunt-rsync#51, I've added code to rsyncwrapper which automatically escapes any space characters in file paths within rsyncwrapper's arguments.

I've also added some rudimentary tests to verify that:
- Copying from a single source path with space characters does not error, etc.
- Copying from a single source path with spaces *to* a destination with spaces does not error, etc.
- Passing in source and destination strings with already-escaped space character results in an identical rsync command

This is only partial test coverage, but there is now at least one test failure when removing the new space-escaping code, so I believe it's a reasonable first pass.
@jedrichards
Copy link
Owner

jedrichards commented Aug 25, 2016

This looks great, definitely going to merge this - thanks!

Might need to make a few minor changes to your test process though - specifically that your new tests aren't cleaning up after themselves so could lead to some false positives I think?

So the following changes are needed I think:

  • Add your new tmp with spaces to gitignore and uncommit it from the repo
  • Add tmp with spaces to the clean Grunt task
  • Add tmp with spaces to shell:tmpdir Grunt task

That should do it?

Apologies that the tests aren't very cleanly implemented and using old libraries, but I haven't got round to updating the infrastructure since I built the lib in 2012!

@kohlmannj
Copy link
Contributor Author

Ah, got it, sorry for the delayed reply. Will look into making those changes!

@kohlmannj
Copy link
Contributor Author

Alright, test-oriented fixes are in.

@jedrichards
Copy link
Owner

👌

Thanks! Will cut a new major release for this...

@jedrichards jedrichards merged commit 085ee74 into jedrichards:master Aug 29, 2016
@kohlmannj
Copy link
Contributor Author

kohlmannj commented Aug 29, 2016

@jedrichards One favor: could you also please cut a release for grunt-rsync? That way I can specify a newer version of that package as a dependency on a few tools. Thank you!

@jedrichards
Copy link
Owner

On it :)

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