Make ReceiveCommits non-public

Callers used to have access to both the AsyncReceiveCommits and the
underlying ReceiveCommits. This was confusing at best, and in the
presence of multiple threads (or, eventually, retrying) broken at worst.
Try to simplify the package interface by forcing all callers to go
through AsyncReceiveCommits. It's still somewhat non-obvious, but at
least there is only one choice.

To support tests that want to assert over specific error message
strings, factor out a public ReceiveConstants class.

Change-Id: I1760faed4c2d4d508c38ec8a698f3e5c2aae2c35

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java

@@ -297,7 +297,7 @@ class InProcessProtocol extends TestProtocol<Context> {
         }
 
         AsyncReceiveCommits arc = factory.create(ctl, db);
-        ReceivePack rp = arc.getReceiveCommits().getReceivePack();
+        ReceivePack rp = arc.getReceivePack();
 
         Capable r = arc.canUpload();
         if (r != Capable.OK) {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -64,7 +64,7 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.git.ProjectConfig;
-import com.google.gerrit.server.git.receive.ReceiveCommits;
+import com.google.gerrit.server.git.receive.ReceiveConstants;
 import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.server.project.Util;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -490,7 +490,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     GitUtil.fetch(testRepo, r.getPatchSet().getRefName() + ":ps");
     testRepo.reset("ps");
     r = amendChange(r.getChangeId(), "refs/for/master%ready", admin, testRepo);
-    r.assertErrorStatus(ReceiveCommits.ONLY_OWNER_CAN_MODIFY_WIP);
+    r.assertErrorStatus(ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP);
 
     // Other user trying to move from WIP to WIP should succeed.
     r = amendChange(r.getChangeId(), "refs/for/master%wip", admin, testRepo);
@@ -506,7 +506,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     GitUtil.fetch(testRepo, r.getPatchSet().getRefName() + ":ps");
     testRepo.reset("ps");
     r = amendChange(r.getChangeId(), "refs/for/master%wip", admin, testRepo);
-    r.assertErrorStatus(ReceiveCommits.ONLY_OWNER_CAN_MODIFY_WIP);
+    r.assertErrorStatus(ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP);
 
     // Other user trying to move from ready to ready should succeed.
     r = amendChange(r.getChangeId(), "refs/for/master%ready", admin, testRepo);

gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java

@@ -28,9 +28,8 @@ import org.eclipse.jgit.transport.ReceivePack;
 /**
  * Pre-receive hook to check signed pushes.
  *
- * <p>If configured, prior to processing any push using {@link
- * com.google.gerrit.server.git.receive.ReceiveCommits}, requires that any push certificate present
- * must be valid.
+ * <p>If configured, prior to processing any push using {@code ReceiveCommits}, requires that any
+ * push certificate present must be valid.
  */
 @Singleton
 public class SignedPushPreReceiveHook implements PreReceiveHook {

gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java

@@ -295,9 +295,9 @@ public class GitOverHttpServlet extends GitServlet {
       }
 
       AsyncReceiveCommits arc = factory.create(pc, db);
-      arc.getReceiveCommits().init();
+      arc.init();
 
-      ReceivePack rp = arc.getReceiveCommits().getReceivePack();
+      ReceivePack rp = arc.getReceivePack();
       req.setAttribute(ATT_ARC, arc);
       return rp;
     }
@@ -325,7 +325,7 @@ public class GitOverHttpServlet extends GitServlet {
       boolean isGet = "GET".equalsIgnoreCase(((HttpServletRequest) request).getMethod());
 
       AsyncReceiveCommits arc = (AsyncReceiveCommits) request.getAttribute(ATT_ARC);
-      ReceivePack rp = arc.getReceiveCommits().getReceivePack();
+      ReceivePack rp = arc.getReceivePack();
       rp.getAdvertiseRefsHook().advertiseRefs(rp);
       ProjectControl pc = (ProjectControl) request.getAttribute(ATT_CONTROL);
       Project.NameKey projectName = pc.getProject().getNameKey();

gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java

@@ -14,7 +14,9 @@
 
 package com.google.gerrit.server.git.receive;
 
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.Capable;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.config.ConfigUtil;
 import com.google.gerrit.server.config.GerritServerConfig;
@@ -129,6 +131,7 @@ public class AsyncReceiveCommits implements PreReceiveHook {
   }
 
   private final ReceiveCommits rc;
+  private final ReceivePack rp;
   private final ExecutorService executor;
   private final RequestScopePropagator scopePropagator;
   private final MultiProgressMonitor progress;
@@ -154,7 +157,8 @@ public class AsyncReceiveCommits implements PreReceiveHook {
     this.timeoutMillis = timeoutMillis;
 
     rc = factory.create(projectControl, repo);
-    rc.getReceivePack().setPreReceiveHook(this);
+    rp = rc.getReceivePack();
+    rp.setPreReceiveHook(this);
     progress = new MultiProgressMonitor(new MessageSenderOutputStream(), "Processing changes");
   }
 
@@ -196,7 +200,22 @@ public class AsyncReceiveCommits implements PreReceiveHook {
     }
   }
 
-  public ReceiveCommits getReceiveCommits() {
-    return rc;
+  public ReceivePack getReceivePack() {
+    return rp;
+  }
+
+  public void init() {
+    init(null, null);
+  }
+
+  public void init(
+      @Nullable Collection<Account.Id> extraReviewers, @Nullable Collection<Account.Id> extraCcs) {
+    rc.init();
+    if (extraReviewers != null) {
+      rc.addReviewers(extraReviewers);
+    }
+    if (extraCcs != null) {
+      rc.addExtraCC(extraCcs);
+    }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -21,6 +21,9 @@ import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES;
 import static com.google.gerrit.server.change.HashtagsUtil.cleanupHashtag;
 import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN;
+import static com.google.gerrit.server.git.receive.ReceiveConstants.COMMAND_REJECTION_MESSAGE_FOOTER;
+import static com.google.gerrit.server.git.receive.ReceiveConstants.ONLY_OWNER_CAN_MODIFY_WIP;
+import static com.google.gerrit.server.git.receive.ReceiveConstants.SAME_CHANGE_ID_IN_MULTIPLE_CHANGES;
 import static com.google.gerrit.server.git.validators.CommitValidators.NEW_PATCHSET_PATTERN;
 import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
 import static java.util.Comparator.comparingInt;
@@ -200,22 +203,10 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /** Receives change upload using the Git receive-pack protocol. */
-public class ReceiveCommits {
+class ReceiveCommits {
   private static final Logger log = LoggerFactory.getLogger(ReceiveCommits.class);
   private static final String BYPASS_REVIEW = "bypass-review";
 
-  private static final String COMMAND_REJECTION_MESSAGE_FOOTER =
-      "Please read the documentation and contact an administrator\n"
-          + "if you feel the configuration is incorrect";
-
-  private static final String SAME_CHANGE_ID_IN_MULTIPLE_CHANGES =
-      "same Change-Id in multiple changes.\n"
-          + "Squash the commits with the same Change-Id or "
-          + "ensure Change-Ids are unique for each commit";
-
-  public static final String ONLY_OWNER_CAN_MODIFY_WIP =
-      "only change owner can modify Work-in-Progress";
-
   private enum Error {
     CONFIG_UPDATE(
         "You are not allowed to perform this operation.\n"
@@ -237,7 +228,7 @@ public class ReceiveCommits {
       this.value = value;
     }
 
-    public String get() {
+    String get() {
       return value;
     }
   }
@@ -246,7 +237,7 @@ public class ReceiveCommits {
     ReceiveCommits create(ProjectControl projectControl, Repository repository);
   }
 
-  public interface MessageSender {
+  interface MessageSender {
     void sendMessage(String what);
 
     void sendError(String what);
@@ -540,24 +531,24 @@ public class ReceiveCommits {
     rp.setAllowPushOptions(true);
   }
 
-  public void init() {
+  void init() {
     for (ReceivePackInitializer i : initializers) {
       i.init(projectControl.getProject().getNameKey(), rp);
     }
   }
 
   /** Add reviewers for new (or updated) changes. */
-  public void addReviewers(Collection<Account.Id> who) {
+  void addReviewers(Collection<Account.Id> who) {
     reviewersFromCommandLine.addAll(who);
   }
 
   /** Add reviewers for new (or updated) changes. */
-  public void addExtraCC(Collection<Account.Id> who) {
+  void addExtraCC(Collection<Account.Id> who) {
     ccFromCommandLine.addAll(who);
   }
 
   /** Set a message sender for this operation. */
-  public void setMessageSender(MessageSender ms) {
+  void setMessageSender(MessageSender ms) {
     messageSender = ms != null ? ms : new ReceivePackMessageSender();
   }
 
@@ -572,8 +563,7 @@ public class ReceiveCommits {
     return project;
   }
 
-  /** @return the ReceivePack instance to speak the native Git protocol. */
-  public ReceivePack getReceivePack() {
+  ReceivePack getReceivePack() {
     return rp;
   }
 
@@ -1437,7 +1427,7 @@ public class ReceiveCommits {
       return ref.substring(0, split);
     }
 
-    public NotifyHandling getNotify() {
+    NotifyHandling getNotify() {
       if (notify != null) {
         return notify;
       }
@@ -1447,7 +1437,7 @@ public class ReceiveCommits {
       return NotifyHandling.ALL;
     }
 
-    public NotifyHandling getNotify(ChangeNotes notes) {
+    NotifyHandling getNotify(ChangeNotes notes) {
       if (notify != null) {
         return notify;
       }
@@ -1467,7 +1457,7 @@ public class ReceiveCommits {
    * @return an unmodifiable view of pushOptions.
    */
   @Nullable
-  public ListMultimap<String, String> getPushOptions() {
+  ListMultimap<String, String> getPushOptions() {
     return ImmutableListMultimap.copyOf(pushOptions);
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConstants.java

@@ -0,0 +1,34 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed 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 com.google.gerrit.server.git.receive;
+
+import com.google.common.annotations.VisibleForTesting;
+
+public final class ReceiveConstants {
+  @VisibleForTesting
+  public static final String ONLY_OWNER_CAN_MODIFY_WIP =
+      "only change owner can modify Work-in-Progress";
+
+  static final String COMMAND_REJECTION_MESSAGE_FOOTER =
+      "Please read the documentation and contact an administrator\n"
+          + "if you feel the configuration is incorrect";
+
+  static final String SAME_CHANGE_ID_IN_MULTIPLE_CHANGES =
+      "same Change-Id in multiple changes.\n"
+          + "Squash the commits with the same Change-Id or "
+          + "ensure Change-Ids are unique for each commit";
+
+  private ReceiveConstants() {}
+}

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java

@@ -19,7 +19,6 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.git.VisibleRefFilter;
 import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
-import com.google.gerrit.server.git.receive.ReceiveCommits;
 import com.google.gerrit.sshd.AbstractGitCommand;
 import com.google.gerrit.sshd.CommandMetaData;
 import com.google.gerrit.sshd.SshSession;
@@ -88,11 +87,8 @@ final class Receive extends AbstractGitCommand {
       throw die(r.getMessage());
     }
 
-    ReceiveCommits receive = arc.getReceiveCommits();
-    receive.init();
-    receive.addReviewers(reviewerId);
-    receive.addExtraCC(ccId);
-    ReceivePack rp = receive.getReceivePack();
+    arc.init(reviewerId, ccId);
+    ReceivePack rp = arc.getReceivePack();
     try {
       rp.receive(in, out, err);
       session.setPeerAgent(rp.getPeerUserAgent());