overwrite:true removes dirs if they exist

dschmidt commented 8 years ago

Hey,

cpr removes dirs if they already exist, if overwrite:true is specified in options (which I assumed was just the same as -f).

So for a test.js which contains this:

var cpr = require('cpr');

cpr('build/', 'deployments/', {
    deleteFirst: false,
    overwrite: true,
    confirm: false
}, function(err, files) {
    if(err) {
        console.error('ERROR', err);
        return;
    }

    console.log('SUCCESS');
});

I see this behavior:

% find build             
build
build/assets
build/assets/2

% find deployments
deployments
deployments/assets
deployments/assets/1

% node test.js
SUCCESS

% find deployments/         
deployments/
deployments/assets
deployments/assets/2

whereas cp -R -f does the following:

% find build 
build
build/assets
build/assets/2

% find deployments 
deployments
deployments/assets
deployments/assets/1

% cp -R build/* deployments 
% find deployments
deployments
deployments/assets
deployments/assets/1
deployments/assets/2

and without overwrite: true it fails on the already existing assets/ dir (and possibly other already existing files). So right now there seems to be no way to merge to directory trees with cpr. ... or am I overlooking anything?

AFAICT this is the code causing this behavior: https://github.com/davglass/cpr/blob/master/lib/index.js#L113

Why was that code added in the first place?

BR, Domme

davglass commented 8 years ago

Sorry, I've been away with some family issues. I'll have a patch that you can test shortly.

dschmidt commented 8 years ago

Don't worry, I'm glad you agree it's a bug :+1:

I just wasn't sure it wasn't desired behavior, or how you wanted it to be fixed, thus I did not send a PR directly. If you fix, even better :beers:

davglass commented 8 years ago

I have a fix locally, I'm just writing the tests for it now. I'll push it to master without publishing so you can test it to confirm that it behaves as you wanted.

davglass commented 8 years ago

GH closed it as I pushed up the fix. Can you try master and see if that works as you expect it to? If it does then I'll publish this as 1.0.0.

dschmidt commented 8 years ago

Nice! Will test when back from dinner

dschmidt commented 8 years ago

Works, great!

davglass commented 8 years ago

Cool, published as cpr@1.0.0.

dschmidt commented 8 years ago

Yay, so is ember-cli-deploy-cp@0.3.0 ;-)

Thanks for taking care of this so quickly :beer:

jbonhag commented 8 years ago

:+1: Thanks for the fix

davglass commented 8 years ago

Always glad to help! :beers:

davglass / cpr

overwrite:true removes dirs if they exist #18