Merge "Refactor BranchCommitValidator API"

java/com/google/gerrit/server/git/receive/BranchCommitValidator.java

@@ -17,6 +17,8 @@ package com.google.gerrit.server.git.receive;
 import static com.google.gerrit.git.ObjectIds.abbreviateName;
 import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_REASON;
 
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.BranchNameKey;
@@ -27,14 +29,12 @@ import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.validators.CommitValidationException;
 import com.google.gerrit.server.git.validators.CommitValidationMessage;
 import com.google.gerrit.server.git.validators.CommitValidators;
-import com.google.gerrit.server.git.validators.ValidationMessage;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.ssh.SshInfo;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 import java.io.IOException;
-import java.util.List;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -56,6 +56,23 @@ public class BranchCommitValidator {
         ProjectState projectState, BranchNameKey branch, IdentifiedUser user);
   }
 
+  /** A boolean validation status and a list of additional messages. */
+  @AutoValue
+  abstract static class Result {
+    static Result create(boolean isValid, ImmutableList<CommitValidationMessage> messages) {
+      return new AutoValue_BranchCommitValidator_Result(isValid, messages);
+    }
+
+    /** Whether the commit is valid. */
+    abstract boolean isValid();
+
+    /**
+     * A list of messages related to the validation. Messages may be present regardless of the
+     * {@link #isValid()} status.
+     */
+    abstract ImmutableList<CommitValidationMessage> messages();
+  }
+
   @Inject
   BranchCommitValidator(
       CommitValidators.Factory commitValidatorsFactory,
@@ -80,16 +97,17 @@ public class BranchCommitValidator {
    * @param commit the commit being validated.
    * @param isMerged whether this is a merge commit created by magicBranch --merge option
    * @param change the change for which this is a new patchset.
+   * @return The validation {@link Result}.
    */
-  public boolean validCommit(
+  Result validateCommit(
       ObjectReader objectReader,
       ReceiveCommand cmd,
       RevCommit commit,
       boolean isMerged,
-      List<ValidationMessage> messages,
       NoteMap rejectCommits,
       @Nullable Change change)
       throws IOException {
+    ImmutableList.Builder<CommitValidationMessage> messages = new ImmutableList.Builder<>();
     try (CommitReceivedEvent receiveEvent =
         new CommitReceivedEvent(cmd, project, branch.branch(), objectReader, commit, user)) {
       CommitValidators validators;
@@ -123,9 +141,9 @@ public class BranchCommitValidator {
                 messageForCommit(commit, m.getMessage(), objectReader), m.getType()));
       }
       cmd.setResult(REJECTED_OTHER_REASON, messageForCommit(commit, e.getMessage(), objectReader));
-      return false;
+      return Result.create(false, messages.build());
     }
-    return true;
+    return Result.create(true, messages.build());
   }
 
   private String messageForCommit(RevCommit c, String msg, ObjectReader objectReader)

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -1949,14 +1949,16 @@ class ReceiveCommits {
     BranchCommitValidator validator =
         commitValidatorFactory.create(projectState, changeEnt.getDest(), user);
     try {
-      if (validator.validCommit(
-          receivePack.getRevWalk().getObjectReader(),
-          cmd,
-          newCommit,
-          false,
-          messages,
-          rejectCommits,
-          changeEnt)) {
+      BranchCommitValidator.Result validationResult =
+          validator.validateCommit(
+              receivePack.getRevWalk().getObjectReader(),
+              cmd,
+              newCommit,
+              false,
+              rejectCommits,
+              changeEnt);
+      messages.addAll(validationResult.messages());
+      if (validationResult.isValid()) {
         logger.atFine().log("Replacing change %s", changeEnt.getId());
         requestReplace(cmd, true, changeEnt, newCommit);
       }
@@ -2114,14 +2116,16 @@ class ReceiveCommits {
           logger.atFine().log("Creating new change for %s even though it is already tracked", name);
         }
 
-        if (!validator.validCommit(
-            receivePack.getRevWalk().getObjectReader(),
-            magicBranch.cmd,
-            c,
-            magicBranch.merged,
-            messages,
-            rejectCommits,
-            null)) {
+        BranchCommitValidator.Result validationResult =
+            validator.validateCommit(
+                receivePack.getRevWalk().getObjectReader(),
+                magicBranch.cmd,
+                c,
+                magicBranch.merged,
+                rejectCommits,
+                null);
+        messages.addAll(validationResult.messages());
+        if (!validationResult.isValid()) {
           // Not a change the user can propose? Abort as early as possible.
           logger.atFine().log("Aborting early due to invalid commit");
           return Collections.emptyList();
@@ -3113,8 +3117,10 @@ class ReceiveCommits {
           continue;
         }
 
-        if (!validator.validCommit(
-            walk.getObjectReader(), cmd, c, false, messages, rejectCommits, null)) {
+        BranchCommitValidator.Result validationResult =
+            validator.validateCommit(walk.getObjectReader(), cmd, c, false, rejectCommits, null);
+        messages.addAll(validationResult.messages());
+        if (!validationResult.isValid()) {
           break;
         }
       }