Copilot commented on code in PR #1381:
URL: https://github.com/apache/maven-scm/pull/1381#discussion_r3542601509
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/info/JGitInfoCommand.java:
##########
@@ -81,11 +84,30 @@ protected ScmResult executeCommand(
}
}
- protected InfoItem getInfoItem(Repository repository, ObjectId
headObjectId, File file) throws IOException {
- RevCommit commit = getMostRecentCommitForPath(repository,
headObjectId, JGitUtils.toNormalizedFilePath(file));
+ protected InfoItem getInfoItem(Repository repository, ObjectId
headObjectId, File file, boolean skipMergeCommits)
+ throws IOException {
+ RevCommit commit = getMostRecentCommitForPath(
+ repository, headObjectId,
JGitUtils.toNormalizedFilePath(file), skipMergeCommits);
return getInfoItem(commit, file);
}
+ /**
+ * Returns the most recent commit reachable from {@code headObjectId},
optionally ignoring merge commits
+ * (mimics {@code git log -1 --no-merges} when {@code skipMergeCommits} is
{@code true}).
+ */
+ private RevCommit getMostRecentCommit(Repository repository, ObjectId
headObjectId, boolean skipMergeCommits)
+ throws IOException {
+ try (RevWalk revWalk = new RevWalk(repository)) {
+ RevCommit headCommit = revWalk.parseCommit(headObjectId);
+ if (!skipMergeCommits) {
+ return headCommit;
+ }
+ revWalk.markStart(headCommit);
+ revWalk.setRevFilter(RevFilter.NO_MERGES);
+ return revWalk.next();
+ }
Review Comment:
`getMostRecentCommit` sets `RevFilter.NO_MERGES` but relies on RevWalk’s
default traversal order and doesn’t guard against `revWalk.next()` returning
null. This can pick a non-deterministic commit (not necessarily the most recent
by commit time) and can lead to a later NPE when the returned commit is used.
Consider explicitly sorting by commit time (to mimic `git log -1`) and falling
back safely if no non-merge commit is found (e.g., shallow history).
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/main/java/org/apache/maven/scm/provider/git/jgit/command/info/JGitInfoCommand.java:
##########
@@ -81,11 +84,30 @@ protected ScmResult executeCommand(
}
}
- protected InfoItem getInfoItem(Repository repository, ObjectId
headObjectId, File file) throws IOException {
- RevCommit commit = getMostRecentCommitForPath(repository,
headObjectId, JGitUtils.toNormalizedFilePath(file));
+ protected InfoItem getInfoItem(Repository repository, ObjectId
headObjectId, File file, boolean skipMergeCommits)
+ throws IOException {
+ RevCommit commit = getMostRecentCommitForPath(
+ repository, headObjectId,
JGitUtils.toNormalizedFilePath(file), skipMergeCommits);
return getInfoItem(commit, file);
Review Comment:
`getMostRecentCommitForPath(...)` may return null (e.g., file has no commits
/ is newly added), but `getInfoItem(commit, file)` dereferences the commit.
This currently risks an NPE in the JGit provider where the gitexe provider
would just return an InfoItem without revision metadata when `git log -1 --
<file>` produces no output. Handle the null case explicitly and return a
minimal InfoItem.
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/test/java/org/apache/maven/scm/provider/git/jgit/command/info/JGitInfoCommandTest.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.scm.provider.git.jgit.command.info;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+
+import org.apache.maven.scm.CommandParameter;
+import org.apache.maven.scm.CommandParameters;
+import org.apache.maven.scm.ScmFileSet;
+import org.apache.maven.scm.command.info.InfoScmResult;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.MergeCommand;
+import org.eclipse.jgit.api.MergeResult;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.StoredConfig;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/**
+ * Unit tests for the {@code skipMergeCommits} handling of {@link
JGitInfoCommand}.
+ * A repository whose {@code HEAD} is a merge commit is built with the JGit
API, then the
+ * {@code info} command is invoked with and without the {@link
CommandParameter#SCM_SKIP_MERGE_COMMITS} flag.
+ */
+class JGitInfoCommandTest {
+
+ @TempDir
+ File workDir;
+
+ @Test
+ void includesMergeCommitWhenSkipMergeCommitsIsFalse() throws Exception {
+ ObjectId mergeCommit = buildRepositoryWithMergeHead();
+
+ CommandParameters parameters = new CommandParameters();
+ parameters.setString(CommandParameter.SCM_SKIP_MERGE_COMMITS,
Boolean.FALSE.toString());
+
+ InfoScmResult result = info(parameters);
+
+ assertNotNull(result);
+ assertEquals(
+ mergeCommit.getName(),
+ result.getInfoItems().get(0).getRevision(),
+ "HEAD merge commit must be reported when
skipMergeCommits=false");
Review Comment:
The tests index `result.getInfoItems().get(0)` without first asserting the
list size. If the command unexpectedly returns zero items, the test will fail
with an IndexOutOfBoundsException rather than a clear assertion message. Add an
explicit size assertion before accessing element 0.
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/test/java/org/apache/maven/scm/provider/git/jgit/command/info/JGitInfoCommandTest.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.scm.provider.git.jgit.command.info;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+
+import org.apache.maven.scm.CommandParameter;
+import org.apache.maven.scm.CommandParameters;
+import org.apache.maven.scm.ScmFileSet;
+import org.apache.maven.scm.command.info.InfoScmResult;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.MergeCommand;
+import org.eclipse.jgit.api.MergeResult;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.StoredConfig;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/**
+ * Unit tests for the {@code skipMergeCommits} handling of {@link
JGitInfoCommand}.
+ * A repository whose {@code HEAD} is a merge commit is built with the JGit
API, then the
+ * {@code info} command is invoked with and without the {@link
CommandParameter#SCM_SKIP_MERGE_COMMITS} flag.
+ */
+class JGitInfoCommandTest {
+
+ @TempDir
+ File workDir;
+
+ @Test
+ void includesMergeCommitWhenSkipMergeCommitsIsFalse() throws Exception {
+ ObjectId mergeCommit = buildRepositoryWithMergeHead();
+
+ CommandParameters parameters = new CommandParameters();
+ parameters.setString(CommandParameter.SCM_SKIP_MERGE_COMMITS,
Boolean.FALSE.toString());
+
+ InfoScmResult result = info(parameters);
+
+ assertNotNull(result);
+ assertEquals(
+ mergeCommit.getName(),
+ result.getInfoItems().get(0).getRevision(),
+ "HEAD merge commit must be reported when
skipMergeCommits=false");
+ }
+
+ @Test
+ void skipsMergeCommitByDefault() throws Exception {
+ ObjectId mergeCommit = buildRepositoryWithMergeHead();
+
+ InfoScmResult result = info(new CommandParameters());
+
+ assertNotNull(result);
+ assertNotEquals(
+ mergeCommit.getName(),
+ result.getInfoItems().get(0).getRevision(),
+ "merge commit must be skipped by default, a non-merge commit
must be reported");
+ }
+
+ private InfoScmResult info(CommandParameters parameters) throws Exception {
+ // executeCommand is package-private accessible and ignores the
repository argument
+ return (InfoScmResult) new JGitInfoCommand().executeCommand(null, new
ScmFileSet(workDir), parameters);
+ }
+
+ /**
+ * Builds a repository whose {@code HEAD} is a no-fast-forward merge
commit (two parents).
+ *
+ * @return the id of the merge commit which is now {@code HEAD}
+ */
+ private ObjectId buildRepositoryWithMergeHead() throws Exception {
+ try (Git git = Git.init().setDirectory(workDir).call()) {
+ StoredConfig config = git.getRepository().getConfig();
+ config.setString("user", null, "name", "Test User");
+ config.setString("user", null, "email", "[email protected]");
+ config.setBoolean("commit", null, "gpgsign", false);
+ config.save();
+
+ commit(git, "a.txt");
+ String mainBranch = git.getRepository().getBranch();
+
+ git.checkout().setCreateBranch(true).setName("feature").call();
+ commit(git, "b.txt");
+
+ git.checkout().setName(mainBranch).call();
+ commit(git, "c.txt");
+
+ MergeResult mergeResult = git.merge()
+ .include(git.getRepository().resolve("feature"))
+ .setFastForward(MergeCommand.FastForwardMode.NO_FF)
+ .setMessage("merge feature")
+ .call();
+ return mergeResult.getNewHead();
Review Comment:
The test repository setup assumes the JGit merge succeeded, but
`MergeResult#getNewHead()` can be null if the merge is not performed (e.g.,
already up-to-date or failure). Add assertions on merge status/new head to make
the test failure mode clearer and avoid returning null into later assertions.
##########
maven-scm-providers/maven-scm-providers-git/maven-scm-provider-jgit/src/test/java/org/apache/maven/scm/provider/git/jgit/command/info/JGitInfoCommandTest.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.scm.provider.git.jgit.command.info;
+
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+
+import org.apache.maven.scm.CommandParameter;
+import org.apache.maven.scm.CommandParameters;
+import org.apache.maven.scm.ScmFileSet;
+import org.apache.maven.scm.command.info.InfoScmResult;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.MergeCommand;
+import org.eclipse.jgit.api.MergeResult;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.StoredConfig;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/**
+ * Unit tests for the {@code skipMergeCommits} handling of {@link
JGitInfoCommand}.
+ * A repository whose {@code HEAD} is a merge commit is built with the JGit
API, then the
+ * {@code info} command is invoked with and without the {@link
CommandParameter#SCM_SKIP_MERGE_COMMITS} flag.
+ */
+class JGitInfoCommandTest {
+
+ @TempDir
+ File workDir;
+
+ @Test
+ void includesMergeCommitWhenSkipMergeCommitsIsFalse() throws Exception {
+ ObjectId mergeCommit = buildRepositoryWithMergeHead();
+
+ CommandParameters parameters = new CommandParameters();
+ parameters.setString(CommandParameter.SCM_SKIP_MERGE_COMMITS,
Boolean.FALSE.toString());
+
+ InfoScmResult result = info(parameters);
+
+ assertNotNull(result);
+ assertEquals(
+ mergeCommit.getName(),
+ result.getInfoItems().get(0).getRevision(),
+ "HEAD merge commit must be reported when
skipMergeCommits=false");
+ }
+
+ @Test
+ void skipsMergeCommitByDefault() throws Exception {
+ ObjectId mergeCommit = buildRepositoryWithMergeHead();
+
+ InfoScmResult result = info(new CommandParameters());
+
+ assertNotNull(result);
+ assertNotEquals(
+ mergeCommit.getName(),
+ result.getInfoItems().get(0).getRevision(),
+ "merge commit must be skipped by default, a non-merge commit
must be reported");
Review Comment:
Same as above: add an explicit assertion on `result.getInfoItems().size()`
before indexing element 0, so failures are reported with a clear message
instead of IndexOutOfBoundsException.
--
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]
