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

Launch the command under a shell #15

Merged
merged 2 commits into from
Feb 20, 2014
Merged

Launch the command under a shell #15

merged 2 commits into from
Feb 20, 2014

Conversation

AlbertoGP
Copy link
Contributor

to get parsing of quoted arguments and
file name globbing, as in the old version that
used exec() instead of spawn()
Fixes #12 and maybe #11,
alternative to pull request #13

arguments and file name globbing, as in the old version that
used exec() instead of spawn()
Fixes #12 and maybe #11, alternative to pull request #13
@mattijs
Copy link
Owner

mattijs commented Feb 20, 2014

I don't really like this sollution, I'd rather rely on node solving this and not invoking particular shells. c2e4d82 fixes the escaping so this is a bonus.

On another note, I was think about supporting both spawn and exec for the command (through an option) and this could be another option. However, i'm still experimenting with this.

@mattijs mattijs closed this Feb 20, 2014
@mattijs
Copy link
Owner

mattijs commented Feb 20, 2014

@AlbertoGP Reconsidering this. Would this work on all platforms? It probably does since node does the same thing in its source.

@AlbertoGP
Copy link
Contributor Author

I didn't try it in Windows, but the idea was exactly to copy node: that's why I put a link to the original source code.

@mattijs
Copy link
Owner

mattijs commented Feb 20, 2014

I think we should try this, it looks good and gives us the best of both worlds (spawn and exec).

mattijs pushed a commit that referenced this pull request Feb 20, 2014
@mattijs mattijs merged commit ccf1552 into mattijs:master Feb 20, 2014
@AlbertoGP
Copy link
Contributor Author

Hi, I'm sorry to rear my ugly head again :neckbeard: but there is a problem with #16: you can either use #13 with #16 (no quoting/escaping of arguments), or #15 without #16 (for arguments with spaces etc.).

I copy here the test case from #13 (comment) with a correction: .source('test-dir/') instead of .source('test-dir') so that the contents of test-dir are copied inside test-dir-copy, not the directory itself.

Prepare the test files:

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

Then run this:

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);
});

It should create a directory called "test-dir-copy" with only the "text.txt" file in it, skipping ".git".
As it is now, #16 fails with error code 1.

@mattijs
Copy link
Owner

mattijs commented Feb 21, 2014

On the contrary, your input is very welcome. It's great that you are taking the time to make this library better.

I update master to bring back argument escaping, see 4eb52e9. Running it under a shell (#15) still has the advantage of argument expansion (over #13).

Bringing back escaping passes your example above. I opened a ticket (#17) to include tests for command output (not the execution), something I wanted for a while now. @AlbertoGP do you maybe have some commands I could include for testing (commands known to execute successfully)?

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.

Filters do not work
2 participants