From 2cee70dae844261241e7476706245a7eaa0c5e6b Mon Sep 17 00:00:00 2001 From: Bruce Zu Date: Sat, 8 Jun 2013 14:10:50 +0800 Subject: [PATCH] Fix: failed to validate Change-Id of some new patch-sets pushed by 'refs/changes' Those new patch-sets pushed by 'refs/changes/xxxxx/new' or 'refs/changes/xxxxx' can not get Change-Id validation, because the CommitValidators.NEW_PATCHSET can not match them. Before 15d5ac0e7, the CommitValidators.NEW_PATCHSET was same with ReceiveCommits.NEW_PATCHSET and there was not this hole. 15d5ac0e7 updated CommitValidators.NEW_PATCHSET to ensure validating the Change-Id of [a] and [b]. This commit replaces CommitValidators.NEW_PATCHSET with ReceiveCommits.NEW_PATCHSET and only keep the later one to plug this hole, and updates the ref name of ReceiveCommand for [a] and [b] to keep [a] and [b]'s Chang-Id get validation as before. [a] is of new change: 'refs/changes/nn/mmmnn/x' -> 'refs/publish/ [b] is of new patch-set of an existing change: 'refs/changes/nn/mmmnn/x' -> 'refs/changes/nn/mmmnn/new' Now the ref pattern of ReceiveCommand created via UI falls into line with those pushed via ssh command. [a]: new change created by 'Revert' button. [b]: new patch-set created by 'Edit Commit Message' icon. Change-Id: Idbc7764db9edcc067445e7246c03eb1c3e686955 --- .../java/com/google/gerrit/server/ChangeUtil.java | 14 ++++++++++---- .../google/gerrit/server/git/ReceiveCommits.java | 2 +- .../server/git/validators/CommitValidators.java | 11 +++++------ 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index c9fb4fe2b6..9569a97737 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -42,6 +42,7 @@ import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.util.IdGenerator; +import com.google.gerrit.server.util.MagicBranch; import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmConcurrencyException; import com.google.gwtorm.server.OrmException; @@ -256,11 +257,14 @@ public class ChangeUtil { ps.setUploader(change.getOwner()); ps.setRevision(new RevId(revertCommit.name())); + String ref = refControl.getRefName(); + final String cmdRef = + MagicBranch.NEW_PUBLISH_CHANGE + + ref.substring(ref.lastIndexOf("/") + 1); CommitReceivedEvent commitReceivedEvent = new CommitReceivedEvent(new ReceiveCommand(ObjectId.zeroId(), - revertCommit.getId(), ps.getRefName()), refControl - .getProjectControl().getProject(), refControl.getRefName(), - revertCommit, user); + revertCommit.getId(), cmdRef), refControl.getProjectControl() + .getProject(), refControl.getRefName(), revertCommit, user); try { commitValidators.validateForGerritCommits(commitReceivedEvent); @@ -377,9 +381,11 @@ public class ChangeUtil { final PatchSetInfo info = patchSetInfoFactory.get(newCommit, newPatchSet.getId()); + final String refName = newPatchSet.getRefName(); CommitReceivedEvent commitReceivedEvent = new CommitReceivedEvent(new ReceiveCommand(ObjectId.zeroId(), - newCommit.getId(), newPatchSet.getRefName()), refControl + newCommit.getId(), refName.substring(0, + refName.lastIndexOf("/") + 1) + "new"), refControl .getProjectControl().getProject(), refControl.getRefName(), newCommit, user); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 7f3f2fd369..bcc30144fc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -144,7 +144,7 @@ public class ReceiveCommits { private static final Logger log = LoggerFactory.getLogger(ReceiveCommits.class); - private static final Pattern NEW_PATCHSET = + public static final Pattern NEW_PATCHSET = Pattern.compile("^refs/changes/(?:[0-9][0-9]/)?([1-9][0-9]*)(?:/new)?$"); private static final FooterKey CHANGE_ID = new FooterKey("Change-Id"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index 8abe501cde..a57f9239fa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -22,6 +22,7 @@ import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.git.ReceiveCommits; import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.RefControl; @@ -47,7 +48,6 @@ import java.net.URL; import java.util.Collections; import java.util.LinkedList; import java.util.List; -import java.util.regex.Pattern; import javax.annotation.Nullable; @@ -57,9 +57,6 @@ public class CommitValidators { private static final FooterKey CHANGE_ID = new FooterKey("Change-Id"); - private static final Pattern NEW_PATCHSET = Pattern - .compile("^refs/changes/(?:[0-9][0-9])?(/[1-9][0-9]*){1,2}(?:/new)?$"); - public interface Factory { CommitValidators create(RefControl refControl, SshInfo sshInfo, Repository repo); @@ -99,7 +96,8 @@ public class CommitValidators { validators.add(new CommitterUploaderValidator(refControl, canonicalWebUrl)); validators.add(new SignedOffByValidator(refControl, canonicalWebUrl)); if (MagicBranch.isMagicBranch(receiveEvent.command.getRefName()) - || NEW_PATCHSET.matcher(receiveEvent.command.getRefName()).matches()) { + || ReceiveCommits.NEW_PATCHSET.matcher( + receiveEvent.command.getRefName()).matches()) { validators.add(new ChangeIdValidator(refControl, canonicalWebUrl, sshInfo)); } validators.add(new ConfigValidator(refControl, repo)); @@ -132,7 +130,8 @@ public class CommitValidators { validators.add(new AuthorUploaderValidator(refControl, canonicalWebUrl)); validators.add(new SignedOffByValidator(refControl, canonicalWebUrl)); if (MagicBranch.isMagicBranch(receiveEvent.command.getRefName()) - || NEW_PATCHSET.matcher(receiveEvent.command.getRefName()).matches()) { + || ReceiveCommits.NEW_PATCHSET.matcher( + receiveEvent.command.getRefName()).matches()) { validators.add(new ChangeIdValidator(refControl, canonicalWebUrl, sshInfo)); } validators.add(new ConfigValidator(refControl, repo));