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

[Martin Buchholz]

"utf8" is a thing.  "utf" is not.

On Thu, Oct 4, 2018 at 12:58 PM, Philipp Kunz <philipp.kunz at paratix.ch> wrote:
> Thanks to Sherman's hint, I revised the test to use the terms unicode
> character and utf encoding consistently and not utf character. Affects
> only the test, mostly in comments and a few identifiers.
>
> Philipp
>
>
> On Wed, 2018-10-03 at 12:48 -0700, Xueming Shen wrote:
>> +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
>>