RFR: 8073061: (fs) Files.copy(foo, bar, REPLACE_EXISTING) deletes bar even if foo is not readable [v5]

[Alan Bateman]

On Fri, 15 Sep 2023 16:25:02 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> src/java.base/share/classes/java/nio/file/CopyMoveHelper.java line 138:
>> 
>>> 136: 
>>> 137:             Files.deleteIfExists(target);
>>> 138:         } else if (Files.exists(target))
>> 
>> Looking at again and I think this can be dropped from the cross provider case. Files.copy(InputStream, Path) can optionally replace the target so this only need a delete of the target the the source is a directory. So something like this might be simpler:
>> 
>> 
>> @@ -69,6 +71,14 @@ static CopyOptions parse(CopyOption... options) {
>>              }
>>              return result;
>>          }
>> +
>> +        CopyOption[] replaceExitingOrEmpty() {
>> +            if (replaceExisting) {
>> +                return new CopyOption[] { StandardCopyOption.REPLACE_EXISTING };
>> +            } else {
>> +                return new CopyOption[0];
>> +            }
>> +        }
>>      }
>>  
>>      /**
>> @@ -129,18 +139,14 @@ static void copyToForeignTarget(Path source, Path target,
>>          if (sourceAttrs.isSymbolicLink())
>>              throw new IOException("Copying of symbolic links not supported");
>>  
>> -        // delete target if it exists and REPLACE_EXISTING is specified
>> -        if (opts.replaceExisting) {
>> -            Files.deleteIfExists(target);
>> -        } else if (Files.exists(target))
>> -            throw new FileAlreadyExistsException(target.toString());
>> -
>>          // create directory or copy file
>>          if (sourceAttrs.isDirectory()) {
>> +            if (opts.replaceExisting)
>> +                Files.deleteIfExists(target);
>>              Files.createDirectory(target);
>>          } else {
>>              try (InputStream in = Files.newInputStream(source)) {
>> -                Files.copy(in, target);
>> +                Files.copy(in, target, opts.replaceExitingOrEmpty());
>>              }
>>          }
>
> This change looks reasonable, but it causes `CopyAndMove` to fail at line 777 in
> 
> * Test: copy regular file, target exists
> 
> due to an exception in `Files.copy`:
> 
> try (InputStream in = Files.newInputStream(source)) {
>     Files.copy(in, target, opts.replaceExistingOrEmpty());
> }
> 
> The new method `replaceExistingOrEmpty` returns a zero-length array as `copyAttributes` is `false`.

There may be a typo, it should be an empty array when replaceExisting (not copyAttributes) is false. The main point is that copy from an input stream to a file already supports REPLACE_EXISTING option.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15501#discussion_r1327547600