Copilot commented on code in PR #539:
URL: https://github.com/apache/maven-ear-plugin/pull/539#discussion_r3667973016
##########
src/main/java/org/apache/maven/plugins/ear/EarMojo.java:
##########
@@ -776,34 +776,34 @@ private void changeManifestClasspath(
}
// Modify the classpath entries in the manifest
- final boolean forceClassPathModification =
- javaEEVersion.lt(JavaEEVersion.FIVE) ||
defaultLibBundleDir == null;
- final boolean classPathExtension = !skipClassPathModification ||
forceClassPathModification;
- for (EarModule otherModule : getModules()) {
- if (module.equals(otherModule)) {
- continue;
- }
- final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
- if (moduleClassPathIndex != -1) {
- if (otherModule.isClassPathItem()) {
- classPathElements.set(moduleClassPathIndex,
otherModule.getUri());
- } else {
- classPathElements.remove(moduleClassPathIndex);
+ if (!skipClassPathModification) {
+ final boolean forceClassPathModification =
+ javaEEVersion.lt(JavaEEVersion.FIVE) ||
defaultLibBundleDir == null;
+ final boolean classPathExtension = !skipClassPathModification
|| forceClassPathModification;
+ for (EarModule otherModule : getModules()) {
+ if (module.equals(otherModule)) {
+ continue;
+ }
+ final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
+ if (moduleClassPathIndex != -1) {
+ if (otherModule.isClassPathItem()) {
+ classPathElements.set(moduleClassPathIndex,
otherModule.getUri());
+ } else {
+ classPathElements.remove(moduleClassPathIndex);
+ }
+ } else if (otherModule.isClassPathItem() &&
classPathExtension) {
+ classPathElements.add(otherModule.getUri());
}
- } else if (otherModule.isClassPathItem() &&
classPathExtension) {
- classPathElements.add(otherModule.getUri());
}
- }
- // Remove provided modules from classpath
- for (EarModule otherModule : getProvidedEarModules()) {
- final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
- if (moduleClassPathIndex != -1) {
- classPathElements.remove(moduleClassPathIndex);
+ // Remove provided modules from classpath
+ for (EarModule otherModule : getProvidedEarModules()) {
+ final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
+ if (moduleClassPathIndex != -1) {
+ classPathElements.remove(moduleClassPathIndex);
+ }
}
- }
- if (!skipClassPathModification || !classPathElements.isEmpty() ||
classPathExists) {
classPath.setValue(StringUtils.join(classPathElements.iterator(), " "));
mf.getMainSection().addConfiguredAttribute(classPath);
Review Comment:
The `Class-Path` attribute is still being written back unconditionally
(lines 807-808), even when `skipClassPathModification` is true. This can still
rewrite the manifest (and can still set `Class-Path` to an empty string when
`classPathElements` is empty), contradicting the PR goal of leaving manifests
untouched. Suggested fix: only set/add the `Class-Path` attribute when
modification is allowed (e.g., move lines 807-808 inside the `if
(!skipClassPathModification)` block, or restore a guard equivalent to the
previous `if (!skipClassPathModification || !classPathElements.isEmpty() ||
classPathExists)` so you don’t add an empty attribute).
##########
src/main/java/org/apache/maven/plugins/ear/EarMojo.java:
##########
@@ -776,34 +776,34 @@ private void changeManifestClasspath(
}
// Modify the classpath entries in the manifest
- final boolean forceClassPathModification =
- javaEEVersion.lt(JavaEEVersion.FIVE) ||
defaultLibBundleDir == null;
- final boolean classPathExtension = !skipClassPathModification ||
forceClassPathModification;
- for (EarModule otherModule : getModules()) {
- if (module.equals(otherModule)) {
- continue;
- }
- final int moduleClassPathIndex =
findModuleInClassPathElements(classPathElements, otherModule);
- if (moduleClassPathIndex != -1) {
- if (otherModule.isClassPathItem()) {
- classPathElements.set(moduleClassPathIndex,
otherModule.getUri());
- } else {
- classPathElements.remove(moduleClassPathIndex);
+ if (!skipClassPathModification) {
+ final boolean forceClassPathModification =
+ javaEEVersion.lt(JavaEEVersion.FIVE) ||
defaultLibBundleDir == null;
+ final boolean classPathExtension = !skipClassPathModification
|| forceClassPathModification;
Review Comment:
Inside `if (!skipClassPathModification)`, `classPathExtension =
!skipClassPathModification || forceClassPathModification` will always evaluate
to `true` because `!skipClassPathModification` is guaranteed. This makes
`forceClassPathModification` effectively redundant here and obscures intent.
Suggested fix: simplify the boolean(s) to reflect the actual decision (e.g.,
remove `classPathExtension` and the `&& classPathExtension` checks, or set
`classPathExtension` based solely on what you still intend to gate within this
branch).
--
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]
