-
Notifications
You must be signed in to change notification settings - Fork 96
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 shell glob in assets svn propset
commands
#99
Conversation
The glob of `*.png`, `*.jpg`, `*.gif`, and `*.svg` in the `svn propset svn:mime-type` commands fail due to being quoted. SVN does not support expanding the globs itself, so we need to rely on the shell expanding them. Shell globs are only expanded when the `*` is unquoted, but currently the whole path including the glob is within quotes meaning that we're trying to pass the literal string `*.png` etc with the `*` unexpanded. * Unquote the globs in `svn propset svn:mime-type` commands to allow expansion by the shell
@diddledani thanks for the issue and PR, we'll get this through review and get back to you with any questions or concerns! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much, @diddledani, this is working as expected in my test. 🎉 I left a minor comment about the quote, can you please take a look?
@@ -135,16 +135,16 @@ svn cp "trunk" "tags/$VERSION" | |||
# Fix screenshots getting force downloaded when clicking them | |||
# https://developer.wordpress.org/plugins/wordpress-org/plugin-assets/ | |||
if test -d "$SVN_DIR/assets" && test -n "$(find "$SVN_DIR/assets" -maxdepth 1 -name "*.png" -print -quit)"; then | |||
svn propset svn:mime-type "image/png" "$SVN_DIR/assets/*.png" || true | |||
svn propset svn:mime-type "image/png" "$SVN_DIR/assets/"*.png || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the quote entirely instead of partial quoting the file path glob.
svn propset svn:mime-type "image/png" "$SVN_DIR/assets/"*.png || true | |
svn propset svn:mime-type "image/png" $SVN_DIR/assets/*.png || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for other file types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's best to leave it quoted as the variable $SVN_DIR
is not guaranteed to never have spaces in its value. If GitHub change their setup, then the $HOME
directory, which is used to generate the $SVN_DIR
variable, might change to a location that includes a space. Therefore defensive programming suggests that we should retain the quotes to mitigate potential future breakage caused by platform changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense to me! Thanks so much for explanation!
Description of the Change
Unquote the globs in
svn propset svn:mime-type
commands to allow expansion by the shellCloses #98
Alternate Designs
None
Possible Drawbacks
None
Verification Process
Checklist:
Changelog Entry
Fixed - failure to set assets mime-type with
svn propset
Credits
Props @diddledani