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

Add zstd file completion to tar #45

Closed
wants to merge 1 commit into from
Closed

Add zstd file completion to tar #45

wants to merge 1 commit into from

Conversation

svenstaro
Copy link

@svenstaro svenstaro commented Jan 16, 2020

zstd is a modern compression format that is gaining more support. For instance, Arch Linux recently switched its packages to .zstd.

@svenstaro svenstaro changed the title Add zstd file completion Add zstd file completion to tar Jan 16, 2020
@@ -20,8 +20,10 @@ if [[ "$1" = *[urtx]* ]]; then
_files "$expl[@]" -g '*.(tar|TAR).bz2(-.)'
elif [[ "$1" = *J* ]]; then
_files "$expl[@]" -g '*.(tar|TAR).(lzma|xz)(-.)'
elif [[ "$1" = *zstd* ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

zstd is never assigned to "$1" (which comes from $_tar_cmd) so under what condition will this condition evaluate to true? In the absense of a single letter option to tar for zstd, I'm not sure how _tar should be changed to adapt to this. Perhaps replace _tar_cmd with a more specific _tar_compression variable.

Copy link
Author

Choose a reason for hiding this comment

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

You're right. I thought I tested this locally but it was an old file. I had a look for an obvious way to implement what you said but I'm not really knowledgable enough about the structure of this whole completion structure to be honest.

elif [[ "$_cmd_variant[$service]" == (gnu|libarchive) ]]; then
_files "$expl[@]" -g '*.((tar|TAR)(.gz|.GZ|.Z|.bz2|.lzma|.xz|)|(tbz|tgz|txz))(-.)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you verified with both libarchive and GNU tar? I only have the libarchive variant available to me and it appears to have a --zstd option with no short form.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

GNU tar supports --zstd too, with no short form either.

elif [[ "$_cmd_variant[$service]" == (gnu|libarchive) ]]; then
_files "$expl[@]" -g '*.((tar|TAR)(.gz|.GZ|.Z|.bz2|.lzma|.xz|)|(tbz|tgz|txz))(-.)'
_files "$expl[@]" -g '*.((tar|TAR)(.gz|.GZ|.Z|.bz2|.lzma|.xz|.zst|.zstd)|(tbz|tgz|txz|tzst|tzstd))(-.)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Are tzst and and tzstd extensions really a thing or are you just making assumptions while copy/pasting? The original purpose behind tgz, tbz extensions was to cope with 8.3 filename limitations on old operating systems so they mostly are just a relic from the past. tzst doesn't even fit in 3 characters.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, today I learned. I had no idea it was more than just an abbreviation for the heck of it. So to answer your question: It was just an assumption that this kind of abbreviation should be continued. However, I see now that it doesn't really make sense. I'll remove it.

@okapia
Copy link
Contributor

okapia commented Jul 6, 2020

Looks like someone applied a similar patch separately covering .lzo too. Sorry, I should have got around to applying this quicker. I'll close this now. Thanks for the contribution. If you're unhappy with the state of the function in the current git repo, you can always open a fresh request.

@okapia okapia closed this Jul 6, 2020
@svenstaro
Copy link
Author

Eh, it's fine. If it works, it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants