On Fri, 14 Jul 2023 16:09:15 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> minor - remove duplicated code comment > > src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 666: > >> 664: return false; >> 665: } >> 666: return Files.isSameFile(javaBasePath, javaBaseInDefaultPath); > > Files.isSameFile checks the file contents, this doesn't look right here. > Shouldn't this be > > return javaBasePath.toRealPath().equals(javaBaseInDefaultPath.toRealPath()) ; > > > Also, just to say that !Files.exists should be Files.notExists. Hello Alan, > Files.isSameFile checks the file contents, this doesn't look right here. I was always under the impression that `Files.isSameFile()` only checks file paths and attributes for testing whether they are same. The API doc of `Files.isSameFile()` too don't mention about contents and looking at the implementation in `UnixFileSystemProvider` and `ZipFileSystemProvider` (for example), then don't seem to check the contents either. There's however a mention in the API doc of `Files.isSameFile()` which says: > and depending on the implementation, may require to open or access both files. So I'm guessing that, that's what allows `FileSystemProvider` implementations to actually check the content of the files. I'll change this to use `toRealPath()` as you noted. > Also, just to say that !Files.exists should be Files.notExists. I've overlooked Files.notExists. I'll update this as well. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/11943#discussion_r1264264298
