Upstream +2 should not override merge conflict -2.
Change-Id: Id870fe4706b1231befed1dea34a7b618e2ecc0c1
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 e6e298d..618385b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
@@ -256,6 +256,11 @@
return getConfig().getBoolean("global", "disableMinAutomergeVote", false);
}
+ public boolean contextUserIdIsSet() throws ConfigInvalidException {
+ int contextUserId = getConfig().getInt("global", "contextUserId", -1);
+ return contextUserId > 0;
+ }
+
public Account.Id getContextUserId() throws ConfigInvalidException {
int contextUserId = getConfig().getInt("global", "contextUserId", -1);
if (contextUserId > 0) {
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 5a8e380..0fbde41 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
@@ -82,6 +82,7 @@
protected GerritApi gApi;
protected ConfigLoader config;
protected CurrentUser user;
+ protected MergeValidator mergeValidator;
private final OneOffRequestContext oneOffRequestContext;
@@ -91,6 +92,7 @@
this.gApi = gApi;
this.config = config;
this.oneOffRequestContext = oneOffRequestContext;
+ mergeValidator = new MergeValidator(gApi, config);
}
/**
@@ -215,21 +217,34 @@
for (Integer changeNumber : existingDownstream) {
ChangeInfo downstreamChange =
gApi.changes().id(changeNumber).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
- for (Map.Entry<String, LabelInfo> labelEntry : labels.entrySet()) {
- if (labelEntry.getValue().all.size() > 0) {
- OptionalInt maxVote =
- labelEntry
- .getValue()
- .all
- .stream()
- .filter(o -> o.value != null)
- .mapToInt(i -> i.value)
- .max();
+ if (shouldReceivePropagatedVote(downstreamChange)) {
+ for (Map.Entry<String, LabelInfo> labelEntry : labels.entrySet()) {
+ if (labelEntry.getValue().all.size() > 0) {
+ OptionalInt maxVote =
+ labelEntry
+ .getValue()
+ .all
+ .stream()
+ .filter(o -> o.value != null)
+ .mapToInt(i -> i.value)
+ .max();
- if (maxVote.isPresent()) {
- updateVote(downstreamChange, labelEntry.getKey(), (short) maxVote.getAsInt());
+ if (maxVote.isPresent()) {
+ updateVote(downstreamChange, labelEntry.getKey(), (short) maxVote.getAsInt());
+ }
}
}
+ } else {
+ try {
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.message(
+ "Downstream change is missing. Vote will not be propagated to this change "
+ + "until all merge conflicts are resolved and added to the topic.");
+ reviewInput.notify = NotifyHandling.NONE;
+ gApi.changes().id(downstreamChange._number).revision(CURRENT).review(reviewInput);
+ } catch (RestApiException e) {
+ log.error("Failed to comment on change for automerger plugin.", e);
+ }
}
}
} catch (RestApiException | InvalidQueryParameterException e) {
@@ -333,16 +348,10 @@
reviewInput.labels = labels;
// if this fails, i.e. -2 is restricted, catch it and still post message without a vote.
try {
- gApi.changes()
- .id(mdsMergeInput.changeNumber)
- .revision(CURRENT)
- .review(reviewInput);
+ gApi.changes().id(mdsMergeInput.changeNumber).revision(CURRENT).review(reviewInput);
} catch (AuthException e) {
reviewInput.labels = null;
- gApi.changes()
- .id(mdsMergeInput.changeNumber)
- .revision(CURRENT)
- .review(reviewInput);
+ gApi.changes().id(mdsMergeInput.changeNumber).revision(CURRENT).review(reviewInput);
}
}
}
@@ -578,6 +587,21 @@
return null;
}
+ private boolean shouldReceivePropagatedVote(ChangeInfo change)
+ throws RestApiException, IOException, ConfigInvalidException, InvalidQueryParameterException {
+ boolean shouldPropagate = true;
+ // Check change is created by contextUser if contextUser is set
+ if (config.contextUserIdIsSet()) {
+ Integer accountId = change.revisions.get(change.currentRevision).uploader._accountId;
+
+ shouldPropagate = accountId == config.getContextUserId().get();
+ }
+ // Check that the change does not have any downstream conflicts
+ shouldPropagate =
+ shouldPropagate && mergeValidator.getMissingDownstreamMerges(change).isEmpty();
+ return shouldPropagate;
+ }
+
private void automergeChanges(ChangeInfo change, RevisionInfo revisionInfo)
throws RestApiException, IOException, ConfigInvalidException, InvalidQueryParameterException,
OrmException {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java b/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java
index 1a041b7..62e0019 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java
@@ -245,6 +245,18 @@
assertThat(configLoader.getContextUserId()).isEqualTo(user.get().getAccountId());
}
+ @Test
+ public void contextUserIdIsSetTest() throws Exception {
+ defaultSetup("context_user.config");
+ assertThat(configLoader.contextUserIdIsSet()).isEqualTo(true);
+ }
+
+ @Test
+ public void contextUserIdIsSetTest_noContextUser() throws Exception {
+ defaultSetup("automerger.config");
+ assertThat(configLoader.contextUserIdIsSet()).isEqualTo(false);
+ }
+
private void setupTestRepo(
String resourceName, Project.NameKey projectNameKey, String branchName, String filename)
throws Exception {
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 552683d..89bbad0 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
@@ -912,6 +912,81 @@
assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(1);
}
+ @Test
+ public void testContextUser_conflict() throws Exception {
+ Project.NameKey manifestNameKey = defaultSetup();
+ // Create initial change
+ PushOneCommit.Result initialResult = createChange("subject", "filename", "echo Hello");
+ // Project name is scoped by test, so we need to get it from our initial change
+ Project.NameKey projectNameKey = initialResult.getChange().project();
+ String projectName = projectNameKey.get();
+ createBranch(new Branch.NameKey(projectName, "ds_one"));
+ createBranch(new Branch.NameKey(projectName, "ds_two"));
+ initialResult.assertOkStatus();
+ merge(initialResult);
+ // Reset to create a sibling
+ ObjectId initial = repo().exactRef("HEAD").getLeaf().getObjectId();
+ testRepo.reset(initial);
+ // Set up a merge conflict between master and ds_one
+ PushOneCommit.Result ds2Result =
+ createChange(
+ testRepo, "ds_two", "subject", "filename", "echo \"Hello asdfsd World\"", "randtopic");
+ ds2Result.assertOkStatus();
+ merge(ds2Result);
+ // Reset to allow our merge conflict to come
+ testRepo.reset(initial);
+
+ // Create normalUserGroup, containing current user, and contextUserGroup, containing contextUser
+ String normalUserGroup = createGroup("normalUserGroup");
+ gApi.groups().id(normalUserGroup).addMembers(user.get().getAccountId().toString());
+ AccountApi contextUserApi = gApi.accounts().create("aContextUser");
+ String contextUserGroup = createGroup("contextUserGroup");
+ gApi.groups().id(contextUserGroup).addMembers(contextUserApi.get().name);
+
+ // Grant +2 to context user, since it doesn't have it by default
+ grantLabel(
+ "Code-Review",
+ -2,
+ 2,
+ projectNameKey,
+ "refs/heads/*",
+ false,
+ AccountGroup.UUID.parse(gApi.groups().id(contextUserGroup).get().id),
+ false);
+ pushContextUserConfig(manifestNameKey.get(), projectName, contextUserApi.get()._accountId);
+
+ // After we upload our config, we upload a new patchset to create the downstreams
+ PushOneCommit.Result result =
+ createChange("subject", "filename", "echo 'Hello World!'", "sometopic");
+ result.assertOkStatus();
+ // Check that there are the correct number of changes in the topic
+ List<ChangeInfo> changesInTopic =
+ gApi.changes()
+ .query("topic:" + gApi.changes().id(result.getChangeId()).topic())
+ .withOptions(ListChangesOption.CURRENT_REVISION, ListChangesOption.CURRENT_COMMIT)
+ .get();
+ assertThat(changesInTopic).hasSize(2);
+
+ List<ChangeInfo> sortedChanges = sortedChanges(changesInTopic);
+
+ ChangeInfo dsOneChangeInfo = sortedChanges.get(0);
+ assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
+ ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
+ // This is -2 because there is a merge conflict downstream
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(-2);
+ assertThat(getVote(dsOneChange, "Code-Review").tag).isEqualTo("autogenerated:MergeConflict");
+
+ ChangeInfo masterChangeInfo = sortedChanges.get(1);
+ ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
+ assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
+ assertThat(masterChangeInfo.branch).isEqualTo("master");
+
+ // Test that +2 on master won't propagate to ds_one, as conflict is not resolved.
+ approve(masterChangeInfo.id);
+ assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(2);
+ assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(-2);
+ }
+
private Project.NameKey defaultSetup() throws Exception {
Project.NameKey manifestNameKey = createProject("platform/manifest");
setupTestRepo("default.xml", manifestNameKey, "master", "default.xml");
diff --git a/src/test/resources/com/googlesource/gerrit/plugins/automerger/context_user.config b/src/test/resources/com/googlesource/gerrit/plugins/automerger/context_user.config
index bb2d262..aeb71d9 100644
--- a/src/test/resources/com/googlesource/gerrit/plugins/automerger/context_user.config
+++ b/src/test/resources/com/googlesource/gerrit/plugins/automerger/context_user.config
@@ -14,4 +14,5 @@
manifestFile = default.xml
manifestProject = platform/manifest
ignoreProjects = platform/ignore/me
- contextUserId = 102304
\ No newline at end of file
+ contextUserId = 102304
+ disableMaxAutomergeVote = true
\ No newline at end of file
