gnodet-bot commented on code in PR #13652:
URL: https://github.com/apache/maven/pull/13652#discussion_r4325569039
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -337,8 +337,10 @@ if exist "%JVM_CONFIG_TEMP%" (
type "%JVM_CONFIG_TEMP%"
)
set /p JVM_CONFIG_MAVEN_OPTS=<"%JVM_CONFIG_TEMP%" 2>nul
- rem Retry once after a brief delay if the read failed (Windows Defender file
lock)
- if not defined JVM_CONFIG_MAVEN_OPTS (
+ rem Retry once after a brief delay if the read returned empty (Windows
Defender file lock).
+ rem Note: use string comparison instead of 'if not defined' because 'set
JVM_CONFIG_MAVEN_OPTS='
+ rem (empty assignment) counts as defined, so 'if not defined' would never
trigger the retry.
+ if "%JVM_CONFIG_MAVEN_OPTS%"=="" (
Review Comment:
⚠️ **Parse-time expansion bug — this condition is always true.**
cmd.exe expands `%JVM_CONFIG_MAVEN_OPTS%` when it first parses the opening
`(` at line 334 — before `set /p` on line 339 has run. At parse time,
`JVM_CONFIG_MAVEN_OPTS` is still the empty string set at line 283, so this
becomes `if ""==""` — unconditionally true.
The `ping -n 2 127.0.0.1` retry will execute on **every** `mvn` invocation
with a `.mvn/jvm.config` file, adding ~1 second of unnecessary delay.
Fix: use delayed expansion (`!JVM_CONFIG_MAVEN_OPTS!` with `setlocal
EnableDelayedExpansion`) or move the check outside this compound block.
```suggestion
if "!JVM_CONFIG_MAVEN_OPTS!"=="" (
```
(Requires adding `setlocal EnableDelayedExpansion` before this block and
`endlocal & set "JVM_CONFIG_MAVEN_OPTS=%JVM_CONFIG_MAVEN_OPTS%"` after it.)
--
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]
