Merge changes I3aa95f4c,Ic6055faa,I80430ef0 into stable-3.13 * changes: Ignore false positive errorprone error UnusedMethod Cleanup the `EmptyCatch` error in tests Fix errorprone error UnnecessaryAssignment
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 84a3339..a816bbe 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt
@@ -1904,17 +1904,24 @@ This section allows to configure change cleanups and schedules them to run periodically. ++ +Cleanup only runs if link:#changeCleanup.startTime[startTime] +and link:#changeCleanup.interval[interval] are configured and at least one of +link:#changeCleanup.abandonAfter[abandonAfter] or +link:#changeCleanup.query[query] is set. ++ +[WARNING] Auto-Abandoning changes may confuse/annoy users. When +enabling this, make sure to choose a reasonably large grace period and +inform users in advance. [[changeCleanup.abandonAfter]]changeCleanup.abandonAfter:: + Period of inactivity after which open changes should be abandoned automatically. + -By default `0`, never abandon open changes. -+ -[WARNING] Auto-Abandoning changes may confuse/annoy users. When -enabling this, make sure to choose a reasonably large grace period and -inform users in advance. +By default `0`, meaning no age constraint is applied. An age constraint can +alternatively be expressed via link:#changeCleanup.query[changeCleanup.query] +(e.g. `age:4w`). + The following suffixes are supported to define the time unit: + @@ -1953,6 +1960,30 @@ ${URL}Documentation/user-change-cleanup.html#auto-abandon\n\n If this change is still wanted it should be restored.". +[[changeCleanup.query]]changeCleanup.query:: ++ +Additional query predicates appended to the query used for selecting changes to +auto-abandon. The configured query is combined with the built-in predicates +that always restrict cleanup to open changes (`status:new`) and may additionally +include predicates derived from other cleanup options such as +link:#changeCleanup.abandonAfter[abandonAfter] and +link:#changeCleanup.abandonIfMergeable[abandonIfMergeable]. + +Any valid link:user-search.html[change search] expression is accepted. This +allows limiting the batch size, excluding specific changes, or combining both: ++ +---- + [changeCleanup] + query = age:4w limit:100 -project:some/repo -hashtag:keep-alive +---- ++ +Setting this option enables cleanup even if +link:#changeCleanup.abandonAfter[changeCleanup.abandonAfter] is not set. In +that case the query is solely responsible for determining which changes are +abandoned, so it should include an appropriate age constraint, e.g. `age:4w`. ++ +By default unset, meaning no extra constraints are applied. + [[changeCleanup.startTime]]changeCleanup.startTime:: + The link:#schedule-configuration-startTime[start time] for running
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 4598b88..b8b011d 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -5374,6 +5374,47 @@ } ---- +If the change has already been merged, the endpoint still returns +`200 OK`. In that case the change is considered mergeable because its +revision is already integrated into the destination branch. The response +also sets `commit_merged` and `content_merged`, so clients can tell this +case apart from an open change that could still be submitted. + +.Response +---- + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + { + submit_type: "MERGE_IF_NECESSARY", + strategy: "recursive", + mergeable: true, + commit_merged: true, + content_merged: true + } +---- + +If the change is abandoned, then by definition the change is not mergeable. +The endpoint returns `200 OK` with: + +.Response +---- + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + { + submit_type: "MERGE_IF_NECESSARY", + strategy: "recursive", + mergeable: false, + commit_merged: false, + content_merged: false + } +---- + If the `other-branches` parameter is specified, the mergeability will also be checked for all other branches which are listed in the link:config-project-config.html#branchOrder-section[branchOrder] section in the @@ -8715,7 +8756,7 @@ The strategy of the merge, can be `recursive`, `resolve`, `simple-two-way-in-core`, `ours` or `theirs`. |`mergeable` || -`true` if this change is cleanly mergeable, `false` otherwise +`true` if this change is cleanly mergeable or already merged, `false` otherwise |`commit_merged` |optional| `true` if this change is already merged, `false` otherwise |`content_merged`|optional|
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt index dbbf0e32..8c4a96e 100644 --- a/Documentation/rest-api-config.txt +++ b/Documentation/rest-api-config.txt
@@ -1877,8 +1877,9 @@ { "after": "3 months", "if_mergeable": true, - "message": "Abandoning stale changes." - } + "message": "Abandoning stale changes.", + "query": "age:4w limit:100 -project:some/repo -hashtag:keep-alive" +} ---- .Response @@ -2884,6 +2885,9 @@ that are mergeable |`message`|optional|Message to post to changes abandoned by the cleanup +|`query`|optional|Additional query predicates appended to the base cleanup +query. Can be used to limit the batch size, exclude changes, or both, e.g. +`age:4w limit:100 -project:some/repo -hashtag:keep-alive`. By default unset. |============================= [[migrate-passwords-to-tokens-input]]
diff --git a/java/com/google/gerrit/auth/ldap/Helper.java b/java/com/google/gerrit/auth/ldap/Helper.java index c9720cc..53e8ac5 100644 --- a/java/com/google/gerrit/auth/ldap/Helper.java +++ b/java/com/google/gerrit/auth/ldap/Helper.java
@@ -353,7 +353,9 @@ try { while (groups.hasMore()) { final String nextDN = (String) groups.next(); - recursivelyExpandGroups(groupDNs, schema, ctx, nextDN); + try (Timer0.Context ignored = groupExpansionLatencyTimer.start()) { + recursivelyExpandGroups(groupDNs, schema, ctx, nextDN); + } } } catch (PartialResultException e) { // Ignored
diff --git a/java/com/google/gerrit/server/change/AbandonUtil.java b/java/com/google/gerrit/server/change/AbandonUtil.java index 1a2c63d..5f0f861 100644 --- a/java/com/google/gerrit/server/change/AbandonUtil.java +++ b/java/com/google/gerrit/server/change/AbandonUtil.java
@@ -14,6 +14,7 @@ package com.google.gerrit.server.change; +import com.google.common.base.Strings; import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; @@ -60,15 +61,22 @@ BatchUpdate.Factory updateFactory, long abandonAfterMillis, boolean abandonIfMergeable, - String message) { - if (abandonAfterMillis <= 0) { + String message, + String additionalQuery) { + if (abandonAfterMillis <= 0 && Strings.isNullOrEmpty(additionalQuery)) { return; } try { - String query = "status:new age:" + TimeUnit.MILLISECONDS.toMinutes(abandonAfterMillis) + "m"; + String query = "status:new"; + if (abandonAfterMillis > 0) { + query += " age:" + TimeUnit.MILLISECONDS.toMinutes(abandonAfterMillis) + "m"; + } if (!abandonIfMergeable) { query += " -is:mergeable"; } + if (!Strings.isNullOrEmpty(additionalQuery)) { + query += " (%s)".formatted(additionalQuery); + } ImmutableList<ChangeData> changesToAbandon = queryProvider
diff --git a/java/com/google/gerrit/server/change/ChangeCleanupRunner.java b/java/com/google/gerrit/server/change/ChangeCleanupRunner.java index e3a8b06..67e6a0e 100644 --- a/java/com/google/gerrit/server/change/ChangeCleanupRunner.java +++ b/java/com/google/gerrit/server/change/ChangeCleanupRunner.java
@@ -46,7 +46,11 @@ public interface Factory { ChangeCleanupRunner create(); - ChangeCleanupRunner create(long abandonAfterMillis, boolean abandonIfMergeable, String message); + ChangeCleanupRunner create( + long abandonAfterMillis, + boolean abandonIfMergeable, + @Assisted("message") String message, + @Assisted("query") String query); } static class Lifecycle implements LifecycleListener { @@ -79,6 +83,7 @@ private final long abandonAfterMillis; private final boolean abandonIfMergeable; @Nullable private final String message; + private final String query; @AssistedInject ChangeCleanupRunner( @@ -88,7 +93,8 @@ LockManager lockManager, @Assisted long abandonAfterMillis, @Assisted boolean abandonIfMergeable, - @Assisted @Nullable String message) { + @Assisted("message") @Nullable String message, + @Assisted("query") String query) { this.oneOffRequestContext = oneOffRequestContext; this.abandonUtil = abandonUtil; this.retryHelper = retryHelper; @@ -96,6 +102,7 @@ this.abandonAfterMillis = abandonAfterMillis; this.abandonIfMergeable = abandonIfMergeable; this.message = message; + this.query = query; } @AssistedInject @@ -112,6 +119,7 @@ this.abandonAfterMillis = cfg.getAbandonAfter(); this.abandonIfMergeable = cfg.getAbandonIfMergeable(); this.message = cfg.getAbandonMessage(); + this.query = cfg.getQuery(); } @Override @@ -136,7 +144,7 @@ "abandonInactiveOpenChanges", updateFactory -> { abandonUtil.abandonInactiveOpenChanges( - updateFactory, abandonAfterMillis, abandonIfMergeable, message); + updateFactory, abandonAfterMillis, abandonIfMergeable, message, query); return null; }) .call();
diff --git a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java index cb4bff8..1d1298c 100644 --- a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java +++ b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java
@@ -34,6 +34,7 @@ private static final String KEY_ABANDON_IF_MERGEABLE = "abandonIfMergeable"; private static final String KEY_ABANDON_MESSAGE = "abandonMessage"; private static final String KEY_CLEANUP_ACCOUNT_PATCH_REVIEW = "cleanupAccountPatchReview"; + private static final String KEY_QUERY = "query"; private static final String DEFAULT_ABANDON_MESSAGE = "Auto-Abandoned due to inactivity, see " + "${URL}\n" @@ -46,6 +47,7 @@ private final boolean abandonIfMergeable; private final boolean cleanupAccountPatchReview; private final String abandonMessage; + private final String query; @Inject ChangeCleanupConfig(@GerritServerConfig Config cfg, DynamicItem<UrlFormatter> urlFormatter) { @@ -66,6 +68,7 @@ cleanupAccountPatchReview = cfg.getBoolean(SECTION, null, KEY_CLEANUP_ACCOUNT_PATCH_REVIEW, false); abandonMessage = readAbandonMessage(cfg); + query = Strings.nullToEmpty(cfg.getString(SECTION, null, KEY_QUERY)); } private boolean readAbandonIfMergeable(Config cfg) { @@ -99,6 +102,10 @@ return cleanupAccountPatchReview; } + public String getQuery() { + return query; + } + public String getAbandonMessage() { String docUrl = urlFormatter.get().getDocUrl("user-change-cleanup.html", "auto-abandon").orElse("");
diff --git a/java/com/google/gerrit/server/git/WorkQueue.java b/java/com/google/gerrit/server/git/WorkQueue.java index ffbb4d4..1f87f1b 100644 --- a/java/com/google/gerrit/server/git/WorkQueue.java +++ b/java/com/google/gerrit/server/git/WorkQueue.java
@@ -60,6 +60,7 @@ import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -343,13 +344,16 @@ /** An isolated queue. */ private class Executor extends ScheduledThreadPoolExecutor { - private class ParkedTask implements Comparable<ParkedTask> { - public final CancellableCountDownLatch latch = new CancellableCountDownLatch(1); - public final Task<?> task; + private class ParkedTask implements Comparable<ParkedTask>, AutoCloseable { + private final CancellableCountDownLatch latch = new CancellableCountDownLatch(1); + private final Task<?> task; private final Long priority = priorityGenerator.getAndIncrement(); + private final AtomicBoolean isParked = new AtomicBoolean(true); public ParkedTask(Task<?> task) { this.task = task; + task.runningState.set(Task.State.PARKED); + incrementCorePoolSizeBy(1); } @Override @@ -364,12 +368,34 @@ * method. */ public void cancel() { + close(); latch.cancel(); } public boolean isEqualTo(Task<?> task) { return this.task.taskId == task.taskId; } + + public void await() { + try { + latch.await(); + } catch (InterruptedException e) { + logger.atSevere().withCause(e).log("Parked Task(%s) Interrupted", task); + parked.remove(this); + } + } + + public void unpark() { + close(); + latch.countDown(); + } + + @Override + public void close() { + if (isParked.compareAndSet(true, false)) { + incrementCorePoolSizeBy(-1); + } + } } private static class CancellableCountDownLatch extends CountDownLatch { @@ -660,17 +686,9 @@ public void waitUntilReadyToStart(Task<?> task) { if (!listeners.isEmpty() && !isReadyToStart(task)) { - ParkedTask parkedTask = new ParkedTask(task); - parked.offer(parkedTask); - task.runningState.set(Task.State.PARKED); - incrementCorePoolSizeBy(1); - try { - parkedTask.latch.await(); - } catch (InterruptedException e) { - logger.atSevere().withCause(e).log("Parked Task(%s) Interrupted", task); - parked.remove(parkedTask); - } finally { - incrementCorePoolSizeBy(-1); + try (ParkedTask parkedTask = new ParkedTask(task)) { + parked.offer(parkedTask); + parkedTask.await(); } } } @@ -732,7 +750,7 @@ parked.addAll(notReady); if (ready != null) { - ready.latch.countDown(); + ready.unpark(); } }
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java index fb02de6..8e21d51 100644 --- a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java +++ b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java
@@ -42,6 +42,7 @@ import com.google.gerrit.server.query.change.AndChangeSource; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeDataSource; +import com.google.gerrit.server.query.change.ChangeIndexPredicate; import com.google.gerrit.server.query.change.ChangeQueryBuilder; import com.google.gerrit.server.query.change.ChangeStatusPredicate; import com.google.gerrit.server.query.change.IsSubmittablePredicate; @@ -202,6 +203,11 @@ // should have already searched the predicate tree for limit predicates // and included that in their limit computation. return new LimitPredicate<>(ChangeQueryBuilder.FIELD_LIMIT, opts.limit()); + } else if (in instanceof OrPredicate && in.getChildCount() == 0) { + ++leafTerms.value; + // An empty OrPredicate will never match anything, but will scan all + // changes unless handled here. + return ChangeIndexPredicate.none(); } else if (!isRewritePossible(in)) { if (in instanceof IndexPredicate) { throw new QueryParseException("Unsupported index predicate: " + in.toString()); @@ -236,6 +242,11 @@ } } + if (in instanceof AndPredicate + && newChildren.stream().anyMatch(c -> c.equals(ChangeIndexPredicate.none()))) { + ++leafTerms.value; + return ChangeIndexPredicate.none(); + } if (isIndexed.cardinality() == n) { return in; // All children are indexed, leave as-is for parent. } else if (notIndexed.cardinality() == n) {
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicates.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicates.java index 488df59..369ddd2 100644 --- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicates.java +++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicates.java
@@ -90,6 +90,48 @@ } } + /** + * Label predicate that trusts the index result without post-filtering. + * + * <p>Used when the query has no group or count constraint; the index result is exact and + * re-verification is unnecessary. + */ + public static class IndexOnlyEqualsLabelPredicate extends ChangeIndexPredicate { + private final Matcher matcher; + + public IndexOnlyEqualsLabelPredicate( + LabelPredicate.Args args, String label, int expVal, @Nullable Account.Id account) { + super(ChangeField.LABEL_SPEC, ChangeField.formatLabel(label, expVal, account, null)); + this.matcher = new Matcher(args, label, expVal, account, null); + } + + @Override + public boolean match(ChangeData object) { + return matcher.match(object); + } + + @Override + public int getCost() { + return 1; + } + } + + /** + * Returns a label predicate that post-filters only when group membership or vote count must be + * verified at query time; otherwise trusts the index result directly. + */ + public static ChangeIndexPredicate indexPredicate( + LabelPredicate.Args args, + String label, + int expVal, + @Nullable Account.Id account, + @Nullable Integer count) { + if (args.group != null || count != null) { + return new IndexEqualsLabelPredicate(args, label, expVal, account, count); + } + return new IndexOnlyEqualsLabelPredicate(args, label, expVal, account); + } + private static class Matcher { protected final AccountResolver accountResolver; protected final ProjectCache projectCache; @@ -182,6 +224,9 @@ hasVote = true; if (match(cd, psa)) { matchingVotes += 1; + if (count == null) { + break; + } } } } @@ -296,13 +341,15 @@ } } - IdentifiedUser reviewer = userFactory.create(approver); - if (group != null && !reviewer.getEffectiveGroups().contains(group)) { - logger.atFine().log( - "vote %s on change %s doesn't match since the approver %s is not a member of the" - + " expected group %s", - psa, cd.change().getChangeId(), approver, group); - return false; + if (group != null) { + IdentifiedUser reviewer = userFactory.create(approver); + if (!reviewer.getEffectiveGroups().contains(group)) { + logger.atFine().log( + "vote %s on change %s doesn't match since the approver %s is not a member of the" + + " expected group %s", + psa, cd.change().getChangeId(), approver, group); + return false; + } } // Check the user has 'READ' permission.
diff --git a/java/com/google/gerrit/server/query/change/LabelPredicate.java b/java/com/google/gerrit/server/query/change/LabelPredicate.java index dc859a3..c838a70 100644 --- a/java/com/google/gerrit/server/query/change/LabelPredicate.java +++ b/java/com/google/gerrit/server/query/change/LabelPredicate.java
@@ -203,11 +203,11 @@ return new EqualsLabelPredicates.PostFilterEqualsLabelPredicate(args, label, expVal, count); } if (args.accounts == null || args.accounts.isEmpty()) { - return new EqualsLabelPredicates.IndexEqualsLabelPredicate(args, label, expVal, count); + return EqualsLabelPredicates.indexPredicate(args, label, expVal, null, count); } List<Predicate<ChangeData>> r = new ArrayList<>(); for (Account.Id a : args.accounts) { - r.add(new EqualsLabelPredicates.IndexEqualsLabelPredicate(args, label, expVal, a, count)); + r.add(EqualsLabelPredicates.indexPredicate(args, label, expVal, a, count)); } return or(r); }
diff --git a/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java b/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java index 6321ccb..12fb38d 100644 --- a/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java +++ b/java/com/google/gerrit/server/query/change/MagicLabelPredicates.java
@@ -87,8 +87,7 @@ @Override protected Predicate<ChangeData> numericPredicate(String label, short value) { - return new EqualsLabelPredicates.IndexEqualsLabelPredicate( - args, label, value, account, count); + return EqualsLabelPredicates.indexPredicate(args, label, value, account, count); } }
diff --git a/java/com/google/gerrit/server/restapi/change/Mergeable.java b/java/com/google/gerrit/server/restapi/change/Mergeable.java index 8d6f03e..71dcbba 100644 --- a/java/com/google/gerrit/server/restapi/change/Mergeable.java +++ b/java/com/google/gerrit/server/restapi/change/Mergeable.java
@@ -28,7 +28,6 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestReadView; -import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.change.MergeabilityCache; import com.google.gerrit.server.change.MergeabilityComputationBehavior; import com.google.gerrit.server.change.RevisionResource; @@ -102,10 +101,9 @@ PatchSet ps = resource.getPatchSet(); MergeableInfo result = new MergeableInfo(); - if (!change.isNew()) { - throw new ResourceConflictException("change is " + ChangeUtil.status(change)); - } else if (!ps.id().equals(change.currentPatchSetId())) { - // Only the current revision is mergeable. Others always fail. + if (!ps.id().equals(change.currentPatchSetId()) || change.isAbandoned()) { + // Only the current revision of non-abandoned changes is mergeable. + // Others always fail. return Response.ok(result); } @@ -119,6 +117,14 @@ projectCache.get(change.getProject()).orElseThrow(illegalState(change.getProject())); String strategy = mergeUtilFactory.create(projectState).mergeStrategyName(); result.strategy = strategy; + + if (change.isMerged()) { + result.mergeable = true; + result.commitMerged = true; + result.contentMerged = true; + return Response.ok(result); + } + result.mergeable = isMergable(git, change, commit, ref, result.submitType, strategy); if (otherBranches) {
diff --git a/java/com/google/gerrit/server/restapi/config/CleanupChanges.java b/java/com/google/gerrit/server/restapi/config/CleanupChanges.java index 9ae3637..54aa68c 100644 --- a/java/com/google/gerrit/server/restapi/config/CleanupChanges.java +++ b/java/com/google/gerrit/server/restapi/config/CleanupChanges.java
@@ -39,6 +39,7 @@ String after; boolean ifMergeable; String message; + String query; } @Inject @@ -59,7 +60,8 @@ runnerFactory.create( ConfigUtil.getTimeUnit(input.after, 0, TimeUnit.MILLISECONDS), input.ifMergeable, - input.message); + input.message, + input.query); @SuppressWarnings("unused") Future<?> possiblyIgnoredError = workQueue.getDefaultQueue().submit(() -> runner.run()); return Response.accepted("Change cleaner task added to work queue.");
diff --git a/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java b/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java index 18969ad..13a16ba 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java
@@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.UseClockStep; @@ -33,6 +34,7 @@ import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.entities.Permission; import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.api.changes.HashtagsInput; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.common.ChangeInfo; @@ -319,6 +321,50 @@ assertThat(thrown).hasMessageThat().contains("restore not permitted"); } + @Test + @UseClockStep + @GerritConfig(name = "changeCleanup.abandonAfter", value = "1w") + @GerritConfig(name = "changeCleanup.query", value = "-(hashtag:keep-alive)") + public void abandonQueryExcludesChanges() throws Exception { + int id1 = createChange().getChange().getId().get(); + int id2 = createChange().getChange().getId().get(); + + // Mark id2 with the hashtag that should be excluded from cleanup + HashtagsInput hashtags = new HashtagsInput(); + hashtags.add = Sets.newHashSet("keep-alive"); + gApi.changes().id(project.get(), id2).setHashtags(hashtags); + + TestTimeUtil.incrementClock(7 * 24, HOURS); + + assertThat(toChangeNumbers(query("is:open"))).containsExactly(id1, id2); + assertThat(query("is:abandoned")).isEmpty(); + + cleanupRunner.create().run(); + + assertThat(toChangeNumbers(query("is:open"))).containsExactly(id2); + assertThat(toChangeNumbers(query("is:abandoned"))).containsExactly(id1); + } + + @Test + @UseClockStep + @GerritConfig(name = "changeCleanup.abandonAfter", value = "1w") + @GerritConfig(name = "changeCleanup.query", value = "limit:2") + public void abandonQueryLimitsChanges() throws Exception { + int id1 = createChange().getChange().getId().get(); + int id2 = createChange().getChange().getId().get(); + int id3 = createChange().getChange().getId().get(); + + TestTimeUtil.incrementClock(7 * 24, HOURS); + + assertThat(toChangeNumbers(query("is:open"))).containsExactly(id1, id2, id3); + assertThat(query("is:abandoned")).isEmpty(); + + cleanupRunner.create().run(); + + assertThat(toChangeNumbers(query("is:abandoned"))).hasSize(2); + assertThat(toChangeNumbers(query("is:open"))).hasSize(1); + } + private List<Integer> toChangeNumbers(List<ChangeInfo> changes) { return changes.stream().map(i -> i._number).collect(toList()); }
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java index eebcbb8..6f43935 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -1795,6 +1795,28 @@ } @Test + public void mergeableForMergedChange() throws Exception { + PushOneCommit.Result r = createChange(); + merge(r); + + MergeableInfo mergeableInfo = gApi.changes().id(r.getChangeId()).current().mergeable(); + assertThat(mergeableInfo.mergeable).isTrue(); + assertThat(mergeableInfo.commitMerged).isTrue(); + assertThat(mergeableInfo.contentMerged).isTrue(); + } + + @Test + public void mergeableForAbandonedChange() throws Exception { + PushOneCommit.Result r = createChange(); + gApi.changes().id(r.getChangeId()).abandon(); + + MergeableInfo mergeableInfo = gApi.changes().id(r.getChangeId()).current().mergeable(); + assertThat(mergeableInfo.mergeable).isFalse(); + assertThat(mergeableInfo.commitMerged).isFalse(); + assertThat(mergeableInfo.contentMerged).isFalse(); + } + + @Test public void mergeableOtherBranches() throws Exception { String head = getHead(repo(), HEAD).name(); createBranchWithRevision(BranchNameKey.create(project, "mergeable-other-branch"), head);
diff --git a/javatests/com/google/gerrit/acceptance/server/util/TaskParkerIT.java b/javatests/com/google/gerrit/acceptance/server/util/TaskParkerIT.java index 3b82ebe..df63b69 100644 --- a/javatests/com/google/gerrit/acceptance/server/util/TaskParkerIT.java +++ b/javatests/com/google/gerrit/acceptance/server/util/TaskParkerIT.java
@@ -463,12 +463,34 @@ assertStateIsEventually(forwarder.task, State.PARKED); // interrupt the thread with parked task - for (Thread t : Thread.getAllStackTraces().keySet()) { - if (t.getName().contains(taskName)) { - t.interrupt(); - break; - } - } + interruptThreadContaining(taskName); + + assertCorePoolSizeIsEventually(1); + } + + @Test + public void interruptingTaskWhileUnparkingDoesNotDoubleDecrementCorePoolSize() + throws InterruptedException { + String taskName = "to-be-unparked"; + LatchedRunnable parkedRunnable = new LatchedRunnable(taskName); + LatchedRunnable blockerRunnable = new LatchedRunnable("blocker"); + assertCorePoolSizeIs(1); + + // park parkedRunnable + executor.execute(parkedRunnable); + parker.isReadyToStart.assertCalledEventuallyThenComplete(false); + assertCorePoolSizeIsEventually(2); + assertStateIsEventually(forwarder.task, State.PARKED); + + // start blockerRunnable and unblock it to trigger updateParked() from onStop() + executor.execute(blockerRunnable); + blockerRunnable.run.assertCalledEventuallyThenComplete(null); + + // Wait for updateParked() to poll parkedRunnable and call isReadyToStart(), then interrupt + // the parked thread before releasing isReadyToStart() as ready. + parker.isReadyToStart.assertCalledEventually(); + interruptThreadContaining(taskName); + parker.isReadyToStart.complete(true); assertCorePoolSizeIsEventually(1); } @@ -502,6 +524,15 @@ TaskListenerIT.assertTaskCountIsEventually(workQueue, count); } + private void interruptThreadContaining(String taskName) { + for (Thread t : Thread.getAllStackTraces().keySet()) { + if (t.getName().contains(taskName)) { + t.interrupt(); + break; + } + } + } + private void assertCorePoolSizeIs(int count) { assertThat(count).isEqualTo(((ScheduledThreadPoolExecutor) executor).getCorePoolSize()); }
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java index c65e552..8e278ae 100644 --- a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java +++ b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java
@@ -24,6 +24,7 @@ import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static org.junit.Assert.assertEquals; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.gerrit.entities.Change; import com.google.gerrit.index.IndexConfig; @@ -36,6 +37,7 @@ import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.server.query.change.AndChangeSource; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.ChangeIndexPredicate; import com.google.gerrit.server.query.change.ChangeQueryBuilder; import com.google.gerrit.server.query.change.ChangeStatusPredicate; import com.google.gerrit.server.query.change.OrSource; @@ -198,6 +200,19 @@ } @Test + public void emptyOrPredicate() throws Exception { + Predicate<ChangeData> in = Predicate.or(ImmutableList.of()); + assertThat(rewrite(in)).isEqualTo(query(ChangeIndexPredicate.none())); + } + + @Test + public void emptyOrPredicateInsideAnd() throws Exception { + Predicate<ChangeData> fileA = parse("file:a"); + Predicate<ChangeData> in = Predicate.and(fileA, Predicate.or(ImmutableList.of())); + assertThat(rewrite(in)).isEqualTo(query(ChangeIndexPredicate.none())); + } + + @Test public void indexAndNonIndexPredicates() throws Exception { Predicate<ChangeData> in = parse("status:new bar:p file:a"); Predicate<ChangeData> out = rewrite(in);
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 4a492d0..5220137 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -104,6 +104,7 @@ import com.google.gerrit.index.PaginationType; import com.google.gerrit.index.Schema; import com.google.gerrit.index.query.IndexPredicate; +import com.google.gerrit.index.query.PostFilterPredicate; import com.google.gerrit.index.query.Predicate; import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.lifecycle.LifecycleManager; @@ -163,6 +164,7 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.junit.TestRepository; @@ -863,6 +865,18 @@ } @Test + public void byOwnerIn_emptyGroupDoesNotRunPostFilterMatches() throws Exception { + Project.NameKey project = Project.nameKey("repo"); + repo = createAndOpenProject(project); + insert(project, newChange(repo), userId); + + String emptyGroup = createGroup("empty-owner-group", "Administrators"); + Predicate<ChangeData> ownerIn = queryBuilderProvider.get().parse("ownerin:" + emptyGroup); + + assertNoPostFilterMatches(ownerIn); + } + + @Test public void byUploaderIn() throws Exception { assume().that(getSchema().hasField(ChangeField.UPLOADER_SPEC)).isTrue(); Project.NameKey project = Project.nameKey("repo"); @@ -911,6 +925,19 @@ } @Test + public void byUploaderIn_emptyGroupDoesNotRunPostFilterMatches() throws Exception { + assume().that(getSchema().hasField(ChangeField.UPLOADER_SPEC)).isTrue(); + Project.NameKey project = Project.nameKey("repo"); + repo = createAndOpenProject(project); + insert(project, newChange(repo), userId); + + String emptyGroup = createGroup("empty-uploader-group", "Administrators"); + Predicate<ChangeData> uploaderIn = queryBuilderProvider.get().parse("uploaderin:" + emptyGroup); + + assertNoPostFilterMatches(uploaderIn); + } + + @Test public void byProject() throws Exception { Project.NameKey project1 = Project.nameKey("repo1"); repo = createAndOpenProject(project1); @@ -4821,6 +4848,26 @@ .inOrder(); } + private void assertNoPostFilterMatches(Predicate<ChangeData> predicate) throws Exception { + AtomicInteger postFilterMatches = new AtomicInteger(); + Predicate<ChangeData> countingPostFilter = + new PostFilterPredicate<ChangeData>("post_filter_probe", "post_filter_probe") { + @Override + public boolean match(ChangeData object) { + postFilterMatches.incrementAndGet(); + return true; + } + + @Override + public int getCost() { + return 1; + } + }; + + assertQuery(Predicate.and(predicate, countingPostFilter)); + assertThat(postFilterMatches.get()).isEqualTo(0); + } + private String format(String query, Iterable<Change> actualChanges, Change... expectedChanges) { return "query '" + query
diff --git a/resources/com/google/gerrit/pgm/init/gerrit.sh b/resources/com/google/gerrit/pgm/init/gerrit.sh index 8c958c9..9eaadc5 100755 --- a/resources/com/google/gerrit/pgm/init/gerrit.sh +++ b/resources/com/google/gerrit/pgm/init/gerrit.sh
@@ -51,7 +51,7 @@ usage() { me=`basename "$0"` - echo >&2 "Usage: $me {start|stop|restart|check|status|run|supervise|threads} [-d site] [--debug [--debug_port|--debug_address ...] [--suspend]] [--count=n]" + echo >&2 "Usage: $me {start|stop|restart|check|status|run|supervise|threads} [-d site] [--debug [--debug-port|--debug-address ...] [--suspend]] [--count=n]" exit 1 }