gnodet-bot commented on code in PR #362:
URL:
https://github.com/apache/maven-clean-plugin/pull/362#discussion_r4121873329
##########
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:
💡 `ProtoSession.getRootDirectory()` is annotated `@Nonnull` in the Maven 4
API. This null guard protects against something the contract says cannot happen.
Two concerns:
1. **Masking upstream bugs** — if a future Maven version violates
`@Nonnull`, this silently falls back instead of surfacing the contract
violation.
2. **`java.io.tmpdir` fallback** — writing a `.clean` staging directory to
the shared system temp could collide with other processes, and OS temp cleaners
could remove it mid-build.
If the intent is purely defensive (e.g. for test harnesses where session is
partially initialized), a comment explaining *why* the `@Nonnull` method might
return null would help future maintainers. Otherwise, trusting the contract and
dropping the null check (as on `master`) seems safer — a `NullPointerException`
from a broken contract is a clearer signal than a silent fallback to
`java.io.tmpdir`.
--
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]
