Merge "Deprecate pushing to refs/changes"

Documentation/config-gerrit.txt

@@ -3533,6 +3533,17 @@ Name of the groups of users that are allowed to execute
 If no groups are added, any user will be allowed to execute
 'receive-pack' on the server.
 
+[[receive.allowPushToRefsChanges]]receive.allowPushToRefsChanges::
++
+If true, it is possible to push directly to a change using `refs/changes/`.
+The possibility to push to `refs/changes/` is deprecated and it might be
+removed in future releases.
+See link:user-upload.html#manual_replacement_mapping[Manual Replacement Mapping].
++
+False means pushing to `refs/changes/` is prohibited.
++
+Defaults to false.
+
 [[receive.certNonceSeed]]receive.certNonceSeed::
 +
 If set to a non-empty value and server-side signed push validation is

Documentation/error-messages.txt

@@ -34,6 +34,7 @@ occurring and what can be done to solve it.
 * link:error-same-change-id-in-multiple-changes.html[same Change-Id in multiple changes]
 * link:error-too-many-commits.html[too many commits]
 * link:error-upload-denied.html[Upload denied for project \'...']
+* link:error-push-refschanges-not-allowed.html[upload to refs/changes not allowed]
 * link:error-not-allowed-to-upload-merges.html[you are not allowed to upload merges]
 
 

Documentation/error-push-refschanges-not-allowed.txt

@@ -0,0 +1,13 @@
+= upload to refs/changes not allowed
+
+Pushing to `refs/changes/` is deprecated and is not allowed on this Gerrit server.
+See the documentation for link:user-upload.html#push_create[creating changes] for
+alternate ways to push to existing changes.
+
+
+GERRIT
+------
+Part of link:error-messages.html[Gerrit Error Messages]
+
+SEARCHBOX
+---------

Documentation/intro-user.txt

@@ -184,9 +184,7 @@ when pushing the very first patch set. This reduces the risk of
 accidentally creating a new change instead of uploading a new patch
 set. Any push without Change-Id then fails with
 link:error-missing-changeid.html[missing Change-Id in commit message
-footer]. New patch sets can always be uploaded to a specific change
+footer].
-(even without any Change-Id) by pushing to the change ref, e.g.
-`refs/changes/74/67374`.
 
 Amending and rebasing a commit preserves the Change-Id so that the new
 commit automatically becomes a new patch set of the existing change,

Documentation/user-upload.txt

@@ -431,6 +431,11 @@ commit messages, as the process is then fully automated by Gerrit
 during normal uploads.
 
 See above for the preferred technique of replacing changes.
+
+Pushing directly to `refs/changes/` is deprecated. If you see the error
+message 'upload to refs/changes not allowed', it means that pushing directly
+to `refs/changes` is disabled on the Gerrit server and the below section does
+not apply to you.
 --
 
 To add an additional patch set to a change, replacing it with an

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

@@ -99,6 +99,7 @@ import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.SetHashtagsOp;
 import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.config.CanonicalWebUrl;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.PluginConfig;
 import com.google.gerrit.server.config.ProjectConfigEntry;
 import com.google.gerrit.server.edit.ChangeEdit;
@@ -187,6 +188,7 @@ import java.util.regex.Matcher;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
@@ -349,6 +351,7 @@ class ReceiveCommits {
   private final ReceivePack rp;
 
   // Immutable fields derived from constructor arguments.
+  private final boolean allowPushToRefsChanges;
   private final LabelTypes labelTypes;
   private final NoteMap rejectCommits;
   private final PermissionBackend.ForProject permissions;
@@ -397,6 +400,7 @@ class ReceiveCommits {
       AccountsUpdate.Server accountsUpdate,
       AllProjectsName allProjectsName,
       BatchUpdate.Factory batchUpdateFactory,
+      @GerritServerConfig Config cfg,
       ChangeEditUtil editUtil,
       ChangeIndexer indexer,
       ChangeInserter.Factory changeInserterFactory,
@@ -480,6 +484,7 @@ class ReceiveCommits {
     this.rp = rp;
 
     // Immutable fields derived from constructor arguments.
+    allowPushToRefsChanges = cfg.getBoolean("receive", "allowPushToRefsChanges", false);
     repo = rp.getRepository();
     project = projectState.getProject();
     labelTypes = projectState.getLabelTypes();
@@ -861,10 +866,14 @@ class ReceiveCommits {
 
       Matcher m = NEW_PATCHSET_PATTERN.matcher(cmd.getRefName());
       if (m.matches()) {
+        if (allowPushToRefsChanges) {
           // The referenced change must exist and must still be open.
           //
           Change.Id changeId = Change.Id.parse(m.group(1));
           parseReplaceCommand(cmd, changeId);
+        } else {
+          reject(cmd, "upload to refs/changes not allowed");
+        }
         continue;
       }
 

javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -954,8 +954,27 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     assertThatUserIsOnlyReviewer(ci, admin);
   }
 
+  @Test
+  @GerritConfig(name = "receive.allowPushToRefsChanges", value = "true")
+  public void pushToRefsChangesAllowed() throws Exception {
+    PushOneCommit.Result r = pushOneCommitToRefsChanges();
+    r.assertOkStatus();
+  }
+
   @Test
   public void pushNewPatchsetToRefsChanges() throws Exception {
+    PushOneCommit.Result r = pushOneCommitToRefsChanges();
+    r.assertErrorStatus("upload to refs/changes not allowed");
+  }
+
+  @Test
+  @GerritConfig(name = "receive.allowPushToRefsChanges", value = "false")
+  public void pushToRefsChangesNotAllowed() throws Exception {
+    PushOneCommit.Result r = pushOneCommitToRefsChanges();
+    r.assertErrorStatus("upload to refs/changes not allowed");
+  }
+
+  private PushOneCommit.Result pushOneCommitToRefsChanges() throws Exception {
     PushOneCommit.Result r = pushTo("refs/for/master");
     r.assertOkStatus();
     PushOneCommit push =
@@ -967,8 +986,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
             "b.txt",
             "anotherContent",
             r.getChangeId());
-    r = push.to("refs/changes/" + r.getChange().change().getId().get());
+    return push.to("refs/changes/" + r.getChange().change().getId().get());
-    r.assertOkStatus();
   }
 
   @Test
@@ -1323,6 +1341,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
   }
 
   @Test
+  @GerritConfig(name = "receive.allowPushToRefsChanges", value = "true")
   public void testPushWithChangedChangeId() throws Exception {
     PushOneCommit.Result r = pushTo("refs/for/master");
     r.assertOkStatus();
@@ -1523,6 +1542,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
   }
 
   @Test
+  @GerritConfig(name = "receive.allowPushToRefsChanges", value = "true")
   public void accidentallyPushNewPatchSetDirectlyToBranchAndRecoverByPushingToRefsChanges()
       throws Exception {
     Change.Id id = accidentallyPushNewPatchSetDirectlyToBranch();