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