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

Fix remove not working properly #336

Merged
merged 10 commits into from
Mar 6, 2020
Merged
4 changes: 2 additions & 2 deletions bin/gh-pages.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ function main(args) {
'-v, --remove <pattern>',
'Remove files that match the given pattern ' +
'(ignored if used together with --add).',
ghpages.defaults.only
ghpages.defaults.remove
)
.option('-n, --no-push', 'Commit only (with no push)')
.option(
Expand Down Expand Up @@ -96,7 +96,7 @@ function main(args) {
depth: program.depth,
dotfiles: !!program.dotfiles,
add: !!program.add,
only: program.remove,
remove: program.remove,
remote: program.remote,
push: !!program.push,
history: !!program.history,
Expand Down
16 changes: 11 additions & 5 deletions lib/git.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ function Git(cwd, cmd) {
* or rejected with an error.
*/
Git.prototype.exec = function() {
return spawn(this.cmd, [].slice.call(arguments), this.cwd).then(output => {
return spawn(this.cmd, [...arguments], this.cwd).then(output => {
this.output = output;
return this;
});
Expand Down Expand Up @@ -136,20 +136,26 @@ Git.prototype.checkout = function(remote, branch) {

/**
* Remove all unversioned files.
* @param {string} files Files argument.
* @param {string|string[]} files Files argument.
* @return {Promise} A promise.
*/
Git.prototype.rm = function(files) {
return this.exec('rm', '--ignore-unmatch', '-r', '-f', files);
if (!Array.isArray(files)) {
files = [files];
}
return this.exec('rm', '--ignore-unmatch', '-r', '-f', ...files);
};

/**
* Add files.
* @param {string} files Files argument.
* @param {string|string[]} files Files argument.
* @return {Promise} A promise.
*/
Git.prototype.add = function(files) {
return this.exec('add', files);
if (!Array.isArray(files)) {
files = [files];
}
return this.exec('add', ...files);
};

/**
Expand Down
26 changes: 18 additions & 8 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ exports.defaults = {
branch: 'gh-pages',
remote: 'origin',
src: '**/*',
only: '.',
remove: '.',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default scares me. I haven't found time to do a more thorough review, but I don't get why we would want to remove everything by default.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that, I see that is the currently documented behavior. Clearly I need to get my head back into this before understanding if this change does the right thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing all files default seems ok to me, as options.add is present for controlling this behaviour.
For a reference, please take a look https://github.com/marketplace/actions/github-pages-action#%EF%B8%8F-keeping-existing-files

push: true,
history: true,
message: 'Updates',
Expand All @@ -52,6 +52,11 @@ exports.publish = function publish(basePath, config, callback) {

const options = Object.assign({}, exports.defaults, config);

// For backward compatibility before fixing #334
if (options.only) {
options.remove = options.only;
}

if (!callback) {
callback = function(err) {
if (err) {
Expand Down Expand Up @@ -94,10 +99,6 @@ exports.publish = function publish(basePath, config, callback) {
return;
}

const only = globby.sync(options.only, {cwd: basePath}).map(file => {
return path.join(options.dest, file);
});

let repoUrl;
let userPromise;
if (options.user) {
Expand Down Expand Up @@ -151,9 +152,18 @@ exports.publish = function publish(basePath, config, callback) {
}
})
.then(git => {
if (!options.add) {
log('Removing files');
return git.rm(only.join(' '));
if (options.add) {
return git;
}

log('Removing files');
const files = globby
.sync(options.remove, {
cwd: path.join(git.cwd, options.dest)
})
.map(file => path.join(options.dest, file));
if (files.length > 0) {
return git.rm(files);
} else {
return git;
}
Expand Down
1 change: 1 addition & 0 deletions test/integration/fixtures/remove/expected/new.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// to be copied
1 change: 1 addition & 0 deletions test/integration/fixtures/remove/expected/stable.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This file should not be removed.
1 change: 1 addition & 0 deletions test/integration/fixtures/remove/local/new.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// to be copied
1 change: 1 addition & 0 deletions test/integration/fixtures/remove/remote/removed.css
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/* to be removed */
1 change: 1 addition & 0 deletions test/integration/fixtures/remove/remote/removed.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// to be removed
1 change: 1 addition & 0 deletions test/integration/fixtures/remove/remote/stable.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This file should not be removed.
67 changes: 67 additions & 0 deletions test/integration/remove.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
const helper = require('../helper');
const ghPages = require('../../lib/');
const path = require('path');

const fixtures = path.join(__dirname, 'fixtures');
const fixtureName = 'remove';

beforeEach(() => {
ghPages.clean();
});

describe('the remove option', () => {
it('removes matched files in remote branch', done => {
const local = path.join(fixtures, fixtureName, 'local');
const expected = path.join(fixtures, fixtureName, 'expected');
const branch = 'gh-pages';
const remove = '*.{js,css}';

helper.setupRemote(fixtureName, {branch}).then(url => {
const options = {
repo: url,
user: {
name: 'User Name',
email: '[email protected]'
},
remove: remove
};

ghPages.publish(local, options, err => {
if (err) {
return done(err);
}
helper
.assertContentsMatch(expected, url, branch)
.then(() => done())
.catch(done);
});
});
});

it('skips removing files if there are no files to be removed', done => {
const local = path.join(fixtures, fixtureName, 'remote');
const branch = 'gh-pages';
const remove = 'non-exist-file';

helper.setupRemote(fixtureName, {branch}).then(url => {
const options = {
repo: url,
user: {
name: 'User Name',
email: '[email protected]'
},
remove: remove
};

ghPages.publish(local, options, err => {
if (err) {
return done(err);
}
helper
.assertContentsMatch(local, url, branch)
.then(() => done())
.catch(done);
});
});
});
});