Fix wrong comment on simple reviews
When a review is not cross-repo and cannot be merged because of a conflict,
we were showing a message prefixed with "Cross-repo comment".
Now, the messages are no more prefixed.
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 9c519b5..4801546 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
@@ -25,6 +25,7 @@
import com.google.gerrit.server.change.GetRelated;
import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.Submit;
+import com.google.gerrit.server.data.ApprovalAttribute;
import com.google.gerrit.server.data.ChangeAttribute;
import com.google.gerrit.server.events.ChangeEvent;
import com.google.gerrit.server.events.CommentAddedEvent;
@@ -110,14 +111,7 @@
}
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())) {
+ if (!shouldProcessCommentEvent(newComment)) {
return;
}
@@ -135,6 +129,26 @@
}
}
+ /**
+ * Returns true if the plugin must handle this comment, i.e. if we are sure it does not come
+ * from this plugin (to avoid infinite loop).
+ *
+ * @param comment
+ * @return a boolean
+ */
+ private boolean shouldProcessCommentEvent(CommentAddedEvent comment) {
+ if (!config.getBotEmail().equals(comment.author.email)) {
+ return true;
+ }
+ for (ApprovalAttribute approval : comment.approvals) {
+ // See ReviewUpdate#setMinusOne
+ if (!("Code-Review".equals(approval.type) && "-1".equals(approval.value))) {
+ return true;
+ }
+ }
+ return false;
+ }
+
private void attemptToMerge(ChangeAttribute change) throws RestApiException, OrmException, NoSuchChangeException, IOException {
final List<ChangeInfo> related = Lists.newArrayList();
final String topic = change.topic;
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 3ff8c17..135c8df 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java
@@ -21,8 +21,6 @@
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;
@@ -35,7 +33,6 @@
private String botEmail;
private final File templatesPath;
private String topicPrefix;
- private String commentPrefix;
@Inject
public AutomergeConfig(@GerritServerConfig final Config config, final SitePaths paths) {
@@ -47,10 +44,6 @@
if (topicPrefix == null) {
topicPrefix = defaultTopicPrefix;
}
- commentPrefix = config.getString(AUTOMERGE_SECTION, null, COMMENT_PREFIX_KEY);
- if (commentPrefix == null) {
- commentPrefix = defaultCommentPrefix;
- }
templatesPath = paths.etc_dir;
}
@@ -73,8 +66,4 @@
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 ef6a7a1..32c05fe 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java
@@ -3,7 +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.RestApiException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.RevisionResource;
@@ -38,33 +41,32 @@
private AtomicityHelper atomicityHelper;
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);
- }
-
- private final String getCommentFromFile(final String filename) {
- try {
- return Files.toString(new File(config.getTemplatesPath(), filename), Charsets.UTF_8);
-
- } catch (final IOException exc) {
- final String errmsg =
- String.format("Cannot find %s file in gerrit etc dir. Please check your gerrit configuration", filename);
- log.error(errmsg);
- return errmsg;
- }
+ final ReviewInput comment = createComment(commentTemplate);
+ applyComment(number, comment);
}
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);
+ final ReviewInput message = createComment(commentTemplate).label("Code-Review", -1);
+ applyComment(number, message);
}
- private ReviewInput createCrossrepoComment(final String commentTemplate) {
- final ReviewInput message = new ReviewInput();
- message.message = config.getCommentPrefix() + getCommentFromFile(commentTemplate);
- return message;
+ private ReviewInput createComment(final String commentTemplate) {
+ return new ReviewInput().message(getCommentFromFile(commentTemplate));
+ }
+
+ private String getCommentFromFile(final String filename) {
+ try {
+ return Files.toString(new File(config.getTemplatesPath(), filename), Charsets.UTF_8);
+ } catch (final IOException exc) {
+ final String errmsg =
+ String.format("Cannot find %s file in gerrit etc dir. Please check your gerrit configuration", filename);
+ log.error(errmsg);
+ return errmsg;
+ }
+ }
+
+ private void applyComment(final int number, ReviewInput comment) throws RestApiException, OrmException, IOException, NoSuchChangeException {
+ final RevisionResource r = atomicityHelper.getRevisionResource(number);
+ reviewer.get().apply(r, comment);
}
}
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 ab0053f..3ff50ee 100644
--- a/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java
+++ b/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java
@@ -36,12 +36,10 @@
conf.setString(AutomergeConfig.AUTOMERGE_SECTION, null, AutomergeConfig.BOT_EMAIL_KEY, "Foo@bar.com");
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_topic_prefix");
- assertEquals(amconf.getCommentPrefix(), "fake_comment_prefix");
} catch (final FileNotFoundException e) {
fail();
}
diff --git a/tests.txt b/tests.txt
index 1bd34d2..02ea468 100644
--- a/tests.txt
+++ b/tests.txt
@@ -1,9 +1,23 @@
-Create review1 on repo1.
-Create review1b on top of review1. Set the topic to "crossrepo/test":
+One repo:
+
+Create review0 on repo1.
+Create review0b on top of review1. Set the topic to "crossrepo/test":
log: Setting -1
qabot Code-review -1
message: "This review depends on an unmerged commit."
-Abandon review1b
+Abandon review0b
+Create a parallel review0c on repo1 that conflicts with review0.
+Submit review0c.
+Approve review0:
+ logged: "Change %d is submittable. Will try to merge all related changes."
+ comment: "This review is not mergeable because there is a conflict."
+Make review0 mergeable by rebasing.
+approve review0:
+ merged: review0
+
+Two repos:
+
+Create review1 on repo1.
Set to the topic of review1 to "crossrepo/test":
message: "This review is a cross-repo refactoring. It will be merged when all dependant reviews are mergeable."
logged: "Detected atomic review on change xxx."
@@ -14,7 +28,8 @@
logged: "Change %d is submittable. Will try to merge all related changes."
Approve review1 but make it not mergeable (merge an other review that conflicts), then comment on review2:
logged: "Change %d is submittable. Will try to merge all related changes."
- comment: "Some related reviews to this cross-repo refactoring are not mergeable yet."
+ comment: "This review is not mergeable because there is a conflict."
Make review1 mergeable:
log DEBUG: "Change %d is mergeable"
merged: review1 and review2
+
