-
-
Notifications
You must be signed in to change notification settings - Fork 384
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 lzip support #401
Add lzip support #401
Conversation
e939b4f
to
395e228
Compare
395e228
to
699b325
Compare
Nice, thank you! This is looking good. Will merge this shortly. Appreciate your contribution! |
Hm, lz4 compression with extension is failing. @sorairolake Does it fail for you locally? |
@mholt Just now, I ran the test locally 10 times, 8 passed but 2 failed. I don't know why the test sometimes fails. |
Hmm, sounds like something funky with the test suite. I'll look into it, but your change seems sound. |
Both lz4 and lzip starts with "lz". So I think this may be related. |
lzip.go
Outdated
var mr MatchResult | ||
|
||
// match filename | ||
if strings.Contains(strings.ToLower(filename), lz.Name()) { |
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, indeed -- make this comparison more strict and that should hopefully fix the test.
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 this can be fixed with the following changes:
if strings.Contains(strings.ToLower(filename), lz.Name()) { | |
if filepath.Ext(strings.ToLower(filename)) == lz.Name() { |
However, this requires that the extension be the last element of the path.
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.
That would fail for those cases as you say, but maybe a check for both a suffix of .lz
or the string contains .lz.
?
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 there is almost no possibility that .lz
and .lz4
exists anywhere other than at the end of the path.
I think /path/to/foo.tar.lz
or bar.7z.lz4
are possible, but /path/to/foo.lz.tar
or bar.lz4.7z
are almost impossible.
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.
That's a good point.
lz4.go
Outdated
@@ -23,7 +24,7 @@ func (lz Lz4) Match(filename string, stream io.Reader) (MatchResult, error) { | |||
var mr MatchResult | |||
|
|||
// match filename | |||
if strings.Contains(strings.ToLower(filename), lz.Name()) { | |||
if filepath.Ext(strings.ToLower(filename)) == lz.Name() { |
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 was this one changed?
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 thought this would be a conflict, but it was a misunderstanding. So I restored this.
4d2e93b
to
82a374e
Compare
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.
Awesome, looks good now :) Thank you!
PS. I hope to travel to Japan someday :D |
Closes #400