Merge changes I72fcb546,I71047cd0
* changes:
Fix ISE during validation of new merge commit that was created via REST
CodeOwnerConfigValidator: Log cause if validation fails
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index 1a3bb94..57e72ef 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -114,17 +114,32 @@
public ImmutableSet<ChangedFile> compute(
Project.NameKey project, Config repoConfig, RevWalk revWalk, RevCommit revCommit)
throws IOException, PatchListNotAvailableException {
+ return compute(
+ project,
+ repoConfig,
+ revWalk,
+ revCommit,
+ codeOwnersPluginConfiguration.getMergeCommitStrategy(project));
+ }
+
+ public ImmutableSet<ChangedFile> compute(
+ Project.NameKey project,
+ Config repoConfig,
+ RevWalk revWalk,
+ RevCommit revCommit,
+ MergeCommitStrategy mergeCommitStrategy)
+ throws IOException, PatchListNotAvailableException {
requireNonNull(project, "project");
requireNonNull(repoConfig, "repoConfig");
requireNonNull(revWalk, "revWalk");
requireNonNull(revCommit, "revCommit");
+ requireNonNull(mergeCommitStrategy, "mergeCommitStrategy");
logger.atFine().log(
"computing changed files for revision %s in project %s", revCommit.name(), project);
if (revCommit.getParentCount() > 1
- && MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(
- codeOwnersPluginConfiguration.getMergeCommitStrategy(project))) {
+ && MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) {
return computeByComparingAgainstAutoMerge(project, revCommit);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index f0023ff..682ce27 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -30,6 +30,7 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.plugins.codeowners.JgitPath;
+import com.google.gerrit.plugins.codeowners.api.MergeCommitStrategy;
import com.google.gerrit.plugins.codeowners.backend.ChangedFile;
import com.google.gerrit.plugins.codeowners.backend.ChangedFiles;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
@@ -264,8 +265,23 @@
try {
CodeOwnerBackend codeOwnerBackend = codeOwnersPluginConfiguration.getBackend(branchNameKey);
+ // For merge commits, always do the comparison against the destination branch
+ // (MergeCommitStrategy.ALL_CHANGED_FILES). Doing the comparison against the auto-merge
+ // (MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION) is not possible because the auto-merge
+ // is loaded via the PatchListCache to which we cannot pass the rev walk which should be used
+ // to load the newly created merge commit and hence trying to load it from PatchListCache
+ // would fail with a missing object exception. This is why we use
+ // MergeCommitStrategy.ALL_CHANGED_FILES here even if
+ // MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION is configured.
ImmutableList<ChangedFile> modifiedCodeOwnerConfigFiles =
- changedFiles.compute(branchNameKey.project(), repoConfig, revWalk, revCommit).stream()
+ changedFiles
+ .compute(
+ branchNameKey.project(),
+ repoConfig,
+ revWalk,
+ revCommit,
+ MergeCommitStrategy.ALL_CHANGED_FILES)
+ .stream()
// filter out deletions (files without new path)
.filter(changedFile -> changedFile.newPath().isPresent())
// filter out non code owner config files
@@ -319,7 +335,7 @@
"failed to validate code owner config files in revision %s"
+ " (project = %s, branch = %s)",
revCommit.getName(), branchNameKey.project(), branchNameKey.branch());
- logger.atSevere().log(errorMessage);
+ logger.atSevere().withCause(e).log(errorMessage);
throw new StorageException(errorMessage, e);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
index dcc6464..320e720 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -1211,6 +1211,20 @@
@Test
public void validateMergeCommitCreatedViaTheCreateChangeRestApi() throws Exception {
+ testValidateMergeCommitCreatedViaTheCreateChangeRestApi();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "plugin.code-owners.mergeCommitStrategy",
+ value = "FILES_WITH_CONFLICT_RESOLUTION")
+ public void
+ validateMergeCommitCreatedViaTheCreateChangeRestApi_FilesWithConflictResolutionAsMergeCommitStrategy()
+ throws Exception {
+ testValidateMergeCommitCreatedViaTheCreateChangeRestApi();
+ }
+
+ private void testValidateMergeCommitCreatedViaTheCreateChangeRestApi() throws Exception {
// Create another branch.
String branchName = "stable";
createBranch(BranchNameKey.create(project, branchName));
