-
-
Notifications
You must be signed in to change notification settings - Fork 761
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
add zip plugin #1805
add zip plugin #1805
Conversation
plugins/zip
Outdated
zip -r -$compression "$abspath.zip" "$abspath" | ||
else | ||
tar -cvf "$abspath.tar" "$abspath" | ||
compress_tar "$abspath" |
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.
You can generally just pipe the output of tar
over to the compressor program without needing to create any intermediate file.
plugins/zip
Outdated
password_protect(){ # filename | ||
if [ "$encrypt" = "y" ] || [ "$encrypt" = "Y" ]; then | ||
cd "$working_dir" || exit # some needs this | ||
find . -maxdepth 1 -type f \( -name "$1.zip" -o -name "$1.tar*" \) \! -name "*.gpg" -exec gpg -c "{}" \; -exec rm "{}" \; |
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.
Why not just pass the full filename via argument instead of doing this. Also gpg
might accept stdin
as well. So you might be able to just pipe it in and avoid the intermediate file once again.
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 only marked a single occurrence of each issue. Please change all of them.
plugins/zip
Outdated
if [ $alg != tar ] && [ $alg != zip ]; then | ||
cd "$working_dir" || exit # some needs this | ||
# 'keep' is due to zstd not following standard | ||
if $alg -"$compression" --keep "$1.tar"; then | ||
rm "$1.tar" |
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.
CI is flagging this. Use "$alg"
.
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.
There are more variables should I put ""
for all of them?
Also is it ok to amend commit? Don't know what's proper way when fixing issues like that.
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.
Please do, I missed those. It'll keep complaining until all of them are quoted. In general, if it has $
it goes between ""
.
You can add more commits, amend, force push, whatever you like. PRs usually just get squashed anyway.
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.
You know how every rule has an exception? This is it for the quoting rule: $(tr '\0' '\n' < "$selection")
shouldn't be quoted. We want that to split at whitespace.
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.
what if filename has white space?
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.
anyway selection
should only contain selected files nothing else right? So doesn't matter
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.
Ah, for loop isn't great for this since it will break if filename contains \n
.
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.
well I will have to come with other solution or maybe I'm not right and already talking bullshits :D.
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.
Just do it with xargs -0
and add a alg_to_extension()
function or something to add the extension.
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.
Yeah, it would be best if you can get it to work with xargs -0
. Just keep in mind that you can't run functions inside xargs
. That's a bash extension.
Well boys, yesterday I tried many things like perl's sed and creating newfiles but that didn't seem ok to me. I come with this solution. Will this work in other shells? I tried to preserve original behavior, but it's kinda weird. I mean it works for me :D. I tested it for dirs and files with/without space/newline. I could omit something. Maybe I should include some tests. I could possibly rewrite it in way in which I specify file extensions in It works now in a way that users need to specify only name without extensions. |
@jarun There are still some outstanding comments from me that should be addressed. CI is currently failing; I'd say we probably just silence it in this case. I myself haven't tried the plugin out; will probably do that at some point. When those issues are address and if we confirm that it does work, we'll be ready to merge. Personally, I'd like to see the script simplified a bit, there's a lot of commands nested inside of |
There are some issues. I finally made somewhat testing script, so now it will be easier to fix them. I will provide script with fix tomorrow. |
When testing using script comment line
test script
|
Should I fix anything else? |
Did some basic tests (didn't test encryption). Couple remarks:
|
Also, I'm really not a fan of this type of thing that's going on: $ xargs -0 tar -cvf - < "$selection" | gzip -c -9 > archive.tar.gz Or if you want to remove the $pwd from the paths: sed -z "s|^$PWD/||g" < "$selection" | xargs -0 tar -cvf - | zstd -c -19 > archive.tar.zst EDIT: and if you don't want to refactor things to be stdin/out based then leave a TODO comment at the top about this. |
does this work because after trying it i got something like this which is not what we want.
also doing this piping to gpg and thus creating another
As a workaround to prevent including more if-statements we could do |
@Darukutsu CI is failing, can you check? Also, this is open for a while, if this doesn't move, I'll close it. |
just like @KlzXS said
I couldn't come up with better solution. It's pretty much working for me(haven't spotted any issue, when using/testing), but that's not what means "production ready". Feel free to close. In either case thank you guys for perfectioning this script. |
The main issue I have with the script is that it's a nightmare to maintain with all those nested substitutions, extracting and constructing names and a lot of If you're fine with such code merge it in. @Darukutsu Thank you for giving it a shot. Personally, I'd have written this in something like Python as bash isn't really great at dealing with complex logic. You get ugly things like that We don't really have examples of non-shell scripts, but a plugin can be any old executable file, we just happen to mostly do it in |
I think it's best that we close this for now. I've added it as a task in the Tracker. Someone can give it a shot in the future. My suggestions for those attempting this task:
Now that I've written this it sounds more like a whole separate tool instead of a "simple" plugin. Make of that what you will. All in all, this is functionality that would be nice to have in |
I don't like that built-in zip includes absolute paths by default. I also miss zstd as option. This could do probably any
compression. You can also password protect archive.
Related issues #1105.