diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index e0346b30a5..63ea2df3ca 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -45,7 +45,6 @@ import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ConsistencyChecker; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.config.AnonymousCowardName; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.notedb.ChangeNoteUtil; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.project.ChangeControl; @@ -802,7 +801,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { ins = changeInserterFactory .create(id, commit, dest) - .setValidatePolicy(CommitValidators.Policy.NONE) + .setValidate(false) .setNotify(NotifyHandling.NONE) .setFireRevisionCreated(false) .setSendMail(false); @@ -826,7 +825,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { ins = patchSetInserterFactory .create(ctl, nextPatchSetId(ctl), commit) - .setValidatePolicy(CommitValidators.Policy.NONE) + .setValidate(false) .setFireRevisionCreated(false) .setNotify(NotifyHandling.NONE); bu.addOp(ctl.getId(), ins).execute(); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java index 9ad125090c..17ed280177 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java @@ -37,7 +37,6 @@ import com.google.gerrit.server.change.PostReviewers; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ProjectConfig; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; @@ -147,7 +146,7 @@ public class ReviewProjectAccess extends ProjectAccessHandler { bu.insertChange( changeInserterFactory .create(changeId, commit, RefNames.REFS_CONFIG) - .setValidatePolicy(CommitValidators.Policy.NONE) + .setValidate(false) .setUpdateRef(false)); // Created by commitToNewRef. bu.execute(); } catch (UpdateException | RestApiException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 50a4167eac..9e7e3456d7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java @@ -113,7 +113,7 @@ public class ChangeInserter implements InsertChangeOp { private String patchSetDescription; private boolean isPrivate; private List groups = Collections.emptyList(); - private CommitValidators.Policy validatePolicy = CommitValidators.Policy.GERRIT; + private boolean validate = true; private NotifyHandling notify = NotifyHandling.ALL; private ListMultimap accountsToNotify = ImmutableListMultimap.of(); private Set reviewers; @@ -238,8 +238,8 @@ public class ChangeInserter implements InsertChangeOp { return this; } - public ChangeInserter setValidatePolicy(CommitValidators.Policy validate) { - this.validatePolicy = checkNotNull(validate); + public ChangeInserter setValidate(boolean validate) { + this.validate = validate; return this; } @@ -512,7 +512,7 @@ public class ChangeInserter implements InsertChangeOp { } private void validate(RepoContext ctx) throws IOException, ResourceConflictException { - if (validatePolicy == CommitValidators.Policy.NONE) { + if (!validate) { return; } @@ -529,7 +529,7 @@ public class ChangeInserter implements InsertChangeOp { commitId, ctx.getIdentifiedUser())) { commitValidatorsFactory - .create(validatePolicy, refControl, new NoSshInfo(), ctx.getRepository()) + .forGerritCommits(refControl, new NoSshInfo(), ctx.getRepository()) .validate(event); } } catch (CommitValidationException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java index 2da4465147..ac092b5c63 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java @@ -40,7 +40,6 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.IntegrationException; import com.google.gerrit.server.git.MergeIdenticalTreeException; import com.google.gerrit.server.git.MergeUtil; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.ProjectState; @@ -291,11 +290,7 @@ public class CherryPickChange { throws OrmException, IOException { Change.Id changeId = new Change.Id(seq.nextChangeId()); ChangeInserter ins = - changeInserterFactory - .create(changeId, cherryPickCommit, refName) - .setValidatePolicy(CommitValidators.Policy.GERRIT) - .setTopic(topic); - + changeInserterFactory.create(changeId, cherryPickCommit, refName).setTopic(topic); ins.setMessage(messageForDestinationChange(ins.getPatchSetId(), sourceBranch, sourceCommit)); bu.insertChange(ins); return changeId; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index 41845e3422..13da6e4a14 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -44,7 +44,6 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.PatchSetState; import com.google.gerrit.server.patch.PatchSetInfoFactory; @@ -516,7 +515,7 @@ public class ConsistencyChecker { bu.addOp( ctl.getId(), inserter - .setValidatePolicy(CommitValidators.Policy.NONE) + .setValidate(false) .setFireRevisionCreated(false) .setNotify(NotifyHandling.NONE) .setAllowClosed(true) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java index 164647caa0..374a8ccf7f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java @@ -53,7 +53,6 @@ import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.ProjectControl; @@ -252,10 +251,7 @@ public class CreateChange implements RestModifyView groups = Collections.emptyList(); @@ -146,8 +146,8 @@ public class PatchSetInserter implements BatchUpdateOp { return this; } - public PatchSetInserter setValidatePolicy(CommitValidators.Policy validate) { - this.validatePolicy = checkNotNull(validate); + public PatchSetInserter setValidate(boolean validate) { + this.validate = validate; return this; } @@ -306,7 +306,7 @@ public class PatchSetInserter implements BatchUpdateOp { if (checkAddPatchSetPermission && !origCtl.canAddPatchSet(ctx.getDb())) { throw new AuthException("cannot add patch set"); } - if (validatePolicy == CommitValidators.Policy.NONE) { + if (!validate) { return; } @@ -323,7 +323,7 @@ public class PatchSetInserter implements BatchUpdateOp { commitId, ctx.getIdentifiedUser())) { commitValidatorsFactory - .create(validatePolicy, origCtl.getRefControl(), new NoSshInfo(), ctx.getRepository()) + .forGerritCommits(origCtl.getRefControl(), new NoSshInfo(), ctx.getRepository()) .validate(event); } catch (CommitValidationException e) { throw new ResourceConflictException(e.getFullMessage()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java index 81859c2ec2..059beafae9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java @@ -34,7 +34,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.RebaseUtil.Base; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.permissions.ChangePermission; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ChangeControl; @@ -111,8 +110,7 @@ public class Rebase rebaseFactory .create(control, rsrc.getPatchSet(), findBaseRev(repo, rw, rsrc, input)) .setForceContentMerge(true) - .setFireRevisionCreated(true) - .setValidatePolicy(CommitValidators.Policy.GERRIT)); + .setFireRevisionCreated(true)); bu.execute(); } return json.create(OPTIONS).format(change.getProject(), change.getId()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java index 71d76a0c8f..4b5f24de95 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java @@ -25,7 +25,6 @@ import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.change.RebaseUtil.Base; import com.google.gerrit.server.git.MergeUtil; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -61,7 +60,7 @@ public class RebaseChangeOp implements BatchUpdateOp { private ObjectId baseCommitId; private PersonIdent committerIdent; private boolean fireRevisionCreated = true; - private CommitValidators.Policy validate; + private boolean validate = true; private boolean checkAddPatchSetPermission = true; private boolean forceContentMerge; private boolean copyApprovals = true; @@ -96,7 +95,7 @@ public class RebaseChangeOp implements BatchUpdateOp { return this; } - public RebaseChangeOp setValidatePolicy(CommitValidators.Policy validate) { + public RebaseChangeOp setValidate(boolean validate) { this.validate = validate; return this; } @@ -170,7 +169,8 @@ public class RebaseChangeOp implements BatchUpdateOp { .setNotify(NotifyHandling.NONE) .setFireRevisionCreated(fireRevisionCreated) .setCopyApprovals(copyApprovals) - .setCheckAddPatchSetPermission(checkAddPatchSetPermission); + .setCheckAddPatchSetPermission(checkAddPatchSetPermission) + .setValidate(validate); if (postMessage) { patchSetInserter.setMessage( "Patch Set " @@ -183,9 +183,6 @@ public class RebaseChangeOp implements BatchUpdateOp { if (base != null) { patchSetInserter.setGroups(base.patchSet().getGroups()); } - if (validate != null) { - patchSetInserter.setValidatePolicy(validate); - } patchSetInserter.updateRepo(ctx); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java index dc3c948a98..0783ccabd0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java @@ -40,7 +40,6 @@ import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.extensions.events.ChangeReverted; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.mail.send.RevertedSender; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; @@ -200,7 +199,6 @@ public class Revert ChangeInserter ins = changeInserterFactory .create(changeId, revertCommit, ctl.getChange().getDest().get()) - .setValidatePolicy(CommitValidators.Policy.GERRIT) .setTopic(changeToRevert.getTopic()); ins.setMessage("Uploaded patch set 1."); 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 ae00b2eb14..c0aa5c1167 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 @@ -2147,7 +2147,7 @@ public class ReceiveCommits { .setTopic(magicBranch.topic) .setPrivate(magicBranch.isPrivate) // Changes already validated in validateNewCommits. - .setValidatePolicy(CommitValidators.Policy.NONE); + .setValidate(false); if (magicBranch.draft) { ins.setDraft(magicBranch.draft); @@ -2765,19 +2765,17 @@ public class ReceiveCommits { RevCommit c = rw.parseCommit(id); rw.parseBody(c); - CommitValidators.Policy policy; - if (magicBranch != null - && cmd.getRefName().equals(magicBranch.cmd.getRefName()) - && magicBranch.merged) { - policy = CommitValidators.Policy.MERGED; - } else { - policy = CommitValidators.Policy.RECEIVE_COMMITS; - } - try (CommitReceivedEvent receiveEvent = new CommitReceivedEvent(cmd, project, ctl.getRefName(), rw.getObjectReader(), c, user)) { - messages.addAll( - commitValidatorsFactory.create(policy, ctl, sshInfo, repo).validate(receiveEvent)); + boolean isMerged = + magicBranch != null + && cmd.getRefName().equals(magicBranch.cmd.getRefName()) + && magicBranch.merged; + CommitValidators validators = + isMerged + ? commitValidatorsFactory.forMergedCommits(ctl) + : commitValidatorsFactory.forReceiveCommits(ctl, sshInfo, repo); + messages.addAll(validators.validate(receiveEvent)); } catch (CommitValidationException e) { logDebug("Commit validation failed on {}", c.name()); messages.addAll(e.getMessages()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java index 5e1f9ffada..7fb572871a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java @@ -29,7 +29,6 @@ import com.google.gerrit.server.git.IntegrationException; import com.google.gerrit.server.git.MergeIdenticalTreeException; import com.google.gerrit.server.git.MergeTip; import com.google.gerrit.server.git.RebaseSorter; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.update.ChangeContext; @@ -168,7 +167,7 @@ public class RebaseSubmitStrategy extends SubmitStrategy { // Bypass approval copier since SubmitStrategyOp copy all approvals // later anyway. .setCopyApprovals(false) - .setValidatePolicy(CommitValidators.Policy.NONE) + .setValidate(false) .setCheckAddPatchSetPermission(false) // RebaseAlways should set always modify commit message like // Cherry-Pick strategy. 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 630dd32e9a..85ae80cd65 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 @@ -66,20 +66,6 @@ import org.slf4j.LoggerFactory; public class CommitValidators { private static final Logger log = LoggerFactory.getLogger(CommitValidators.class); - public enum Policy { - /** Use {@link Factory#forGerritCommits}. */ - GERRIT, - - /** Use {@link Factory#forReceiveCommits}. */ - RECEIVE_COMMITS, - - /** Use {@link Factory#forMergedCommits}. */ - MERGED, - - /** Do not validate commits. */ - NONE - } - @Singleton public static class Factory { private final PersonIdent gerritIdent; @@ -103,23 +89,7 @@ public class CommitValidators { cfg != null ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null; } - public CommitValidators create( - Policy policy, RefControl refControl, SshInfo sshInfo, Repository repo) throws IOException { - switch (policy) { - case RECEIVE_COMMITS: - return forReceiveCommits(refControl, sshInfo, repo); - case GERRIT: - return forGerritCommits(refControl, sshInfo, repo); - case MERGED: - return forMergedCommits(refControl); - case NONE: - return none(); - default: - throw new IllegalArgumentException("unspported policy: " + policy); - } - } - - private CommitValidators forReceiveCommits( + public CommitValidators forReceiveCommits( RefControl refControl, SshInfo sshInfo, Repository repo) throws IOException { try (RevWalk rw = new RevWalk(repo)) { NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw); @@ -139,7 +109,7 @@ public class CommitValidators { } } - private CommitValidators forGerritCommits( + public CommitValidators forGerritCommits( RefControl refControl, SshInfo sshInfo, Repository repo) { return new CommitValidators( ImmutableList.of( @@ -154,7 +124,7 @@ public class CommitValidators { new BlockExternalIdUpdateListener(allUsers))); } - private CommitValidators forMergedCommits(RefControl refControl) { + public CommitValidators forMergedCommits(RefControl refControl) { // Generally only include validators that are based on permissions of the // user creating a change for a merged commit; generally exclude // validators that would require amending the change in order to correct. @@ -174,10 +144,6 @@ public class CommitValidators { new AuthorUploaderValidator(refControl, canonicalWebUrl), new CommitterUploaderValidator(refControl, canonicalWebUrl))); } - - private CommitValidators none() { - return new CommitValidators(ImmutableList.of()); - } } private final List validators; diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index fac0a80c22..4f97ef5a31 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -74,7 +74,6 @@ import com.google.gerrit.server.change.ChangeTriplet; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.MetaDataUpdate; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.index.IndexConfig; import com.google.gerrit.server.index.QueryOptions; import com.google.gerrit.server.index.change.ChangeField; @@ -1971,7 +1970,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { ChangeInserter ins = changeFactory .create(id, commit, branch) - .setValidatePolicy(CommitValidators.Policy.NONE) + .setValidate(false) .setStatus(status) .setTopic(topic); return ins; @@ -2016,7 +2015,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { .create(ctl, new PatchSet.Id(c.getId(), n), commit) .setNotify(NotifyHandling.NONE) .setFireRevisionCreated(false) - .setValidatePolicy(CommitValidators.Policy.NONE); + .setValidate(false); try (BatchUpdate bu = updateFactory.create(db, c.getProject(), user, TimeUtil.nowTs()); ObjectInserter oi = repo.getRepository().newObjectInserter()) { bu.setRepository(repo.getRepository(), repo.getRevWalk(), oi);