You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@commons.apache.org by ctron <gi...@git.apache.org> on 2018/07/09 09:52:56 UTC
[GitHub] commons-compress pull request #67: [COMPRESS-459] Fix reading of multibyte n...
GitHub user ctron opened a pull request:
https://github.com/apache/commons-compress/pull/67
[COMPRESS-459] Fix reading of multibyte name entries
This fixes COMPRESS-459 by using the name number of bytes from the field
in the stream instead of relying on the assumption that each character
is exactly one byte, which isn't true for UTF-8, UTF-16 or other
multi-byte character encodings.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ctron/commons-compress feature/fix_COMPRESS_459_1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-compress/pull/67.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #67
----
commit 715352d3343358539a40b9623a9a00beb115ff30
Author: Jens Reimann <jr...@...>
Date: 2018-07-09T09:41:43Z
[COMPRESS-459] Fix reading of multibyte name entries
This fixes COMPRESS-459 by using the name number of bytes from the field
in the stream instead of relying on the assumption that each character
is exactly one byte, which isn't true for UTF-8, UTF-16 or other
multi-byte character encodings.
----
---
[GitHub] commons-compress issue #67: [COMPRESS-459] Fix reading of multibyte name ent...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/commons-compress/pull/67
Many thanks. We are not that squash-happy over here, two commits is fine.
I would have gone for `ZipEncoding` rather than `Charset` for consistency, but can change that later myself, I really don't want to play "fetch me a rock" here. Will merge the PR soonish.
---
[GitHub] commons-compress issue #67: [COMPRESS-459] Fix reading of multibyte name ent...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/commons-compress/pull/67
`ZipEncoding` is a better choice for our internal use as it is used to encode the name (and deals with "use null as the encoding to use the platform's default encoding" transparently). You don't have to make the changes yourself (but if you do, please add spaces around operators :-) ), but I won't complain if you are faster than me.
---
[GitHub] commons-compress issue #67: [COMPRESS-459] Fix reading of multibyte name ent...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/commons-compress/pull/67
Many thanks!
---
[GitHub] commons-compress pull request #67: [COMPRESS-459] Fix reading of multibyte n...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/commons-compress/pull/67
---
[GitHub] commons-compress issue #67: [COMPRESS-459] Fix reading of multibyte name ent...
Posted by ctron <gi...@git.apache.org>.
Github user ctron commented on the issue:
https://github.com/apache/commons-compress/pull/67
Thanks @bodewig for reviewing this PR. Yes you are right, with everything :wink:
I amended the PR, fixing the issues with the original PR and added the functionality to the output stream as well. The made them two commits for to easier understand what was fixed and what added. It can be squashed later on if required.
---
[GitHub] commons-compress issue #67: [COMPRESS-459] Fix reading of multibyte name ent...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/commons-compress/pull/67
Thanks @ctron!
Don't we need to adjust `getHeaderPadCount` in `readOldBinaryEntry` as well?
At first I stumbled over the `-1` in the call to `getHeaderPadCount` but I think it is because the method already accounts for the trailing NUL, I guess a comment could help :-)
One more nit, you removed a since tag from one of `CpioArchiveEntry`'s constructors by accident.
Also, but this probably is a separate issue: `CpioArchiveOutputStream` seems to have the same problem (even worse, as the length we write into the header may be wrong). It seems we should deprecate the no-arg version of `getHeaderPadCount` as it only works for single bye encodings.
---
[GitHub] commons-compress issue #67: [COMPRESS-459] Fix reading of multibyte name ent...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/commons-compress/pull/67
@ctron I've just started merging your changes locally
---
[GitHub] commons-compress issue #67: [COMPRESS-459] Fix reading of multibyte name ent...
Posted by ctron <gi...@git.apache.org>.
Github user ctron commented on the issue:
https://github.com/apache/commons-compress/pull/67
Yes, that might have been a solution as well. But I was thinking to use something more Java-standard. If you want me to change this, I can do this as well.
---
[GitHub] commons-compress issue #67: [COMPRESS-459] Fix reading of multibyte name ent...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-compress/pull/67
[](https://coveralls.io/builds/17891202)
Coverage increased (+0.002%) to 86.355% when pulling **715352d3343358539a40b9623a9a00beb115ff30 on ctron:feature/fix_COMPRESS_459_1** into **f5330f7e667f5a7245c8a5f3007cda04554c5fe2 on apache:master**.
---