[vim/vim] Fix three bugs with extracting lz4'd tar archives. (PR #19925)
Aaron B
The tar plugin allows users to extract files from tar archives that are compressed with lz4. But, tar#Extract() builds malformed extraction commands for lz4-compressed tar archives. This commit fixes three issues in that code. The first affects archives with a .tlz4 extension and the other two affect archives with .tar.lz4 extension (but one of these is symmetric to the issue that .tlz4 archives had).
(1) When trying to extract .tlz4 archives the command created by tar#Extract looked like this:
tar -I lz4pxf foo.tlz4 foo
This isn't right. It should be something like this:
tar -I lz4 -pxf foo.tlz4 foo
This was happening because tar.plugin is just substituting on the first - in "tar -pxf". This works fine if we just add a simple flag for extraction (eg, z for .tgz), but for lz4 we need to add "-I lz4".
I don't believe that there is an obvious good way to fix this without reworking the way the command is generated. Probably we should collect the command and flags separately and the flags should be stored in a set. Then put everything together into a string just before issuing it as an extraction command. Unfortunately, this might break things for users because they have access to tar_extractcmd.
This patch just makes the substitution a little bit more clever so that it does the right thing when substituting on a string like "tar -pxf".
(2) .tar.lz4 extractions had the same issue, which my patch fixes in the same way.
(3) .tar.lz4 extractions had another issue. There was a space missing in the command generated by tar#Extract. This meant that commands looked like this (notice the lack of space between the archive and output file names):
tar -I lz4pxf foo.tar.lz4foo
This patch just puts a space where it should be.
Finally, I should note that ChatGPT 5.4 initially identified this issue in the code and generated the test cases. I reviewed the test cases, wrote the patch, and actually ran vim against the tests (both with and without the patch).
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/19925
Commit Summary
- 327739f Fix three bugs with extracting lz4'd tar archives.
File Changes
(2 files)
- M runtime/autoload/tar.vim (6)
- M src/testdir/test_plugin_tar.vim (54)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Aaron B
7188ce06 left a comment (vim/vim#19925)The two tests are failing on non-Linux systems. tar is being supplied with options on the assumption that it is GNU tar compatible. This patch doesn't introduce that assumption, it was already in the code. I see two ways to move forward:
- Just deactivate the tests outside of Linux. This might be fine because these tests would also fail for a vim on non-Linux without my patch.
- Replacing the "-I" flag with "--use-compress-program" might do the right thing on all of the systems.
I'm open to suggestions.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aaron B
@7188ce06 pushed 1 commit.
- d783af1 Try --use-compress-program instead of -I.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aaron B
7188ce06 left a comment (vim/vim#19925)"--use-compress-program=lz4" did not work for FreeBSD and OS X. d783af1
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19925)If possible we should try to be portable, meaning we should aim for --use-compress-program if this is portable accross different Unix flavors. If this doesn't work, let's just disable the test on non-Linux systems
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aaron B
7188ce06 left a comment (vim/vim#19925)@chrisbra "--use-compress-program=lz4" did not work as you can see in the tests for the second commit on this branch. I'll setup a FreeBSD server and see what's going on, but I don't have access to an OS X system.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aaron B
7188ce06 left a comment (vim/vim#19925)@chrisbra FreeBSD man page for tar says "Note that this tar implementation recognizes lz4 compression automatically when reading archives." So this new patch only does the substitution on Linux and then let's FreeBSD's (and OS X's?) tar just use the automatic lz4 detection feature.
What do you think is best?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19925)That sounds reasonable. Thanks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Vladimír Marek
vlmarek left a comment (vim/vim#19925)Hi guys, this change broke Solaris tests. My understanding is that there are four flavors of tar with regards to lz4.
a) Native support (as for example FreeBSD). tar xf archive.tar.lz4
b) GNU which has ... -I lz4 ...
c) GNU which has ... --use-compress-program lz4 ...
d) tar which has none of those.
On Solaris /usr/bin/tar is d), /usr/gnu/bin/tar is b)+c). I'll be patching the file by
call system("lz4 --decompress --stdout -- ".shellescape(tarball)." | ".extractcmd." - ".shellescape(fname))
That should work in all cases. But is it the best solution? I do not know, sorry :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#19925)Sorry about that. can you submit a PR for this?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aaron B
7188ce06 left a comment (vim/vim#19925)@chrisbra @vlmarek Sorry, I didn't do enough research.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Vladimír Marek
vlmarek left a comment (vim/vim#19925)@chrisbra No need to be sorry! On the other way I am very grateful that you spend time on vim even on such minor things! This is hard task to do as there is unlimited number of OS variants behaving differently. I'm more than happy to create PR (it's trivial after all), question is how it should behave?
- I can make vim always use
lz4 --decompress --stdout -- | tar ... - I can keep vim using
... -I lz4 - ...on Linux as it is now and for any non-linux system uselz4 --decompress --stdout -- | tar ...(that's my current patch for solaris build) - I can keep vim using
... -I lz4 - ...on Linux as it is now and make Solaris uselz4 --decompress --stdout -- | tar ...and on any other system expect tar to handle .tar.lz4 directly on it's own (as it does now).
Personally I would uselz4 --decompress --stdout -- | tar ...everywhere, but this is matter of personal preference more than anything else.
@7188ce06 yes. Opening a tar.lz4 file by vim 9.2.209 is using
(lz4 --decompress --stdout -- '/builds/vmarek/merge/make-rules.tar.lz4' | /usr/sfw/bin/gtar -tf - )>/tmp/vBZOFTd/0 2>&1
(lz4 --decompress --stdout -- '/builds/vmarek/merge/make-rules.tar.lz4' | /usr/sfw/bin/gtar -pxf - -- 'make-rules/x11-component.mk')>/tmp/vBZOFTd/2 2>&1
I did verify with older vim 9.1.1591 and behaves the same too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.