Review Request: 6202130: java.util.jar.Attributes.writeMain() can't handle multi-byte chars

[Xueming Shen]

+1

only nit is it might be preferred to use "utf8" directly in the test instead of current "utf"

-sherman

On 10/03/2018 11:55 AM, Philipp Kunz wrote:
> In short, bug 6202130 is about removal of comments in Attributes like
>
>> XXX Need to handle UTF8 values and break up lines longer than 72
> The patch here contains a test that shows that utf characters are
> handled correctly, which has been the case before. Breaking lines has
> also been working before.
>
> In the main code, the patch only removes a few comments and does not
> change actual code. It all has been working before and now we know.
>
> Philipp
>
>
> https://bugs.openjdk.java.net/browse/JDK-6202130
>
>
>
>
> On Wed, 2018-09-26 at 07:08 +0200, Philipp Kunz wrote:
>> Hi,
>>
>> There are some comments in the source code like this:
>>
>>   >  XXX Need to handle UTF8 values and break up lines longer than 72 bytes
>>
>> in Attributes.java on lines 299, 327, and 370. Bug 6202130 also suggests
>> to add a test to show that the character set is fully supported before
>> actually removing those comments.
>>
>> Hence, I added such a test, see ValueUtfEncoding in attached patch, and
>> it finds the complete utf character set supported correctly and that it
>> has been so already before.
>>
>> The comments in Attributes also say that lines longer than 72 bytes need
>> to be broken. That is a different issue and has been already addressed
>> and solved in bug 6372077 and is now tested in LineBreakLineWidth and
>> therefore, that line break part of the comment is obsolete now as well
>> with the exception of version headers ("Manifest-Version" or
>> "Signature-Version") at least when taking strictly.
>>
>> Version headers are not subject of bug 6202130 for which my patch
>> intends to provide a fix and if only because multi-byte characters are
>> not allowed in version headers and wasn't subject of 6372077 either
>> which was about too short lines rather than too long ones. If I would
>> change only the minimum I could argue was safe, there would have to
>> remain a comment in Attributes.java on line 327 like
>>
>>   >  XXX Need to handle version headers longer than 72 bytes
>>
>> but I guess that should probably better be represented as another bug
>> rather than this style of comment if at all desired. There are some bugs
>> concerning version headers and particularly the behavior of the
>> manifests in their absence, among possibly others, 6910466, 4935610,
>> 4271239, 8196371 and 4256580 which have some relation but I haven't
>> found one that really fits the 72 byte line length limit of version
>> headers. Whether such a bug is created or found or not, it does not
>> directly relate to the one I'm trying to fix here, 6202130.
>>
>> If also the test is good I would assume that the comments can be
>> removed now without actually changing the Manifest and Attributes
>> implementation.
>>
>> Regards,
>> Philipp