RFR(JDK12/NIO) 8202285: (fs) Add a method to Files for comparing file contents

[Stuart Marks]

On 9/19/18 11:48 AM, Joe Wang wrote:
> After much discussion and 10 iterations of reviews, this proposal has evolved 
> from what was the original isSameContent method to a mismatch method. API-wise, 
> a compare method was also considered as it looked like just a short step forward 
> from mismatch, however, it was eventually dropped since there is no convincing 
> use case comparing files lexicographically by contents. Impl-wise, extensive 
> performance benchmarking has been done to compare a buffered reading vs memory 
> mapping, the result was that a simple buffered reading performed better among 
> small files, and those with the mismatched byte closer to the beginning of 
> files. Since the proposed method's targeted files are small ones, the impl 
> currently does a buffered reading only.

Hi Joe,

Thanks for being persistent with this one!

A very small spec nitpick:

> specdiff: 
> http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff/java/nio/file/Files.html

1544      *   <li> {@link #isSameFile(Path, Path) isSameFile(path, path2)} 
returns true,</li>

I would add "or" after the trailing comma. This makes it similar to the two-item 
list that follows.

> webrev: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev/

A couple minor comments on FilesMismatch.java:

If mismatchByAttrs() is replaced with a call to isSameFile(), as Alan suggested, 
this simplifies things considerably. It looks like the mismatch() implementation 
will be reduced to around ~40 lines of code. Does it deserve its own file, or 
can it be placed directly into Files.java? That file has over 4,000 lines 
already though.

  106             if (nRead1 == 0 || nRead2 == 0) {
  107                 if (nRead1 == 0 && nRead2 == 0) {
  108                     // both files reach EOF
  109                     return -1;
  110                 } else {
  111                     // one of the files reaches EOF
  112                     return totalRead;
  113                 }
  114             } else if (nRead1 != nRead2) {

I think it reads slightly better if the nested 'if' at lines 107-113 were 
flattened into the else-if chain, like so:

             if (nRead1 == 0 && nRead2 == 0) {
                 // both files reached EOF
                 return -1;
             } else if (nRead1 == 0 || nRead2 == 0) {
                 // one but not both files reached EOF
                 return totalRead;
             } else if (nRead1 != nRead2) {

There are a couple places where lines can be joined. The resulting lines are 
longer than 80 characters but are still less than 100 characters, which I think 
is OK. (I don't think we have a strict limit of 80 anymore.)

   97     private static long mismatch(InputStream in1, InputStream in2)
   98         throws IOException {

  117                 return totalRead +
  118                     Arrays.mismatch(buffer1, 0, len, buffer2, 0, len);

Comments on tests in Comparison.java:

* This file, and several names within this file, should probably should be 
renamed to focus on mismatch instead of comparison.

* I'm finding it quite difficult to read the test cases.

The test files are placed into a Path[] and referenced by index. This makes it 
quite difficult to determine, for example, if all six cases within the mismatch 
loop are covered.

Instead of storing the files at specific array indexes, I'd suggest doing the 
following.

1) Establish good naming conventions for test files, including size information, 
covering duplicate files (e.g., 'a' and 'b'), and a modification at a particular 
offset. For example, you might do something like the following:

     test120a    - a test file with size 120
     test120b    - a test file with size 120, identical to test120a
     test120m110 - a test file with size 120, with an altered byte at 110
     etc.

2) Declare fields of the same name containing the file paths.

3) Add some logic in prepareTestFile() to append each file's path to a list 
after the file is created. Then have the @AfterClass method run through this 
list and delete the files.

4) The test data generator method can then have lines of test data that 
reference the test files using a meaningful name, so it's easy to understand the 
names and the expected results. Also, the test data generator should have a 
comment that describes the test method parameters.

* The generation of the test data files seems overly complicated.

A StringBuilder is used to accumulate data, then it's turned into a String, then 
(optionally) a character is replaced by the insertChar() method, and then the 
string is written to the file.

I think you can just accumulate the test data into a StringBuilder, (optionally) 
call sb.setCharAt() to modify the data, and then write the SB to the file. After 
all, Files.writeString() takes a CharSequence.

The generateFixedString() method should be changed to create and return a fresh 
StringBuilder with the desired contents. (See above.) It can also be simplified 
quite a bit.

I don't think you need to have methods with Charset parameters. It's reasonable 
to have the test files be text, which makes things easier to develop and debug. 
But the test depends on characters being written in a single-byte charset, 
because mismatch() reports *byte* positions and not *character* positions.

Using the method overloads that are implicitly UTF-8 is incorrect. If a 
non-single-byte character were to slip into the test data (seems unlikely, but 
possible), it'll be expanded to multiple bytes by the UTF-8 encoder, which may 
throw off the result. Instead I'd suggest hardwiring the charset to be US_ASCII 
to enforce the one-byte-per-character invariant.

This is a lot of stuff. If you can't follow it all, perhaps we can work together 
directly and go over the test file.

Thanks,

s'marks