You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@commons.apache.org by dalbani <gi...@git.apache.org> on 2017/11/20 15:54:24 UTC
[GitHub] commons-compress pull request #56: Provide information about presence of Uni...
GitHub user dalbani opened a pull request:
https://github.com/apache/commons-compress/pull/56
Provide information about presence of Unicode name / comment in ZipArchiveEntry
…ArchiveEntry
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dalbani/commons-compress COMPRESS-429
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-compress/pull/56.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 #56
----
commit 36d504d38cae2edcc4e389bb132ece0888c32c78
Author: Damiano Albani <da...@gmail.com>
Date: 2017-11-20T15:46:42Z
Provide information about presence of Unicode name and comment in ZipArchiveEntry
----
---
[GitHub] commons-compress pull request #56: [COMPRESS-429] Provide information about ...
Posted by dalbani <gi...@git.apache.org>.
Github user dalbani commented on a diff in the pull request:
https://github.com/apache/commons-compress/pull/56#discussion_r160002542
--- Diff: src/main/java/org/apache/commons/compress/archivers/zip/ZipUtil.java ---
@@ -239,6 +239,7 @@ static void setNameAndCommentFromExtraFields(final ZipArchiveEntry ze,
originalNameBytes);
if (newName != null && !originalName.equals(newName)) {
--- End diff --
When I think about it, I would remove the second test in this `if`, so that the Unicode field always have "priority" over the original name as source.
Scenario where it would matter is when the `originalName` contains a non ASCII value (which is properly decoded) and which matches with the Unicode field value.
---
[GitHub] commons-compress pull request #56: [COMPRESS-429] Provide information about ...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:
https://github.com/apache/commons-compress/pull/56#discussion_r160044510
--- Diff: src/main/java/org/apache/commons/compress/archivers/zip/ZipUtil.java ---
@@ -239,6 +239,7 @@ static void setNameAndCommentFromExtraFields(final ZipArchiveEntry ze,
originalNameBytes);
if (newName != null && !originalName.equals(newName)) {
--- End diff --
After some digging through the history it seems the reason no longer exists, I've removed the second part with ced2075c.
---
[GitHub] commons-compress issue #56: [COMPRESS-429] Provide information about source ...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/commons-compress/pull/56
@dalbani I've extended your PR with https://github.com/apache/commons-compress/tree/COMPRESS-429 - could you please have a look?
---
[GitHub] commons-compress issue #56: [COMPRESS-429] Provide information about source ...
Posted by dalbani <gi...@git.apache.org>.
Github user dalbani commented on the issue:
https://github.com/apache/commons-compress/pull/56
@bodewig: my apologies for not getting back to you sooner, I was on vacation.
What you wrote looks just fine to me — I would definitely not have done any better.
Kudos!
I have just added one comment though for a very specific use case.
---
[GitHub] commons-compress pull request #56: [COMPRESS-429] Provide information about ...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on a diff in the pull request:
https://github.com/apache/commons-compress/pull/56#discussion_r160044269
--- Diff: src/main/java/org/apache/commons/compress/archivers/zip/ZipUtil.java ---
@@ -239,6 +239,7 @@ static void setNameAndCommentFromExtraFields(final ZipArchiveEntry ze,
originalNameBytes);
if (newName != null && !originalName.equals(newName)) {
--- End diff --
I think the second part is there for a reason, even though my memory is failing me right now - and I stumbled over it myself when I realized the same check is not present for the comment.
I understand what you mean, though, changed with c2906092
---
[GitHub] commons-compress issue #56: [COMPRESS-429] Provide information about source ...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-compress/pull/56
[](https://coveralls.io/builds/14619830)
Coverage decreased (-0.04%) to 85.865% when pulling **db0df3deff3301351e49e5b5e80f1e0926317063 on dalbani:COMPRESS-429** into **78cb9bf36dab81e15887f451fbc964cf57e94739 on apache:master**.
---
[GitHub] commons-compress issue #56: [COMPRESS-429] Provide information about source ...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/commons-compress/pull/56
Don't worry about any delay @dalbani many thanks.
I've merged the branch to master by now, will be included in Compress 1.16.
---
[GitHub] commons-compress issue #56: [COMPRESS-429] Provide information about source ...
Posted by bodewig <gi...@git.apache.org>.
Github user bodewig commented on the issue:
https://github.com/apache/commons-compress/pull/56
Many thanks @dalbani this looks good. I've got two wishes, though:
* could you please revert the change in imports to `ZipFile`? We don't want to use `*`-imports add all.
* the new enums could use some javadoc inclusing a @since marker
Of course, adding tests woudln't hurt , either;-)
---
[GitHub] commons-compress issue #56: [COMPRESS-429] Provide information about source ...
Posted by dalbani <gi...@git.apache.org>.
Github user dalbani commented on the issue:
https://github.com/apache/commons-compress/pull/56
@bodewig: thanks for your great overall reactivity for this feature request! I look forward for release 1.16.
---
[GitHub] commons-compress issue #56: Provide information about presence of Unicode na...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-compress/pull/56
[](https://coveralls.io/builds/14619802)
Coverage decreased (-0.02%) to 85.889% when pulling **db0df3deff3301351e49e5b5e80f1e0926317063 on dalbani:COMPRESS-429** into **78cb9bf36dab81e15887f451fbc964cf57e94739 on apache:master**.
---
[GitHub] commons-compress issue #56: Provide information about presence of Unicode na...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-compress/pull/56
[](https://coveralls.io/builds/14288073)
Coverage decreased (-0.01%) to 85.869% when pulling **36d504d38cae2edcc4e389bb132ece0888c32c78 on dalbani:COMPRESS-429** into **10ff1c341d37da1cdb2a9ecb280a7e73670c00f5 on apache:master**.
---
[GitHub] commons-compress pull request #56: [COMPRESS-429] Provide information about ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/commons-compress/pull/56
---