Merge pull request #1 from criteo/fix-infinite-loop
Fix infinite loop
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java b/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
index a2f7d33..3f3196e 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
@@ -1,6 +1,9 @@
package com.criteo.gerrit.plugins.automerge;
import com.google.gerrit.common.data.SubmitRecord;
+import com.google.gerrit.extensions.api.changes.SubmitInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -76,11 +79,7 @@
* @throws OrmException
*/
public boolean hasDependentReview(final int number) throws IOException, NoSuchChangeException, OrmException {
- final Set<Account.Id> ids = byEmailCache.get(config.getBotEmail());
- final IdentifiedUser bot = factory.create(ids.iterator().next());
- final ChangeControl ctl = changeFactory.controlFor(new Change.Id(number), bot);
- final ChangeData changeData = changeDataFactory.create(db.get(), new Change.Id(number));
- final RevisionResource r = new RevisionResource(collection.parse(ctl), changeData.currentPatchSet());
+ final RevisionResource r = getRevisionResource(number);
final RelatedInfo related = getRelated.apply(r);
log.debug(String.format("Checking for related changes on review %d", number));
return related.changes.size() > 0;
@@ -119,4 +118,36 @@
log.debug(String.format("Change %d is submitable", change));
return true;
}
+
+ /**
+ * Merge a review.
+ *
+ * @param info
+ * @throws RestApiException
+ * @throws NoSuchChangeException
+ * @throws OrmException
+ * @throws IOException
+ */
+ public void mergeReview(ChangeInfo info) throws RestApiException, NoSuchChangeException, OrmException, IOException {
+ final SubmitInput input = new SubmitInput();
+ input.waitForMerge = true;
+ final RevisionResource r = getRevisionResource(info._number);
+ submitter.apply(r, input);
+ }
+
+ public RevisionResource getRevisionResource(int changeNumber) throws NoSuchChangeException, OrmException {
+ final ChangeControl ctl = changeFactory.controlFor(new Change.Id(changeNumber), getBotUser());
+ final ChangeData changeData = changeDataFactory.create(db.get(), new Change.Id(changeNumber));
+ final RevisionResource r = new RevisionResource(collection.parse(ctl), changeData.currentPatchSet());
+ return r;
+ }
+
+ private IdentifiedUser getBotUser() {
+ final Set<Account.Id> ids = byEmailCache.get(config.getBotEmail());
+ if (ids.isEmpty()) {
+ throw new RuntimeException("No user found with email: " + config.getBotEmail());
+ }
+ final IdentifiedUser bot = factory.create(ids.iterator().next());
+ return bot;
+ }
}
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
index c21c973..1b5f1c1 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
@@ -17,23 +17,13 @@
import com.google.common.collect.Lists;
import com.google.gerrit.common.ChangeListener;
import com.google.gerrit.extensions.api.GerritApi;
-import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ListChangesOption;
import com.google.gerrit.extensions.events.LifecycleListener;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountByEmailCache;
-import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.GetRelated;
import com.google.gerrit.server.change.PostReview;
-import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.data.ChangeAttribute;
import com.google.gerrit.server.events.ChangeEvent;
@@ -41,7 +31,6 @@
import com.google.gerrit.server.events.PatchSetCreatedEvent;
import com.google.gerrit.server.events.TopicChangedEvent;
import com.google.gerrit.server.git.MergeUtil;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
@@ -54,7 +43,6 @@
import java.io.IOException;
import java.util.EnumSet;
import java.util.List;
-import java.util.Set;
/**
* Starts at the same time as the gerrit server, and sets up our change hook
@@ -71,27 +59,15 @@
private AtomicityHelper atomicityHelper;
@Inject
- private AccountByEmailCache byEmailCache;
-
- @Inject
ChangeData.Factory changeDataFactory;
@Inject
- private ChangeControl.GenericFactory changeFactory;
-
- @Inject
- private ChangesCollection collection;
-
- @Inject
private AutomergeConfig config;
@Inject
Provider<ReviewDb> db;
@Inject
- private IdentifiedUser.GenericFactory factory;
-
- @Inject
GetRelated getRelated;
@Inject
@@ -106,105 +82,112 @@
@Inject
Submit submitter;
-
@Override
synchronized public void onChangeEvent(final ChangeEvent event) {
if (event instanceof TopicChangedEvent) {
- final TopicChangedEvent newComment = (TopicChangedEvent) event;
- final ChangeAttribute change = newComment.change;
- processAtomicChange(change);
+ onTopicChanged((TopicChangedEvent)event);
}
- if (event instanceof PatchSetCreatedEvent) {
- final PatchSetCreatedEvent newComment = (PatchSetCreatedEvent) event;
- final ChangeAttribute change = newComment.change;
- Integer.parseInt(change.number);
- processAtomicChange(change);
+ else if (event instanceof PatchSetCreatedEvent) {
+ onPatchSetCreated((PatchSetCreatedEvent)event);
+ }
+ else if (event instanceof CommentAddedEvent) {
+ onCommendAdded((CommentAddedEvent)event);
+ }
+ }
+
+ private void onTopicChanged(final TopicChangedEvent event) {
+ if (!atomicityHelper.isAtomicReview(event.change)) {
+ return;
+ }
+ processNewAtomicPatchSet(event.change);
+ }
+
+ private void onPatchSetCreated(final PatchSetCreatedEvent event) {
+ if (!atomicityHelper.isAtomicReview(event.change)) {
+ return;
+ }
+ processNewAtomicPatchSet(event.change);
+ }
+
+ private void onCommendAdded(final CommentAddedEvent newComment) {
+ // Avoid infinite loop when this plugin comments the review.
+ //
+ // A plugin comment will typically look like:
+ // Patch Set 1:
+ //
+ // Cross-repo comment:
+ // ...
+ if (config.getBotEmail().equals(newComment.author.email) && newComment.comment.contains(config.getCommentPrefix())) {
+ return;
}
- if (event instanceof CommentAddedEvent) {
- final CommentAddedEvent newComment = (CommentAddedEvent) event;
- final ChangeAttribute change = newComment.change;
- final int reviewNumber = Integer.parseInt(change.number);
- try {
- api.changes().id(reviewNumber).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
- if (newComment.author.email == config.getBotEmail()) {
- return;
- }
- final String topic = change.topic;
- if (atomicityHelper.isSubmittable(Integer.parseInt(newComment.change.number))) {
- log.info(String.format("Change %d is submittable. Will try to merge all related changes.", reviewNumber));
- final List<ChangeInfo> related = Lists.newArrayList();
- if (topic != null) {
- related.addAll(api.changes().query("status: open AND topic: " + topic)
- .withOption(ListChangesOption.CURRENT_REVISION).get());
- } else {
- related.add(api.changes().id(change.id).get(EnumSet.of(ListChangesOption.CURRENT_REVISION)));
- }
- boolean mergeable = true;
- String why = null;
- for (final ChangeInfo info : related) {
- api.changes().id(change.id).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
- if (!info.mergeable) {
- mergeable = false;
- why = String.format("Review %s is approved but not mergeable.", info._number);
- }
- if (!atomicityHelper.isSubmittable(info._number)) {
- mergeable = false;
- // why = String.format("Review %s is not approved.",
- // info._number);
- }
- }
- if (mergeable) {
- log.debug(String.format("Change %d is mergeable", reviewNumber));
- for (final ChangeInfo info : related) {
- final SubmitInput input = new SubmitInput();
- input.waitForMerge = true;
- final Set<Account.Id> ids = byEmailCache.get(config.getBotEmail());
- final IdentifiedUser bot = factory.create(ids.iterator().next());
- final ChangeControl ctl = changeFactory.controlFor(new Change.Id(info._number), bot);
- final ChangeData changeData = changeDataFactory.create(db.get(), new Change.Id(info._number));
+ final ChangeAttribute change = newComment.change;
+ final int reviewNumber = Integer.parseInt(change.number);
+ try {
+ checkReviewExists(reviewNumber);
+ if (atomicityHelper.isSubmittable(reviewNumber)) {
+ log.info(String.format("Change %d is submittable. Will try to merge all related changes.", reviewNumber));
+ attemptToMerge(change);
+ }
+ } catch (final RestApiException | NoSuchChangeException | OrmException | IOException e) {
+ log.error("An exception occured while trying to atomic merge a change.", e);
+ throw new RuntimeException(e);
+ }
+ }
- final RevisionResource r = new RevisionResource(collection.parse(ctl), changeData.currentPatchSet());
- submitter.apply(r, input);
- }
- } else {
- if (why != null) {
- reviewUpdater.commentOnReview(reviewNumber, AutomergeConfig.CANT_MERGE_COMMENT_FILE);
- }
- }
- }
- } catch (final RestApiException | NoSuchChangeException | OrmException | IOException e) {
- log.error("An exception occured while trying to atomic merge a change.", e);
- throw new RuntimeException(e);
+ private void attemptToMerge(ChangeAttribute change) throws RestApiException, OrmException, NoSuchChangeException, IOException {
+ final List<ChangeInfo> related = Lists.newArrayList();
+ final String topic = change.topic;
+ if (topic != null) {
+ related.addAll(api.changes().query("status: open AND topic: " + topic)
+ .withOption(ListChangesOption.CURRENT_REVISION).get());
+ } else {
+ related.add(api.changes().id(change.id).get(EnumSet.of(ListChangesOption.CURRENT_REVISION)));
+ }
+ boolean mergeable = true;
+ boolean approvedButNotMergeable = false;
+ for (final ChangeInfo info : related) {
+ api.changes().id(change.id).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
+ if (!info.mergeable) {
+ mergeable = false;
+ approvedButNotMergeable = true;
+ }
+ if (!atomicityHelper.isSubmittable(info._number)) {
+ mergeable = false;
+ }
+ }
+ final int reviewNumber = Integer.parseInt(change.number);
+ if (mergeable) {
+ log.debug(String.format("Change %d is mergeable", reviewNumber));
+ for (final ChangeInfo info : related) {
+ atomicityHelper.mergeReview(info);
+ }
+ } else {
+ if (approvedButNotMergeable) {
+ reviewUpdater.commentOnReview(reviewNumber, AutomergeConfig.CANT_MERGE_COMMENT_FILE);
}
}
}
- private void processAtomicChange(final ChangeAttribute change) {
+ private void processNewAtomicPatchSet(final ChangeAttribute change) {
final int reviewNumber = Integer.parseInt(change.number);
- log.info(String.format("Change on review %d is a topic change.", reviewNumber));
try {
- api.changes().id(reviewNumber).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
- } catch (final RestApiException e1) {
- throw new RuntimeException(e1);
- }
-
- if (atomicityHelper.isAtomicReview(change)) {
- try {
- if (atomicityHelper.hasDependentReview(reviewNumber)) {
- log.info(String.format("Setting -2 on change %d, other atomic changes exists on the same repository.",
- reviewNumber));
- reviewUpdater.setMinusTwo(reviewNumber, AutomergeConfig.ATOMIC_REVIEWS_SAME_REPO_FILE);
- } else {
- log.info(String.format("Detected atomic review on change %d.", reviewNumber));
- reviewUpdater.commentOnReview(reviewNumber, AutomergeConfig.ATOMIC_REVIEW_DETECTED_FILE);
- }
- } catch (AuthException | BadRequestException | UnprocessableEntityException | IOException | NoSuchChangeException
- | OrmException e) {
- throw new RuntimeException(e);
+ checkReviewExists(reviewNumber);
+ if (atomicityHelper.hasDependentReview(reviewNumber)) {
+ log.info(String.format("Warn the user by setting -1 on change %d, as other atomic changes exists on the same repository.",
+ reviewNumber));
+ reviewUpdater.setMinusOne(reviewNumber, AutomergeConfig.ATOMIC_REVIEWS_SAME_REPO_FILE);
+ } else {
+ log.info(String.format("Detected atomic review on change %d.", reviewNumber));
+ reviewUpdater.commentOnReview(reviewNumber, AutomergeConfig.ATOMIC_REVIEW_DETECTED_FILE);
}
+ } catch (RestApiException | IOException | NoSuchChangeException | OrmException e) {
+ throw new RuntimeException(e);
}
+ }
+ private void checkReviewExists(int reviewNumber) throws RestApiException {
+ api.changes().id(reviewNumber).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
}
@Override
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java
index 135c8df..3ff8c17 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java
@@ -21,6 +21,8 @@
private final static String defaultBotEmail = "qabot@criteo.com";
private final static String defaultTopicPrefix = "crossrepo/";
public final static String TOPIC_PREFIX_KEY = "topicPrefix";
+ private final static String defaultCommentPrefix = "Cross-repo comment:" + System.lineSeparator();
+ public final static String COMMENT_PREFIX_KEY = "commitPrefix";
public static final String getDefaultBotEmail() {
return defaultBotEmail;
@@ -33,6 +35,7 @@
private String botEmail;
private final File templatesPath;
private String topicPrefix;
+ private String commentPrefix;
@Inject
public AutomergeConfig(@GerritServerConfig final Config config, final SitePaths paths) {
@@ -44,6 +47,10 @@
if (topicPrefix == null) {
topicPrefix = defaultTopicPrefix;
}
+ commentPrefix = config.getString(AUTOMERGE_SECTION, null, COMMENT_PREFIX_KEY);
+ if (commentPrefix == null) {
+ commentPrefix = defaultCommentPrefix;
+ }
templatesPath = paths.etc_dir;
}
@@ -66,4 +73,8 @@
public final String getTopicPrefix() {
return topicPrefix;
}
+
+ public String getCommentPrefix() {
+ return commentPrefix;
+ }
}
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java b/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java
index 01923de..ef6a7a1 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java
@@ -3,18 +3,10 @@
import com.google.common.base.Charsets;
import com.google.common.io.Files;
import com.google.gerrit.extensions.api.changes.ReviewInput;
-import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountByEmailCache;
-import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
@@ -26,45 +18,28 @@
import java.io.File;
import java.io.IOException;
-import java.util.Set;
public class ReviewUpdater {
private final static Logger log = LoggerFactory.getLogger(AutomaticMerger.class);
@Inject
- private AccountByEmailCache byEmailCache;
-
- @Inject
ChangeData.Factory changeDataFactory;
@Inject
- private ChangeControl.GenericFactory changeFactory;
-
- @Inject
- private ChangesCollection collection;
-
- @Inject
AutomergeConfig config;
@Inject
Provider<ReviewDb> db;
@Inject
- private IdentifiedUser.GenericFactory factory;
-
- @Inject
Provider<PostReview> reviewer;
- public void commentOnReview(final int number, final String commentTemplate) throws NoSuchChangeException,
- OrmException, AuthException, BadRequestException, UnprocessableEntityException, IOException {
- final ReviewInput message = new ReviewInput();
- message.message = getCommentFromFile(commentTemplate);
- final Set<Account.Id> ids = byEmailCache.get(config.getBotEmail());
- final IdentifiedUser bot = factory.create(ids.iterator().next());
- final ChangeControl ctl = changeFactory.controlFor(new Change.Id(number), bot);
- final ChangeData changeData = changeDataFactory.create(db.get(), new Change.Id(number));
+ @Inject
+ private AtomicityHelper atomicityHelper;
- final RevisionResource r = new RevisionResource(collection.parse(ctl), changeData.currentPatchSet());
+ public void commentOnReview(final int number, final String commentTemplate) throws RestApiException, OrmException, IOException, NoSuchChangeException {
+ final ReviewInput message = createCrossrepoComment(commentTemplate);
+ final RevisionResource r = atomicityHelper.getRevisionResource(number);
reviewer.get().apply(r, message);
}
@@ -80,18 +55,16 @@
}
}
- public void setMinusTwo(final int number, final String commentTemplate) throws NoSuchChangeException, OrmException,
- AuthException, BadRequestException, UnprocessableEntityException, IOException {
- final ReviewInput message = new ReviewInput();
- message.message = getCommentFromFile(commentTemplate);
- message.label("Code-Review", -2);
- final Set<Account.Id> ids = byEmailCache.get(config.getBotEmail());
- final IdentifiedUser bot = factory.create(ids.iterator().next());
- final ChangeControl ctl = changeFactory.controlFor(new Change.Id(number), bot);
- final ChangeData changeData = changeDataFactory.create(db.get(), new Change.Id(number));
-
- final RevisionResource r = new RevisionResource(collection.parse(ctl), changeData.currentPatchSet());
+ public void setMinusOne(final int number, final String commentTemplate) throws RestApiException, OrmException, IOException, NoSuchChangeException {
+ final ReviewInput message = createCrossrepoComment(commentTemplate);
+ message.label("Code-Review", -1);
+ final RevisionResource r = atomicityHelper.getRevisionResource(number);
reviewer.get().apply(r, message);
}
+ private ReviewInput createCrossrepoComment(final String commentTemplate) {
+ final ReviewInput message = new ReviewInput();
+ message.message = config.getCommentPrefix() + getCommentFromFile(commentTemplate);
+ return message;
+ }
}
diff --git a/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java b/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java
index f3b2ace..ab0053f 100644
--- a/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java
+++ b/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java
@@ -35,11 +35,13 @@
final SitePaths paths = new SitePaths(new File("."));
conf.setString(AutomergeConfig.AUTOMERGE_SECTION, null, AutomergeConfig.BOT_EMAIL_KEY, "Foo@bar.com");
- conf.setString(AutomergeConfig.AUTOMERGE_SECTION, null, AutomergeConfig.TOPIC_PREFIX_KEY, "fake_prefix");
+ conf.setString(AutomergeConfig.AUTOMERGE_SECTION, null, AutomergeConfig.TOPIC_PREFIX_KEY, "fake_topic_prefix");
+ conf.setString(AutomergeConfig.AUTOMERGE_SECTION, null, AutomergeConfig.COMMENT_PREFIX_KEY, "fake_comment_prefix");
final AutomergeConfig amconf = new AutomergeConfig(conf, paths);
assertEquals(amconf.getBotEmail(), "Foo@bar.com");
- assertEquals(amconf.getTopicPrefix(), "fake_prefix");
+ assertEquals(amconf.getTopicPrefix(), "fake_topic_prefix");
+ assertEquals(amconf.getCommentPrefix(), "fake_comment_prefix");
} catch (final FileNotFoundException e) {
fail();
}