Fwd: Codereview request for 7130915: File.equals does not give expected results when path contains Non-English characters on Mac OS X

[Alan Bateman]

On 26/06/2012 07:00, Xueming Shen wrote:
> On 6/25/12 10:58 PM, Xueming Shen wrote:
>> Hi,
>>
>> While I still believe that case-insensitive is the right choice for 
>> File/Path on MacOSX, it is
>> suggested that we might want to be a little conservative in this 
>> patch, with the assumption
>> that this patch will be backport to 7u release after being baked in 
>> jdk8 for a while (given
>> Apple's JDK6 is case sensitive for File, it might be too much for a 
>> update releases to go
>> with two in-compatible changes, case sensitive and hash32).
>>
>> So here is the webrev for a strip-down version from the original 
>> patch, in which it only
>> targets the nfd/nfc issue raised in 7130915 and 7168427. The proposed 
>> changes for
>> case insensitive compare and hash32 for both java.io.File and 
>> java.nio.file.Path are
>> removed.
>>
>> http://cr.openjdk.java.net/~sherman/7130915_7168427/webrev
>>
>> (The webrev for the "full" version has been moved to
>>  http://cr.openjdk.java.net/~sherman/7130915_7168427/webrev_full)
>>
>> Thanks,
>> -Sherman
I had to dig up the File Systems, Unicode, and Normalization 
presentation [1] before reviewing this, it's been a while.

I think the changes for java.io for fine, I just worry that there may be 
a few odd cases where it will be different to Apple JDK6. I looked 
through the macosx-port/macosx-port/jdk forest and there is one patch 
from Apple that does the NFC->NFD conversation. I don't get that patch 
because you say that the syscalls handle NFC okay. In your changes then 
you normalize when decoding the file names to Strings, which seems right 
but that seems to be completely missing from the changes that Apple 
brought over. The only minor comment is that we probably need to check 
the return from core foundation functions such as CFStringCreateMutable 
(this goes for the other native code too).

Adding the FileSystemProvider for MacOSX is great to see. The approach 
is fine for but is somewhat inefficient when ->NFD or ->NFC is needed. 
One inefficiency that can be fixed is in sun.nio.fs.MacOSXFileSystem. 
normalizeJavaPath then you can eliminate the second call to toCharArray. 
I think the new methods on sun.no.fs.UnixFileSystem should be given 
comments so that it's clear whether they should be overridden (the Unix* 
provider tends to be starting point for most ports).  A minor comment is 
that MacOSXNativeDispatcher has a bunch of blank lines at the end, also 
I think "static final" is more normal than "final static". At some point 
I think we should re-write MacOSXNativeDispatcher.normalizepath so that 
it deosn't require the critical section or malloc as in this area we try 
to keep the native methods to a bare minimum.

Do the tests assume they are run on HFS? Just wondering if you you need 
to look at the FileStore name/type to check. Some of variable namesa bit 
non-standard, very C like. Minor nit is that the copyright in the header 
isn't right in the tests. Also the tests list 2 CRs whereas I assume 
this is one CR (with the other closed as a dup).

On case sensitive vs. insensitive then I think it should be insensitive 
as per the first round but that would be a significant change given that 
Apple's JDK does not appear to have changed File.equals. Maybe we should 
think about this again once these changes are in 8.

-Alan

[1] 
http://developers.sun.com/global/products_platforms/solaris/reference/presentations/IUC29-FileSystems.pdf