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

Conversation

sunghwan2789
Copy link
Contributor

@sunghwan2789 sunghwan2789 commented Feb 25, 2020

This PR fixes some issues described below and resolves #334, fixes #337 .

Remove globbed files

return git.rm(only.join(' '));

Joining files in a string causes incorrect shell argument passing. A globbed file can be removed, but globbed files cannot be removed since the actual command will be

git rm -r -f "abc.js abc.css"

This PR makes git.rm and git.add accept string[] and handle them properly.

Remove files in remote dist directory, not in local one

gh-pages/lib/index.js

Lines 97 to 99 in 3579ab2

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

This shows that gh-pages globs files to be removed in local dist directory, not in remote branch. It causes git a fatal error(like #337) since it tries to remove files that are not in remote dist directory. globby.sync should be executed in remote dist directory.

Skip removing files if there are no globbed files

When globby.sync globs files, it returns empty array if there are no files to be removed. A guard of it should be added before

return git.rm(only.join(' '));

@sunghwan2789 sunghwan2789 changed the title Skip removing files if no files need to be removed. Fix remove not working properly. Feb 26, 2020
@sunghwan2789 sunghwan2789 changed the title Fix remove not working properly. Fix remove not working properly Feb 26, 2020
@codler
Copy link

codler commented Feb 29, 2020

I encountered this bug also #337, i hope this will be merged

@tschaub
Copy link
Owner

tschaub commented Mar 2, 2020

Thanks for the proposed fix, @sunghwan2789.

Can you please remove the added .prettierrc file? Formatting is handled by ESLint, and editor integrations should be able to use the eslintConfig member already in the package.json.

@sunghwan2789
Copy link
Contributor Author

Oh I did not notice that. Removed now!

@@ -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

@tschaub tschaub merged commit 6f5ac52 into tschaub:master Mar 6, 2020
@tschaub
Copy link
Owner

tschaub commented Mar 6, 2020

Thanks for your work on this, @sunghwan2789.

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.

empty string is not a valid pathspec bug: README options 'remove' is not functioning as described
3 participants