Fix abandonIfMergeable when indexMergeable is disabled
When changeCleanup.abandonIfMergeable is set to false, the auto abandon
implementation adds "-is:mergeable" to the query that is used to find
changes to be abandoned. When index.change.indexMergeable is disabled,
this causes the query to fail and no changes get auto abandoned.
Fix the ChangeCleanupConfig to disregard abandonIfMergeable when the
indexMergeable is disabled, and log a warning message that the setting
is ineffective.
Extend the documentation of changeCleanup.abandonIfMergeable to mention
this case.
Add test coverage.
Change-Id: Iafe1ef8637deed30b41c48a9f64f199793992870
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 0ad6905..d126c96 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1391,9 +1391,15 @@
[[changeCleanup.abandonIfMergeable]]changeCleanup.abandonIfMergeable::
+
-Whether changes which are mergeable should be auto-abandoned.
+Whether changes which are mergeable should be auto-abandoned. When set
+to `false`, `-is:mergeable` is appended to the query used to find
+the changes to auto-abandon.
+
-By default `true`.
+By default `true`, meaning mergeable changes are auto-abandoned.
++
+If link:#index.change.indexMergeable[`index.change.indexMergeable`]
+is disabled, setting this option to `false` has no effect and it
+behaves as though it were set to `true`.
[[changeCleanup.cleanupAccountPatchReview]]changeCleanup.cleanupAccountPatchReview::
+
diff --git a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java
index 4d41ed7..afd159c 100644
--- a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java
+++ b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.config;
import com.google.common.base.Strings;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.server.config.ScheduleConfig.Schedule;
import com.google.inject.Inject;
@@ -25,6 +26,8 @@
@Singleton
public class ChangeCleanupConfig {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private static String SECTION = "changeCleanup";
private static String KEY_ABANDON_AFTER = "abandonAfter";
private static String KEY_ABANDON_IF_MERGEABLE = "abandonIfMergeable";
@@ -48,12 +51,26 @@
this.urlFormatter = urlFormatter;
schedule = ScheduleConfig.createSchedule(cfg, SECTION);
abandonAfter = readAbandonAfter(cfg);
- abandonIfMergeable = cfg.getBoolean(SECTION, null, KEY_ABANDON_IF_MERGEABLE, true);
+ boolean indexMergeable = cfg.getBoolean("index", "change", "indexMergeable", true);
+ if (!indexMergeable) {
+ if (!readAbandonIfMergeable(cfg)) {
+ logger.atWarning().log(
+ "index.change.indexMergeable is disabled; %s.%s=false will be ineffective",
+ SECTION, KEY_ABANDON_IF_MERGEABLE);
+ }
+ abandonIfMergeable = true;
+ } else {
+ abandonIfMergeable = readAbandonIfMergeable(cfg);
+ }
cleanupAccountPatchReview =
cfg.getBoolean(SECTION, null, KEY_CLEANUP_ACCOUNT_PATCH_REVIEW, false);
abandonMessage = readAbandonMessage(cfg);
}
+ private boolean readAbandonIfMergeable(Config cfg) {
+ return cfg.getBoolean(SECTION, null, KEY_ABANDON_IF_MERGEABLE, true);
+ }
+
private long readAbandonAfter(Config cfg) {
long abandonAfter =
ConfigUtil.getTimeUnit(cfg, SECTION, null, KEY_ABANDON_AFTER, 0, TimeUnit.MILLISECONDS);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java b/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java
index 0090ed1..abfc23d 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java
@@ -21,6 +21,7 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.util.concurrent.TimeUnit.HOURS;
import static java.util.stream.Collectors.toList;
+import static org.eclipse.jgit.lib.Constants.HEAD;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
@@ -32,9 +33,11 @@
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.AbandonUtil;
@@ -45,6 +48,7 @@
import java.util.List;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
public class AbandonIT extends AbstractDaemonTest {
@@ -136,6 +140,93 @@
}
@Test
+ @UseClockStep
+ @GerritConfig(name = "changeCleanup.abandonAfter", value = "1w")
+ @GerritConfig(name = "changeCleanup.abandonIfMergeable", value = "false")
+ @GerritConfig(name = "index.change.indexMergeable", value = "true")
+ public void notAbandonedIfMergeableWhenMergeableOperatorIsEnabled() throws Exception {
+ ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
+
+ // create 2 changes
+ int id1 = createChange().getChange().getId().get();
+ int id2 = createChange().getChange().getId().get();
+
+ // create 2 changes that conflict with each other
+ testRepo.reset(initial);
+ int id3 = createChange("change 3", "file.txt", "content").getChange().getId().get();
+ testRepo.reset(initial);
+ int id4 = createChange("change 4", "file.txt", "other content").getChange().getId().get();
+
+ // make all 4 previously created changes older than 1 week
+ TestTimeUtil.incrementClock(7 * 24, HOURS);
+
+ // create 1 new change that will not be abandoned because it is not older than 1 week
+ testRepo.reset(initial);
+ ChangeData cd = createChange().getChange();
+ int id5 = cd.getId().get();
+
+ assertThat(toChangeNumbers(query("is:open"))).containsExactly(id1, id2, id3, id4, id5);
+ assertThat(query("is:abandoned")).isEmpty();
+
+ // submit one of the conflicting changes
+ gApi.changes().id(id3).current().review(ReviewInput.approve());
+ gApi.changes().id(id3).current().submit();
+ assertThat(toChangeNumbers(query("is:merged"))).containsExactly(id3);
+ assertThat(toChangeNumbers(query("-is:mergeable"))).containsExactly(id4);
+
+ abandonUtil.abandonInactiveOpenChanges(batchUpdateFactory);
+ assertThat(toChangeNumbers(query("is:open"))).containsExactly(id5, id2, id1);
+ assertThat(toChangeNumbers(query("is:abandoned"))).containsExactly(id4);
+ }
+
+ @Test
+ @UseClockStep
+ @GerritConfig(name = "changeCleanup.abandonAfter", value = "1w")
+ @GerritConfig(name = "changeCleanup.abandonIfMergeable", value = "false")
+ @GerritConfig(name = "index.change.indexMergeable", value = "false")
+ /**
+ * When indexMergeable is disabled then the abandonIfMergeable option is ineffective and the auto
+ * abandon behaves as though it were set to its default value (true).
+ */
+ public void abandonedIfMergeableWhenMergeableOperatorIsDisabled() throws Exception {
+ ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
+
+ // create 2 changes
+ int id1 = createChange().getChange().getId().get();
+ int id2 = createChange().getChange().getId().get();
+
+ // create 2 changes that conflict with each other
+ testRepo.reset(initial);
+ int id3 = createChange("change 3", "file.txt", "content").getChange().getId().get();
+ testRepo.reset(initial);
+ int id4 = createChange("change 4", "file.txt", "other content").getChange().getId().get();
+
+ // make all 4 previously created changes older than 1 week
+ TestTimeUtil.incrementClock(7 * 24, HOURS);
+
+ // create 1 new change that will not be abandoned because it is not older than 1 week
+ testRepo.reset(initial);
+ ChangeData cd = createChange().getChange();
+ int id5 = cd.getId().get();
+
+ assertThat(toChangeNumbers(query("is:open"))).containsExactly(id1, id2, id3, id4, id5);
+ assertThat(query("is:abandoned")).isEmpty();
+
+ // submit one of the conflicting changes
+ gApi.changes().id(id3).current().review(ReviewInput.approve());
+ gApi.changes().id(id3).current().submit();
+ assertThat(toChangeNumbers(query("is:merged"))).containsExactly(id3);
+
+ BadRequestException thrown =
+ assertThrows(BadRequestException.class, () -> query("-is:mergeable"));
+ assertThat(thrown).hasMessageThat().contains("operator is not supported");
+
+ abandonUtil.abandonInactiveOpenChanges(batchUpdateFactory);
+ assertThat(toChangeNumbers(query("is:open"))).containsExactly(id5);
+ assertThat(toChangeNumbers(query("is:abandoned"))).containsExactly(id4, id2, id1);
+ }
+
+ @Test
public void changeCleanupConfigDefaultAbandonMessage() throws Exception {
assertThat(cleanupConfig.getAbandonMessage())
.startsWith(
