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
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java
index 860afb2..ea5d614 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java
@@ -297,7 +297,7 @@
}
AsyncReceiveCommits arc = factory.create(ctl, db);
- ReceivePack rp = arc.getReceiveCommits().getReceivePack();
+ ReceivePack rp = arc.getReceivePack();
Capable r = arc.canUpload();
if (r != Capable.OK) {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index ee3ba2f..91cbc6d 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -64,7 +64,7 @@
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 @@
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 @@
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);
diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java
index e839ced..21a5b6e 100644
--- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java
+++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/SignedPushPreReceiveHook.java
@@ -28,9 +28,8 @@
/**
* 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 {
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java
index f49fa95..6e02bde 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java
@@ -295,9 +295,9 @@
}
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 @@
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();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
index b25697b..6895277 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
+++ b/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 @@
}
private final ReceiveCommits rc;
+ private final ReceivePack rp;
private final ExecutorService executor;
private final RequestScopePropagator scopePropagator;
private final MultiProgressMonitor progress;
@@ -154,7 +157,8 @@
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 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);
+ }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 9c5f303..4d274a8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -21,6 +21,9 @@
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.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 @@
this.value = value;
}
- public String get() {
+ String get() {
return value;
}
}
@@ -246,7 +237,7 @@
ReceiveCommits create(ProjectControl projectControl, Repository repository);
}
- public interface MessageSender {
+ interface MessageSender {
void sendMessage(String what);
void sendError(String what);
@@ -540,24 +531,24 @@
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 @@
return project;
}
- /** @return the ReceivePack instance to speak the native Git protocol. */
- public ReceivePack getReceivePack() {
+ ReceivePack getReceivePack() {
return rp;
}
@@ -1437,7 +1427,7 @@
return ref.substring(0, split);
}
- public NotifyHandling getNotify() {
+ NotifyHandling getNotify() {
if (notify != null) {
return notify;
}
@@ -1447,7 +1437,7 @@
return NotifyHandling.ALL;
}
- public NotifyHandling getNotify(ChangeNotes notes) {
+ NotifyHandling getNotify(ChangeNotes notes) {
if (notify != null) {
return notify;
}
@@ -1467,7 +1457,7 @@
* @return an unmodifiable view of pushOptions.
*/
@Nullable
- public ListMultimap<String, String> getPushOptions() {
+ ListMultimap<String, String> getPushOptions() {
return ImmutableListMultimap.copyOf(pushOptions);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConstants.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveConstants.java
new file mode 100644
index 0000000..99742f3
--- /dev/null
+++ b/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() {}
+}
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java
index 11d33be..d8aac07 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java
@@ -19,7 +19,6 @@
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 @@
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());
