gnodet-bot commented on code in PR #362:
URL:
https://github.com/apache/maven-clean-plugin/pull/362#discussion_r4121884072
##########
src/main/java/org/apache/maven/plugins/clean/CleanMojo.java:
##########
@@ -276,7 +273,10 @@ public void execute() {
if (fast && session != null) {
Path tmpDir = fastDir;
if (tmpDir == null) {
- tmpDir =
session.getRootDirectory().resolve("target").resolve(".clean");
+ Path rootDir = session.getRootDirectory();
+ tmpDir = rootDir != null
+ ? rootDir.resolve("target").resolve(".clean")
+ :
Path.of(System.getProperty("java.io.tmpdir")).resolve(".clean");
Review Comment:
💡 `Session.getRootDirectory()` is `@Nonnull` in the Maven API — it throws
`IllegalStateException` if the root directory is not set, but never returns
`null`. This null check creates a dead branch: the `java.io.tmpdir` fallback is
unreachable.
If the intent is to guard against the `IllegalStateException` (e.g. embedded
scenarios), catch that instead:
```suggestion
Path rootDir;
try {
rootDir = session.getRootDirectory();
} catch (IllegalStateException e) {
rootDir = null;
}
tmpDir = rootDir != null
? rootDir.resolve("target").resolve(".clean")
:
Path.of(System.getProperty("java.io.tmpdir")).resolve(".clean");
```
Alternatively, if the fallback is not actually needed (session is already
null-checked, and a real session always has a root directory), just drop the
guard and keep the original one-liner.
##########
src/main/java/org/apache/maven/plugins/clean/CleanMojo.java:
##########
@@ -182,24 +182,21 @@ public class CleanMojo implements
org.apache.maven.api.plugin.Mojo {
private boolean excludeDefaultDirectories;
/**
- * Enables fast clean if possible. If set to {@code true}, when the plugin
is executed, a directory to
- * be deleted will be atomically moved inside the {@code
maven.clean.fastDir} directory and a thread will
- * be launched to delete the needed files in the background. When the
build is completed, maven will wait
- * until all the files have been deleted. If any problem occurs during
the atomic move of the directories,
- * the plugin will default to the traditional deletion mechanism.
+ * Enables fast clean. When set to {@code true}, each directory to be
deleted is first atomically moved
+ * inside the {@code maven.clean.fastDir} staging directory, immediately
freeing the original path, and
+ * the actual file deletion is then performed in the background. If an
atomic move is not supported
+ * (e.g. cross-device), the plugin falls back to immediate synchronous
deletion transparently.
*
- * <p>Note that for small projects with few files to delete, the "fast"
clean tends to be actually slower.
- * It is also more at risk that errors occurring during the deletion of a
file get unnoticed, or are noticed
- * late in the build process. This option should be used only when it has
been verified to be worth.</p>
+ * <p>This is the default mode as of 4.0.0: the atomic move is essentially
free, so even small projects
+ * benefit from the freed directory being available immediately. The
background deletion of the staging
+ * area does not block the build and any failure there does not affect
build correctness.</p>
Review Comment:
⚠️ This paragraph contradicts the `failOnError` Javadoc updated in this same
PR (lines 152-155). The `failOnError` doc says that when both `fast` and
`failOnError` are `true` (the defaults), deletion runs **synchronously** so
errors fail the build. But this paragraph says background deletion "does not
affect build correctness" — which is only true when `failOnError=false`.
Since this PR depends on #355 (still open), and #355 is what makes
`failOnError` effective in fast mode, consider qualifying the statement:
```suggestion
* benefit from the freed directory being available immediately. When
{@link #failOnError} is {@code true}
* (the default), the actual deletion runs synchronously so that errors
can still fail the build;
* when {@code failOnError} is {@code false}, deletion is fully
asynchronous.</p>
```
--
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]
