Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Fix for option values when using spawn instead of exec #13

Closed
wants to merge 1 commit into from
Closed

Fix for option values when using spawn instead of exec #13

wants to merge 1 commit into from

Conversation

AlbertoGP
Copy link
Contributor

There were two issues:

  • option values must not be quoted when using spawn
  • values for short options must have their own place
    in the args array, instead of being joined to the option
    with a space

This is the reason behind issue #12 and probably #11

There were two issues:
- option values must not be quoted when using spawn
- values for short options must have their own place
  in the args array, instead of being joined to the option
  with a space
@mattijs
Copy link
Owner

mattijs commented Feb 20, 2014

I agree on not quoting the arguments. I did some testing and it does seem to work, even though all examples in the rsync man pages are quoted.

I do think it can be solved a little simpler and less invasive. I propose a little tweak to the buildOption function making escaping optional and disabled by default. See c2e4d82 for implementation.

As for the short options, in this PR they are untouched and still grouped. It might be that I don't quite understand the second point but I think it does not relate to the problem. Without touching the short options escaping can still be disabled, see my commit again.

@AlbertoGP
Copy link
Contributor Author

The second point is about short options with arguments, for instance rsync -rav -f '- .git' source/ destination instead of rsync -rav --filter='- .git' source/ destination.

If you look carefully you'll see that in that case, instead of joining the short option and its value with a space into -f "- .git", I first append -f and then separately - .git. That would be line 746:
https://github.com/mattijs/node-rsync/pull/13/files#diff-cc09a83e4da70d654ed5c79ebe1c19b8R746

You can try it out with the following:

var Rsync = require('./rsync');// local rsync.js file to make it easier to swap versions

cmd = new Rsync()
    .flags('rav')
    .set('f', '- .git')
    .source('test-dir')
    .destination('test-dir-copy');

cmd.execute(function(error, code, cmd) {
    console.log('error: ' + error);
    console.log('code: ' + code);
    console.log('All done executing', cmd);
});

To prepare the test files:

mkdir test-dir
echo git >test-dir/.git
echo 'some text' >test-dir/text.txt

This fails with your fix:
https://github.com/mattijs/node-rsync/blob/option-values/rsync.js
and works with both #13 and #15

@mattijs
Copy link
Owner

mattijs commented Feb 20, 2014

I see, but wouldn't that be same with my solution? (the new one) I simply glue the short option together with the value and append the combination to the arguments (without escaping in the new version). This would result in the same arguments -f - .git right?

@AlbertoGP
Copy link
Contributor Author

By "append" I mean "push", that is, ["-f", "- .git"] instead of ["-f \"- .git\""].

@mattijs
Copy link
Owner

mattijs commented Feb 21, 2014

Now I get what you mean with "must have their own place in the args array". Not the option args array but the eventual args array.

I'm closing this PR because #15 was merged into master using that solution to fix this problem.

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

Successfully merging this pull request may close these issues.

None yet

2 participants