gnodet-bot commented on code in PR #356: URL: https://github.com/apache/maven-clean-plugin/pull/356#discussion_r4112124682
########## src/main/java/org/apache/maven/plugins/clean/CleanOrphansMojo.java: ########## @@ -0,0 +1,230 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.plugins.clean; + +import java.io.IOException; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; + +import org.apache.maven.api.Project; +import org.apache.maven.api.Session; +import org.apache.maven.api.di.Inject; +import org.apache.maven.api.plugin.Log; +import org.apache.maven.api.plugin.MojoException; +import org.apache.maven.api.plugin.annotations.Mojo; +import org.apache.maven.api.plugin.annotations.Parameter; +import org.apache.maven.api.services.PathMatcherFactory; + +/** + * Removes orphaned build output directories left behind by sub-projects that have been + * deleted or renamed since the last build. + * + * <p>When a sub-project is removed from the reactor (e.g. after a {@code git pull} or a branch + * switch), its {@code target/} directory may remain on disk even though its {@code pom.xml} is + * gone. Because the sub-project is no longer part of the reactor, {@code mvn clean} cannot know + * about it and will skip it. This goal detects such orphaned directories and removes them.</p> + * + * <p>Detection heuristic: a direct child directory of the current project's {@code basedir} is + * considered orphaned when its <em>only non-hidden child</em> is the build output directory + * (typically {@code target/}). A freshly checked-out or live sub-project always has at least a + * {@code pom.xml} alongside its build directory, so a directory whose sole visible content is a + * {@code target/} folder can safely be assumed to be a leftover.</p> + * + * @since 3.5.1 + */ +@Mojo(name = "purge-check", defaultPhase = "initialize") +public class CleanOrphansMojo implements org.apache.maven.api.plugin.Mojo { + + /** + * The logger where to send information about what the plugin is doing. + */ + @Inject + private Log logger; + + /** + * The current project instance, used to resolve {@code basedir}. + */ + @Inject + private Project project; + + /** + * The build output directory of the current project. Used only to determine the directory + * name (e.g. {@code target}) so that the same name is recognised in sibling directories. + */ + @Parameter(defaultValue = "${project.build.directory}", readonly = true, required = true) + private Path directory; + + /** + * The current session. + */ + @Inject + private Session session; Review Comment: ⚠️ **Dead code:** `Session` is injected but never referenced anywhere in this class. In `CleanMojo`, it's used for the `fast` deletion feature (background cleaner). Since `CleanOrphansMojo` doesn't support fast deletion, both the field and its import can be removed. ```suggestion ``` ########## src/it/purge-check-orphan/verify.groovy: ########## @@ -0,0 +1,36 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// Verify purge-check behaviour: +// - sub-orphan/target/ must have been removed (only non-hidden child was target/) +// - sub-existing/target/ must remain (pom.xml + src/ are siblings of target/) + +def log = new File(basedir, 'build.log').text +assert log.contains('BUILD SUCCESS') : 'expected BUILD SUCCESS' + +// Orphaned build directory must be gone +def orphanTarget = new File(basedir, 'sub-orphan/target') +assert !orphanTarget.exists() : "sub-orphan/target/ should have been removed by purge-check, but still exists" + +// Active sub-project's build directory must be untouched +// (purge-check runs on initialize; clean:clean runs later and removes it — so we only +// check the log to confirm purge-check did NOT report it as orphaned) +assert !log.contains('sub-existing') || log.contains('Removing orphaned') == false \ + || !log.find('Removing orphaned.*sub-existing') \ + : "sub-existing/target/ must not be treated as orphaned" Review Comment: Confirming gnodet-bot's readability finding — and adding that the logic is also **partially dead**. Given that line 36 requires `log.contains('Removing orphaned build directory')`, condition B (`log.contains('Removing orphaned') == false`) can never be true at this point. Condition A (`!log.contains('sub-existing')`) is also redundant — if sub-existing doesn't appear in the log at all, condition C already covers it (no match for `'Removing orphaned.*sub-existing'`). The only condition that matters is C. Suggested simplification: ```suggestion assert !log.find('Removing orphaned.*sub-existing') \ : "sub-existing/target/ must not be treated as orphaned" ``` -- 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]
