opendev/gerrit
Code Issues Proposed changes

Remove CommitValidators.Policy

Browse Source 
Almost all callers are just switching between a single policy, e.g. the
default of GERRIT for ChangeInserter/PatchSetInserter, and no
validation. The internals of CommitValidators.Factory#create just called
one of the for* factory methods depending on the enum value; it's just
as readable to do this at the source.

Within ChangeInserter/PatchSetInserter, it's just as easy to represent
do/don't validate with a single boolean value. The interaction with
RebaseChangeOp is slightly nonobvious: it used a null enum field value
to mean don't override PatchSetInserter's default. Thus we need to
explicitly set the default of the boolean value to true. (Or use
tri-state Boolean, but boolean is more consistent with the rest of the
fields in this class.)

It's worth noting that forGerritCommits is always called from within a
BatchUpdateOp, which means we can later change its parameters to not
pass a full Repository instance. By contrast forReceiveCommits is only
called from ReceiveCommits, outside of the BatchUpdate, so it always has
a full Repository available. Splitting the factory methods up in this
way allows us to pass different arguments to the different paths.

Change-Id: I833b739f2c4ab4a65ea80382b1c147dd6adc1892
This commit is contained in:
Dave Borowitz
2017-04-06 15:51:05 -04:00
parent 17e262e92b
commit 9bad11795a
14 changed files with 37 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
View File

@@ -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();

3
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java
View File

@@ -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<Change.Id> {
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) {

10
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -113,7 +113,7 @@ public class ChangeInserter implements InsertChangeOp {
private String patchSetDescription;
private boolean isPrivate;
private List<String> groups = Collections.emptyList();
private CommitValidators.Policy validatePolicy = CommitValidators.Policy.GERRIT;
private boolean validate = true;
private NotifyHandling notify = NotifyHandling.ALL;
private ListMultimap<RecipientType, Account.Id> accountsToNotify = ImmutableListMultimap.of();
private Set<Account.Id> 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) {

7
gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
View File

@@ -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;

3
gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java
View File

@@ -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)

6
gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
View File

@@ -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<TopLevelResource, ChangeInpu
}
Change.Id changeId = new Change.Id(seq.nextChangeId());
ChangeInserter ins =
changeInserterFactory
.create(changeId, c, refName)
.setValidatePolicy(CommitValidators.Policy.GERRIT);
ChangeInserter ins = changeInserterFactory.create(changeId, c, refName);
ins.setMessage(String.format("Uploaded patch set %s.", ins.getPatchSetId().get()));
String topic = input.topic;
if (topic != null) {

10
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -88,7 +88,7 @@ public class PatchSetInserter implements BatchUpdateOp {
// Fields exposed as setters.
private String message;
private String description;
private CommitValidators.Policy validatePolicy = CommitValidators.Policy.GERRIT;
private boolean validate = true;
private boolean checkAddPatchSetPermission = true;
private boolean draft;
private List<String> 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());

4
gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java
View File

@@ -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());

11
gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
View File

@@ -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);
}

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java
View File

@@ -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.");

22
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -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());

3
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
View File

@@ -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.

40
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@@ -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.<CommitValidationListener>of());
}
}
private final List<CommitValidationListener> validators;

5
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -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);