Copilot commented on code in PR #2198:
URL: https://github.com/apache/maven-resolver/pull/2198#discussion_r4250755962
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +214,22 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ if (hardLink) {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
+ }
Review Comment:
The explicit directory check throws an `IOException` with a clear message,
but that exception is immediately caught by the broad fallback catch (which
also logs a misleading ‘Hard link not supported’ warning) and then overwritten
by whatever `copyWithTimestamp` throws. If the intent is to preserve previous
error semantics for directory destinations, avoid routing this case through the
hard-link fallback catch (e.g., handle the directory case separately and
proceed directly to `copyWithTimestamp` without warning, or rethrow without
swallowing the more specific message).
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +214,22 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ if (hardLink) {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
+ }
+ try {
+ Files.deleteIfExists(dstPath);
+ Files.createLink(dstPath, srcPath);
+ } catch (UnsupportedOperationException |
IllegalArgumentException | IOException e) {
+ // cross-device link (EXDEV), provider mismatch (e.g.
Jimfs), or unsupported FS — fall back to copy
+ logger.warn("Hard link not supported for {}, falling back
to copy: {}", dstPath, e.getMessage());
+ pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ }
Review Comment:
Deleting the destination before the hard-link attempt can cause data loss if
both the hard-link and the subsequent copy fail (e.g., permissions/disk-full
during copy): the previously-installed artifact at `dstPath` is removed and not
restored. To avoid leaving the local repository in a worse state on failure,
use a temp destination strategy (create link/copy to a temp path, then
move/replace) or defer removal of the existing destination until after a
successful replacement.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +214,22 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ if (hardLink) {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
+ }
+ try {
+ Files.deleteIfExists(dstPath);
+ Files.createLink(dstPath, srcPath);
+ } catch (UnsupportedOperationException |
IllegalArgumentException | IOException e) {
+ // cross-device link (EXDEV), provider mismatch (e.g.
Jimfs), or unsupported FS — fall back to copy
+ logger.warn("Hard link not supported for {}, falling back
to copy: {}", dstPath, e.getMessage());
+ pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ }
+ } else {
Review Comment:
The PR description states the installer will ‘silently’ fall back to copy,
but the implementation logs a WARN on every hard-link failure. This can create
noisy builds (especially in CI with cross-device mounts) and is inconsistent
with the documented behavior. Consider removing the log entirely, or
downgrading it to DEBUG/TRACE (and ideally include the throwable when enabled)
so fallback remains silent by default.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultInstaller.java:
##########
@@ -205,7 +214,22 @@ private void install(RepositorySystemSession session,
RequestTrace trace, Artifa
throw new IllegalStateException("cannot install " + dstPath +
" to same path");
}
- pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ if (hardLink) {
+ Files.createDirectories(dstPath.getParent());
+ if (Files.isDirectory(dstPath)) {
+ throw new IOException("destination is a directory: " +
dstPath);
+ }
+ try {
+ Files.deleteIfExists(dstPath);
+ Files.createLink(dstPath, srcPath);
+ } catch (UnsupportedOperationException |
IllegalArgumentException | IOException e) {
+ // cross-device link (EXDEV), provider mismatch (e.g.
Jimfs), or unsupported FS — fall back to copy
+ logger.warn("Hard link not supported for {}, falling back
to copy: {}", dstPath, e.getMessage());
+ pathProcessor.copyWithTimestamp(srcPath, dstPath);
+ }
Review Comment:
The explicit directory check throws an `IOException` with a clear message,
but that exception is immediately caught by the broad fallback catch (which
also logs a misleading ‘Hard link not supported’ warning) and then overwritten
by whatever `copyWithTimestamp` throws. If the intent is to preserve previous
error semantics for directory destinations, avoid routing this case through the
hard-link fallback catch (e.g., handle the directory case separately and
proceed directly to `copyWithTimestamp` without warning, or rethrow without
swallowing the more specific message).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
