RFR: 8211917: (zipfs) Creating or updating a JAR file system should put the MANIFEST.MF at the start

[Lance Andersen]

Hi Christoph

> On Mar 5, 2020, at 4:03 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
> 
> Hi Lance,
> 
> Thanks for addressing my points. Here my answer to those things which weren't in full agreement yet:

Please see below
> 
>> I dislike a bit the fact that we introduce a new testng subfolder in
>> test/jdk/nio/zipfs. Wouldn’t it be possible to consolidate in the current test
>> folder?
>> 
>> I am trying to add some organization separating the non-testng  tests from
>> the  testng tests and other testng tests will be moved here.  I did the same
>> for JDBC a few years back
> 
> ok, maybe it's a good idea to do this here and gradually move all testng tests over.
> 
>> - line 387: Manfiest -> Manifest
> 
> I think you missed that one

Fixed must have not saved it but it is there now
> 
> 
>> - line 417: Parameter "final Map<?, ?>
>> attributes" of ManifestOrderTest::verify should be renamed to
>> "manifestAttributes" to make it easier to understand its purpose
>> 
>> 
>> Prefer to leave as is as it gets wordy and I believe the description is clear in
>> the comments
> 
> Hm, I needed to look twice to grasp it. So, I'd still prefer something with "manifest" in the variable name. But I leave it to you since it's probably a personal taste thing ��

I left the variable name as to your point it sometimes is a personal preference ;-) 
> 
> However, two more things here:
> 
> The Javadoc of verify says (line 412):
> * @param attributes  Is there a Manifest to check
> 
> You should rather go with the Javadoc of validateManifest here as well:
> * @param attributes A Map containing the attributes expected in the Manifest;
> *                   otherwise empty

Updated…  Thought I had done that previously as that @param was originally a boolean ….
> 
> Also, I spotted in the Javadoc, line 413:
> * @param entries     Entries to validate are in the JAR
> 
> I guess the "are" is wrong here.

Fixed

> 
>> test/jdk/jdk/nio/zipfs/testng/util/ZipFsBaseTest.java:
>> - rename to ZipFSBaseTest (Capital ‘S’)??
>> hmm  I had that originally but did not like it…  but don’t have a strong
>> preference
> 
> Ok, leave it as you have it ��

:-)
> 
>> - line 120: public static void verify - > this method is not used by
>> ManifestOrderTest. I think we should only add it when having a test that
>> really uses this verify method. But I generally agree that the verify method is
>> a candidate for communalization
>> 
>> This will be used by some tests I have created and some I will be moving so
>> rather add it now on the initial push.  This is used by several tests that will be
>> migrated
>> 
>> - line 176: public void zip: dito, this method doesn’t seem used. So I suggest
>> to remove it for this change
>> Same comment as above
> 
> OK.
> 
>> The webrev for the above
>> is http://cr.openjdk.java.net/~lancea/8211917/webrev.01/index.html
> 
> Looks good, apart from my comments above.

Thank you again for the review

The revised webrev is at http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html <http://cr.openjdk.java.net/~lancea/8211917/webrev.02/index.html>

Best
Lance
> 
> Thanks
> Christoph
> 

 <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>