From 146635a467c3795e9d837e555f74036e42ddce17 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 12 Sep 2016 15:28:37 -0400 Subject: [PATCH 1/2] Add test for pushing initial review in empty repo Change-Id: I8d80186c4b0295fb81a2a7cad20f94a87111c895 --- .../acceptance/git/AbstractPushForReview.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index c673e7b497..b49bdbac99 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -33,6 +33,7 @@ import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.TestProjectInput; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.NotifyHandling; @@ -128,6 +129,27 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { r.assertChange(Change.Status.NEW, null); } + @Test + @TestProjectInput(createEmptyCommit = false) + public void pushInitialCommitForMasterBranch() throws Exception { + RevCommit c = + testRepo.commit().message("Initial commit").insertChangeId().create(); + String id = GitUtil.getChangeId(testRepo, c).get(); + testRepo.reset(c); + + String r = "refs/for/master"; + PushResult pr = pushHead(testRepo, r, false); + assertPushOk(pr, r); + + ChangeInfo change = gApi.changes().id(id).info(); + assertThat(change.branch).isEqualTo("master"); + assertThat(change.status).isEqualTo(ChangeStatus.NEW); + + try (Repository repo = repoManager.openRepository(project)) { + assertThat(repo.resolve("master")).isNull(); + } + } + @Test public void output() throws Exception { String url = canonicalWebUrl.get(); From 1010738a9980bcd653b35afb9e5c14e4939474d8 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 12 Sep 2016 15:52:41 -0400 Subject: [PATCH 2/2] Refactor CommitValidators to reduce repetition Use a manual factory. Move the banned commit map loading into the factory method, throwing IOException, which we couldn't do with an assisted factory. At this point CommitValidators itself is just a thin wrapper around the list of CommitValidationListeners. Since the BanCommit stuff is now in the factory, we can move the switch on the Policy into the factory method as well, which greatly reduces complexity at call sites. Callers are still responsible for constructing their own CommitReceivedEvents, so there is still some code there. Change-Id: I9a7060e3a9c43f92f34da66e958ab5d1cbdaf116 --- .../gerrit/pgm/util/BatchGitModule.java | 2 - .../gerrit/server/change/ChangeInserter.java | 22 +-- .../server/change/PatchSetInserter.java | 25 +-- .../server/config/GerritGlobalModule.java | 2 - .../gerrit/server/git/ReceiveCommits.java | 7 +- .../git/validators/CommitValidators.java | 175 +++++++++--------- 6 files changed, 104 insertions(+), 129 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchGitModule.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchGitModule.java index 0360cd606a..d39c2fdc4e 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchGitModule.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchGitModule.java @@ -19,7 +19,6 @@ import com.google.gerrit.extensions.events.GitReferenceUpdatedListener; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.server.git.GitModule; import com.google.gerrit.server.git.validators.CommitValidationListener; -import com.google.gerrit.server.git.validators.CommitValidators; /** Module for batch programs that need git access. */ public class BatchGitModule extends FactoryModule { @@ -27,7 +26,6 @@ public class BatchGitModule extends FactoryModule { protected void configure() { DynamicSet.setOf(binder(), GitReferenceUpdatedListener.class); DynamicSet.setOf(binder(), CommitValidationListener.class); - factory(CommitValidators.Factory.class); install(new GitModule()); } } 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 84781f58e6..a3c992fe06 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 @@ -41,7 +41,6 @@ import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.extensions.events.CommentAdded; import com.google.gerrit.server.extensions.events.RevisionCreated; -import com.google.gerrit.server.git.BanCommit; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.BatchUpdate.Context; @@ -66,7 +65,6 @@ import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.util.ChangeIdUtil; @@ -468,9 +466,6 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { try { RefControl refControl = projectControlFactory .controlFor(ctx.getProject(), ctx.getUser()).controlForRef(refName); - CommitValidators cv = commitValidatorsFactory.create( - refControl, new NoSshInfo(), ctx.getRepository()); - String refName = psId.toRefName(); CommitReceivedEvent event = new CommitReceivedEvent( new ReceiveCommand( @@ -481,19 +476,10 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp { change.getDest().get(), commit, ctx.getIdentifiedUser()); - - switch (validatePolicy) { - case RECEIVE_COMMITS: - NoteMap rejectCommits = BanCommit.loadRejectCommitsMap( - ctx.getRepository(), ctx.getRevWalk()); - cv.validateForReceiveCommits(event, rejectCommits); - break; - case GERRIT: - cv.validateForGerritCommits(event); - break; - case NONE: - break; - } + commitValidatorsFactory + .create( + validatePolicy, refControl, new NoSshInfo(), ctx.getRepository()) + .validate(event); } catch (CommitValidationException e) { throw new ResourceConflictException(e.getFullMessage()); } catch (NoSuchProjectException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java index 0760dc61a5..3f38fc3544 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java @@ -35,7 +35,6 @@ import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.extensions.events.RevisionCreated; -import com.google.gerrit.server.git.BanCommit; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.BatchUpdate.Context; @@ -52,7 +51,6 @@ import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.transport.ReceiveCommand; import org.slf4j.Logger; @@ -275,12 +273,12 @@ public class PatchSetInserter extends BatchUpdate.Op { private void validate(RepoContext ctx) throws AuthException, ResourceConflictException, IOException, OrmException { - CommitValidators cv = commitValidatorsFactory.create( - origCtl.getRefControl(), new NoSshInfo(), ctx.getRepository()); - if (!origCtl.canAddPatchSet(ctx.getDb())) { throw new AuthException("cannot add patch set"); } + if (validatePolicy == CommitValidators.Policy.NONE) { + return; + } String refName = getPatchSetId().toRefName(); CommitReceivedEvent event = new CommitReceivedEvent( @@ -293,18 +291,11 @@ public class PatchSetInserter extends BatchUpdate.Op { commit, ctx.getIdentifiedUser()); try { - switch (validatePolicy) { - case RECEIVE_COMMITS: - NoteMap rejectCommits = BanCommit.loadRejectCommitsMap( - ctx.getRepository(), ctx.getRevWalk()); - cv.validateForReceiveCommits(event, rejectCommits); - break; - case GERRIT: - cv.validateForGerritCommits(event); - break; - case NONE: - break; - } + commitValidatorsFactory + .create( + validatePolicy, 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/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 8753f527f7..c91ca3bd81 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -116,7 +116,6 @@ import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.TransferConfig; import com.google.gerrit.server.git.strategy.SubmitStrategy; import com.google.gerrit.server.git.validators.CommitValidationListener; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.git.validators.MergeValidationListener; import com.google.gerrit.server.git.validators.MergeValidators; import com.google.gerrit.server.git.validators.MergeValidators.ProjectConfigValidator; @@ -372,7 +371,6 @@ public class GerritGlobalModule extends FactoryModule { bind(AnonymousUser.class); - factory(CommitValidators.Factory.class); factory(RefOperationValidators.Factory.class); factory(MergeValidators.Factory.class); factory(ProjectConfigValidator.Factory.class); 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 b22f7dbcfc..53e8ee8585 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 @@ -2588,12 +2588,11 @@ public class ReceiveCommits { rw.parseBody(c); CommitReceivedEvent receiveEvent = new CommitReceivedEvent(cmd, project, ctl.getRefName(), c, user); - CommitValidators commitValidators = - commitValidatorsFactory.create(ctl, sshInfo, repo); + CommitValidators commitValidators = commitValidatorsFactory.create( + CommitValidators.Policy.RECEIVE_COMMITS, ctl, sshInfo, repo); try { - messages.addAll(commitValidators.validateForReceiveCommits( - receiveEvent, rejectCommits)); + messages.addAll(commitValidators.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/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index d4956abb94..4e44a6c659 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 @@ -16,8 +16,10 @@ package com.google.gerrit.server.git.validators; import static com.google.gerrit.reviewdb.client.Change.CHANGE_ID_PATTERN; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG; +import static com.google.gerrit.server.git.ReceiveCommits.NEW_PATCHSET; import com.google.common.base.CharMatcher; +import com.google.common.collect.ImmutableList; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.PageLinks; @@ -31,15 +33,15 @@ import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.events.CommitReceivedEvent; +import com.google.gerrit.server.git.BanCommit; 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; import com.google.gerrit.server.ssh.SshInfo; import com.google.gerrit.server.util.MagicBranch; import com.google.inject.Inject; -import com.google.inject.assistedinject.Assisted; +import com.google.inject.Singleton; import com.jcraft.jsch.HostKey; @@ -51,6 +53,7 @@ import org.eclipse.jgit.notes.NoteMap; import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.FooterLine; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.util.SystemReader; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -68,107 +71,99 @@ public class CommitValidators { .getLogger(CommitValidators.class); public enum Policy { - /** Use {@link #validateForGerritCommits}. */ + /** Use {@link Factory#forGerritCommits}. */ GERRIT, - /** Use {@link #validateForReceiveCommits}. */ + /** Use {@link Factory#forReceiveCommits}. */ RECEIVE_COMMITS, /** Do not validate commits. */ NONE } - public interface Factory { - CommitValidators create(RefControl refControl, SshInfo sshInfo, - Repository repo); - } + @Singleton + public static class Factory { + private final PersonIdent gerritIdent; + private final String canonicalWebUrl; + private final DynamicSet pluginValidators; + private final AllUsersName allUsers; + private final String installCommitMsgHookCommand; - private final PersonIdent gerritIdent; - private final RefControl refControl; - private final String canonicalWebUrl; - private final String installCommitMsgHookCommand; - private final SshInfo sshInfo; - private final Repository repo; - private final DynamicSet commitValidationListeners; - private final AllUsersName allUsers; - - @Inject - CommitValidators(@GerritPersonIdent PersonIdent gerritIdent, - @CanonicalWebUrl @Nullable String canonicalWebUrl, - @GerritServerConfig Config config, - DynamicSet commitValidationListeners, - AllUsersName allUsers, - @Assisted SshInfo sshInfo, - @Assisted Repository repo, - @Assisted RefControl refControl) { - this.gerritIdent = gerritIdent; - this.canonicalWebUrl = canonicalWebUrl; - this.installCommitMsgHookCommand = - config.getString("gerrit", null, "installCommitMsgHookCommand"); - this.commitValidationListeners = commitValidationListeners; - this.allUsers = allUsers; - this.sshInfo = sshInfo; - this.repo = repo; - this.refControl = refControl; - } - - public List validateForReceiveCommits( - CommitReceivedEvent receiveEvent, NoteMap rejectCommits) - throws CommitValidationException { - - List validators = new LinkedList<>(); - - validators.add(new UploadMergesPermissionValidator(refControl)); - validators.add(new AmendedGerritMergeCommitValidationListener( - refControl, gerritIdent)); - validators.add(new AuthorUploaderValidator(refControl, canonicalWebUrl)); - validators.add(new CommitterUploaderValidator(refControl, canonicalWebUrl)); - validators.add(new SignedOffByValidator(refControl)); - if (MagicBranch.isMagicBranch(receiveEvent.command.getRefName()) - || ReceiveCommits.NEW_PATCHSET.matcher( - receiveEvent.command.getRefName()).matches()) { - validators.add(new ChangeIdValidator(refControl, canonicalWebUrl, - installCommitMsgHookCommand, sshInfo)); + @Inject + Factory(@GerritPersonIdent PersonIdent gerritIdent, + @CanonicalWebUrl @Nullable String canonicalWebUrl, + @GerritServerConfig Config cfg, + DynamicSet pluginValidators, + AllUsersName allUsers) { + this.gerritIdent = gerritIdent; + this.canonicalWebUrl = canonicalWebUrl; + this.pluginValidators = pluginValidators; + this.allUsers = allUsers; + this.installCommitMsgHookCommand = cfg != null + ? cfg.getString("gerrit", null, "installCommitMsgHookCommand") : null; } - validators.add(new ConfigValidator(refControl, repo, allUsers)); - validators.add(new BannedCommitsValidator(rejectCommits)); - validators.add(new PluginCommitValidationListener(commitValidationListeners)); - List messages = new LinkedList<>(); - - try { - for (CommitValidationListener commitValidator : validators) { - messages.addAll(commitValidator.onCommitReceived(receiveEvent)); + 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 NONE: + return none(); + default: + throw new IllegalArgumentException("unspported policy: " + policy); } - } catch (CommitValidationException e) { - // Keep the old messages (and their order) in case of an exception - messages.addAll(e.getMessages()); - throw new CommitValidationException(e.getMessage(), messages); } - return messages; + + private CommitValidators forReceiveCommits(RefControl refControl, + SshInfo sshInfo, Repository repo) throws IOException { + try (RevWalk rw = new RevWalk(repo)) { + NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw); + return new CommitValidators(ImmutableList.of( + new UploadMergesPermissionValidator(refControl), + new AmendedGerritMergeCommitValidationListener( + refControl, gerritIdent), + new AuthorUploaderValidator(refControl, canonicalWebUrl), + new CommitterUploaderValidator(refControl, canonicalWebUrl), + new SignedOffByValidator(refControl), + new ChangeIdValidator(refControl, canonicalWebUrl, + installCommitMsgHookCommand, sshInfo), + new ConfigValidator(refControl, repo, allUsers), + new BannedCommitsValidator(rejectCommits), + new PluginCommitValidationListener(pluginValidators))); + } + } + + private CommitValidators forGerritCommits(RefControl refControl, + SshInfo sshInfo, Repository repo) { + return new CommitValidators(ImmutableList.of( + new UploadMergesPermissionValidator(refControl), + new AmendedGerritMergeCommitValidationListener( + refControl, gerritIdent), + new AuthorUploaderValidator(refControl, canonicalWebUrl), + new SignedOffByValidator(refControl), + new ChangeIdValidator(refControl, canonicalWebUrl, + installCommitMsgHookCommand, sshInfo), + new ConfigValidator(refControl, repo, allUsers), + new PluginCommitValidationListener(pluginValidators))); + } + + private CommitValidators none() { + return new CommitValidators(ImmutableList.of()); + } } - public List validateForGerritCommits( + private final List validators; + + CommitValidators(List validators) { + this.validators = validators; + } + + public List validate( CommitReceivedEvent receiveEvent) throws CommitValidationException { - - List validators = new LinkedList<>(); - - validators.add(new UploadMergesPermissionValidator(refControl)); - validators.add(new AmendedGerritMergeCommitValidationListener( - refControl, gerritIdent)); - validators.add(new AuthorUploaderValidator(refControl, canonicalWebUrl)); - validators.add(new SignedOffByValidator(refControl)); - if (MagicBranch.isMagicBranch(receiveEvent.command.getRefName()) - || ReceiveCommits.NEW_PATCHSET.matcher( - receiveEvent.command.getRefName()).matches()) { - validators.add(new ChangeIdValidator(refControl, canonicalWebUrl, - installCommitMsgHookCommand, sshInfo)); - } - validators.add(new ConfigValidator(refControl, repo, allUsers)); - validators.add(new PluginCommitValidationListener(commitValidationListeners)); - List messages = new LinkedList<>(); - try { for (CommitValidationListener commitValidator : validators) { messages.addAll(commitValidator.onCommitReceived(receiveEvent)); @@ -221,6 +216,9 @@ public class CommitValidators { @Override public List onCommitReceived( CommitReceivedEvent receiveEvent) throws CommitValidationException { + if (!shouldValidateChangeId(receiveEvent)) { + return Collections.emptyList(); + } RevCommit commit = receiveEvent.commit; List messages = new LinkedList<>(); List idList = commit.getFooterLines(FooterConstants.CHANGE_ID); @@ -255,6 +253,11 @@ public class CommitValidators { return Collections.emptyList(); } + private static boolean shouldValidateChangeId(CommitReceivedEvent event) { + return MagicBranch.isMagicBranch(event.command.getRefName()) + || NEW_PATCHSET.matcher(event.command.getRefName()).matches(); + } + private CommitValidationMessage getMissingChangeIdErrorMsg( final String errMsg, final RevCommit c) { StringBuilder sb = new StringBuilder();