On Tue, 17 Dec 2024 04:05:36 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Hello Alan, I had thought about that, but then I looked at the
>> implementation of `Files.copy(...)` with `REPLACE_EXISTING`. If that option
>> is specified, the `Files.copy(...)` implementation first deletes the
>> existing file:
>>
>> // attempt to delete an existing file
>> if (replaceExisting) {
>> deleteIfExists(target);
>> }
>>
>> before it initiates the copying. It didn't feel right to be explicitly
>> creating a file (before the call to Files.copy) and then having it deleted
>> due to the use of `REPLACE_EXISTING`. So I went ahead with this alternate
>> approach.
>>
>> Would you still prefer that we use `REPLACE_EXISTING` here?
>
> Thank you Lance for the review. Alan, is it OK to proceed with this current
> change or do you think we should pursue the `REPLACE_EXISTING` option here?
I don't object to what you have, it's the mixing of old and new APIs that
jumped out. Maybe some day there will be some wider updates to the jar tool in
this area, e.g. it could have use a temp directory rather than a temp file.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22734#discussion_r1888058322
