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

feat: Use rsync instead of io.cp() #144

Closed
wants to merge 5 commits into from
Closed

feat: Use rsync instead of io.cp() #144

wants to merge 5 commits into from

Conversation

peaceiris
Copy link
Owner

@peaceiris peaceiris commented Mar 6, 2020

Prototype for #103

  • Add more unit testing
  • Consider error handing

@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #144 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #144   +/-   ##
=======================================
  Coverage   69.06%   69.06%           
=======================================
  Files           3        3           
  Lines         139      139           
  Branches       26       26           
=======================================
  Hits           96       96           
  Misses         43       43

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fab0628...ca0ce3c. Read the comment docs.

core.info(`[INFO] copy ${file}`);
} else {
await exec.exec('rsync', [
'-rptgoDv',
Copy link
Contributor

Choose a reason for hiding this comment

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

long args here would be nice to help viewers quickly know what's going on

Copy link
Owner Author

Choose a reason for hiding this comment

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

Long option name? --recursive instead of -r? I agree.

Comment on lines +35 to +36
'--copy-links',
'--copy-dirlinks',
Copy link
Contributor

Choose a reason for hiding this comment

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

From rsync --help:

 -l, --links                 copy symlinks as symlinks
 -L, --copy-links            transform symlink into referent file/dir
     --copy-unsafe-links     only "unsafe" symlinks are transformed
     --safe-links            ignore symlinks that point outside the source tree
     --munge-links           munge symlinks to make them safer (but unusable)
 -k, --copy-dirlinks         transform symlink to a dir into referent dir
 -K, --keep-dirlinks         treat symlinked dir on receiver as dir
 -H, --hard-links            preserve hard links

Extended documentation at https://superuser.com/a/1388910

My understanding is that --copy-links includes --copy-dirlinks, such that you should only need to specify the former.

Related to my comment at #103 (comment) and from looking at the rsync docs, I think there are two options for this action:

  1. --copy-links: dereference all symlinks
  2. --copy-unsafe-links: only dereference "unsafe" symlinks (symbolic links that point outside the copied tree)

I like option 2 the most because GitHub Pages can handle symlinks that point within the file tree. However, I can imagine users would like access to both modes. Some applications might want to dereference all symlinks while others might want to only dereference unsafe symlinks.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thank you for investigating.

I like option 2 the most because GitHub Pages can handle symlinks that point within the file tree. However, I can imagine users would like access to both modes. Some applications might want to dereference all symlinks while others might want to only dereference unsafe symlinks.

I have understood the two cases clearly.

@peaceiris
Copy link
Owner Author

I close this Pull Request to use the release 3.6.0 for another feature. This topic continues on #103

@peaceiris peaceiris closed this Mar 28, 2020
@peaceiris peaceiris deleted the feat-rsync branch March 28, 2020 09:15
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.

inconsistent handling of symbolic links in publish_dir
2 participants