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

Infer file extension when decompressing #154

Merged
merged 5 commits into from
Nov 10, 2021
Merged

Conversation

sigmaSd
Copy link
Contributor

@sigmaSd sigmaSd commented Nov 3, 2021

@marcospb19
Copy link
Member

Thanks for your first contribution to ouch! We really appreciate it, review will be coming the next days (probably tomorrow).

@marcospb19 marcospb19 added the enhancement New feature or request label Nov 4, 2021
src/commands.rs Outdated Show resolved Hide resolved
src/utils/fs.rs Outdated Show resolved Hide resolved
src/utils/fs.rs Outdated Show resolved Hide resolved
src/utils/fs.rs Outdated Show resolved Hide resolved
@sigmaSd
Copy link
Contributor Author

sigmaSd commented Nov 10, 2021

I updated the pr, I can squash the commits later when the review is done.

@marcospb19
Copy link
Member

Sorry for my late review.

I updated the pr, I can squash the commits later when the review is done.

I left one conversation unresolved for that warning, before you squash it.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Nov 10, 2021

I added the info!

@marcospb19
Copy link
Member

I think I can squash that for you.

@marcospb19
Copy link
Member

I found a small bug at the zip detection and fixed it, unfortunately git history says I commited and you authored, I don't understand why it's not the opposite, but anyways, thanks for your contribution! This is a great addition.

@marcospb19 marcospb19 merged commit 1026fa1 into ouch-org:master Nov 10, 2021
@sigmaSd
Copy link
Contributor Author

sigmaSd commented Nov 10, 2021

Thanks as well!

That function is copied from infer, so I'll let the author know.

@marcospb19
Copy link
Member

marcospb19 commented Nov 10, 2021

For reference, you sent the https://en.wikipedia.org/wiki/List_of_file_signatures link, there, we have the line:

image

So it's about .zip, here is the first column:

50 4B 03 04
50 4B 05 06 (empty archive)
50 4B 07 08 (spanned archive) 

So, it should be (03 && 04) || (05 && 06) || (07 && 08), but here is the implementation you copied:

https://github.com/sigmaSd/ouch/blob/266cf6e27ae318f143e738b4091a7ac8b4a60ea1/src/utils/fs.rs#L86-L92

Which mixes things up, you could have (03 && 08) for example, here are all the combinations that would be accepted by the previous implementation:

50 4B 03 04 (valid)
50 4B 03 06 (**invalid**)
50 4B 03 08 (**invalid**)
50 4B 05 06 (valid)
50 4B 05 04 (**invalid**)
50 4B 05 08 (**invalid**)
50 4B 07 08 (valid)
50 4B 07 04 (**invalid**)
50 4B 07 06 (**invalid**)

@marcospb19
Copy link
Member

(There is also a chance that our source contains the wrong information and the original code was right all along. Pinging @bojand here too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants