diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index db673dd875..57553224b7 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -952,6 +952,15 @@ public abstract class AbstractDaemonTest { } } + protected void setRequireChangeId(InheritableBoolean value) throws Exception { + try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) { + ProjectConfig config = ProjectConfig.read(md); + config.getProject().setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, value); + config.commit(md); + projectCache.evict(config.getProject()); + } + } + protected void deny(String ref, String permission, AccountGroup.UUID id) throws Exception { deny(project, ref, permission, id); } diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java index 04934c9b34..d5db92f9a1 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -82,7 +82,7 @@ public class CommitValidators { private static final Logger log = LoggerFactory.getLogger(CommitValidators.class); public static final Pattern NEW_PATCHSET_PATTERN = - Pattern.compile("^" + REFS_CHANGES + "(?:[0-9][0-9]/)?([1-9][0-9]*)(?:/new)?$"); + Pattern.compile("^" + REFS_CHANGES + "(?:[0-9][0-9]/)?([1-9][0-9]*)(?:/[1-9][0-9]*)?$"); @Singleton public static class Factory { @@ -291,15 +291,13 @@ public class CommitValidators { String sha1 = commit.abbreviate(RevId.ABBREV_LEN).name(); if (idList.isEmpty()) { + String shortMsg = commit.getShortMessage(); + if (shortMsg.startsWith(CHANGE_ID_PREFIX) + && CHANGE_ID.matcher(shortMsg.substring(CHANGE_ID_PREFIX.length()).trim()).matches()) { + String errMsg = String.format(MISSING_SUBJECT_MSG, sha1); + throw new CommitValidationException(errMsg); + } if (projectState.is(BooleanProjectConfig.REQUIRE_CHANGE_ID)) { - String shortMsg = commit.getShortMessage(); - if (shortMsg.startsWith(CHANGE_ID_PREFIX) - && CHANGE_ID - .matcher(shortMsg.substring(CHANGE_ID_PREFIX.length()).trim()) - .matches()) { - String errMsg = String.format(MISSING_SUBJECT_MSG, sha1); - throw new CommitValidationException(errMsg); - } String errMsg = String.format(MISSING_CHANGE_ID_MSG, sha1); messages.add(getMissingChangeIdErrorMsg(errMsg, commit)); throw new CommitValidationException(errMsg, messages); diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java index d95d2c17fd..867ace4cce 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -1332,11 +1332,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { assertThat(GitUtil.getChangeId(testRepo, c)).isEmpty(); pushForReviewRejected(testRepo, "missing Change-Id in commit message footer"); - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config - .getProject() - .setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, InheritableBoolean.FALSE); - saveProjectConfig(project, config); + setRequireChangeId(InheritableBoolean.FALSE); pushForReviewOk(testRepo); } @@ -1383,11 +1379,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { + "Change-Id: Ie9a132e107def33bdd513b7854b50de911edba0a\n"); pushForReviewRejected(testRepo, "multiple Change-Id lines in commit message footer"); - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config - .getProject() - .setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, InheritableBoolean.FALSE); - saveProjectConfig(project, config); + setRequireChangeId(InheritableBoolean.FALSE); pushForReviewRejected(testRepo, "multiple Change-Id lines in commit message footer"); } @@ -1406,11 +1398,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { createCommit(testRepo, "Message with invalid Change-Id\n\nChange-Id: X\n"); pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer"); - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config - .getProject() - .setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, InheritableBoolean.FALSE); - saveProjectConfig(project, config); + setRequireChangeId(InheritableBoolean.FALSE); pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer"); } @@ -1434,15 +1422,19 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { + "Change-Id: I0000000000000000000000000000000000000000\n"); pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer"); - ProjectConfig config = projectCache.checkedGet(project).getConfig(); - config - .getProject() - .setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, InheritableBoolean.FALSE); - - saveProjectConfig(project, config); + setRequireChangeId(InheritableBoolean.FALSE); pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer"); } + @Test + public void pushWithChangeIdInSubjectLine() throws Exception { + createCommit(testRepo, "Change-Id: I1234000000000000000000000000000000000000"); + pushForReviewRejected(testRepo, "missing subject; Change-Id must be in commit message footer"); + + setRequireChangeId(InheritableBoolean.FALSE); + pushForReviewRejected(testRepo, "missing subject; Change-Id must be in commit message footer"); + } + @Test public void pushCommitWithSameChangeIdAsPredecessorChange() throws Exception { PushOneCommit push = diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index 2875dd5c07..af609a6e8a 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -42,6 +42,7 @@ import com.google.gerrit.extensions.common.CommitInfo; import com.google.gerrit.extensions.common.MergeInput; import com.google.gerrit.extensions.common.RevisionInfo; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; @@ -95,6 +96,16 @@ public class CreateChangeIT extends AbstractDaemonTest { assertCreateFails(ci, BadRequestException.class, "unsupported change status"); } + @Test + public void createEmptyChange_InvalidChangeId() throws Exception { + ChangeInput ci = newChangeInput(ChangeStatus.NEW); + ci.subject = "Subject\n\nChange-Id: I0000000000000000000000000000000000000000"; + assertCreateFails( + ci, + ResourceConflictException.class, + "invalid Change-Id line format in commit message footer"); + } + @Test public void createNewChange() throws Exception { assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));