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

[Xueming Shen]

Alan,

Webrev has been updated accordingly at

http://cr.openjdk.java.net/~sherman/7130915/webrev

with changes

(1) added a CFStringCreateMutable(...) != null check in both io and nio 
native, though it is
      unlikely  to fail here because we are passing a NULL and 0 length, 
like new StringBuilder()
      invocation, if it fails the system probably goes very wrong. Both 
FStringAppendCharacters
      and CFStringNormalize are void return type.

(2) The first line of path.toCharArray in normalizeJavaPath is a typo, 
it is supposed to be
      deleted. The implementation only goes toCharArray when it needs to 
go native. Currently
      it uses >0x80 as the fast path criteria, it is possible to expose 
some sun.text.normalizer's
      internal methods to have a "quick nfc" check, but I'm not sure how 
much the performance
      gain would be, but might worth some investigation later.

(3) Comments have been added for those override-able methods in 
UnixFileSystem.

(4) blank lines have been removed from dispatcher.c

(5) I don't think we need to do the HFS check, given we are only doing 
nfc/nfd stuff now, as
      long as it is a MacOSX, I believe its nfc/nfd layer will be there. 
Copyright has been re-copy/
      pasted and we now only use only bugid.

-Sherman



On 6/26/12 8:02 AM, Alan Bateman wrote:
> 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