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

[Brian Burkhalter]

On Fri, 15 Sep 2023 13:21:30 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8073061: Improve file access checking; modify a test; add a test
>
> 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 (copyAttributes) {
> +                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`.

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

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