Merge "Fix submit rule for changes for which the target branch doesn't exist"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index b2e2127..97ece59 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -32,7 +32,6 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -120,11 +119,9 @@
* @param start number of owned paths to skip
* @param limit the max number of owned paths that should be returned (0 = unlimited)
* @return the paths of the files in the given patch set that are owned by the specified account
- * @throws ResourceConflictException if the destination branch of the change no longer exists
*/
public ImmutableList<OwnedChangedFile> getOwnedPaths(
- ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit)
- throws ResourceConflictException {
+ ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit) {
try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
logger.atFine().log(
"compute owned paths for account %d (project = %s, change = %d, patch set = %d,"
@@ -190,7 +187,7 @@
* @return whether the given change has sufficient code owner approvals to be submittable
*/
public boolean isSubmittable(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
logger.atFine().log(
"checking if change %d in project %s is submittable",
@@ -235,8 +232,7 @@
* @see #getFileStatuses(CodeOwnerConfigHierarchy, CodeOwnerResolver, ChangeNotes)
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
- ChangeNotes changeNotes, int start, int limit)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ ChangeNotes changeNotes, int start, int limit) throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
logger.atFine().log(
@@ -289,7 +285,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputation.start()) {
logger.atFine().log(
@@ -358,8 +354,13 @@
overrides);
BranchNameKey branch = changeNotes.getChange().getDest();
- ObjectId revision = getDestBranchRevision(changeNotes.getChange());
- logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
+ Optional<ObjectId> revision = getDestBranchRevision(changeNotes.getChange());
+ if (revision.isPresent()) {
+ logger.atFine().log(
+ "dest branch %s has revision %s", branch.branch(), revision.get().name());
+ } else {
+ logger.atFine().log("dest branch %s does not exist", branch.branch());
+ }
CodeOwnerResolverResult globalCodeOwners =
codeOwnerResolver.resolveGlobalCodeOwners(changeNotes.getProjectName());
@@ -383,7 +384,7 @@
codeOwnerConfigHierarchy,
codeOwnerResolver,
branch,
- revision,
+ revision.orElse(null),
globalCodeOwners,
enableImplicitApproval ? changeOwner : null,
reviewerAccountIds,
@@ -410,7 +411,7 @@
@VisibleForTesting
public Stream<FileCodeOwnerStatus> getFileStatusesForAccount(
ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
requireNonNull(patchSet, "patchSet");
requireNonNull(accountId, "accountId");
@@ -430,8 +431,13 @@
logger.atFine().log("requiredApproval = %s", requiredApproval);
BranchNameKey branch = changeNotes.getChange().getDest();
- ObjectId revision = getDestBranchRevision(changeNotes.getChange());
- logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
+ Optional<ObjectId> revision = getDestBranchRevision(changeNotes.getChange());
+ if (revision.isPresent()) {
+ logger.atFine().log(
+ "dest branch %s has revision %s", branch.branch(), revision.get().name());
+ } else {
+ logger.atFine().log("dest branch %s does not exist", branch.branch());
+ }
FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
logger.atFine().log("fallbackCodeOwner = %s", fallbackCodeOwners);
@@ -447,7 +453,7 @@
codeOwnerConfigHierarchy,
codeOwnerResolver,
branch,
- revision,
+ revision.orElse(null),
/* globalCodeOwners= */ CodeOwnerResolverResult.createEmpty(),
// Do not check for implicit approvals since implicit approvals of other users
// should be ignored. For the given account we do not need to check for
@@ -503,7 +509,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
- ObjectId revision,
+ @Nullable ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -568,7 +574,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
- ObjectId revision,
+ @Nullable ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -1004,18 +1010,18 @@
* <p>This is the revision from which the code owner configs should be read when computing code
* owners for the files that are touched in the change.
*
- * @throws ResourceConflictException thrown if the destination branch is not found, e.g. when the
- * branch got deleted after the change was created
+ * @return the current revision of the destination branch of the given change, {@link
+ * Optional#empty()} if the destination branch is not found (e.g. when the initial change is
+ * uploaded to an unborn branch or when the branch got deleted after the change was created)
*/
- private ObjectId getDestBranchRevision(Change change)
- throws IOException, ResourceConflictException {
+ private Optional<ObjectId> getDestBranchRevision(Change change) throws IOException {
try (Repository repository = repoManager.openRepository(change.getProject());
RevWalk rw = new RevWalk(repository)) {
Ref ref = repository.exactRef(change.getDest().branch());
if (ref == null) {
- throw new ResourceConflictException("destination branch not found");
+ return Optional.empty();
}
- return rw.parseCommit(ref.getObjectId());
+ return Optional.of(rw.parseCommit(ref.getObjectId()));
}
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index 7ba3352..8fb6d09 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -18,6 +18,7 @@
import static java.util.Objects.requireNonNull;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
@@ -42,6 +43,10 @@
* config in the root folder of the branch. The same as any other parent it can be ignored (e.g. by
* using {@code set noparent} in the root code owner config if the {@code find-owners} backend is
* used).
+ *
+ * <p>Visiting the code owner configs also works for non-existing branches (provided branch revision
+ * is {@code null}). In this case only the default code owner config in {@code refs/meta/config} is
+ * visited (if it exists).
*/
public class CodeOwnerConfigHierarchy {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -65,7 +70,8 @@
* the path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner
@@ -73,7 +79,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
CodeOwnerConfigVisitor codeOwnerConfigVisitor) {
visit(
@@ -89,7 +95,8 @@
* the path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner
@@ -99,7 +106,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
CodeOwnerConfigVisitor codeOwnerConfigVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -119,7 +126,8 @@
* path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param pathCodeOwnersVisitor visitor that should be invoked for the applying path code owners
@@ -128,7 +136,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -153,7 +161,8 @@
* (e.g. for large changes).
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absoluteFilePath the path for which the code owner configs should be visited; the path
* must be absolute; must be the path of a file; the path may or may not exist
* @param pathCodeOwnersVisitor visitor that should be invoked for the applying path code owners
@@ -162,7 +171,7 @@
*/
public void visitForFile(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absoluteFilePath,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -177,13 +186,12 @@
private void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
Path startFolder,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
requireNonNull(branchNameKey, "branch");
- requireNonNull(revision, "revision");
requireNonNull(absolutePath, "absolutePath");
requireNonNull(pathCodeOwnersVisitor, "pathCodeOwnersVisitor");
requireNonNull(parentCodeOwnersIgnoredCallback, "parentCodeOwnersIgnoredCallback");
@@ -191,47 +199,52 @@
logger.atFine().log(
"visiting code owner configs for '%s' in branch '%s' in project '%s' (revision = '%s')",
- absolutePath, branchNameKey.shortName(), branchNameKey.project(), revision.name());
+ absolutePath,
+ branchNameKey.shortName(),
+ branchNameKey.project(),
+ revision != null ? revision.name() : "n/a");
- // Next path in which we look for a code owner configuration. We start at the given folder and
- // then go up the parent hierarchy.
- Path ownerConfigFolder = startFolder;
+ if (revision != null) {
+ // Next path in which we look for a code owner configuration. We start at the given folder and
+ // then go up the parent hierarchy.
+ Path ownerConfigFolder = startFolder;
- // Iterate over the parent code owner configurations.
- while (ownerConfigFolder != null) {
- // Read code owner config and invoke the codeOwnerConfigVisitor if the code owner config
- // exists.
- logger.atFine().log("inspecting code owner config for %s", ownerConfigFolder);
- CodeOwnerConfig.Key codeOwnerConfigKey =
- CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
- Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(
- transientCodeOwnerConfigCache, codeOwnerConfigKey, revision, absolutePath);
- if (pathCodeOwners.isPresent()) {
- logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
- boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
- boolean ignoreParentCodeOwners =
- pathCodeOwners.get().resolveCodeOwnerConfig().get().ignoreParentCodeOwners();
- if (ignoreParentCodeOwners) {
- parentCodeOwnersIgnoredCallback.accept(codeOwnerConfigKey);
+ // Iterate over the parent code owner configurations.
+ while (ownerConfigFolder != null) {
+ // Read code owner config and invoke the codeOwnerConfigVisitor if the code owner config
+ // exists.
+ logger.atFine().log("inspecting code owner config for %s", ownerConfigFolder);
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
+ Optional<PathCodeOwners> pathCodeOwners =
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCache, codeOwnerConfigKey, revision, absolutePath);
+ if (pathCodeOwners.isPresent()) {
+ logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
+ boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
+ boolean ignoreParentCodeOwners =
+ pathCodeOwners.get().resolveCodeOwnerConfig().get().ignoreParentCodeOwners();
+ if (ignoreParentCodeOwners) {
+ parentCodeOwnersIgnoredCallback.accept(codeOwnerConfigKey);
+ }
+ logger.atFine().log(
+ "visitFurtherCodeOwnerConfigs = %s, ignoreParentCodeOwners = %s",
+ visitFurtherCodeOwnerConfigs, ignoreParentCodeOwners);
+ if (!visitFurtherCodeOwnerConfigs || ignoreParentCodeOwners) {
+ // If no further code owner configs should be visited or if all parent code owner
+ // configs are ignored, we are done.
+ // No need to check further parent code owner configs (including the default code owner
+ // config in refs/meta/config which is the parent of the root code owner config), hence
+ // we can return here.
+ return;
+ }
+ } else {
+ logger.atFine().log("no code owner config found in %s", ownerConfigFolder);
}
- logger.atFine().log(
- "visitFurtherCodeOwnerConfigs = %s, ignoreParentCodeOwners = %s",
- visitFurtherCodeOwnerConfigs, ignoreParentCodeOwners);
- if (!visitFurtherCodeOwnerConfigs || ignoreParentCodeOwners) {
- // If no further code owner configs should be visited or if all parent code owner configs
- // are ignored, we are done.
- // No need to check further parent code owner configs (including the default code owner
- // config in refs/meta/config which is the parent of the root code owner config), hence we
- // can return here.
- return;
- }
- } else {
- logger.atFine().log("no code owner config found in %s", ownerConfigFolder);
+
+ // Continue the loop with the next parent folder.
+ ownerConfigFolder = ownerConfigFolder.getParent();
}
-
- // Continue the loop with the next parent folder.
- ownerConfigFolder = ownerConfigFolder.getParent();
}
if (!RefNames.REFS_CONFIG.equals(branchNameKey.branch())) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index ec53945..58a500e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -21,8 +21,6 @@
import com.google.gerrit.entities.LegacySubmitRequirement;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.annotations.Exports;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
@@ -95,11 +93,6 @@
return Optional.of(getSubmitRecord(changeData.notes()));
}
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- "Couldn't evaluate code owner statuses for patch set %d of change %d.",
- changeData.currentPatchSet().id().get(), changeData.change().getId().get());
- return Optional.of(notReady());
} catch (Exception e) {
// Whether the exception should be treated as RULE_ERROR.
// RULE_ERROR must only be returned if the exception is caused by user misconfiguration (e.g.
@@ -162,7 +155,7 @@
}
private SubmitRecord getSubmitRecord(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
return codeOwnerApprovalCheck.isSubmittable(changeNotes) ? ok() : notReady();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 0e0ec81..4e81f46 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -23,7 +23,6 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.events.ReviewerAddedListener;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -206,23 +205,15 @@
Project.NameKey projectName, Change.Id changeId, Account.Id reviewerAccountId) {
ChangeNotes changeNotes = changeNotesFactory.create(projectName, changeId);
- ImmutableList<Path> ownedPaths;
- try {
- // limit + 1, so that we can show an indicator if there are more than <limit> files.
- ownedPaths =
- OwnedChangedFile.getOwnedPaths(
- codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes,
- changeNotes.getCurrentPatchSet(),
- reviewerAccountId,
- /* start= */ 0,
- limit + 1));
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- "Couldn't compute owned paths of change %s for account %s",
- changeNotes.getChangeId(), reviewerAccountId.get());
- return Optional.empty();
- }
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
+ ImmutableList<Path> ownedPaths =
+ OwnedChangedFile.getOwnedPaths(
+ codeOwnerApprovalCheck.getOwnedPaths(
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ reviewerAccountId,
+ /* start= */ 0,
+ limit + 1));
if (ownedPaths.isEmpty()) {
// this reviewer doesn't own any of the modified paths
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index 5b6f6dd..b538ebd 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -17,9 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -53,8 +51,6 @@
*/
@Singleton
class OnCodeOwnerApproval implements OnPostReview {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
private final CodeOwnerMetrics codeOwnerMetrics;
@@ -120,23 +116,15 @@
int limit) {
LabelVote newVote = getNewVote(requiredApproval, approvals);
- ImmutableList<Path> ownedPaths;
- try {
- // limit + 1, so that we can show an indicator if there are more than <limit> files.
- ownedPaths =
- OwnedChangedFile.getOwnedPaths(
- codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes,
- changeNotes.getCurrentPatchSet(),
- user.getAccountId(),
- /* start= */ 0,
- limit + 1));
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- "Couldn't compute owned paths of change %s for account %s",
- changeNotes.getChangeId(), user.getAccountId().get());
- return Optional.empty();
- }
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
+ ImmutableList<Path> ownedPaths =
+ OwnedChangedFile.getOwnedPaths(
+ codeOwnerApprovalCheck.getOwnedPaths(
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ user.getAccountId(),
+ /* start= */ 0,
+ limit + 1));
if (ownedPaths.isEmpty()) {
// the user doesn't own any of the modified paths
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index 22cc758..f3e7d1b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -16,8 +16,10 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerStatusInfoSubject.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.LegacySubmitRequirementInfoSubject.assertThatCollection;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
@@ -25,8 +27,10 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestMetricMaker;
+import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.RefNames;
@@ -53,6 +57,7 @@
/** Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.CodeOwnerSubmitRule}. */
public class CodeOwnerSubmitRuleIT extends AbstractCodeOwnersIT {
+ @Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
@Inject private TestMetricMaker testMetricMaker;
@@ -100,7 +105,17 @@
}
@Test
- public void changeWithInsufficentReviewersIsNotSubmittable() throws Exception {
+ public void changeWithInsufficientReviewersIsNotSubmittable() throws Exception {
+ testChangeWithInsufficientReviewersIsNotSubmittable();
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithInsufficientReviewersIsNotSubmittable() throws Exception {
+ testChangeWithInsufficientReviewersIsNotSubmittable();
+ }
+
+ private void testChangeWithInsufficientReviewersIsNotSubmittable() throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Approve by a non-code-owner.
@@ -151,10 +166,23 @@
.addCodeOwnerEmail(user.email())
.create();
+ testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(user);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithPendingCodeOwnerApprovalsIsNotSubmittable() throws Exception {
+ setAsDefaultCodeOwners(user);
+
+ testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(user);
+ }
+
+ private void testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(TestAccount codeOwner)
+ throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Add a reviewer that is a code owner.
- gApi.changes().id(changeId).addReviewer(user.email());
+ gApi.changes().id(changeId).addReviewer(codeOwner.email());
// Approve by a non-code-owner.
approve(changeId);
@@ -201,13 +229,33 @@
.project(project)
.branch("master")
.folderPath("/foo/")
- .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
.create();
+ testChangeWithCodeOwnerApprovalsIsSubmittable(user);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithCodeOwnerApprovalsIsSubmittable() throws Exception {
+ setAsDefaultCodeOwners(user);
+
+ testChangeWithCodeOwnerApprovalsIsSubmittable(user);
+ }
+
+ private void testChangeWithCodeOwnerApprovalsIsSubmittable(TestAccount codeOwner)
+ throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Approve by a code-owner.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(codeOwner.id());
approve(changeId);
+ requestScopeOperations.setApiUser(admin.id());
// Verify that the code owner status for the changed file is APPROVED.
CodeOwnerStatusInfo codeOwnerStatus =
@@ -239,6 +287,17 @@
@Test
@GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
public void changeWithOverrideApprovalIsSubmittable() throws Exception {
+ testChangeWithOverrideApprovalIsSubmittable();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithOverrideApprovalIsSubmittable() throws Exception {
+ testChangeWithOverrideApprovalIsSubmittable();
+ }
+
+ private void testChangeWithOverrideApprovalIsSubmittable() throws Exception {
createOwnersOverrideLabel();
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 34642d5..88ff7d0 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -42,7 +42,6 @@
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelDefinitionInput;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
@@ -1482,19 +1481,204 @@
}
@Test
- public void getStatus_branchDeleted() throws Exception {
+ public void getStatus_branchDeleted_defaultCodeOwner() throws Exception {
String branchName = "tempBranch";
createBranch(BranchNameKey.create(project, branchName));
- String changeId = createChange("refs/for/" + branchName).getChangeId();
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = ImmutableList.of(branchName);
gApi.projects().name(project.get()).deleteBranches(input);
- ResourceConflictException exception =
- assertThrows(ResourceConflictException.class, () -> getFileCodeOwnerStatuses(changeId));
- assertThat(exception).hasMessageThat().isEqualTo("destination branch not found");
+ testGetStatusBranchDoesNotExistWithDefaultCodeOwner(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void getStatus_initialChange_defaultCodeOwner() throws Exception {
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExistWithDefaultCodeOwner(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithDefaultCodeOwner(String changeId, Path path)
+ throws Exception {
+ setAsDefaultCodeOwners(admin);
+
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add default code owner as a reviewer.
+ gApi.changes().id(changeId).addReviewer(admin.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a default code owner",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+
+ // Approve as default code owner.
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a default code owner",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ public void getStatus_branchDeleted_globalCodeOwner() throws Exception {
+ // Create a bot user that is a global code owner.
+ TestAccount bot =
+ accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExistWithGlobalCodeOwner(changeId, path, bot);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ public void getStatus_initialChange_globalCodeOwner() throws Exception {
+ // Create a bot user that is a global code owner.
+ TestAccount bot =
+ accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExistWithGlobalCodeOwner(changeId, path, bot);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithGlobalCodeOwner(
+ String changeId, Path path, TestAccount globalCodeOwner) throws Exception {
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add global code owner as a reviewer.
+ gApi.changes().id(changeId).addReviewer(globalCodeOwner.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a global code owner",
+ AccountTemplateUtil.getAccountTemplate(globalCodeOwner.id()))));
+
+ // Approve as default code owner.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(globalCodeOwner.id());
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a global code owner",
+ AccountTemplateUtil.getAccountTemplate(globalCodeOwner.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void getStatus_branchDeleted_override() throws Exception {
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExistWithOverride(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void getStatus_initialChange_override() throws Exception {
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExistWithOverride(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithOverride(String changeId, Path path)
+ throws Exception {
+ createOwnersOverrideLabel();
+
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Apply an override
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
index c91d897..5c31eed 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
@@ -16,11 +16,15 @@
import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatCollection;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
@@ -459,6 +463,70 @@
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
+ @Test
+ public void getStatus_branchDeleted() throws Exception {
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExist(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void getStatus_initialChange() throws Exception {
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExist(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExist(String changeId, Path path) throws Exception {
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add code owner as a reviewer (all users are fallback code owners).
+ gApi.changes().id(changeId).addReviewer(admin.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a fallback code owner (all users are fallback code owners)",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+
+ // Approve as a code owner (all users are fallback code owners).
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a fallback code owner"
+ + " (all users are fallback code owners)",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+ }
+
private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
throws Exception {
return codeOwnerApprovalCheck.getFileStatusesAsSet(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
index df5ad05..0422551 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
@@ -89,17 +89,13 @@
}
@Test
- public void cannotVisitCodeOwnerConfigsForNullRevision() throws Exception {
- NullPointerException npe =
- assertThrows(
- NullPointerException.class,
- () ->
- codeOwnerConfigHierarchy.visit(
- BranchNameKey.create(project, "master"),
- /* revision= */ null,
- Paths.get("/foo/bar/baz.md"),
- visitor));
- assertThat(npe).hasMessageThat().isEqualTo("revision");
+ public void visitorNotInvokedForNullRevision() throws Exception {
+ codeOwnerConfigHierarchy.visit(
+ BranchNameKey.create(project, "master"),
+ /* revision= */ null,
+ Paths.get("/foo/bar/baz.md"),
+ visitor);
+ verifyNoInteractions(visitor);
}
@Test
@@ -583,6 +579,27 @@
}
@Test
+ public void visitorInvokedForCodeOwnerConfigInRefsMetaConfig_nullRevision() throws Exception {
+ CodeOwnerConfig.Key metaCodeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch(RefNames.REFS_CONFIG)
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ when(visitor.visit(any(CodeOwnerConfig.class))).thenReturn(true);
+ codeOwnerConfigHierarchy.visit(
+ BranchNameKey.create(project, "master"),
+ /* revision= */ null,
+ Paths.get("/foo/bar/baz.md"),
+ visitor);
+ verify(visitor).visit(codeOwnerConfigOperations.codeOwnerConfig(metaCodeOwnerConfigKey).get());
+ verifyNoMoreInteractions(visitor);
+ }
+
+ @Test
public void visitorInvokedForCodeOwnerConfigInRefsMetaConfigIfItDoesntApply() throws Exception {
CodeOwnerConfig.Key metaCodeOwnerConfigKey =
codeOwnerConfigOperations
