Use tags instead of votes to denote automerger-created CLs.
Instead of using votes (which can be tampered with, and introduce
extra complexity w.r.t. what shows up in the UI), we instead use
tags. These tags ensure that we can mark a patchset as
automerger-created without letting the user change that fact.
Change-Id: I41bf0a9cc3dffbb5462b9d8b5dfcc5bbe150f8a2
diff --git a/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java b/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
index df51610..8691038 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
@@ -223,11 +223,11 @@
}
public short getMaxAutomergeVote() throws ConfigInvalidException {
- return (short) getConfig().getInt("global", "maxAutomergeVote", 1);
+ return (short) getConfig().getInt("global", "maxAutomergeVote", 2);
}
public short getMinAutomergeVote() throws ConfigInvalidException {
- return (short) getConfig().getInt("global", "minAutomergeVote", 1);
+ return (short) getConfig().getInt("global", "minAutomergeVote", -2);
}
// Returns overriden manifest config if specified, default if not
diff --git a/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java b/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
index 810d615..7d52446 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
@@ -65,6 +65,8 @@
RevisionCreatedListener,
TopicEditedListener {
private static final Logger log = LoggerFactory.getLogger(DownstreamCreator.class);
+ private static final String AUTOMERGER_TAG = "autogenerated:Automerger";
+ private static final String MERGE_CONFLICT_TAG = "autogenerated:MergeConflict";
protected GerritApi gApi;
protected ConfigLoader config;
@@ -167,10 +169,10 @@
ChangeInfo downstreamChange =
gApi.changes().id(changeNumber).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
for (Map.Entry<String, ApprovalInfo> label : approvals.entrySet()) {
- propagateVote(downstreamChange, label.getKey(), label.getValue().value.shortValue());
+ updateVote(downstreamChange, label.getKey(), label.getValue().value.shortValue());
}
}
- } catch (RestApiException | ConfigInvalidException e) {
+ } catch (RestApiException e) {
log.error("Exception when updating downstream votes of {}", change.id, e);
}
}
@@ -232,7 +234,6 @@
throws RestApiException, ConfigInvalidException {
ReviewInput reviewInput = new ReviewInput();
Map<String, Short> labels = new HashMap<String, Short>();
- short vote = 0;
try {
createDownstreamMerges(mdsMergeInput);
@@ -244,10 +245,10 @@
} catch (FailedMergeException e) {
reviewInput.message = e.getDisplayString();
reviewInput.notify = NotifyHandling.ALL;
+ reviewInput.tag = MERGE_CONFLICT_TAG;
// Vote minAutomergeVote if we hit a conflict.
- vote = config.getMinAutomergeVote();
+ labels.put(config.getAutomergeLabel(), config.getMinAutomergeVote());
}
- labels.put(config.getAutomergeLabel(), vote);
reviewInput.labels = labels;
gApi.changes()
.id(mdsMergeInput.sourceId)
@@ -466,15 +467,6 @@
}
}
- private void propagateVote(ChangeInfo change, String label, short vote)
- throws RestApiException, ConfigInvalidException {
- if (label.equals(config.getAutomergeLabel())) {
- log.debug("Not updating automerge label, as it blocks when there is a merge conflict.");
- return;
- }
- updateVote(change, label, vote);
- }
-
private void updateVote(ChangeInfo change, String label, short vote) throws RestApiException {
log.debug("Giving {} for label {} to {}", vote, label, change.id);
// Vote on all downstream branches unless merge conflict.
@@ -483,6 +475,7 @@
labels.put(label, vote);
reviewInput.labels = labels;
reviewInput.notify = NotifyHandling.NONE;
+ reviewInput.tag = AUTOMERGER_TAG;
gApi.changes().id(change.id).revision(change.currentRevision).review(reviewInput);
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
index a9863ca..fe9f8ab 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
@@ -24,6 +24,7 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -32,6 +33,9 @@
import com.google.gerrit.reviewdb.client.RefNames;
import java.io.InputStream;
import java.io.InputStreamReader;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
import java.util.EnumSet;
import java.util.List;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
@@ -64,6 +68,25 @@
assertThat(changesInTopic).hasSize(3);
// +2 and submit
merge(result);
+ List<ChangeInfo> sortedChanges = sortedChanges(changesInTopic);
+
+ ChangeInfo masterChangeInfo = sortedChanges.get(0);
+ ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
+ assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(2);
+ assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo(null);
+ assertThat(masterChangeInfo.branch).isEqualTo("master");
+
+ ChangeInfo dsOneChangeInfo = sortedChanges.get(1);
+ assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
+ ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(2);
+ assertThat(getVote(dsOneChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+
+ ChangeInfo dsTwoChangeInfo = sortedChanges.get(2);
+ assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
+ ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
+ assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(2);
+ assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
}
@Test
@@ -86,25 +109,34 @@
List<ChangeInfo> changesInTopic = gApi.changes().query("topic: " + change.topic()).get();
assertThat(changesInTopic).hasSize(3);
- for (ChangeInfo c : changesInTopic) {
- ChangeApi downstreamChange = gApi.changes().id(c._number);
- // It should skip ds_one, since this is a DO NOT MERGE
- if (c.branch.equals("ds_one")) {
- assertThat(getVote(downstreamChange, "Code-Review")).isEqualTo(1);
- assertThat(downstreamChange.get().subject).contains("skipped:");
- assertThat(downstreamChange.current().files().keySet()).contains("filename");
- assertThat(downstreamChange.current().files().get("filename").linesDeleted).isEqualTo(1);
- } else if (c.branch.equals("ds_two")) {
- assertThat(getVote(downstreamChange, "Code-Review")).isEqualTo(1);
- // It should not skip ds_two, since it is marked with mergeAll: true
- assertThat(downstreamChange.get().subject).doesNotContain("skipped:");
- BinaryResult downstreamContent = downstreamChange.current().file("filename").content();
- assertThat(downstreamContent.asString()).isEqualTo(content.asString());
- } else {
- assertThat(getVote(downstreamChange, "Code-Review")).isEqualTo(0);
- assertThat(c.branch).isEqualTo("master");
- }
- }
+
+ List<ChangeInfo> sortedChanges = sortedChanges(changesInTopic);
+
+ ChangeInfo masterChangeInfo = sortedChanges.get(0);
+ ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
+ assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
+ assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo(null);
+ assertThat(masterChangeInfo.branch).isEqualTo("master");
+
+ ChangeInfo dsOneChangeInfo = sortedChanges.get(1);
+ assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
+ ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(0);
+ assertThat(getVote(dsOneChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ // It should skip ds_one, since this is a DO NOT MERGE
+ assertThat(dsOneChange.get().subject).contains("skipped:");
+ assertThat(dsOneChange.current().files().keySet()).contains("filename");
+ assertThat(dsOneChange.current().files().get("filename").linesDeleted).isEqualTo(1);
+
+ ChangeInfo dsTwoChangeInfo = sortedChanges.get(2);
+ assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
+ ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
+ assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(0);
+ assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ // It should not skip ds_two, since it is marked with mergeAll: true
+ assertThat(dsTwoChange.get().subject).doesNotContain("skipped:");
+ BinaryResult dsTwoContent = dsTwoChange.current().file("filename").content();
+ assertThat(dsTwoContent.asString()).isEqualTo(content.asString());
}
@Test
@@ -123,20 +155,36 @@
result.assertOkStatus();
ChangeApi change = gApi.changes().id(result.getChangeId());
-
List<ChangeInfo> changesInTopic = gApi.changes().query("topic: " + change.topic()).get();
assertThat(changesInTopic).hasSize(3);
- for (ChangeInfo c : changesInTopic) {
- ChangeApi downstreamChange = gApi.changes().id(c._number);
- // It should skip ds_one and ds_two, since this is a DO NOT MERGE ANYWHERE
- if (c.branch.equals("ds_one") || c.branch.equals("ds_two")) {
- assertThat(downstreamChange.get().subject).contains("skipped:");
- assertThat(downstreamChange.current().files().keySet()).contains("filename");
- assertThat(downstreamChange.current().files().get("filename").linesDeleted).isEqualTo(1);
- } else {
- assertThat(c.branch).isEqualTo("master");
- }
- }
+
+ List<ChangeInfo> sortedChanges = sortedChanges(changesInTopic);
+
+ ChangeInfo masterChangeInfo = sortedChanges.get(0);
+ ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
+ assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
+ assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo(null);
+ assertThat(masterChangeInfo.branch).isEqualTo("master");
+
+ ChangeInfo dsOneChangeInfo = sortedChanges.get(1);
+ assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
+ ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(0);
+ assertThat(getVote(dsOneChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ // It should skip ds_one, since this is a DO NOT MERGE ANYWHERE
+ assertThat(dsOneChange.get().subject).contains("skipped:");
+ assertThat(dsOneChange.current().files().keySet()).contains("filename");
+ assertThat(dsOneChange.current().files().get("filename").linesDeleted).isEqualTo(1);
+
+ ChangeInfo dsTwoChangeInfo = sortedChanges.get(2);
+ assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
+ ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
+ assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(0);
+ assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ // It should skip ds_one, since this is a DO NOT MERGE ANYWHERE
+ assertThat(dsTwoChange.get().subject).contains("skipped:");
+ assertThat(dsTwoChange.current().files().keySet()).contains("filename");
+ assertThat(dsTwoChange.current().files().get("filename").linesDeleted).isEqualTo(1);
}
@Test
@@ -168,12 +216,28 @@
.create(db, admin.getIdent(), testRepo, "subject", "filename", "echo 'Hello World!'")
.to("refs/for/master");
masterResult.assertOkStatus();
+
+
// Since there's a conflict with ds_one, there should only be two changes in the topic
List<ChangeInfo> changesInTopic =
gApi.changes()
.query("topic: " + gApi.changes().id(masterResult.getChangeId()).topic())
.get();
assertThat(changesInTopic).hasSize(2);
+ List<ChangeInfo> sortedChanges = sortedChanges(changesInTopic);
+
+ ChangeInfo masterChangeInfo = sortedChanges.get(0);
+ ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
+ assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(-2);
+ assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo("autogenerated:MergeConflict");
+ assertThat(masterChangeInfo.branch).isEqualTo("master");
+
+ ChangeInfo dsTwoChangeInfo = sortedChanges.get(1);
+ assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
+ ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
+ // This is -2 because the -2 vote from master propagated to ds_two
+ assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(-2);
+ assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
}
private Project.NameKey defaultSetup() throws Exception {
@@ -219,13 +283,20 @@
}
}
- private int getVote(ChangeApi change, String label) throws RestApiException {
- return change
- .get(EnumSet.of(ListChangesOption.DETAILED_LABELS))
- .labels
- .get(label)
- .all
- .get(0)
- .value;
+ private ApprovalInfo getVote(ChangeApi change, String label) throws RestApiException {
+ return change.get(EnumSet.of(ListChangesOption.DETAILED_LABELS)).labels.get(label).all.get(0);
+ }
+
+ private List<ChangeInfo> sortedChanges(List<ChangeInfo> changes) {
+ List<ChangeInfo> listCopy = new ArrayList<ChangeInfo>(changes);
+ Collections.sort(
+ listCopy,
+ new Comparator<ChangeInfo>() {
+ @Override
+ public int compare(ChangeInfo c1, ChangeInfo c2) {
+ return c1._number - c2._number;
+ }
+ });
+ return listCopy;
}
}
