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

[Philipp Kunz]

Please find another revised and updated patch attached.


On Sun, 2018-10-21 at 15:10 -0700, Martin Buchholz wrote:
> I only took a quick look.
> Looks good, but here's a nitpick - capitalize javadoc that begins
> with "returns"
> On Fri, Oct 19, 2018 at 1:27 AM, Philipp Kunz <philipp.kunz at paratix.c
> h> wrote:
> > Hi Martin and everyone,
> > 
> > 
> > 
> > You were absolutely right to object "utf".
> > 
> > Please find a revised and updated patch attached.
> > 
> > 
> > 
> > Regards,
> > 
> > Philipp
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > On Thu, 2018-10-04 at 15:24 -0700, Martin Buchholz wrote:
> > 
> > > "utf8" is a thing.  "utf" is not.
> > 
> > > 
> > 
> > > On Thu, Oct 4, 2018 at 12:58 PM, Philipp Kunz <philipp.kunz at parat
> > ix.c
> > 
> > > h> 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
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 6202130.patch
Type: text/x-patch
Size: 10021 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20181022/77c74a39/6202130-0001.patch>