opendev/gerrit
Code Issues Proposed changes

Revert "Remove requireChangeId."

Browse Source 
This reverts

* commit ec79eb968296c5900b483c87439901a3ce8862df (I2c407bdded0,
  "Remove requireChangeId.")

* commit 80beb1cac22c5e1607c6235dc6f85ecd86d21327 (I409106ce "Remove
  fields/methods that are unused since requireChangeId has been
  removed")

Reason for revert: This uncovered internal users at Google.

In particular, PutMessage, which is a commonly used REST API endpoint,
requires a Change-Id to be included if requireChangeId=false.

Change-Id: I626dbb4d53fbcbf5983b0a5e79ceadbee8a42db9
This commit is contained in:
Han-Wen Nienhuys 2019-07-24 12:19:16 +02:00
parent 2aee58d0ce
commit cdde7a309f
26 changed files with 263 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
Documentation/config-project-config.txt
View File

@ -157,6 +157,32 @@ is the author of the commit. Its main purpose is to improve tracking of who
did what, especially with patches. Default is `INHERIT`, which means that this
property is inherited from the parent project.
[[receive.requireChangeId]]receive.requireChangeId::
+
The `Require Change-Id in commit message` option defines whether a
link:user-changeid.html[Change-Id] in the commit message is required
for pushing a commit for review. If this option is set, trying to push
a commit for review that doesn't contain a Change-Id in the commit
message fails with link:error-missing-changeid.html[missing Change-Id
in commit message footer].
It is recommended to set this option and use a
link:user-changeid.html#create[commit-msg hook] (or other client side
tooling like EGit) to automatically generate Change-Id's for new
commits. This way the Change-Id is automatically in place when changes
are reworked or rebased and uploading new patch sets gets easy.
If this option is not set, commits can be uploaded without a Change-Id,
but then users have to remember to copy the assigned Change-Id from the
change screen and insert it manually into the commit message when they
want to upload a second patch set.
Default is `INHERIT`, which means that this property is inherited from
the parent project. The global default for new hosts is `true`
This option is deprecated and future releases will behave as if this
is always `true`.
[[receive.maxObjectSizeLimit]]receive.maxObjectSizeLimit::
+
Maximum allowed Git object size that receive-pack will accept. If an object

30
Documentation/rest-api-projects.txt
View File

@ -824,6 +824,11 @@ read access to `refs/meta/config`.
"configured_value": "INHERIT",
"inherited_value": false
},
"require_change_id": {
"value": false,
"configured_value": "FALSE",
"inherited_value": true
},
"max_object_size_limit": {
"value": "15m",
"configured_value": "15m",
@ -882,6 +887,7 @@ link:#config-input[ConfigInput] entity.
"enable_signed_push": "INHERIT",
"require_signed_push": "INHERIT",
"reject_implicit_merges": "INHERIT",
"require_change_id": "TRUE",
"max_object_size_limit": "10m",
"submit_type": "REBASE_IF_NECESSARY",
"state": "ACTIVE"
@ -919,6 +925,11 @@ ConfigInfo] entity.
"configured_value": "INHERIT",
"inherited_value": false
},
"require_change_id": {
"value": true,
"configured_value": "TRUE",
"inherited_value": true
},
"enable_signed_push": {
"value": true,
"configured_value": "INHERIT",
@ -3083,6 +3094,12 @@ the uploader in the commit message.
|`create_new_change_for_all_not_in_target` |optional|
link:#inherited-boolean-info[InheritedBooleanInfo] that tells whether
a new change is created for every commit not in target branch.
|`require_change_id` |optional|
link:#inherited-boolean-info[InheritedBooleanInfo] that tells whether a
valid link:user-changeid.html[Change-Id] footer in any commit uploaded
for review is required. This does not apply to commits pushed directly
to a branch or tag. This property is deprecated and will be removed in
a future release.
|`enable_signed_push`|optional, not set if signed push is disabled|
link:#inherited-boolean-info[InheritedBooleanInfo] that tells whether
signed push validation is enabled on the project.
@ -3163,6 +3180,14 @@ Whether a new change will be created for every commit not in target
branch. +
Can be `TRUE`, `FALSE` or `INHERIT`. +
If not set, this setting is not updated.
|`require_change_id` |optional|
Whether a valid link:user-changeid.html[Change-Id] footer in any commit
uploaded for review is required. This does not apply to commits pushed
directly to a branch or tag. +
Can be `TRUE`, `FALSE` or `INHERIT`. +
If not set, this setting is not updated.
This property is deprecated and will be removed in
a future release.
|`reject_implicit_merges` |optional|
Whether a check for implicit merges will be performed when changes
are pushed for review. +
@ -3528,6 +3553,11 @@ for the project (`TRUE`, `FALSE`, `INHERIT`).
Whether content merge should be enabled for the project (`TRUE`,
`FALSE`, `INHERIT`). +
`FALSE`, if the `submit_type` is `FAST_FORWARD_ONLY`.
|`require_change_id` |`INHERIT` if not set|
Whether the usage of Change-Ids is required for the project (`TRUE`,
`FALSE`, `INHERIT`).
This property is deprecated and will be removed in
a future release.
|`enable_signed_push` |`INHERIT` if not set|
Whether signed push validation is enabled on the project (`TRUE`,
`FALSE`, `INHERIT`).

9
java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@ -888,6 +888,15 @@ public abstract class AbstractDaemonTest {
}
}
protected void setRequireChangeId(InheritableBoolean value) throws Exception {
try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
ProjectConfig config = projectConfigFactory.read(md);
config.getProject().setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, value);
config.commit(md);
projectCache.evict(config.getProject());
}
}
protected PushOneCommit.Result pushTo(String ref) throws Exception {
PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
return push.to(ref);

2
java/com/google/gerrit/acceptance/TestProjectInput.java
View File

@ -43,6 +43,8 @@ public @interface TestProjectInput {
InheritableBoolean useContentMerge() default InheritableBoolean.INHERIT;
InheritableBoolean requireChangeId() default InheritableBoolean.INHERIT;
InheritableBoolean rejectEmptyCommit() default InheritableBoolean.INHERIT;
InheritableBoolean enableSignedPush() default InheritableBoolean.INHERIT;

1
java/com/google/gerrit/extensions/api/projects/ConfigInfo.java
View File

@ -29,6 +29,7 @@ public class ConfigInfo {
public InheritedBooleanInfo useContentMerge;
public InheritedBooleanInfo useSignedOffBy;
public InheritedBooleanInfo createNewChangeForAllNotInTarget;
public InheritedBooleanInfo requireChangeId;
public InheritedBooleanInfo enableSignedPush;
public InheritedBooleanInfo requireSignedPush;
public InheritedBooleanInfo rejectImplicitMerges;

1
java/com/google/gerrit/extensions/api/projects/ConfigInput.java
View File

@ -25,6 +25,7 @@ public class ConfigInput {
public InheritableBoolean useContentMerge;
public InheritableBoolean useSignedOffBy;
public InheritableBoolean createNewChangeForAllNotInTarget;
public InheritableBoolean requireChangeId;
public InheritableBoolean enableSignedPush;
public InheritableBoolean requireSignedPush;
public InheritableBoolean rejectImplicitMerges;

1
java/com/google/gerrit/extensions/api/projects/ProjectInput.java
View File

@ -31,6 +31,7 @@ public class ProjectInput {
public InheritableBoolean useContributorAgreements;
public InheritableBoolean useSignedOffBy;
public InheritableBoolean useContentMerge;
public InheritableBoolean requireChangeId;
public InheritableBoolean createNewChangeForAllNotInTarget;
public InheritableBoolean rejectEmptyCommit;
public InheritableBoolean enableSignedPush;

1
java/com/google/gerrit/reviewdb/client/BooleanProjectConfig.java
View File

@ -32,6 +32,7 @@ public enum BooleanProjectConfig {
USE_CONTRIBUTOR_AGREEMENTS("receive", "requireContributorAgreement"),
USE_SIGNED_OFF_BY("receive", "requireSignedOffBy"),
USE_CONTENT_MERGE("submit", "mergeContent"),
REQUIRE_CHANGE_ID("receive", "requireChangeId"),
CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET("receive", "createNewChangeForAllNotInTarget"),
ENABLE_SIGNED_PUSH("receive", "enableSignedPush"),
REQUIRE_SIGNED_PUSH("receive", "requireSignedPush"),

23
java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@ -146,7 +146,12 @@ public class CommitValidators {
new CommitterUploaderValidator(user, perm, urlFormatter.get()),
new SignedOffByValidator(user, perm, projectState),
new ChangeIdValidator(
user, urlFormatter.get(), installCommitMsgHookCommand, sshInfo, change),
projectState,
user,
urlFormatter.get(),
installCommitMsgHookCommand,
sshInfo,
change),
new ConfigValidator(projectConfigFactory, branch, user, rw, allUsers, allProjects),
new BannedCommitsValidator(rejectCommits),
new PluginCommitValidationListener(pluginValidators, skipValidation),
@ -173,7 +178,12 @@ public class CommitValidators {
new AuthorUploaderValidator(user, perm, urlFormatter.get()),
new SignedOffByValidator(user, perm, projectCache.checkedGet(branch.project())),
new ChangeIdValidator(
user, urlFormatter.get(), installCommitMsgHookCommand, sshInfo, change),
projectState,
user,
urlFormatter.get(),
installCommitMsgHookCommand,
sshInfo,
change),
new ConfigValidator(projectConfigFactory, branch, user, rw, allUsers, allProjects),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
@ -247,6 +257,7 @@ public class CommitValidators {
private static final Pattern CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN);
private final ProjectState projectState;
private final UrlFormatter urlFormatter;
private final String installCommitMsgHookCommand;
private final SshInfo sshInfo;
@ -254,11 +265,13 @@ public class CommitValidators {
private final Change change;
public ChangeIdValidator(
ProjectState projectState,
IdentifiedUser user,
UrlFormatter urlFormatter,
String installCommitMsgHookCommand,
SshInfo sshInfo,
Change change) {
this.projectState = projectState;
this.urlFormatter = urlFormatter;
this.installCommitMsgHookCommand = installCommitMsgHookCommand;
this.sshInfo = sshInfo;
@ -294,8 +307,10 @@ public class CommitValidators {
Type.ERROR));
throw new CommitValidationException(CHANGE_ID_ABOVE_FOOTER_MSG, messages);
}
messages.add(getMissingChangeIdErrorMsg(MISSING_CHANGE_ID_MSG));
throw new CommitValidationException(MISSING_CHANGE_ID_MSG, messages);
if (projectState.is(BooleanProjectConfig.REQUIRE_CHANGE_ID)) {
messages.add(getMissingChangeIdErrorMsg(MISSING_CHANGE_ID_MSG));
throw new CommitValidationException(MISSING_CHANGE_ID_MSG, messages);
}
} else if (idList.size() > 1) {
throw new CommitValidationException(MULTIPLE_CHANGE_ID_MSG, messages);
} else {

3
java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java
View File

@ -38,6 +38,9 @@ public class BooleanProjectConfigTransformations {
.put(
BooleanProjectConfig.USE_CONTENT_MERGE,
new Mapper(i -> i.useContentMerge, (i, v) -> i.useContentMerge = v))
.put(
BooleanProjectConfig.REQUIRE_CHANGE_ID,
new Mapper(i -> i.requireChangeId, (i, v) -> i.requireChangeId = v))
.put(
BooleanProjectConfig.CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
new Mapper(

2
java/com/google/gerrit/server/project/CreateProjectArgs.java
View File

@ -33,6 +33,7 @@ public class CreateProjectArgs {
public List<String> branch;
public InheritableBoolean contentMerge;
public InheritableBoolean newChangeForAllNotInTarget;
public InheritableBoolean changeIdRequired;
public InheritableBoolean rejectEmptyCommit;
public InheritableBoolean enableSignedPush;
public InheritableBoolean requireSignedPush;
@ -43,6 +44,7 @@ public class CreateProjectArgs {
contributorAgreements = InheritableBoolean.INHERIT;
signedOffBy = InheritableBoolean.INHERIT;
contentMerge = InheritableBoolean.INHERIT;
changeIdRequired = InheritableBoolean.INHERIT;
newChangeForAllNotInTarget = InheritableBoolean.INHERIT;
enableSignedPush = InheritableBoolean.INHERIT;
requireSignedPush = InheritableBoolean.INHERIT;

1
java/com/google/gerrit/server/project/ProjectCreator.java
View File

@ -152,6 +152,7 @@ public class ProjectCreator {
newProject.setBooleanConfig(
BooleanProjectConfig.CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
args.newChangeForAllNotInTarget);
newProject.setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, args.changeIdRequired);
newProject.setBooleanConfig(BooleanProjectConfig.REJECT_EMPTY_COMMIT, args.rejectEmptyCommit);
newProject.setMaxObjectSizeLimit(args.maxObjectSizeLimit);
newProject.setBooleanConfig(BooleanProjectConfig.ENABLE_SIGNED_PUSH, args.enableSignedPush);

11
java/com/google/gerrit/server/restapi/change/PutMessage.java
View File

@ -22,6 +22,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
@ -110,7 +111,10 @@ public class PutMessage
String sanitizedCommitMessage = CommitMessageUtil.checkAndSanitizeCommitMessage(input.message);
ensureCanEditCommitMessage(resource.getNotes());
ensureChangeIdIsCorrect(resource.getChange().getKey().get(), sanitizedCommitMessage);
ensureChangeIdIsCorrect(
projectCache.checkedGet(resource.getProject()).is(BooleanProjectConfig.REQUIRE_CHANGE_ID),
resource.getChange().getKey().get(),
sanitizedCommitMessage);
try (Repository repository = repositoryManager.openRepository(resource.getProject());
RevWalk revWalk = new RevWalk(repository);
@ -189,7 +193,8 @@ public class PutMessage
}
}
private static void ensureChangeIdIsCorrect(String currentChangeId, String newCommitMessage)
private static void ensureChangeIdIsCorrect(
boolean requireChangeId, String currentChangeId, String newCommitMessage)
throws ResourceConflictException, BadRequestException {
RevCommit revCommit =
RevCommit.parse(
@ -199,7 +204,7 @@ public class PutMessage
CommitMessageUtil.checkAndSanitizeCommitMessage(revCommit.getShortMessage());
List<String> changeIdFooters = revCommit.getFooterLines(FooterConstants.CHANGE_ID);
if (changeIdFooters.isEmpty()) {
if (requireChangeId && changeIdFooters.isEmpty()) {
throw new ResourceConflictException("missing Change-Id footer");
}
if (!changeIdFooters.isEmpty() && !changeIdFooters.get(0).equals(currentChangeId)) {

2
java/com/google/gerrit/server/restapi/project/CreateProject.java
View File

@ -147,6 +147,8 @@ public class CreateProject
args.newChangeForAllNotInTarget =
MoreObjects.firstNonNull(
input.createNewChangeForAllNotInTarget, InheritableBoolean.INHERIT);
args.changeIdRequired =
MoreObjects.firstNonNull(input.requireChangeId, InheritableBoolean.INHERIT);
args.rejectEmptyCommit =
MoreObjects.firstNonNull(input.rejectEmptyCommit, InheritableBoolean.INHERIT);
args.enableSignedPush =

2
java/com/google/gerrit/server/schema/AllProjectsInput.java
View File

@ -33,6 +33,8 @@ public abstract class AllProjectsInput {
public static final ImmutableMap<BooleanProjectConfig, InheritableBoolean>
DEFAULT_BOOLEAN_PROJECT_CONFIGS =
ImmutableMap.of(
BooleanProjectConfig.REQUIRE_CHANGE_ID,
InheritableBoolean.TRUE,
BooleanProjectConfig.USE_CONTENT_MERGE,
InheritableBoolean.TRUE,
BooleanProjectConfig.USE_CONTRIBUTOR_AGREEMENTS,

1
java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java
View File

@ -40,6 +40,7 @@ public class AllProjectsCreatorTestUtil {
"[receive]",
" requireContributorAgreement = false",
" requireSignedOffBy = false",
" requireChangeId = true",
" enableSignedPush = false");
private static final ImmutableList<String> DEFAULT_ALL_PROJECTS_SUBMIT_SECTION =
ImmutableList.of("[submit]", " mergeContent = true");

12
java/com/google/gerrit/sshd/commands/CreateProjectCommand.java
View File

@ -91,6 +91,9 @@ final class CreateProjectCommand extends SshCommand {
@Option(name = "--content-merge", usage = "allow automatic conflict resolving within files")
private InheritableBoolean contentMerge = InheritableBoolean.INHERIT;
@Option(name = "--change-id", usage = "if change-id is required")
private InheritableBoolean requireChangeID = InheritableBoolean.INHERIT;
@Option(name = "--reject-empty-commit", usage = "if empty commits should be rejected on submit")
private InheritableBoolean rejectEmptyCommit = InheritableBoolean.INHERIT;
@ -120,6 +123,14 @@ final class CreateProjectCommand extends SshCommand {
contentMerge = InheritableBoolean.TRUE;
}
@Option(
name = "--require-change-id",
aliases = {"--id"},
usage = "if change-id is required")
void setRequireChangeId(@SuppressWarnings("unused") boolean on) {
requireChangeID = InheritableBoolean.TRUE;
}
@Option(
name = "--create-new-change-for-all-not-in-target",
aliases = {"--ncfa"},
@ -175,6 +186,7 @@ final class CreateProjectCommand extends SshCommand {
input.useContributorAgreements = contributorAgreements;
input.useSignedOffBy = signedOffBy;
input.useContentMerge = contentMerge;
input.requireChangeId = requireChangeID;
input.createNewChangeForAllNotInTarget = createNewChangeForAllNotInTarget;
input.branches = branch;
input.createEmptyCommit = createEmptyCommit;

20
java/com/google/gerrit/sshd/commands/SetProjectCommand.java
View File

@ -56,6 +56,9 @@ final class SetProjectCommand extends SshCommand {
@Option(name = "--content-merge", usage = "allow automatic conflict resolving within files")
private InheritableBoolean contentMerge;
@Option(name = "--change-id", usage = "if change-id is required")
private InheritableBoolean requireChangeID;
@Option(
name = "--use-contributor-agreements",
aliases = {"--ca"},
@ -100,6 +103,22 @@ final class SetProjectCommand extends SshCommand {
contentMerge = InheritableBoolean.FALSE;
}
@Option(
name = "--require-change-id",
aliases = {"--id"},
usage = "if change-id is required")
void setRequireChangeId(@SuppressWarnings("unused") boolean on) {
requireChangeID = InheritableBoolean.TRUE;
}
@Option(
name = "--no-change-id",
aliases = {"--nid"},
usage = "if change-id is not required")
void setNoChangeId(@SuppressWarnings("unused") boolean on) {
requireChangeID = InheritableBoolean.FALSE;
}
@Option(
name = "--project-state",
aliases = {"--ps"},
@ -114,6 +133,7 @@ final class SetProjectCommand extends SshCommand {
@Override
protected void run() throws Failure {
ConfigInput configInput = new ConfigInput();
configInput.requireChangeId = requireChangeID;
configInput.submitType = submitType;
configInput.useContentMerge = contentMerge;
configInput.useContributorAgreements = contributorAgreements;

18
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@ -3842,6 +3842,24 @@ public class ChangeIT extends AbstractDaemonTest {
.isEqualTo(ChangeMessagesUtil.TAG_UPLOADED_WIP_PATCH_SET);
}
@Test
public void changeCommitMessageWithNoChangeIdSucceedsIfChangeIdNotRequired() throws Exception {
ConfigInput configInput = new ConfigInput();
configInput.requireChangeId = InheritableBoolean.FALSE;
gApi.projects().name(project.get()).config(configInput);
PushOneCommit.Result r = createChange();
r.assertOkStatus();
assertThat(getCommitMessage(r.getChangeId()))
.isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n");
String newMessage = "modified commit\n";
gApi.changes().id(r.getChangeId()).setMessage(newMessage);
RevisionApi rApi = gApi.changes().id(r.getChangeId()).current();
assertThat(rApi.files().keySet()).containsExactly("/COMMIT_MSG", "a.txt");
assertThat(getCommitMessage(r.getChangeId())).isEqualTo(newMessage);
}
@Test
public void changeCommitMessageWithNoChangeIdFails() throws Exception {
PushOneCommit.Result r = createChange();

8
javatests/com/google/gerrit/acceptance/api/project/CheckProjectIT.java
View File

@ -27,6 +27,7 @@ import com.google.gerrit.extensions.api.projects.CheckProjectInput;
import com.google.gerrit.extensions.api.projects.CheckProjectInput.AutoCloseableChangesCheckInput;
import com.google.gerrit.extensions.api.projects.CheckProjectResultInfo;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@ -65,7 +66,7 @@ public class CheckProjectIT extends AbstractDaemonTest {
@Test
public void detectAutoCloseableChangeByCommit() throws Exception {
RevCommit commit = pushCommitForReview();
RevCommit commit = pushCommitWithoutChangeIdForReview();
ChangeInfo change =
Iterables.getOnlyElement(gApi.changes().query("commit:" + commit.name()).get());
@ -89,7 +90,7 @@ public class CheckProjectIT extends AbstractDaemonTest {
@Test
public void fixAutoCloseableChangeByCommit() throws Exception {
RevCommit commit = pushCommitForReview();
RevCommit commit = pushCommitWithoutChangeIdForReview();
ChangeInfo change =
Iterables.getOnlyElement(gApi.changes().query("commit:" + commit.name()).get());
@ -279,7 +280,8 @@ public class CheckProjectIT extends AbstractDaemonTest {
+ ProjectsConsistencyChecker.AUTO_CLOSE_MAX_COMMITS_LIMIT);
}
private RevCommit pushCommitForReview() throws Exception {
private RevCommit pushCommitWithoutChangeIdForReview() throws Exception {
setRequireChangeId(InheritableBoolean.FALSE);
RevCommit commit =
testRepo
.branch("HEAD")

3
javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
View File

@ -316,6 +316,7 @@ public class ProjectIT extends AbstractDaemonTest {
assertThat(info.useSignedOffBy.configuredValue).isEqualTo(input.useSignedOffBy);
assertThat(info.createNewChangeForAllNotInTarget.configuredValue)
.isEqualTo(input.createNewChangeForAllNotInTarget);
assertThat(info.requireChangeId.configuredValue).isEqualTo(input.requireChangeId);
assertThat(info.rejectImplicitMerges.configuredValue).isEqualTo(input.rejectImplicitMerges);
assertThat(info.enableReviewerByEmail.configuredValue).isEqualTo(input.enableReviewerByEmail);
assertThat(info.createNewChangeForAllNotInTarget.configuredValue)
@ -345,6 +346,7 @@ public class ProjectIT extends AbstractDaemonTest {
assertThat(info.useSignedOffBy.configuredValue).isEqualTo(input.useSignedOffBy);
assertThat(info.createNewChangeForAllNotInTarget.configuredValue)
.isEqualTo(input.createNewChangeForAllNotInTarget);
assertThat(info.requireChangeId.configuredValue).isEqualTo(input.requireChangeId);
assertThat(info.rejectImplicitMerges.configuredValue).isEqualTo(input.rejectImplicitMerges);
assertThat(info.enableReviewerByEmail.configuredValue).isEqualTo(input.enableReviewerByEmail);
assertThat(info.createNewChangeForAllNotInTarget.configuredValue)
@ -681,6 +683,7 @@ public class ProjectIT extends AbstractDaemonTest {
input.useContentMerge = InheritableBoolean.TRUE;
input.useSignedOffBy = InheritableBoolean.TRUE;
input.createNewChangeForAllNotInTarget = InheritableBoolean.TRUE;
input.requireChangeId = InheritableBoolean.TRUE;
input.rejectImplicitMerges = InheritableBoolean.TRUE;
input.enableReviewerByEmail = InheritableBoolean.TRUE;
input.createNewChangeForAllNotInTarget = InheritableBoolean.TRUE;

64
javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@ -89,6 +89,7 @@ import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.mail.Address;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
@ -123,6 +124,7 @@ import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
@ -456,6 +458,20 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
.isEqualTo("Change has been successfully pushed.");
}
@Test
public void pushWithoutChangeIdDeprecated() throws Exception {
setRequireChangeId(InheritableBoolean.FALSE);
testRepo
.branch("HEAD")
.commit()
.message("A change")
.author(admin.newIdent())
.committer(new PersonIdent(admin.newIdent(), testRepo.getDate()))
.create();
PushResult result = pushHead(testRepo, "refs/for/master");
assertThat(result.getMessages()).contains("warning: pushing without Change-Id is deprecated");
}
@Test
public void autocloseByChangeId() throws Exception {
// Create a change
@ -1556,6 +1572,9 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
RevCommit c = createCommit(testRepo, "Message without Change-Id");
assertThat(GitUtil.getChangeId(testRepo, c)).isEmpty();
pushForReviewRejected(testRepo, "missing Change-Id in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
pushForReviewOk(testRepo);
}
@Test
@ -1579,6 +1598,9 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
+ "More text, uh oh.\n");
assertThat(GitUtil.getChangeId(testRepo, c)).isEmpty();
pushForReviewRejected(testRepo, "Change-Id must be in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
pushForReviewRejected(testRepo, "Change-Id must be in message footer");
}
@Test
@ -1615,6 +1637,9 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
+ "Change-Id: I10f98c2ef76e52e23aa23be5afeb71e40b350e86\n"
+ "Change-Id: Ie9a132e107def33bdd513b7854b50de911edba0a\n");
pushForReviewRejected(testRepo, "multiple Change-Id lines in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
pushForReviewRejected(testRepo, "multiple Change-Id lines in message footer");
}
@Test
@ -1631,6 +1656,9 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
private void testpushWithInvalidChangeId() throws Exception {
createCommit(testRepo, "Message with invalid Change-Id\n\nChange-Id: X\n");
pushForReviewRejected(testRepo, "invalid Change-Id line format in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
pushForReviewRejected(testRepo, "invalid Change-Id line format in message footer");
}
@Test
@ -1652,12 +1680,18 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
+ "\n"
+ "Change-Id: I0000000000000000000000000000000000000000\n");
pushForReviewRejected(testRepo, "invalid Change-Id line format in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
pushForReviewRejected(testRepo, "invalid Change-Id line format in message footer");
}
@Test
public void pushWithChangeIdInSubjectLine() throws Exception {
createCommit(testRepo, "Change-Id: I1234000000000000000000000000000000000000");
pushForReviewRejected(testRepo, "missing subject; Change-Id must be in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
pushForReviewRejected(testRepo, "missing subject; Change-Id must be in message footer");
}
@Test
@ -1670,6 +1704,19 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
createCommit(testRepo, commitChange1.getFullMessage());
pushForReviewRejected(
testRepo,
"same Change-Id in multiple changes.\n"
+ "Squash the commits with the same Change-Id or ensure Change-Ids are unique for each"
+ " commit");
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.getProject()
.setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, InheritableBoolean.FALSE);
u.save();
}
pushForReviewRejected(
testRepo,
"same Change-Id in multiple changes.\n"
@ -1683,6 +1730,19 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
createCommit(testRepo, commitChange1.getFullMessage());
pushForReviewRejected(
testRepo,
"same Change-Id in multiple changes.\n"
+ "Squash the commits with the same Change-Id or ensure Change-Ids are unique for each"
+ " commit");
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.getProject()
.setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, InheritableBoolean.FALSE);
u.save();
}
pushForReviewRejected(
testRepo,
"same Change-Id in multiple changes.\n"
@ -2663,6 +2723,10 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
return cds.get(0);
}
private static void pushForReviewOk(TestRepository<?> testRepo) throws GitAPIException {
pushForReview(testRepo, RemoteRefUpdate.Status.OK, null);
}
private static void pushForReviewRejected(TestRepository<?> testRepo, String expectedMessage)
throws GitAPIException {
pushForReview(testRepo, RemoteRefUpdate.Status.REJECTED_OTHER_REASON, expectedMessage);

3
javatests/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java
View File

@ -218,6 +218,7 @@ public class CreateProjectIT extends AbstractDaemonTest {
in.useContributorAgreements = InheritableBoolean.TRUE;
in.useSignedOffBy = InheritableBoolean.TRUE;
in.useContentMerge = InheritableBoolean.TRUE;
in.requireChangeId = InheritableBoolean.TRUE;
ProjectInfo p = gApi.projects().create(in).get();
assertThat(p.name).isEqualTo(newProjectName);
Project project = projectCache.get(Project.nameKey(newProjectName)).getProject();
@ -230,6 +231,8 @@ public class CreateProjectIT extends AbstractDaemonTest {
.isEqualTo(in.useSignedOffBy);
assertThat(project.getBooleanConfig(BooleanProjectConfig.USE_CONTENT_MERGE))
.isEqualTo(in.useContentMerge);
assertThat(project.getBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID))
.isEqualTo(in.requireChangeId);
}
@Test

14
javatests/com/google/gerrit/server/project/ProjectConfigTest.java
View File

@ -16,7 +16,7 @@ package com.google.gerrit.server.project;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.reviewdb.client.BooleanProjectConfig.USE_CONTRIBUTOR_AGREEMENTS;
import static com.google.gerrit.reviewdb.client.BooleanProjectConfig.REQUIRE_CHANGE_ID;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
@ -612,13 +612,13 @@ public class ProjectConfigTest {
public void readAllProjectsBaseConfigFromSitePaths() throws Exception {
ProjectConfig cfg = factory.create(ALL_PROJECTS);
cfg.load(db);
assertThat(cfg.getProject().getBooleanConfig(USE_CONTRIBUTOR_AGREEMENTS))
assertThat(cfg.getProject().getBooleanConfig(REQUIRE_CHANGE_ID))
.isEqualTo(InheritableBoolean.INHERIT);
writeDefaultAllProjectsConfig("[receive]", "requireContributorAgreement = false");
writeDefaultAllProjectsConfig("[receive]", "requireChangeId = false");
cfg.load(db);
assertThat(cfg.getProject().getBooleanConfig(USE_CONTRIBUTOR_AGREEMENTS))
assertThat(cfg.getProject().getBooleanConfig(REQUIRE_CHANGE_ID))
.isEqualTo(InheritableBoolean.FALSE);
}
@ -626,17 +626,17 @@ public class ProjectConfigTest {
public void readOtherProjectIgnoresAllProjectsBaseConfig() throws Exception {
ProjectConfig cfg = factory.create(Project.nameKey("test"));
cfg.load(db);
assertThat(cfg.getProject().getBooleanConfig(USE_CONTRIBUTOR_AGREEMENTS))
assertThat(cfg.getProject().getBooleanConfig(REQUIRE_CHANGE_ID))
.isEqualTo(InheritableBoolean.INHERIT);
writeDefaultAllProjectsConfig("[receive]", "requireContributorAgreement = false");
writeDefaultAllProjectsConfig("[receive]", "requireChangeId = false");
cfg.load(db);
// If we went through ProjectState, then this would return FALSE, since project.config for
// All-Projects would inherit from all_projects.config, and this project would inherit from
// All-Projects. But in ProjectConfig itself, there is no inheritance from All-Projects, so this
// continues to return the default.
assertThat(cfg.getProject().getBooleanConfig(USE_CONTRIBUTOR_AGREEMENTS))
assertThat(cfg.getProject().getBooleanConfig(REQUIRE_CHANGE_ID))
.isEqualTo(InheritableBoolean.INHERIT);
}

15
polygerrit-ui/app/elements/admin/gr-repo/gr-repo.html
View File

@ -154,6 +154,21 @@ limitations under the License.
</gr-select>
</span>
</section>
<section>
<span class="title">Require Change-Id in commit message</span>
<span class="value">
<gr-select
id="requireChangeIdSelect"
bind-value="{{_repoConfig.require_change_id.configured_value}}">
<select disabled$="[[_readOnly]]">
<template is="dom-repeat"
items="[[_formatBooleanSelect(_repoConfig.require_change_id)]]">
<option value="[[item.value]]">[[item.label]]</option>
</template>
</select>
</gr-select>
</span>
</section>
<section
id="enableSignedPushSettings"
class$="repositorySettings [[_computeRepositoriesClass(_repoConfig.enable_signed_push)]]">

7
polygerrit-ui/app/elements/admin/gr-repo/gr-repo_test.html
View File

@ -57,6 +57,10 @@ limitations under the License.
value: false,
configured_value: 'FALSE',
},
require_change_id: {
value: false,
configured_value: 'FALSE',
},
enable_signed_push: {
value: false,
configured_value: 'FALSE',
@ -318,6 +322,7 @@ limitations under the License.
use_content_merge: 'TRUE',
use_signed_off_by: 'TRUE',
create_new_change_for_all_not_in_target: 'TRUE',
require_change_id: 'TRUE',
enable_signed_push: 'TRUE',
require_signed_push: 'TRUE',
reject_implicit_merges: 'TRUE',
@ -347,6 +352,8 @@ limitations under the License.
configInputObj.use_content_merge;
element.$.newChangeSelect.bindValue =
configInputObj.create_new_change_for_all_not_in_target;
element.$.requireChangeIdSelect.bindValue =
configInputObj.require_change_id;
element.$.enableSignedPush.bindValue =
configInputObj.enable_signed_push;
element.$.requireSignedPush.bindValue =