Merge branch 'stable-3.7'
* stable-3.7: (35 commits)
Set version to 3.7.0-rc1
Set version to 3.6.2
Add an option to periodically warm the project_list cache
Improve test coverage for internal change query pagination
prolog_rules cache: only use memoryLimit from cache.projects
Revert "Ensure that quoted messages show up in the reply comment"
Set version to 3.5.4-SNAPSHOT
Set version to 3.5.3
Don't update the last updated timestamp when running copy-approvals
CreateRefControl: Check permissions on source refs
Set version to 3.4.7-SNAPSHOT
Set version to 3.4.6
Fix javadoc summary for Google Java Style
Don't depend on predicate order as they get sorted based on cost
Fix DefaultMemoryCacheFactory to correctly set refreshAfterWrite
Update git submodules
Fix Bazel build on Apple M2 ARM64 chip
Shortcut CreateRefControl#checkCreateCommit only for push processing
Paginate internal change index queries
Fix index pagination to set 'more_changes' correctly
...
Release-Notes: skip
Change-Id: I7bd89a8be78f0c00f7477c6b84b137c80944d42e
diff --git a/.bazelversion b/.bazelversion
index 0062ac9..c7cb131 100644
--- a/.bazelversion
+++ b/.bazelversion
@@ -1 +1 @@
-5.0.0
+5.3.1
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index ca72f8b..33c5bbd 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -533,6 +533,24 @@
Certain operations in Gerrit can be validated by plugins by
implementing the corresponding link:config-validation.html[listeners].
+[[taskListeners]]
+== WorkQueue.TaskListeners
+
+It is possible for plugins to listen to
+`com.google.gerrit.server.git.WorkQueue$Task`s directly before they run, and
+directly after they complete. This may be used to delay task executions based
+on custom criteria by blocking, likely on a lock or semaphore, inside
+onStart(), and a lock/semaphore release in onStop(). Plugins may listen to
+tasks by implementing a `com.google.gerrit.server.git.WorkQueue$TaskListener`
+and registering the new listener like this:
+
+[source,java]
+----
+bind(TaskListener.class)
+ .annotatedWith(Exports.named("MyListener"))
+ .to(MyListener.class);
+----
+
[[change-message-modifier]]
== Change Message Modifier
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index ab7fb3b..b160f59 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -7471,8 +7471,13 @@
differ by one from details provided in <<diff-info,DiffInfo>>.
|`size_delta` ||
Number of bytes by which the file size increased/decreased.
-|`size` ||
-File size in bytes.
+|`size` || File size in bytes.
+|`old_mode` |optional|File mode in octal (e.g. 100644) at the old commit.
+The first three digits indicate the file type and the last three digits contain
+the file permission bits. For added files, this field will not be present.
+|`new_mode` |optional|File mode in octal (e.g. 100644) at the new commit.
+The first three digits indicate the file type and the last three digits contain
+the file permission bits. For deleted files, this field will not be present.
|=============================
[[fix-input]]
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index efc746a..685f9d9 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -3804,6 +3804,7 @@
|`enabled` |optional|Whether the commentlink is enabled, as documented
in link:config-gerrit.html#commentlink.name.enabled[
commentlink.name.enabled]. If not set the commentlink is enabled.
+|==================================================
[[commentlink-input]]
=== CommentLinkInput
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index f260b87..9527a56 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -620,7 +620,7 @@
+
Changes containing a top-level or inline comment by 'USER'. The special
case of `commentby:self` will find changes where the caller has
-commented.
+commented. Note that setting a vote is also considered as a comment.
[[from]]
from:'USER'::
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 1177734..c5d319f 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -32,6 +32,7 @@
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static com.google.gerrit.server.project.testing.TestLabels.label;
import static com.google.gerrit.server.project.testing.TestLabels.value;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toList;
@@ -89,6 +90,7 @@
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ProjectWatchInfo;
+import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -98,6 +100,7 @@
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.server.GerritPersonIdent;
@@ -1100,6 +1103,42 @@
gApi.changes().id(id).current().review(ReviewInput.recommend());
}
+ protected void assertThatAccountIsNotVisible(TestAccount... testAccounts) {
+ for (TestAccount testAccount : testAccounts) {
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(testAccount.id().get()).get());
+ }
+ }
+
+ protected void assertReviewers(String changeId, TestAccount... expectedReviewers)
+ throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).containsKey(ReviewerState.REVIEWER);
+ List<Integer> reviewers =
+ reviewerMap.get(ReviewerState.REVIEWER).stream().map(a -> a._accountId).collect(toList());
+ assertThat(reviewers)
+ .containsExactlyElementsIn(
+ Arrays.stream(expectedReviewers).map(a -> a.id().get()).collect(toList()));
+ }
+
+ protected void assertCcs(String changeId, TestAccount... expectedCcs) throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).containsKey(ReviewerState.CC);
+ List<Integer> ccs =
+ reviewerMap.get(ReviewerState.CC).stream().map(a -> a._accountId).collect(toList());
+ assertThat(ccs)
+ .containsExactlyElementsIn(
+ Arrays.stream(expectedCcs).map(a -> a.id().get()).collect(toList()));
+ }
+
+ protected void assertNoCcs(String changeId) throws RestApiException {
+ Map<ReviewerState, Collection<AccountInfo>> reviewerMap =
+ gApi.changes().id(changeId).get().reviewers;
+ assertThat(reviewerMap).doesNotContainKey(ReviewerState.CC);
+ }
+
protected void assertSubmittedTogether(String chId, String... expected) throws Exception {
assertSubmittedTogether(chId, ImmutableSet.of(), expected);
}
diff --git a/java/com/google/gerrit/acceptance/PushOneCommit.java b/java/com/google/gerrit/acceptance/PushOneCommit.java
index d46fb78..8a53184 100644
--- a/java/com/google/gerrit/acceptance/PushOneCommit.java
+++ b/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -287,6 +287,19 @@
return this;
}
+ public PushOneCommit addFile(String path, String content, int fileMode) throws Exception {
+ RevBlob blobId = testRepo.blob(content);
+ commitBuilder.edit(
+ new PathEdit(path) {
+ @Override
+ public void apply(DirCacheEntry ent) {
+ ent.setFileMode(FileMode.fromBits(fileMode));
+ ent.setObjectId(blobId);
+ }
+ });
+ return this;
+ }
+
public PushOneCommit addSymlink(String path, String target) throws Exception {
RevBlob blobId = testRepo.blob(target);
commitBuilder.edit(
diff --git a/java/com/google/gerrit/entities/Patch.java b/java/com/google/gerrit/entities/Patch.java
index 2d28046..3c33490 100644
--- a/java/com/google/gerrit/entities/Patch.java
+++ b/java/com/google/gerrit/entities/Patch.java
@@ -168,33 +168,40 @@
*/
public enum FileMode implements CodedEnum {
/** Mode indicating an entry is a tree (aka directory). */
- TREE('T'),
+ TREE('T', 0040000),
/** Mode indicating an entry is a symbolic link. */
- SYMLINK('S'),
+ SYMLINK('S', 0120000),
/** Mode indicating an entry is a non-executable file. */
- REGULAR_FILE('R'),
+ REGULAR_FILE('R', 0100644),
/** Mode indicating an entry is an executable file. */
- EXECUTABLE_FILE('E'),
+ EXECUTABLE_FILE('E', 0100755),
/** Mode indicating an entry is a submodule commit in another repository. */
- GITLINK('G'),
+ GITLINK('G', 0160000),
/** Mode indicating an entry is missing during parallel walks. */
- MISSING('M');
+ MISSING('M', 0000000);
private final char code;
- FileMode(char c) {
+ private final int mode;
+
+ FileMode(char c, int m) {
code = c;
+ mode = m;
}
@Override
public char getCode() {
return code;
}
+
+ public int getMode() {
+ return mode;
+ }
}
private Patch() {}
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
index 11999ab..8bfe468 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
@@ -14,8 +14,10 @@
package com.google.gerrit.extensions.api.changes;
+import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.FixSuggestionInfo;
@@ -117,11 +119,13 @@
public List<FixSuggestionInfo> fixSuggestions;
}
+ @CanIgnoreReturnValue
public ReviewInput message(String msg) {
message = msg != null && !msg.isEmpty() ? msg : null;
return this;
}
+ @CanIgnoreReturnValue
public ReviewInput patchSetLevelComment(String message) {
Objects.requireNonNull(message);
CommentInput comment = new CommentInput();
@@ -131,6 +135,7 @@
return this;
}
+ @CanIgnoreReturnValue
public ReviewInput label(String name, short value) {
if (name == null || name.isEmpty()) {
throw new IllegalArgumentException();
@@ -142,6 +147,7 @@
return this;
}
+ @CanIgnoreReturnValue
public ReviewInput label(String name, int value) {
if (value < Short.MIN_VALUE || value > Short.MAX_VALUE) {
throw new IllegalArgumentException();
@@ -149,14 +155,22 @@
return label(name, (short) value);
}
+ @CanIgnoreReturnValue
public ReviewInput label(String name) {
return label(name, (short) 1);
}
+ @CanIgnoreReturnValue
public ReviewInput reviewer(String reviewer) {
- return reviewer(reviewer, REVIEWER, false);
+ return reviewer(reviewer, REVIEWER, /* confirmed= */ false);
}
+ @CanIgnoreReturnValue
+ public ReviewInput cc(String cc) {
+ return reviewer(cc, CC, /* confirmed= */ false);
+ }
+
+ @CanIgnoreReturnValue
public ReviewInput reviewer(String reviewer, ReviewerState state, boolean confirmed) {
ReviewerInput input = new ReviewerInput();
input.reviewer = reviewer;
@@ -169,6 +183,7 @@
return this;
}
+ @CanIgnoreReturnValue
public ReviewInput addUserToAttentionSet(String user, String reason) {
AttentionSetInput input = new AttentionSetInput();
input.user = user;
@@ -180,6 +195,7 @@
return this;
}
+ @CanIgnoreReturnValue
public ReviewInput removeUserFromAttentionSet(String user, String reason) {
AttentionSetInput input = new AttentionSetInput();
input.user = user;
@@ -191,17 +207,20 @@
return this;
}
+ @CanIgnoreReturnValue
public ReviewInput blockAutomaticAttentionSetRules() {
ignoreAutomaticAttentionSetRules = true;
return this;
}
+ @CanIgnoreReturnValue
public ReviewInput setWorkInProgress(boolean workInProgress) {
this.workInProgress = workInProgress;
ready = !workInProgress;
return this;
}
+ @CanIgnoreReturnValue
public ReviewInput setReady(boolean ready) {
this.ready = ready;
workInProgress = !ready;
diff --git a/java/com/google/gerrit/extensions/common/FileInfo.java b/java/com/google/gerrit/extensions/common/FileInfo.java
index c732663..9526fbb 100644
--- a/java/com/google/gerrit/extensions/common/FileInfo.java
+++ b/java/com/google/gerrit/extensions/common/FileInfo.java
@@ -18,6 +18,8 @@
public class FileInfo {
public Character status;
+ public Integer oldMode;
+ public Integer newMode;
public Boolean binary;
public String oldPath;
public Integer linesInserted;
diff --git a/java/com/google/gerrit/extensions/common/testing/FileInfoSubject.java b/java/com/google/gerrit/extensions/common/testing/FileInfoSubject.java
index d011d5d..180a94691 100644
--- a/java/com/google/gerrit/extensions/common/testing/FileInfoSubject.java
+++ b/java/com/google/gerrit/extensions/common/testing/FileInfoSubject.java
@@ -45,6 +45,16 @@
return check("linesDeleted").that(fileInfo.linesDeleted);
}
+ public IntegerSubject oldMode() {
+ isNotNull();
+ return check("oldMode").that(fileInfo.oldMode);
+ }
+
+ public IntegerSubject newMode() {
+ isNotNull();
+ return check("newMode").that(fileInfo.newMode);
+ }
+
public ComparableSubject<Character> status() {
isNotNull();
return check("status").that(fileInfo.status);
diff --git a/java/com/google/gerrit/index/query/RegexPredicate.java b/java/com/google/gerrit/index/query/RegexPredicate.java
index 60a2a9e..4c76770 100644
--- a/java/com/google/gerrit/index/query/RegexPredicate.java
+++ b/java/com/google/gerrit/index/query/RegexPredicate.java
@@ -14,14 +14,14 @@
package com.google.gerrit.index.query;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
public abstract class RegexPredicate<I> extends IndexPredicate<I> {
- protected RegexPredicate(FieldDef<I, ?> def, String value) {
+ protected RegexPredicate(SchemaField<I, ?> def, String value) {
super(def, value);
}
- protected RegexPredicate(FieldDef<I, ?> def, String name, String value) {
+ protected RegexPredicate(SchemaField<I, ?> def, String name, String value) {
super(def, name, value);
}
}
diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
index 50efef0..086ad90 100644
--- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
+++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
@@ -265,7 +265,7 @@
protected ChangeData valueFor(Map<String, Object> doc) {
ChangeData cd =
changeDataFactory.create(
- Project.nameKey((String) doc.get(ChangeField.PROJECT.getName())),
+ Project.nameKey((String) doc.get(ChangeField.PROJECT_SPEC.getName())),
Change.id(Integer.valueOf((String) doc.get(ChangeField.LEGACY_ID_STR.getName()))));
for (SchemaField<ChangeData, ?> field : getSchema().getSchemaFields().values()) {
field.setIfPossible(cd, new FakeStoredValue(doc.get(field.getName())));
diff --git a/java/com/google/gerrit/index/testing/TestIndexedFields.java b/java/com/google/gerrit/index/testing/TestIndexedFields.java
index f80b8a1..7a120b7 100644
--- a/java/com/google/gerrit/index/testing/TestIndexedFields.java
+++ b/java/com/google/gerrit/index/testing/TestIndexedFields.java
@@ -182,7 +182,10 @@
public static final IndexedField<TestIndexedData, Entities.Change> STORED_PROTO_FIELD =
IndexedField.<TestIndexedData, Entities.Change>builder(
- "TestChange", new TypeToken<Entities.Change>() {})
+ "TestChange",
+ new TypeToken<Entities.Change>() {
+ private static final long serialVersionUID = 1L;
+ })
.stored()
.build(getter(), setter(), ChangeProtoConverter.INSTANCE);
@@ -192,7 +195,10 @@
public static final IndexedField<TestIndexedData, Iterable<Entities.Change>>
ITERABLE_STORED_PROTO_FIELD =
IndexedField.<TestIndexedData, Iterable<Entities.Change>>builder(
- "IterableTestChange", new TypeToken<Iterable<Entities.Change>>() {})
+ "IterableTestChange",
+ new TypeToken<Iterable<Entities.Change>>() {
+ private static final long serialVersionUID = 1L;
+ })
.stored()
.build(getter(), setter(), ChangeProtoConverter.INSTANCE);
diff --git a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
index 7b47248..6471984 100644
--- a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
+++ b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
@@ -542,7 +542,7 @@
}
}
ScoreDoc searchAfter = scoreDoc;
- return new ListResultSet<T>(b.build()) {
+ return new ListResultSet<>(b.build()) {
@Override
public Object searchAfter() {
return searchAfter;
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 032eb01..755ae7b 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -18,7 +18,7 @@
import static com.google.gerrit.lucene.AbstractLuceneIndex.sortFieldName;
import static com.google.gerrit.server.git.QueueProvider.QueueType.INTERACTIVE;
import static com.google.gerrit.server.index.change.ChangeField.LEGACY_ID_STR;
-import static com.google.gerrit.server.index.change.ChangeField.PROJECT;
+import static com.google.gerrit.server.index.change.ChangeField.PROJECT_SPEC;
import static com.google.gerrit.server.index.change.ChangeIndexRewriter.CLOSED_STATUSES;
import static com.google.gerrit.server.index.change.ChangeIndexRewriter.OPEN_STATUSES;
import static java.util.Objects.requireNonNull;
@@ -473,8 +473,9 @@
private ScoreDoc getSearchAfter(ChangeSubIndex subIndex) {
if (isSearchAfterPagination
&& opts.searchAfter() != null
- && opts.searchAfter() instanceof Map) {
- return ((Map<ChangeSubIndex, ScoreDoc>) opts.searchAfter()).get(subIndex);
+ && opts.searchAfter() instanceof Map
+ && ((Map<?, ?>) opts.searchAfter()).get(subIndex) instanceof ScoreDoc) {
+ return (ScoreDoc) ((Map<?, ?>) opts.searchAfter()).get(subIndex);
}
return null;
}
@@ -563,7 +564,7 @@
Change.Id id = Change.id(Integer.valueOf(f.stringValue()));
// IndexUtils#changeFields ensures either CHANGE or PROJECT is always present.
- IndexableField project = doc.get(PROJECT.getName()).iterator().next();
+ IndexableField project = doc.get(PROJECT_SPEC.getName()).iterator().next();
cd = changeDataFactory.create(Project.nameKey(project.stringValue()), id);
}
diff --git a/java/com/google/gerrit/pgm/Reindex.java b/java/com/google/gerrit/pgm/Reindex.java
index c4e185d..762d988 100644
--- a/java/com/google/gerrit/pgm/Reindex.java
+++ b/java/com/google/gerrit/pgm/Reindex.java
@@ -36,6 +36,7 @@
import com.google.gerrit.server.cache.CacheInfo;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.WorkQueue.WorkQueueModule;
import com.google.gerrit.server.index.IndexModule;
import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
import com.google.gerrit.server.index.options.AutoFlush;
@@ -175,6 +176,8 @@
}
boolean replica = ReplicaUtil.isReplica(globalConfig);
List<Module> modules = new ArrayList<>();
+ modules.add(new WorkQueueModule());
+
Module indexModule;
IndexType indexType = IndexModule.getIndexType(dbInjector);
if (indexType.isLucene()) {
diff --git a/java/com/google/gerrit/server/StarredChangesUtil.java b/java/com/google/gerrit/server/StarredChangesUtil.java
index 0f5629e..cfecd8e 100644
--- a/java/com/google/gerrit/server/StarredChangesUtil.java
+++ b/java/com/google/gerrit/server/StarredChangesUtil.java
@@ -32,7 +32,6 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.git.GitUpdateFailureException;
@@ -194,7 +193,7 @@
}
}
- public void star(Account.Id accountId, Project.NameKey project, Change.Id changeId, Operation op)
+ public void star(Account.Id accountId, Change.Id changeId, Operation op)
throws IllegalLabelException {
try (Repository repo = repoManager.openRepository(allUsers)) {
String refName = RefNames.refsStarredChanges(changeId, accountId);
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index 8824d56..65eb332 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -559,6 +559,11 @@
return searchImpl(input, searchers, this::canSeePredicate, AccountResolver::allVisible);
}
+ public Result resolveIncludeInactiveIgnoreVisibility(String input)
+ throws ConfigInvalidException, IOException {
+ return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::allVisible);
+ }
+
public Result resolveIgnoreVisibility(String input) throws ConfigInvalidException, IOException {
return searchImpl(input, searchers, this::allVisiblePredicate, AccountResolver::isActive);
}
diff --git a/java/com/google/gerrit/server/account/externalids/AllExternalIds.java b/java/com/google/gerrit/server/account/externalids/AllExternalIds.java
index e718bcb..14aa368 100644
--- a/java/com/google/gerrit/server/account/externalids/AllExternalIds.java
+++ b/java/com/google/gerrit/server/account/externalids/AllExternalIds.java
@@ -45,6 +45,13 @@
return new AutoValue_AllExternalIds(byKey.build(), byAccount.build(), byEmail.build());
}
+ static AllExternalIds create(
+ ImmutableMap<ExternalId.Key, ExternalId> byKey,
+ ImmutableSetMultimap<Account.Id, ExternalId> byAccount,
+ ImmutableSetMultimap<String, ExternalId> byEmail) {
+ return new AutoValue_AllExternalIds(byKey, byAccount, byEmail);
+ }
+
public abstract ImmutableMap<ExternalId.Key, ExternalId> byKey();
public abstract ImmutableSetMultimap<Account.Id, ExternalId> byAccount();
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java b/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java
index 27672bd..1edb284 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java
@@ -253,7 +253,7 @@
}
}
}
- return new AutoValue_AllExternalIds(byKey.build(), byAccount.build(), byEmail.build());
+ return AllExternalIds.create(byKey.build(), byAccount.build(), byEmail.build());
}
private AllExternalIds reloadAllExternalIds(ObjectId notesRev)
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index edaca70..a041ff7 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -29,6 +29,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
@@ -242,32 +243,38 @@
return change;
}
+ @CanIgnoreReturnValue
public ChangeInserter setTopic(String topic) {
checkState(change == null, "setTopic(String) only valid before creating change");
this.topic = topic;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setCherryPickOf(PatchSet.Id cherryPickOf) {
this.cherryPickOf = cherryPickOf;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setMessage(String message) {
this.message = message;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setPatchSetDescription(String patchSetDescription) {
this.patchSetDescription = patchSetDescription;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setValidate(boolean validate) {
this.validate = validate;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setReviewersAndCcs(
Iterable<Account.Id> reviewers, Iterable<Account.Id> ccs) {
return setReviewersAndCcsAsStrings(
@@ -275,35 +282,57 @@
Iterables.transform(ccs, Account.Id::toString));
}
+ @CanIgnoreReturnValue
+ public ChangeInserter setReviewersAndCcsIgnoreVisibility(
+ Iterable<Account.Id> reviewers, Iterable<Account.Id> ccs) {
+ return setReviewersAndCcsAsStrings(
+ Iterables.transform(reviewers, Account.Id::toString),
+ Iterables.transform(ccs, Account.Id::toString),
+ /* skipVisibilityCheck= */ true);
+ }
+
+ @CanIgnoreReturnValue
public ChangeInserter setReviewersAndCcsAsStrings(
Iterable<String> reviewers, Iterable<String> ccs) {
+ return setReviewersAndCcsAsStrings(reviewers, ccs, /* skipVisibilityCheck= */ false);
+ }
+
+ @CanIgnoreReturnValue
+ private ChangeInserter setReviewersAndCcsAsStrings(
+ Iterable<String> reviewers, Iterable<String> ccs, boolean skipVisibilityCheck) {
reviewerInputs =
Streams.concat(
Streams.stream(reviewers)
.distinct()
- .map(id -> newReviewerInput(id, ReviewerState.REVIEWER)),
- Streams.stream(ccs).distinct().map(id -> newReviewerInput(id, ReviewerState.CC)))
+ .map(id -> newReviewerInput(id, ReviewerState.REVIEWER, skipVisibilityCheck)),
+ Streams.stream(ccs)
+ .distinct()
+ .map(id -> newReviewerInput(id, ReviewerState.CC, skipVisibilityCheck)))
.collect(toImmutableList());
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setPrivate(boolean isPrivate) {
checkState(change == null, "setPrivate(boolean) only valid before creating change");
this.isPrivate = isPrivate;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setWorkInProgress(boolean workInProgress) {
this.workInProgress = workInProgress;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setStatus(Change.Status status) {
checkState(change == null, "setStatus(Change.Status) only valid before creating change");
this.status = status;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setGroups(List<String> groups) {
requireNonNull(groups, "groups may not be empty");
checkState(patchSet == null, "setGroups(List<String>) only valid before creating change");
@@ -311,6 +340,7 @@
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setValidationOptions(
ImmutableListMultimap<String, String> validationOptions) {
requireNonNull(validationOptions, "validationOptions may not be null");
@@ -322,21 +352,25 @@
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setFireRevisionCreated(boolean fireRevisionCreated) {
this.fireRevisionCreated = fireRevisionCreated;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setSendMail(boolean sendMail) {
this.sendMail = sendMail;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setRequestScopePropagator(RequestScopePropagator r) {
this.requestScopePropagator = r;
return this;
}
+ @CanIgnoreReturnValue
public ChangeInserter setRevertOf(Change.Id revertOf) {
this.revertOf = revertOf;
return this;
@@ -351,6 +385,7 @@
return patchSet;
}
+ @CanIgnoreReturnValue
public ChangeInserter setApprovals(Map<String, Short> approvals) {
this.approvals = approvals;
return this;
@@ -368,6 +403,7 @@
* @param updateRef whether to update the ref during {@link #updateRepo(RepoContext)}.
*/
@Deprecated
+ @CanIgnoreReturnValue
public ChangeInserter setUpdateRef(boolean updateRef) {
this.updateRef = updateRef;
return this;
@@ -595,7 +631,8 @@
}
}
- private static InternalReviewerInput newReviewerInput(String reviewer, ReviewerState state) {
+ private static InternalReviewerInput newReviewerInput(
+ String reviewer, ReviewerState state, boolean skipVisibilityCheck) {
// Disable individual emails when adding reviewers, as all reviewers will receive the single
// bulk new change email.
InternalReviewerInput input =
@@ -606,7 +643,9 @@
// certain commit footers: putting a nonexistent user in a footer should not cause an error. In
// theory we could provide finer control to do this for some reviewers and not others, but it's
// not worth complicating the ChangeInserter interface further at this time.
- input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE;
+ input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE_EXCEPT_NOT_FOUND;
+
+ input.skipVisibilityCheck = skipVisibilityCheck;
return input;
}
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
index 44b4ded..0f9194f 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
@@ -102,6 +102,14 @@
fileInfo.oldPath = FilePathAdapter.getOldPath(fileDiff.oldPath(), fileDiff.changeType());
fileInfo.sizeDelta = fileDiff.sizeDelta();
fileInfo.size = fileDiff.size();
+ fileInfo.oldMode =
+ fileDiff.oldMode().isPresent() && !fileDiff.oldMode().get().equals(Patch.FileMode.MISSING)
+ ? fileDiff.oldMode().get().getMode()
+ : null;
+ fileInfo.newMode =
+ fileDiff.newMode().isPresent() && !fileDiff.newMode().get().equals(Patch.FileMode.MISSING)
+ ? fileDiff.newMode().get().getMode()
+ : null;
if (fileDiff.patchType().get() == Patch.PatchType.BINARY) {
fileInfo.binary = true;
} else {
diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index 9580565..f3ad4f7 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java
@@ -94,9 +94,21 @@
public static final int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10;
public static final int DEFAULT_MAX_REVIEWERS = 20;
+ /**
+ * Controls which failures should be ignored.
+ *
+ * <p>If a failure is ignored the operation succeeds, but the reviewer is not added. If not
+ * ignored a failure means that the operation fails.
+ */
public enum FailureBehavior {
+ // All failures cause the operation to fail.
FAIL,
- IGNORE;
+
+ // Only not found failures cause the operation to fail, all other failures are ignored.
+ IGNORE_EXCEPT_NOT_FOUND,
+
+ // All failures are ignored.
+ IGNORE_ALL;
}
private enum FailureType {
@@ -113,6 +125,9 @@
* resolving to an account/group/email.
*/
public FailureBehavior otherFailureBehavior = FailureBehavior.FAIL;
+
+ /** Whether the visibility check for the reviewer account should be skipped. */
+ public boolean skipVisibilityCheck = false;
}
public static InternalReviewerInput newReviewerInput(
@@ -143,7 +158,7 @@
in.reviewer = accountId.toString();
in.state = CC;
in.notify = notify;
- in.otherFailureBehavior = FailureBehavior.IGNORE;
+ in.otherFailureBehavior = FailureBehavior.IGNORE_ALL;
return Optional.of(in);
}
@@ -262,7 +277,13 @@
IdentifiedUser reviewerUser;
boolean exactMatchFound = false;
try {
- reviewerUser = accountResolver.resolveIncludeInactive(input.reviewer).asUniqueUser();
+ if (input instanceof InternalReviewerInput
+ && ((InternalReviewerInput) input).skipVisibilityCheck) {
+ reviewerUser =
+ accountResolver.resolveIncludeInactiveIgnoreVisibility(input.reviewer).asUniqueUser();
+ } else {
+ reviewerUser = accountResolver.resolveIncludeInactive(input.reviewer).asUniqueUser();
+ }
if (input.reviewer.equalsIgnoreCase(reviewerUser.getName())
|| input.reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) {
exactMatchFound = true;
@@ -577,7 +598,9 @@
(input instanceof InternalReviewerInput)
? ((InternalReviewerInput) input).otherFailureBehavior
: FailureBehavior.FAIL;
- return failureType == FailureType.OTHER && behavior == FailureBehavior.IGNORE;
+ return behavior == FailureBehavior.IGNORE_ALL
+ || (failureType == FailureType.OTHER
+ && behavior == FailureBehavior.IGNORE_EXCEPT_NOT_FOUND);
}
}
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index fa46bf4..f90daff 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -286,7 +286,7 @@
reviewers.remove(user.getAccountId());
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.getAccountId());
- ins.setReviewersAndCcs(reviewers, ccs);
+ ins.setReviewersAndCcsIgnoreVisibility(reviewers, ccs);
ins.setRevertOf(notes.getChangeId());
ins.setWorkInProgress(input.workInProgress);
diff --git a/java/com/google/gerrit/server/git/MultiProgressMonitor.java b/java/com/google/gerrit/server/git/MultiProgressMonitor.java
index 290e1e7..c76c78e 100644
--- a/java/com/google/gerrit/server/git/MultiProgressMonitor.java
+++ b/java/com/google/gerrit/server/git/MultiProgressMonitor.java
@@ -16,8 +16,10 @@
import static com.google.gerrit.server.DeadlineChecker.getTimeoutFormatter;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.MINUTES;
import static java.util.concurrent.TimeUnit.NANOSECONDS;
+import com.google.common.base.CharMatcher;
import com.google.common.base.Strings;
import com.google.common.base.Ticker;
import com.google.common.flogger.FluentLogger;
@@ -592,9 +594,12 @@
}
private void send(StringBuilder s) {
+ String progress = s.toString();
+ logger.atInfo().atMostEvery(1, MINUTES).log(
+ "%s", CharMatcher.javaIsoControl().removeFrom(progress));
if (!clientDisconnected) {
try {
- out.write(Constants.encode(s.toString()));
+ out.write(Constants.encode(progress));
out.flush();
} catch (IOException e) {
logger.atWarning().withCause(e).log(
diff --git a/java/com/google/gerrit/server/git/WorkQueue.java b/java/com/google/gerrit/server/git/WorkQueue.java
index 3032bfe..f516a0c 100644
--- a/java/com/google/gerrit/server/git/WorkQueue.java
+++ b/java/com/google/gerrit/server/git/WorkQueue.java
@@ -20,6 +20,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.events.LifecycleListener;
+import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.MetricMaker;
@@ -27,6 +28,7 @@
import com.google.gerrit.server.config.ScheduleConfig.Schedule;
import com.google.gerrit.server.logging.LoggingContext;
import com.google.gerrit.server.logging.LoggingContextAwareRunnable;
+import com.google.gerrit.server.plugincontext.PluginMapContext;
import com.google.gerrit.server.util.IdGenerator;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -49,8 +51,8 @@
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.AtomicReference;
import org.eclipse.jgit.lib.Config;
/** Delayed execution of tasks using a background thread pool. */
@@ -58,6 +60,30 @@
public class WorkQueue {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ /**
+ * To register a TaskListener, which will be called directly before Tasks run, and directly after
+ * they complete, bind the TaskListener like this:
+ *
+ * <p><code>
+ * bind(TaskListener.class)
+ * .annotatedWith(Exports.named("MyListener"))
+ * .to(MyListener.class);
+ * </code>
+ */
+ public interface TaskListener {
+ public static class NoOp implements TaskListener {
+ @Override
+ public void onStart(Task<?> task) {}
+
+ @Override
+ public void onStop(Task<?> task) {}
+ }
+
+ void onStart(Task<?> task);
+
+ void onStop(Task<?> task);
+ }
+
public static class Lifecycle implements LifecycleListener {
private final WorkQueue workQueue;
@@ -78,6 +104,7 @@
public static class WorkQueueModule extends LifecycleModule {
@Override
protected void configure() {
+ DynamicMap.mapOf(binder(), WorkQueue.TaskListener.class);
bind(WorkQueue.class);
listener().to(Lifecycle.class);
}
@@ -87,18 +114,32 @@
private final IdGenerator idGenerator;
private final MetricMaker metrics;
private final CopyOnWriteArrayList<Executor> queues;
+ private final PluginMapContext<TaskListener> listeners;
@Inject
- WorkQueue(IdGenerator idGenerator, @GerritServerConfig Config cfg, MetricMaker metrics) {
- this(idGenerator, Math.max(cfg.getInt("execution", "defaultThreadPoolSize", 2), 2), metrics);
+ WorkQueue(
+ IdGenerator idGenerator,
+ @GerritServerConfig Config cfg,
+ MetricMaker metrics,
+ PluginMapContext<TaskListener> listeners) {
+ this(
+ idGenerator,
+ Math.max(cfg.getInt("execution", "defaultThreadPoolSize", 2), 2),
+ metrics,
+ listeners);
}
/** Constructor to allow binding the WorkQueue more explicitly in a vhost setup. */
- public WorkQueue(IdGenerator idGenerator, int defaultThreadPoolSize, MetricMaker metrics) {
+ public WorkQueue(
+ IdGenerator idGenerator,
+ int defaultThreadPoolSize,
+ MetricMaker metrics,
+ PluginMapContext<TaskListener> listeners) {
this.idGenerator = idGenerator;
this.metrics = metrics;
this.queues = new CopyOnWriteArrayList<>();
this.defaultQueue = createQueue(defaultThreadPoolSize, "WorkQueue", true);
+ this.listeners = listeners;
}
/** Get the default work queue, for miscellaneous tasks. */
@@ -438,6 +479,14 @@
Collection<Task<?>> getTasks() {
return all.values();
}
+
+ public void onStart(Task<?> task) {
+ listeners.runEach(extension -> extension.getProvider().get().onStart(task));
+ }
+
+ public void onStop(Task<?> task) {
+ listeners.runEach(extension -> extension.getProvider().get().onStop(task));
+ }
}
private static void logUncaughtException(Thread t, Throwable e) {
@@ -474,18 +523,23 @@
* <ol>
* <li>{@link #SLEEPING}: if scheduled with a non-zero delay.
* <li>{@link #READY}: waiting for an available worker thread.
+ * <li>{@link #STARTING}: onStart() actively executing on a worker thread.
* <li>{@link #RUNNING}: actively executing on a worker thread.
+ * <li>{@link #STOPPING}: onStop() actively executing on a worker thread.
* <li>{@link #DONE}: finished executing, if not periodic.
* </ol>
*/
public enum State {
// Ordered like this so ordinal matches the order we would
// prefer to see tasks sorted in: done before running,
- // running before ready, ready before sleeping.
+ // stopping before running, running before starting,
+ // starting before ready, ready before sleeping.
//
DONE,
CANCELLED,
+ STOPPING,
RUNNING,
+ STARTING,
READY,
SLEEPING,
OTHER
@@ -495,15 +549,16 @@
private final RunnableScheduledFuture<V> task;
private final Executor executor;
private final int taskId;
- private final AtomicBoolean running;
private final Instant startTime;
+ // runningState is non-null when listener or task code is running in an executor thread
+ private final AtomicReference<State> runningState = new AtomicReference<>();
+
Task(Runnable runnable, RunnableScheduledFuture<V> task, Executor executor, int taskId) {
this.runnable = runnable;
this.task = task;
this.executor = executor;
this.taskId = taskId;
- this.running = new AtomicBoolean();
this.startTime = Instant.now();
}
@@ -514,10 +569,13 @@
public State getState() {
if (isCancelled()) {
return State.CANCELLED;
+ }
+
+ State r = runningState.get();
+ if (r != null) {
+ return r;
} else if (isDone() && !isPeriodic()) {
return State.DONE;
- } else if (running.get()) {
- return State.RUNNING;
}
final long delay = getDelay(TimeUnit.MILLISECONDS);
@@ -538,14 +596,14 @@
@Override
public boolean cancel(boolean mayInterruptIfRunning) {
if (task.cancel(mayInterruptIfRunning)) {
- // Tiny abuse of running: if the task needs to know it was
- // canceled (to clean up resources) and it hasn't started
+ // Tiny abuse of runningState: if the task needs to know it
+ // was canceled (to clean up resources) and it hasn't started
// yet the task's run method won't execute. So we tag it
// as running and allow it to clean up. This ensures we do
// not invoke cancel twice.
//
if (runnable instanceof CancelableRunnable) {
- if (running.compareAndSet(false, true)) {
+ if (runningState.compareAndSet(null, State.RUNNING)) {
((CancelableRunnable) runnable).cancel();
} else if (runnable instanceof CanceledWhileRunning) {
((CanceledWhileRunning) runnable).setCanceledWhileRunning();
@@ -605,16 +663,21 @@
@Override
public void run() {
- if (running.compareAndSet(false, true)) {
+ if (runningState.compareAndSet(null, State.STARTING)) {
String oldThreadName = Thread.currentThread().getName();
try {
+ executor.onStart(this);
+ runningState.set(State.RUNNING);
Thread.currentThread().setName(oldThreadName + "[" + task.toString() + "]");
task.run();
} finally {
Thread.currentThread().setName(oldThreadName);
+ runningState.set(State.STOPPING);
+ executor.onStop(this);
if (isPeriodic()) {
- running.set(false);
+ runningState.set(null);
} else {
+ runningState.set(State.DONE);
executor.remove(this);
}
}
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index 644f82e..ae7c3a10 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -405,7 +405,7 @@
// Ignore failures for reasons like the reviewer being inactive or being unable to see the
// change. See discussion in ChangeInserter.
- input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE;
+ input.otherFailureBehavior = ReviewerModifier.FailureBehavior.IGNORE_EXCEPT_NOT_FOUND;
return input;
}
diff --git a/java/com/google/gerrit/server/index/IndexUtils.java b/java/com/google/gerrit/server/index/IndexUtils.java
index 80cc463..7d40c00 100644
--- a/java/com/google/gerrit/server/index/IndexUtils.java
+++ b/java/com/google/gerrit/server/index/IndexUtils.java
@@ -16,7 +16,7 @@
import static com.google.gerrit.server.index.change.ChangeField.CHANGE;
import static com.google.gerrit.server.index.change.ChangeField.LEGACY_ID_STR;
-import static com.google.gerrit.server.index.change.ChangeField.PROJECT;
+import static com.google.gerrit.server.index.change.ChangeField.PROJECT_SPEC;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
@@ -81,10 +81,10 @@
// A Change is always sufficient.
return fs;
}
- if (fs.contains(PROJECT.getName()) && fs.contains(LEGACY_ID_STR.getName())) {
+ if (fs.contains(PROJECT_SPEC.getName()) && fs.contains(LEGACY_ID_STR.getName())) {
return fs;
}
- return Sets.union(fs, ImmutableSet.of(LEGACY_ID_STR.getName(), PROJECT.getName()));
+ return Sets.union(fs, ImmutableSet.of(LEGACY_ID_STR.getName(), PROJECT_SPEC.getName()));
}
/**
diff --git a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
index 6cdc9ae..a39cbe5 100644
--- a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
+++ b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
@@ -112,6 +112,10 @@
private static ProjectSlice create(Project.NameKey name, int slice, int slices, ScanResult sr) {
return new AutoValue_AllChangesIndexer_ProjectSlice(name, slice, slices, sr);
}
+
+ private static ProjectSlice oneSlice(Project.NameKey name, ScanResult sr) {
+ return new AutoValue_AllChangesIndexer_ProjectSlice(name, 0, 1, sr);
+ }
}
@Override
@@ -178,47 +182,35 @@
public Callable<Void> reindexProject(
ChangeIndexer indexer, Project.NameKey project, Task done, Task failed) {
try (Repository repo = repoManager.openRepository(project)) {
- return reindexProject(
- indexer, project, 0, 1, ChangeNotes.Factory.scanChangeIds(repo), done, failed);
+ return reindexProjectSlice(
+ indexer,
+ ProjectSlice.oneSlice(project, ChangeNotes.Factory.scanChangeIds(repo)),
+ done,
+ failed);
} catch (IOException e) {
logger.atSevere().log("%s", e.getMessage());
return null;
}
}
- public Callable<Void> reindexProject(
- ChangeIndexer indexer,
- Project.NameKey project,
- int slice,
- int slices,
- ScanResult scanResult,
- Task done,
- Task failed) {
- return new ProjectIndexer(indexer, project, slice, slices, scanResult, done, failed);
+ public Callable<Void> reindexProjectSlice(
+ ChangeIndexer indexer, ProjectSlice projectSlice, Task done, Task failed) {
+ return new ProjectSliceIndexer(indexer, projectSlice, done, failed);
}
- private class ProjectIndexer implements Callable<Void> {
+ private class ProjectSliceIndexer implements Callable<Void> {
private final ChangeIndexer indexer;
- private final Project.NameKey project;
- private final int slice;
- private final int slices;
- private final ScanResult scanResult;
+ private final ProjectSlice projectSlice;
private final ProgressMonitor done;
private final ProgressMonitor failed;
- private ProjectIndexer(
+ private ProjectSliceIndexer(
ChangeIndexer indexer,
- Project.NameKey project,
- int slice,
- int slices,
- ScanResult scanResult,
+ ProjectSlice projectSlice,
ProgressMonitor done,
ProgressMonitor failed) {
this.indexer = indexer;
- this.project = project;
- this.slice = slice;
- this.slices = slices;
- this.scanResult = scanResult;
+ this.projectSlice = projectSlice;
this.done = done;
this.failed = failed;
}
@@ -232,7 +224,10 @@
// but the goal is to invalidate that cache as infrequently as we possibly can. And besides,
// we don't have concrete proof that improving packfile locality would help.
notesFactory
- .scan(scanResult, project, id -> (id.get() % slices) == slice)
+ .scan(
+ projectSlice.scanResult(),
+ projectSlice.name(),
+ id -> (id.get() % projectSlice.slices()) == projectSlice.slice())
.forEach(r -> index(r));
OnlineReindexMode.end();
return null;
@@ -271,7 +266,7 @@
@Override
public String toString() {
- return "Index all changes of project " + project.get();
+ return "Index project slice " + projectSlice;
}
}
@@ -351,12 +346,9 @@
ProjectSlice projectSlice = ProjectSlice.create(name, slice, slices, sr);
ListenableFuture<?> future =
executor.submit(
- reindexProject(
+ reindexProjectSlice(
indexerFactory.create(executor, index),
- name,
- slice,
- slices,
- projectSlice.scanResult(),
+ projectSlice,
doneTask,
failedTask));
String description = "project " + name + " (" + slice + "/" + slices + ")";
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 0e066d6..670f472a 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -62,6 +62,7 @@
import com.google.gerrit.entities.converter.PatchSetProtoConverter;
import com.google.gerrit.entities.converter.ProtoConverter;
import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.IndexedField;
import com.google.gerrit.index.RefState;
import com.google.gerrit.index.SchemaFieldDefs;
import com.google.gerrit.index.SchemaUtil;
@@ -135,39 +136,63 @@
prefix(ChangeQueryBuilder.FIELD_CHANGE_ID).build(changeGetter(c -> c.getKey().get()));
/** Change status string, in the same format as {@code status:}. */
- public static final FieldDef<ChangeData, String> STATUS =
- exact(ChangeQueryBuilder.FIELD_STATUS)
+ public static final IndexedField<ChangeData, String> STATUS_FIELD =
+ IndexedField.<ChangeData>stringBuilder("Status")
+ .required()
+ .size(20)
.build(changeGetter(c -> ChangeStatusPredicate.canonicalize(c.getStatus())));
+ public static final IndexedField<ChangeData, String>.SearchSpec STATUS_SPEC =
+ STATUS_FIELD.exact(ChangeQueryBuilder.FIELD_STATUS);
+
/** Project containing the change. */
- public static final FieldDef<ChangeData, String> PROJECT =
- exact(ChangeQueryBuilder.FIELD_PROJECT)
+ public static final IndexedField<ChangeData, String> PROJECT_FIELD =
+ IndexedField.<ChangeData>stringBuilder("Project")
+ .required()
.stored()
+ .size(200)
.build(changeGetter(c -> c.getProject().get()));
+ public static final IndexedField<ChangeData, String>.SearchSpec PROJECT_SPEC =
+ PROJECT_FIELD.exact(ChangeQueryBuilder.FIELD_PROJECT);
+
/** Project containing the change, as a prefix field. */
- public static final FieldDef<ChangeData, String> PROJECTS =
- prefix(ChangeQueryBuilder.FIELD_PROJECTS).build(changeGetter(c -> c.getProject().get()));
+ public static final IndexedField<ChangeData, String>.SearchSpec PROJECTS_SPEC =
+ PROJECT_FIELD.prefix(ChangeQueryBuilder.FIELD_PROJECTS);
/** Reference (aka branch) the change will submit onto. */
- public static final FieldDef<ChangeData, String> REF =
- exact(ChangeQueryBuilder.FIELD_REF).build(changeGetter(c -> c.getDest().branch()));
+ public static final IndexedField<ChangeData, String> REF_FIELD =
+ IndexedField.<ChangeData>stringBuilder("Ref")
+ .required()
+ .size(300)
+ .build(changeGetter(c -> c.getDest().branch()));
+
+ public static final IndexedField<ChangeData, String>.SearchSpec REF_SPEC =
+ REF_FIELD.exact(ChangeQueryBuilder.FIELD_REF);
/** Topic, a short annotation on the branch. */
- public static final FieldDef<ChangeData, String> EXACT_TOPIC =
- exact("topic4").build(ChangeField::getTopic);
+ public static final IndexedField<ChangeData, String> TOPIC_FIELD =
+ IndexedField.<ChangeData>stringBuilder("Topic").size(500).build(ChangeField::getTopic);
+
+ public static final IndexedField<ChangeData, String>.SearchSpec EXACT_TOPIC =
+ TOPIC_FIELD.exact("topic4");
/** Topic, a short annotation on the branch. */
- public static final FieldDef<ChangeData, String> FUZZY_TOPIC =
- fullText("topic5").build(ChangeField::getTopic);
+ public static final IndexedField<ChangeData, String>.SearchSpec FUZZY_TOPIC =
+ TOPIC_FIELD.fullText("topic5");
/** Topic, a short annotation on the branch. */
- public static final FieldDef<ChangeData, String> PREFIX_TOPIC =
- prefix("topic6").build(ChangeField::getTopic);
+ public static final IndexedField<ChangeData, String>.SearchSpec PREFIX_TOPIC =
+ TOPIC_FIELD.prefix("topic6");
- /** Submission id assigned by MergeOp. */
- public static final FieldDef<ChangeData, String> SUBMISSIONID =
- exact(ChangeQueryBuilder.FIELD_SUBMISSIONID).build(changeGetter(Change::getSubmissionId));
+ /** {@link com.google.gerrit.entities.SubmissionId} assigned by MergeOp. */
+ public static final IndexedField<ChangeData, String> SUBMISSIONID_FIELD =
+ IndexedField.<ChangeData>stringBuilder("SubmissionId")
+ .size(500)
+ .build(changeGetter(Change::getSubmissionId));
+
+ public static final IndexedField<ChangeData, String>.SearchSpec SUBMISSIONID_SPEC =
+ SUBMISSIONID_FIELD.exact(ChangeQueryBuilder.FIELD_SUBMISSIONID);
/** Last update time since January 1, 1970. */
// TODO(issue-15518): Migrate type for timestamp index fields from Timestamp to Instant
diff --git a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
index 6116f5a..0ce08b8 100644
--- a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.index.SchemaUtil.schema;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.index.Schema;
import com.google.gerrit.index.SchemaDefinitions;
import com.google.gerrit.server.query.change.ChangeData;
@@ -32,79 +33,87 @@
static final Schema<ChangeData> V74 =
schema(
/* version= */ 74,
- ChangeField.ADDED,
- ChangeField.APPROVAL,
- ChangeField.ASSIGNEE,
- ChangeField.ATTENTION_SET_FULL,
- ChangeField.ATTENTION_SET_USERS,
- ChangeField.ATTENTION_SET_USERS_COUNT,
- ChangeField.AUTHOR,
- ChangeField.CHANGE,
- ChangeField.CHERRY_PICK,
- ChangeField.CHERRY_PICK_OF_CHANGE,
- ChangeField.CHERRY_PICK_OF_PATCHSET,
- ChangeField.COMMENT,
- ChangeField.COMMENTBY,
- ChangeField.COMMIT,
- ChangeField.COMMIT_MESSAGE,
- ChangeField.COMMITTER,
- ChangeField.DELETED,
- ChangeField.DELTA,
- ChangeField.DIRECTORY,
- ChangeField.DRAFTBY,
- ChangeField.EDITBY,
- ChangeField.EXACT_AUTHOR,
- ChangeField.EXACT_COMMIT,
- ChangeField.EXACT_COMMITTER,
- ChangeField.EXACT_TOPIC,
- ChangeField.EXTENSION,
- ChangeField.FILE_PART,
- ChangeField.FOOTER,
- ChangeField.FUZZY_HASHTAG,
- ChangeField.FUZZY_TOPIC,
- ChangeField.GROUP,
- ChangeField.HASHTAG,
- ChangeField.HASHTAG_CASE_AWARE,
- ChangeField.ID,
- ChangeField.IS_PURE_REVERT,
- ChangeField.IS_SUBMITTABLE,
- ChangeField.LABEL,
- ChangeField.LEGACY_ID_STR,
- ChangeField.MERGE,
- ChangeField.MERGEABLE,
- ChangeField.MERGED_ON,
- ChangeField.ONLY_EXTENSIONS,
- ChangeField.OWNER,
- ChangeField.PATCH_SET,
- ChangeField.PATH,
- ChangeField.PENDING_REVIEWER,
- ChangeField.PENDING_REVIEWER_BY_EMAIL,
- ChangeField.PRIVATE,
- ChangeField.PROJECT,
- ChangeField.PROJECTS,
- ChangeField.REF,
- ChangeField.REF_STATE,
- ChangeField.REF_STATE_PATTERN,
- ChangeField.REVERT_OF,
- ChangeField.REVIEWEDBY,
- ChangeField.REVIEWER,
- ChangeField.REVIEWER_BY_EMAIL,
- ChangeField.STAR,
- ChangeField.STARBY,
- ChangeField.STARTED,
- ChangeField.STATUS,
- ChangeField.STORED_SUBMIT_RECORD_LENIENT,
- ChangeField.STORED_SUBMIT_RECORD_STRICT,
- ChangeField.STORED_SUBMIT_REQUIREMENTS,
- ChangeField.SUBMISSIONID,
- ChangeField.SUBMIT_RECORD,
- ChangeField.SUBMIT_RULE_RESULT,
- ChangeField.TOTAL_COMMENT_COUNT,
- ChangeField.TR,
- ChangeField.UNRESOLVED_COMMENT_COUNT,
- ChangeField.UPDATED,
- ChangeField.UPLOADER,
- ChangeField.WIP);
+ ImmutableList.of(
+ ChangeField.ADDED,
+ ChangeField.APPROVAL,
+ ChangeField.ASSIGNEE,
+ ChangeField.ATTENTION_SET_FULL,
+ ChangeField.ATTENTION_SET_USERS,
+ ChangeField.ATTENTION_SET_USERS_COUNT,
+ ChangeField.AUTHOR,
+ ChangeField.CHANGE,
+ ChangeField.CHERRY_PICK,
+ ChangeField.CHERRY_PICK_OF_CHANGE,
+ ChangeField.CHERRY_PICK_OF_PATCHSET,
+ ChangeField.COMMENT,
+ ChangeField.COMMENTBY,
+ ChangeField.COMMIT,
+ ChangeField.COMMIT_MESSAGE,
+ ChangeField.COMMITTER,
+ ChangeField.DELETED,
+ ChangeField.DELTA,
+ ChangeField.DIRECTORY,
+ ChangeField.DRAFTBY,
+ ChangeField.EDITBY,
+ ChangeField.EXACT_AUTHOR,
+ ChangeField.EXACT_COMMIT,
+ ChangeField.EXACT_COMMITTER,
+ ChangeField.EXTENSION,
+ ChangeField.FILE_PART,
+ ChangeField.FOOTER,
+ ChangeField.FUZZY_HASHTAG,
+ ChangeField.GROUP,
+ ChangeField.HASHTAG,
+ ChangeField.HASHTAG_CASE_AWARE,
+ ChangeField.ID,
+ ChangeField.IS_PURE_REVERT,
+ ChangeField.IS_SUBMITTABLE,
+ ChangeField.LABEL,
+ ChangeField.LEGACY_ID_STR,
+ ChangeField.MERGE,
+ ChangeField.MERGEABLE,
+ ChangeField.MERGED_ON,
+ ChangeField.ONLY_EXTENSIONS,
+ ChangeField.OWNER,
+ ChangeField.PATCH_SET,
+ ChangeField.PATH,
+ ChangeField.PENDING_REVIEWER,
+ ChangeField.PENDING_REVIEWER_BY_EMAIL,
+ ChangeField.PRIVATE,
+ ChangeField.REF_STATE,
+ ChangeField.REF_STATE_PATTERN,
+ ChangeField.REVERT_OF,
+ ChangeField.REVIEWEDBY,
+ ChangeField.REVIEWER,
+ ChangeField.REVIEWER_BY_EMAIL,
+ ChangeField.STAR,
+ ChangeField.STARBY,
+ ChangeField.STARTED,
+ ChangeField.STORED_SUBMIT_RECORD_LENIENT,
+ ChangeField.STORED_SUBMIT_RECORD_STRICT,
+ ChangeField.STORED_SUBMIT_REQUIREMENTS,
+ ChangeField.SUBMIT_RECORD,
+ ChangeField.SUBMIT_RULE_RESULT,
+ ChangeField.TOTAL_COMMENT_COUNT,
+ ChangeField.TR,
+ ChangeField.UNRESOLVED_COMMENT_COUNT,
+ ChangeField.UPDATED,
+ ChangeField.UPLOADER,
+ ChangeField.WIP),
+ ImmutableList.of(
+ ChangeField.SUBMISSIONID_FIELD,
+ ChangeField.STATUS_FIELD,
+ ChangeField.PROJECT_FIELD,
+ ChangeField.REF_FIELD,
+ ChangeField.TOPIC_FIELD),
+ ImmutableList.of(
+ ChangeField.EXACT_TOPIC,
+ ChangeField.FUZZY_TOPIC,
+ ChangeField.SUBMISSIONID_SPEC,
+ ChangeField.STATUS_SPEC,
+ ChangeField.PROJECT_SPEC,
+ ChangeField.PROJECTS_SPEC,
+ ChangeField.REF_SPEC));
/**
* Added new field {@link ChangeField#PREFIX_HASHTAG} and {@link ChangeField#PREFIX_TOPIC} to
@@ -115,7 +124,7 @@
new Schema.Builder<ChangeData>()
.add(V74)
.add(ChangeField.PREFIX_HASHTAG)
- .add(ChangeField.PREFIX_TOPIC)
+ .addSearchSpecs(ChangeField.PREFIX_TOPIC)
.build();
/** Added new field {@link ChangeField#FOOTER_NAME}. */
diff --git a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
index 339d7bb..76610f3 100644
--- a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
+++ b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
@@ -16,7 +16,7 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.index.change.ChangeField.CHANGE;
-import static com.google.gerrit.server.index.change.ChangeField.PROJECT;
+import static com.google.gerrit.server.index.change.ChangeField.PROJECT_SPEC;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
@@ -69,9 +69,9 @@
int limit,
Set<String> fields) {
// Always include project since it is needed to load the change from NoteDb.
- if (!fields.contains(CHANGE.getName()) && !fields.contains(PROJECT.getName())) {
+ if (!fields.contains(CHANGE.getName()) && !fields.contains(PROJECT_SPEC.getName())) {
fields = new HashSet<>(fields);
- fields.add(PROJECT.getName());
+ fields.add(PROJECT_SPEC.getName());
}
return QueryOptions.create(config, start, pageSize, pageSizeMultiplier, limit, fields);
}
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
index 3e4e72d..d1bda5c 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffCacheImpl.java
@@ -97,7 +97,7 @@
persist(DIFF, FileDiffCacheKey.class, FileDiffOutput.class)
.maximumWeight(10 << 20)
.weigher(FileDiffWeigher.class)
- .version(8)
+ .version(9)
.keySerializer(FileDiffCacheKey.Serializer.INSTANCE)
.valueSerializer(FileDiffOutput.Serializer.INSTANCE)
.loader(FileDiffLoader.class);
@@ -443,6 +443,8 @@
.patchType(mainGitDiff.patchType())
.oldPath(mainGitDiff.oldPath())
.newPath(mainGitDiff.newPath())
+ .oldMode(mainGitDiff.oldMode())
+ .newMode(mainGitDiff.newMode())
.headerLines(FileHeaderUtil.getHeaderLines(mainGitDiff.fileHeader()))
.edits(asTaggedEdits(mainGitDiff.edits(), rebaseEdits))
.size(newSize)
diff --git a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
index 31fe77a..9286f47 100644
--- a/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
+++ b/java/com/google/gerrit/server/patch/filediff/FileDiffOutput.java
@@ -17,9 +17,12 @@
import static com.google.gerrit.server.patch.DiffUtil.stringSize;
import com.google.auto.value.AutoValue;
+import com.google.common.base.Converter;
+import com.google.common.base.Enums;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.entities.Patch.FileMode;
import com.google.gerrit.entities.Patch.PatchType;
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.cache.proto.Cache.FileDiffOutputProto;
@@ -61,6 +64,18 @@
*/
public abstract Optional<String> newPath();
+ /**
+ * The file mode of the old file at the old git tree diff identified by {@link #oldCommitId()}
+ * ()}.
+ */
+ public abstract Optional<Patch.FileMode> oldMode();
+
+ /**
+ * The file mode of the new file at the new git tree diff identified by {@link #newCommitId()}
+ * ()}.
+ */
+ public abstract Optional<Patch.FileMode> newMode();
+
/** The change type of the underlying file, e.g. added, deleted, renamed, etc... */
public abstract Patch.ChangeType changeType();
@@ -201,6 +216,10 @@
public abstract Builder newPath(Optional<String> value);
+ public abstract Builder oldMode(Optional<Patch.FileMode> oldMode);
+
+ public abstract Builder newMode(Optional<Patch.FileMode> newMode);
+
public abstract Builder changeType(ChangeType value);
public abstract Builder patchType(Optional<PatchType> value);
@@ -221,6 +240,9 @@
public enum Serializer implements CacheSerializer<FileDiffOutput> {
INSTANCE;
+ private static final Converter<String, FileMode> FILE_MODE_CONVERTER =
+ Enums.stringConverter(Patch.FileMode.class);
+
private static final FieldDescriptor OLD_PATH_DESCRIPTOR =
FileDiffOutputProto.getDescriptor().findFieldByNumber(1);
@@ -233,6 +255,12 @@
private static final FieldDescriptor NEGATIVE_DESCRIPTOR =
FileDiffOutputProto.getDescriptor().findFieldByNumber(12);
+ private static final FieldDescriptor OLD_MODE_DESCRIPTOR =
+ FileDiffOutputProto.getDescriptor().findFieldByNumber(13);
+
+ private static final FieldDescriptor NEW_MODE_DESCRIPTOR =
+ FileDiffOutputProto.getDescriptor().findFieldByNumber(14);
+
@Override
public byte[] serialize(FileDiffOutput fileDiff) {
ObjectIdConverter idConverter = ObjectIdConverter.create();
@@ -277,6 +305,13 @@
builder.setNegative(fileDiff.negative().get());
}
+ if (fileDiff.oldMode().isPresent()) {
+ builder.setOldMode(FILE_MODE_CONVERTER.reverse().convert(fileDiff.oldMode().get()));
+ }
+ if (fileDiff.newMode().isPresent()) {
+ builder.setNewMode(FILE_MODE_CONVERTER.reverse().convert(fileDiff.newMode().get()));
+ }
+
return Protos.toByteArray(builder.build());
}
@@ -318,6 +353,12 @@
if (proto.hasField(NEGATIVE_DESCRIPTOR)) {
builder.negative(Optional.of(proto.getNegative()));
}
+ if (proto.hasField(OLD_MODE_DESCRIPTOR)) {
+ builder.oldMode(Optional.of(FILE_MODE_CONVERTER.convert(proto.getOldMode())));
+ }
+ if (proto.hasField(NEW_MODE_DESCRIPTOR)) {
+ builder.newMode(Optional.of(FILE_MODE_CONVERTER.convert(proto.getNewMode())));
+ }
return builder.build();
}
}
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 47b0a53..654b3a4 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -977,6 +977,7 @@
Map<String, String> lowerNames = new HashMap<>();
submitRequirementSections = new LinkedHashMap<>();
for (String name : rc.getSubsections(SUBMIT_REQUIREMENT)) {
+ checkDuplicateSrDefinition(rc, name);
String lower = name.toLowerCase();
if (lowerNames.containsKey(lower)) {
error(
@@ -1034,6 +1035,40 @@
}
}
+ private void checkDuplicateSrDefinition(Config rc, String srName) {
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_DESCRIPTION).length > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_DESCRIPTION, srName));
+ }
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_APPLICABILITY_EXPRESSION).length > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_APPLICABILITY_EXPRESSION, srName));
+ }
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_SUBMITTABILITY_EXPRESSION).length > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_SUBMITTABILITY_EXPRESSION, srName));
+ }
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_OVERRIDE_EXPRESSION).length > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_OVERRIDE_EXPRESSION, srName));
+ }
+ if (rc.getStringList(SUBMIT_REQUIREMENT, srName, KEY_SR_OVERRIDE_IN_CHILD_PROJECTS).length
+ > 1) {
+ error(
+ String.format(
+ "Multiple definitions of %s for submit requirement '%s'",
+ KEY_SR_OVERRIDE_IN_CHILD_PROJECTS, srName));
+ }
+ }
+
/**
* Report unsupported submit requirement parameters as errors.
*
diff --git a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
index ccd4109..a897a8d 100644
--- a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.IndexPredicate;
import com.google.gerrit.index.query.Predicate;
@@ -32,11 +32,11 @@
return ChangeStatusPredicate.NONE;
}
- protected ChangeIndexPredicate(FieldDef<ChangeData, ?> def, String value) {
+ protected ChangeIndexPredicate(SchemaField<ChangeData, ?> def, String value) {
super(def, value);
}
- protected ChangeIndexPredicate(FieldDef<ChangeData, ?> def, String name, String value) {
+ protected ChangeIndexPredicate(SchemaField<ChangeData, ?> def, String name, String value) {
super(def, name, value);
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index 3363ad7..7dad123 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -170,12 +170,12 @@
* com.google.gerrit.entities.Project.NameKey}.
*/
public static Predicate<ChangeData> project(Project.NameKey id) {
- return new ChangeIndexPredicate(ChangeField.PROJECT, id.get());
+ return new ChangeIndexPredicate(ChangeField.PROJECT_SPEC, id.get());
}
/** Returns a predicate that matches changes targeted at the provided {@code refName}. */
public static Predicate<ChangeData> ref(String refName) {
- return new ChangeIndexPredicate(ChangeField.REF, refName);
+ return new ChangeIndexPredicate(ChangeField.REF_SPEC, refName);
}
/** Returns a predicate that matches changes in the provided {@code topic}. */
@@ -195,7 +195,7 @@
/** Returns a predicate that matches changes submitted in the provided {@code changeSet}. */
public static Predicate<ChangeData> submissionId(String changeSet) {
- return new ChangeIndexPredicate(ChangeField.SUBMISSIONID, changeSet);
+ return new ChangeIndexPredicate(ChangeField.SUBMISSIONID_SPEC, changeSet);
}
/** Returns a predicate that matches changes that modified the provided {@code path}. */
@@ -308,7 +308,7 @@
* its name.
*/
public static Predicate<ChangeData> projectPrefix(String prefix) {
- return new ChangeIndexPredicate(ChangeField.PROJECTS, prefix);
+ return new ChangeIndexPredicate(ChangeField.PROJECTS_SPEC, prefix);
}
/**
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index e99306a..91ec74c 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -42,9 +42,9 @@
import com.google.gerrit.exceptions.NotSignedInException;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.registration.DynamicMap;
-import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.SchemaUtil;
import com.google.gerrit.index.query.LimitPredicate;
import com.google.gerrit.index.query.Predicate;
@@ -1600,7 +1600,7 @@
return Predicate.or(predicates);
}
- protected void checkFieldAvailable(FieldDef<ChangeData, ?> field, String operator)
+ protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String operator)
throws QueryParseException {
if (!args.index.getSchema().hasField(field)) {
throw new QueryParseException(
diff --git a/java/com/google/gerrit/server/query/change/ChangeRegexPredicate.java b/java/com/google/gerrit/server/query/change/ChangeRegexPredicate.java
index 24b8b7a..d1c487e 100644
--- a/java/com/google/gerrit/server/query/change/ChangeRegexPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeRegexPredicate.java
@@ -14,17 +14,17 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.Matchable;
import com.google.gerrit.index.query.RegexPredicate;
public abstract class ChangeRegexPredicate extends RegexPredicate<ChangeData>
implements Matchable<ChangeData> {
- protected ChangeRegexPredicate(FieldDef<ChangeData, ?> def, String value) {
+ protected ChangeRegexPredicate(SchemaField<ChangeData, ?> def, String value) {
super(def, value);
}
- protected ChangeRegexPredicate(FieldDef<ChangeData, ?> def, String name, String value) {
+ protected ChangeRegexPredicate(SchemaField<ChangeData, ?> def, String name, String value) {
super(def, name, value);
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java b/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
index 9aff0c5..0dc923f 100644
--- a/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
@@ -104,7 +104,7 @@
@Nullable private final Change.Status status;
private ChangeStatusPredicate(@Nullable Change.Status status) {
- super(ChangeField.STATUS, status != null ? canonicalize(status) : INVALID_STATUS);
+ super(ChangeField.STATUS_SPEC, status != null ? canonicalize(status) : INVALID_STATUS);
this.status = status;
}
diff --git a/java/com/google/gerrit/server/query/change/RegexProjectPredicate.java b/java/com/google/gerrit/server/query/change/RegexProjectPredicate.java
index bbdfc66..a51dcc4 100644
--- a/java/com/google/gerrit/server/query/change/RegexProjectPredicate.java
+++ b/java/com/google/gerrit/server/query/change/RegexProjectPredicate.java
@@ -24,7 +24,7 @@
protected final RunAutomaton pattern;
public RegexProjectPredicate(String re) {
- super(ChangeField.PROJECT, re);
+ super(ChangeField.PROJECT_SPEC, re);
if (re.startsWith("^")) {
re = re.substring(1);
diff --git a/java/com/google/gerrit/server/query/change/RegexRefPredicate.java b/java/com/google/gerrit/server/query/change/RegexRefPredicate.java
index b2dba72..cc556ba 100644
--- a/java/com/google/gerrit/server/query/change/RegexRefPredicate.java
+++ b/java/com/google/gerrit/server/query/change/RegexRefPredicate.java
@@ -24,7 +24,7 @@
protected final RunAutomaton pattern;
public RegexRefPredicate(String re) throws QueryParseException {
- super(ChangeField.REF, re);
+ super(ChangeField.REF_SPEC, re);
if (re.startsWith("^")) {
re = re.substring(1);
diff --git a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
index 2580a1b..1e7a408 100644
--- a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server.query.change;
-import com.google.gerrit.index.FieldDef;
+import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryBuilder;
import com.google.gerrit.index.query.QueryParseException;
@@ -61,7 +61,7 @@
}
@Override
- protected void checkFieldAvailable(FieldDef<ChangeData, ?> field, String operator) {
+ protected void checkFieldAvailable(SchemaField<ChangeData, ?> field, String operator) {
// Submit requirements don't rely on the index, so they can be used regardless of index schema
// version.
}
diff --git a/java/com/google/gerrit/server/restapi/account/StarredChanges.java b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
index 12abf3d..173f24b 100644
--- a/java/com/google/gerrit/server/restapi/account/StarredChanges.java
+++ b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
@@ -131,10 +131,7 @@
try {
starredChangesUtil.star(
- self.get().getAccountId(),
- change.getProject(),
- change.getId(),
- StarredChangesUtil.Operation.ADD);
+ self.get().getAccountId(), change.getId(), StarredChangesUtil.Operation.ADD);
} catch (MutuallyExclusiveLabelsException e) {
throw new ResourceConflictException(e.getMessage());
} catch (IllegalLabelException e) {
@@ -182,10 +179,7 @@
throw new AuthException("not allowed remove starred change");
}
starredChangesUtil.star(
- self.get().getAccountId(),
- rsrc.getChange().getProject(),
- rsrc.getChange().getId(),
- StarredChangesUtil.Operation.REMOVE);
+ self.get().getAccountId(), rsrc.getChange().getId(), StarredChangesUtil.Operation.REMOVE);
return Response.none();
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index 00c48dc..d0113e5 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -527,7 +527,7 @@
reviewers.remove(user.get().getAccountId());
Set<Account.Id> ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC));
ccs.remove(user.get().getAccountId());
- ins.setReviewersAndCcs(reviewers, ccs);
+ ins.setReviewersAndCcsIgnoreVisibility(reviewers, ccs);
}
// If there is a base, and the base is not merged, the groups will be overridden by the base's
// groups.
diff --git a/java/com/google/gerrit/server/restapi/config/GetSummary.java b/java/com/google/gerrit/server/restapi/config/GetSummary.java
index d0a1498..23b7a80 100644
--- a/java/com/google/gerrit/server/restapi/config/GetSummary.java
+++ b/java/com/google/gerrit/server/restapi/config/GetSummary.java
@@ -90,14 +90,22 @@
private TaskSummaryInfo getTaskSummary() {
Collection<Task<?>> pending = workQueue.getTasks();
int tasksTotal = pending.size();
+ int tasksStopping = 0;
int tasksRunning = 0;
+ int tasksStarting = 0;
int tasksReady = 0;
int tasksSleeping = 0;
for (Task<?> task : pending) {
switch (task.getState()) {
+ case STOPPING:
+ tasksStopping++;
+ break;
case RUNNING:
tasksRunning++;
break;
+ case STARTING:
+ tasksStarting++;
+ break;
case READY:
tasksReady++;
break;
@@ -113,7 +121,9 @@
TaskSummaryInfo taskSummary = new TaskSummaryInfo();
taskSummary.total = toInteger(tasksTotal);
+ taskSummary.stopping = toInteger(tasksStopping);
taskSummary.running = toInteger(tasksRunning);
+ taskSummary.starting = toInteger(tasksStarting);
taskSummary.ready = toInteger(tasksReady);
taskSummary.sleeping = toInteger(tasksSleeping);
return taskSummary;
@@ -247,7 +257,9 @@
public static class TaskSummaryInfo {
public Integer total;
+ public Integer stopping;
public Integer running;
+ public Integer starting;
public Integer ready;
public Integer sleeping;
}
diff --git a/java/com/google/gerrit/sshd/commands/ShowQueue.java b/java/com/google/gerrit/sshd/commands/ShowQueue.java
index 4254e5b..00361ad 100644
--- a/java/com/google/gerrit/sshd/commands/ShowQueue.java
+++ b/java/com/google/gerrit/sshd/commands/ShowQueue.java
@@ -134,7 +134,9 @@
switch (task.state) {
case DONE:
case CANCELLED:
+ case STARTING:
case RUNNING:
+ case STOPPING:
case READY:
start = format(task.state);
break;
@@ -204,8 +206,12 @@
return "....... done";
case CANCELLED:
return "..... killed";
+ case STOPPING:
+ return "... stopping";
case RUNNING:
return "";
+ case STARTING:
+ return "starting ...";
case READY:
return "waiting ....";
case SLEEPING:
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index b00cadb..781965e 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -79,6 +79,7 @@
import com.google.gerrit.server.git.PerThreadRequestScope;
import com.google.gerrit.server.git.SearchingChangeCacheImpl.SearchingChangeCacheImplModule;
import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.git.WorkQueue.WorkQueueModule;
import com.google.gerrit.server.group.testing.TestGroupBackend;
import com.google.gerrit.server.index.account.AccountSchemaDefinitions;
import com.google.gerrit.server.index.account.AllAccountsIndexer;
@@ -195,6 +196,7 @@
install(new AuditModule());
install(new SubscriptionGraphModule());
install(new SuperprojectUpdateSubmissionListenerModule());
+ install(new WorkQueueModule());
bindScope(RequestScoped.class, PerThreadRequestScope.REQUEST);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
index 90ca047..93ac038 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -26,6 +26,7 @@
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
@@ -424,6 +425,46 @@
}
@Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void revertWithNonVisibleUsers() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount reverter = user;
+ TestAccount changeOwner = admin; // must be admin, since admin cloned testRepo
+ TestAccount reviewer = accountCreator.user2();
+ TestAccount cc =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the reverter can neither see the changeOwner, the reviewer nor the cc.
+ requestScopeOperations.setApiUser(reverter.id());
+ assertThatAccountIsNotVisible(changeOwner, reviewer, cc);
+
+ // Create the change.
+ requestScopeOperations.setApiUser(changeOwner.id());
+ PushOneCommit.Result r = createChange();
+
+ // Add reviewer and cc.
+ ReviewInput reviewerInput = ReviewInput.approve();
+ reviewerInput.reviewer(reviewer.email());
+ reviewerInput.cc(cc.email());
+ gApi.changes().id(r.getChangeId()).current().review(reviewerInput);
+
+ // Approve and submit the change.
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // Revert the change.
+ requestScopeOperations.setApiUser(reverter.id());
+ String revertChangeId = gApi.changes().id(r.getChangeId()).revert().get().id;
+
+ // Revert doesn't check the reviewer/CC visibility. Since the reverter can see the reverted
+ // change, they can also see its reviewers/CCs. This means preserving them on the revert change
+ // doesn't expose their account existence and it's OK to keep them even if their accounts are
+ // not visible to the reverter.
+ assertReviewers(revertChangeId, changeOwner, reviewer);
+ assertCcs(revertChangeId, cc);
+ }
+
+ @Test
@TestProjectInput(createEmptyCommit = false)
public void revertInitialCommit() throws Exception {
PushOneCommit.Result r = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
index 168819c..52207db 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
@@ -25,6 +25,8 @@
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.entities.LabelFunction;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmitRequirement;
+import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -36,6 +38,7 @@
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Test;
public class ProjectConfigIT extends AbstractDaemonTest {
@@ -389,6 +392,168 @@
}
@Test
+ public void rejectSubmitRequirement_duplicateDescriptionKeys() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " description = description 1\n "
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n"
+ + " description = description 2\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of description"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectSubmitRequirement_duplicateApplicableIfKeys() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n "
+ + " applicableIf = is:true\n "
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n"
+ + " applicableIf = is:false\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of applicableif"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectSubmitRequirement_duplicateSubmittableIfKeys() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n"
+ + " submittableIf = label:Code-Review=MIN\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of submittableif"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectSubmitRequirement_duplicateOverrideIfKeys() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " overrideIf = is:true\n "
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n"
+ + " overrideIf = is:false\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of overrideif"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void rejectSubmitRequirement_duplicateCanOverrideInChildProjectsKey() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " canOverrideInChildProjects = true\n"
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n "
+ + " canOverrideInChildProjects = false\n");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: project.config: multiple definitions of canoverrideinchildprojects"
+ + " for submit requirement 'foo'",
+ abbreviateName(r.getCommit())));
+ }
+
+ @Test
+ public void submitRequirementsAreParsed_forExistingDuplicateDefinitions() throws Exception {
+ // Duplicate submit requirement definitions are rejected on config change uploads. For setups
+ // already containing duplicate SR definitions, the server is able to parse the "submit
+ // requirements correctly"
+
+ RevCommit revision;
+ // Commit a change to the project config, bypassing server validation.
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(project))) {
+ revision =
+ testRepo
+ .branch(RefNames.REFS_CONFIG)
+ .commit()
+ .add(
+ ProjectConfig.PROJECT_CONFIG,
+ "[submit-requirement \"Foo\"]\n"
+ + " canOverrideInChildProjects = true\n"
+ + " submittableIf = label:Code-Review=MAX\n"
+ + "[submit-requirement \"Foo\"]\n "
+ + " canOverrideInChildProjects = false\n")
+ .parent(projectOperations.project(project).getHead(RefNames.REFS_CONFIG))
+ .create();
+ }
+
+ try (Repository git = repoManager.openRepository(project)) {
+ // Server is able to parse the config.
+ ProjectConfig cfg = projectConfigFactory.create(project);
+ cfg.load(git, revision);
+
+ // One of the two definitions takes precedence and overrides the other.
+ assertThat(cfg.getSubmitRequirementSections())
+ .containsExactly(
+ "Foo",
+ SubmitRequirement.builder()
+ .setName("Foo")
+ .setAllowOverrideInChildProjects(false)
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX"))
+ .build());
+ }
+ }
+
+ @Test
public void testRejectChangingLabelFunction_toMaxWithBlock() throws Exception {
testChangingLabelFunction(
/* initialLabelFunction= */ LabelFunction.NO_BLOCK,
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index 1919810..fca2253 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -227,6 +227,66 @@
}
@Test
+ public void fileModeChangeIsIncludedInListFilesDiff() throws Exception {
+ String fileName = "file.txt";
+ PushOneCommit push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, "Commit Subject", /* files= */ ImmutableMap.of())
+ .addFile(fileName, "content", /* fileMode= */ 0100644);
+ PushOneCommit.Result result = push.to("refs/for/master");
+ String commitRev1 = gApi.changes().id(result.getChangeId()).get().currentRevision;
+ push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, result.getChangeId())
+ .addFile(fileName, "content", /* fileMode= */ 0100755);
+ result = push.to("refs/for/master");
+ String commitRev2 = gApi.changes().id(result.getChangeId()).get().currentRevision;
+
+ Map<String, FileInfo> changedFiles =
+ gApi.changes().id(result.getChangeId()).revision(commitRev2).files(commitRev1);
+
+ assertThat(changedFiles.get(fileName)).oldMode().isEqualTo(0100644);
+ assertThat(changedFiles.get(fileName)).newMode().isEqualTo(0100755);
+ }
+
+ @Test
+ public void fileMode_oldMode_isMissingInListFilesDiff_forAddedFile() throws Exception {
+ String fileName = "file.txt";
+ PushOneCommit push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, "Commit Subject", /* files= */ ImmutableMap.of())
+ .addFile(fileName, "content", /* fileMode= */ 0100644);
+ PushOneCommit.Result result = push.to("refs/for/master");
+ String commitRev = gApi.changes().id(result.getChangeId()).get().currentRevision;
+
+ Map<String, FileInfo> changedFiles =
+ gApi.changes().id(result.getChangeId()).revision(commitRev).files();
+
+ assertThat(changedFiles.get(fileName)).oldMode().isNull();
+ assertThat(changedFiles.get(fileName)).newMode().isEqualTo(0100644);
+ }
+
+ @Test
+ public void fileMode_newMode_isMissingInListFilesDiff_forDeletedFile() throws Exception {
+ String fileName = "file.txt";
+ PushOneCommit push =
+ pushFactory
+ .create(admin.newIdent(), testRepo, "Commit Subject", /* files= */ ImmutableMap.of())
+ .addFile(fileName, "content", /* fileMode= */ 0100644);
+ PushOneCommit.Result result = push.to("refs/for/master");
+ String commitRev1 = gApi.changes().id(result.getChangeId()).get().currentRevision;
+ push = pushFactory.create(admin.newIdent(), testRepo, result.getChangeId()).rmFile(fileName);
+ result = push.to("refs/for/master");
+ String commitRev2 = gApi.changes().id(result.getChangeId()).get().currentRevision;
+
+ Map<String, FileInfo> changedFiles =
+ gApi.changes().id(result.getChangeId()).revision(commitRev2).files(commitRev1);
+
+ assertThat(changedFiles.get(fileName)).oldMode().isEqualTo(0100644);
+ assertThat(changedFiles.get(fileName)).newMode().isNull();
+ }
+
+ @Test
public void numberOfLinesInDiffOfDeletedFileWithoutNewlineAtEndIsCorrect() throws Exception {
String filePath = "a_new_file.txt";
String fileContent = "Line 1\nLine 2\nLine 3";
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 2c80333..8d64cf7 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -1009,6 +1009,84 @@
}
@Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void cherryPickWithNonVisibleUsers() throws Exception {
+ // Create a target branch for the cherry-pick.
+ createBranch(BranchNameKey.create(project, "stable"));
+
+ // Define readable names for the users we use in this test.
+ TestAccount cherryPicker = user;
+ TestAccount changeOwner = admin;
+ TestAccount reviewer = accountCreator.user2();
+ TestAccount cc =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+ TestAccount authorCommitter =
+ accountCreator.create("user4", "user4@example.com", "User4", /* displayName= */ null);
+
+ // Check that the cherry-picker can neither see the changeOwner, the reviewer, the cc nor the
+ // authorCommitter.
+ requestScopeOperations.setApiUser(cherryPicker.id());
+ assertThatAccountIsNotVisible(changeOwner, reviewer, cc, authorCommitter);
+
+ // Create the change with authorCommitter as the author and the committer.
+ requestScopeOperations.setApiUser(changeOwner.id());
+ PushOneCommit push = pushFactory.create(authorCommitter.newIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ // Check that authorCommitter was set as the author and committer.
+ ChangeInfo changeInfo = gApi.changes().id(r.getChangeId()).get();
+ CommitInfo commit = changeInfo.revisions.get(changeInfo.currentRevision).commit;
+ assertThat(commit.author.email).isEqualTo(authorCommitter.email());
+ assertThat(commit.committer.email).isEqualTo(authorCommitter.email());
+
+ // Pushing a commit with a forged author/committer adds the author/committer as a CC.
+ assertCcs(r.getChangeId(), authorCommitter);
+
+ // Remove the author/committer as a CC because because otherwise there are two signals for CCing
+ // authorCommitter on the cherry-pick change: once because they are author and committer and
+ // once because they are a CC. For authorCommitter we only want to test the first signal here
+ // (the second signal is covered by adding an explicit CC below).
+ gApi.changes().id(r.getChangeId()).reviewer(authorCommitter.email()).remove();
+ assertNoCcs(r.getChangeId());
+
+ // Add reviewer and cc.
+ ReviewInput reviewerInput = ReviewInput.approve();
+ reviewerInput.reviewer(reviewer.email());
+ reviewerInput.cc(cc.email());
+ gApi.changes().id(r.getChangeId()).current().review(reviewerInput);
+
+ // Approve and submit the change.
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).current().submit();
+
+ // Cherry-pick the change.
+ requestScopeOperations.setApiUser(cherryPicker.id());
+ CherryPickInput cherryPickInput = new CherryPickInput();
+ cherryPickInput.message = "Cherry-pick to stable branch";
+ cherryPickInput.destination = "stable";
+ cherryPickInput.keepReviewers = true;
+ String cherryPickChangeId =
+ gApi.changes().id(r.getChangeId()).current().cherryPick(cherryPickInput).get().id;
+
+ // Cherry-pick doesn't check the visibility of explicit reviewers/CCs. Since the cherry-picker
+ // can see the cherry-picked change, they can also see its reviewers/CCs. This means preserving
+ // them on the cherry-pick change doesn't expose their account existence and it's OK to keep
+ // them even if their accounts are not visible to the cherry-picker.
+ // In contrast to this for implicit CCs that are added for the author/committer the account
+ // visibility is checked, but if their accounts are not visible the CC is silently dropped (so
+ // that the cherry-pick request can still succeed). Since in this case authorCommitter is not
+ // visible, we expect that CCing them is being dropped and hence authorCommitter is not returned
+ // as a CC here. The reason that the visibility for author/committer must be checked is that
+ // author/committer may not match a Gerrit account (if they are forged). This means by seeing
+ // the author/committer on the cherry-picked change, it's not possible to deduce that these
+ // Gerrit accounts exists, but if they would be added as a CC on the cherry-pick change even if
+ // they are not visible the account existence would be exposed.
+ assertReviewers(cherryPickChangeId, changeOwner, reviewer);
+ assertCcs(cherryPickChangeId, cc);
+ }
+
+ @Test
public void cherryPickToMergedChangeRevision() throws Exception {
createBranch(BranchNameKey.create(project, "foo"));
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index cd1d911..3b94a50 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -128,6 +128,7 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Stream;
import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
@@ -1154,7 +1155,7 @@
.add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
.message(PushOneCommit.SUBJECT)
.create();
- // Push commit as "Admnistrator".
+ // Push commit as "Administrator".
pushHead(testRepo, "refs/for/master");
String changeId = GitUtil.getChangeId(testRepo, c).get();
@@ -1168,6 +1169,71 @@
}
@Test
+ public void pushForMasterWithNonExistingForgedAuthorAndCommitter() throws Exception {
+ // Create a commit with different forged author and committer.
+ RevCommit c =
+ commitBuilder()
+ .author(new PersonIdent("author", "author@example.com"))
+ .committer(new PersonIdent("committer", "committer@example.com"))
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
+ .message(PushOneCommit.SUBJECT)
+ .create();
+ // Push commit as "Administrator".
+ pushHead(testRepo, "refs/for/master");
+
+ String changeId = GitUtil.getChangeId(testRepo, c).get();
+ assertThat(getOwnerEmail(changeId)).isEqualTo(admin.email());
+ assertThat(getReviewerEmails(changeId, ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void pushForMasterWithNonVisibleForgedAuthorAndCommitter() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount uploader = user; // cannot use admin since admin can see all users
+ TestAccount author = accountCreator.user2();
+ TestAccount committer =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the uploader can neither see the author nor the committer.
+ requestScopeOperations.setApiUser(uploader.id());
+ assertThatAccountIsNotVisible(author, committer);
+
+ // Allow the uploader to forge author and committer.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.FORGE_AUTHOR).ref("refs/heads/master").group(REGISTERED_USERS))
+ .add(allow(Permission.FORGE_COMMITTER).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ // Clone the repo as uploader so that the push is done by the uplaoder.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project, uploader);
+
+ // Create a commit with different forged author and committer.
+ RevCommit c =
+ testRepo
+ .branch("HEAD")
+ .commit()
+ .insertChangeId()
+ .author(author.newIdent())
+ .committer(committer.newIdent())
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
+ .message(PushOneCommit.SUBJECT)
+ .create();
+
+ PushResult r = pushHead(testRepo, "refs/for/master");
+ RemoteRefUpdate refUpdate = r.getRemoteUpdate("refs/for/master");
+ assertThat(refUpdate.getStatus()).isEqualTo(RemoteRefUpdate.Status.OK);
+
+ String changeId = GitUtil.getChangeId(testRepo, c).get();
+ assertThat(getOwnerEmail(changeId)).isEqualTo(uploader.email());
+
+ // author and committer have not been CCed because their accounts are not visible
+ assertThat(getReviewerEmails(changeId, ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
public void pushNewPatchSetForMasterWithForgedAuthorAndCommitter() throws Exception {
TestAccount user2 = accountCreator.user2();
// First patch set has author and committer matching change owner.
@@ -1192,6 +1258,74 @@
.containsExactly(user.getNameEmail(), user2.getNameEmail());
}
+ @Test
+ public void pushNewPatchSetForMasterWithNonExistingForgedAuthorAndCommitter() throws Exception {
+ // First patch set has author and committer matching change owner.
+ PushOneCommit.Result r = pushTo("refs/for/master");
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(admin.email());
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.REVIEWER)).isEmpty();
+
+ amendBuilder()
+ .author(new PersonIdent("author", "author@example.com"))
+ .committer(new PersonIdent("committer", "committer@example.com"))
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT + "2")
+ .create();
+ pushHead(testRepo, "refs/for/master");
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(admin.email());
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.CC)).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void pushNewPatchSetForMasterWithNonVisibleForgedAuthorAndCommitter() throws Exception {
+ // Define readable names for the users we use in this test.
+ TestAccount uploader = user; // cannot use admin since admin can see all users
+ TestAccount author = accountCreator.user2();
+ TestAccount committer =
+ accountCreator.create("user3", "user3@example.com", "User3", /* displayName= */ null);
+
+ // Check that the uploader can neither see the author nor the committer.
+ requestScopeOperations.setApiUser(uploader.id());
+ assertThatAccountIsNotVisible(author, committer);
+
+ // Allow the uploader to forge author and committer.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.FORGE_AUTHOR).ref("refs/heads/master").group(REGISTERED_USERS))
+ .add(allow(Permission.FORGE_COMMITTER).ref("refs/heads/master").group(REGISTERED_USERS))
+ .update();
+
+ // Clone the repo as uploader so that the push is done by the uplaoder.
+ TestRepository<InMemoryRepository> testRepo = cloneProject(project, uploader);
+
+ // First patch set has author and committer matching uploader.
+ PushOneCommit push = pushFactory.create(uploader.newIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(uploader.email());
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.REVIEWER)).isEmpty();
+
+ testRepo
+ .amendRef("HEAD")
+ .author(author.newIdent())
+ .committer(committer.newIdent())
+ .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT + "2")
+ .create();
+
+ PushResult r2 = pushHead(testRepo, "refs/for/master");
+ RemoteRefUpdate refUpdate = r2.getRemoteUpdate("refs/for/master");
+ assertThat(refUpdate.getStatus()).isEqualTo(RemoteRefUpdate.Status.OK);
+
+ assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(uploader.email());
+
+ // author and committer have not been CCed because their accounts are not visible
+ assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.CC)).isEmpty();
+ }
+
/**
* There was a bug that allowed a user with Forge Committer Identity access right to upload a
* commit and put *votes on behalf of another user* on it. This test checks that this is not
diff --git a/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
new file mode 100644
index 0000000..23f2609
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
@@ -0,0 +1,285 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.server.util;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.git.WorkQueue.Task;
+import com.google.gerrit.server.git.WorkQueue.TaskListener;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import org.junit.Before;
+import org.junit.Test;
+
+public class TaskListenerIT extends AbstractDaemonTest {
+ /**
+ * Use a LatchedMethod in a method to allow another thread to await the method's call. Once
+ * called, the Latch.call() method will block until another thread calls its LatchedMethods's
+ * complete() method.
+ */
+ private static class LatchedMethod {
+ private static final int AWAIT_TIMEOUT = 20;
+ private static final TimeUnit AWAIT_TIMEUNIT = TimeUnit.MILLISECONDS;
+
+ /** API class meant be used by the class whose method is being latched */
+ private class Latch {
+ /** Ensure that the latched method calls this on entry */
+ public void call() {
+ called.countDown();
+ await(complete);
+ }
+ }
+
+ public Latch latch = new Latch();
+
+ private final CountDownLatch called = new CountDownLatch(1);
+ private final CountDownLatch complete = new CountDownLatch(1);
+
+ /** Assert that the Latch's call() method has not yet been called */
+ public void assertUncalled() {
+ assertThat(called.getCount()).isEqualTo(1);
+ }
+
+ /**
+ * Assert that a timeout does not occur while awaiting Latch's call() method to be called. Fails
+ * if the waiting time elapses before Latch's call() method is called, otherwise passes.
+ */
+ public void assertAwait() {
+ assertThat(await(called)).isEqualTo(true);
+ }
+
+ /** Unblock the Latch's call() method so that it can complete */
+ public void complete() {
+ complete.countDown();
+ }
+
+ @CanIgnoreReturnValue
+ private static boolean await(CountDownLatch latch) {
+ try {
+ return latch.await(AWAIT_TIMEOUT, AWAIT_TIMEUNIT);
+ } catch (InterruptedException e) {
+ return false;
+ }
+ }
+ }
+
+ private static class LatchedRunnable implements Runnable {
+ public LatchedMethod run = new LatchedMethod();
+
+ @Override
+ public void run() {
+ run.latch.call();
+ }
+ }
+
+ private static class ForwardingListener implements TaskListener {
+ public volatile TaskListener delegate;
+ public volatile Task task;
+
+ public void resetDelegate(TaskListener listener) {
+ delegate = listener;
+ task = null;
+ }
+
+ @Override
+ public void onStart(Task<?> task) {
+ if (delegate != null) {
+ if (this.task == null || this.task == task) {
+ this.task = task;
+ delegate.onStart(task);
+ }
+ }
+ }
+
+ @Override
+ public void onStop(Task<?> task) {
+ if (delegate != null) {
+ if (this.task == task) {
+ delegate.onStop(task);
+ }
+ }
+ }
+ }
+
+ private static class LatchedListener implements TaskListener {
+ public LatchedMethod onStart = new LatchedMethod();
+ public LatchedMethod onStop = new LatchedMethod();
+
+ @Override
+ public void onStart(Task<?> task) {
+ onStart.latch.call();
+ }
+
+ @Override
+ public void onStop(Task<?> task) {
+ onStop.latch.call();
+ }
+ }
+
+ private static ForwardingListener forwarder;
+
+ @Inject private WorkQueue workQueue;
+ private ScheduledExecutorService executor;
+
+ private final LatchedListener listener = new LatchedListener();
+ private final LatchedRunnable runnable = new LatchedRunnable();
+
+ @Override
+ public Module createModule() {
+ return new AbstractModule() {
+ @Override
+ public void configure() {
+ // Forwarder.delegate is empty on start to protect test listener from non test tasks
+ // (such as the "Log File Compressor") interference
+ forwarder = new ForwardingListener(); // Only gets bound once for all tests
+ bind(TaskListener.class).annotatedWith(Exports.named("listener")).toInstance(forwarder);
+ }
+ };
+ }
+
+ @Before
+ public void setupExecutorAndForwarder() throws InterruptedException {
+ executor = workQueue.createQueue(1, "TaskListeners");
+
+ // "Log File Compressor"s are likely running and will interfere with tests
+ while (0 != workQueue.getTasks().size()) {
+ for (Task<?> t : workQueue.getTasks()) {
+ @SuppressWarnings("unused")
+ boolean unused = t.cancel(true);
+ }
+ TimeUnit.MILLISECONDS.sleep(1);
+ }
+
+ forwarder.resetDelegate(listener);
+
+ assertQueueSize(0);
+ assertThat(forwarder.task).isEqualTo(null);
+ listener.onStart.assertUncalled();
+ runnable.run.assertUncalled();
+ listener.onStop.assertUncalled();
+ }
+
+ @Test
+ public void onStartThenRunThenOnStopAreCalled() throws Exception {
+ int size = assertQueueBlockedOnExecution(runnable);
+
+ // onStartThenRunThenOnStopAreCalled -> onStart...Called
+ listener.onStart.assertAwait();
+ assertQueueSize(size);
+ runnable.run.assertUncalled();
+ listener.onStop.assertUncalled();
+
+ listener.onStart.complete();
+ // onStartThenRunThenOnStopAreCalled -> ...ThenRun...Called
+ runnable.run.assertAwait();
+ listener.onStop.assertUncalled();
+
+ runnable.run.complete();
+ // onStartThenRunThenOnStopAreCalled -> ...ThenOnStop...Called
+ listener.onStop.assertAwait();
+ assertQueueSize(size);
+
+ listener.onStop.complete();
+ assertAwaitQueueSize(--size);
+ }
+
+ @Test
+ public void firstBlocksSecond() throws Exception {
+ int size = assertQueueBlockedOnExecution(runnable);
+
+ // firstBlocksSecond -> first...
+ listener.onStart.assertAwait();
+ assertQueueSize(size);
+
+ LatchedRunnable runnable2 = new LatchedRunnable();
+ size = assertQueueBlockedOnExecution(runnable2);
+
+ // firstBlocksSecond -> ...BlocksSecond
+ runnable2.run.assertUncalled();
+ assertQueueSize(size); // waiting on first
+
+ listener.onStart.complete();
+ runnable.run.assertAwait();
+ assertQueueSize(size); // waiting on first
+ runnable2.run.assertUncalled();
+
+ runnable.run.complete();
+ listener.onStop.assertAwait();
+ assertQueueSize(size); // waiting on first
+ runnable2.run.assertUncalled();
+
+ listener.onStop.complete();
+ runnable2.run.assertAwait();
+ assertQueueSize(--size);
+
+ runnable2.run.complete();
+ assertAwaitQueueSize(--size);
+ }
+
+ @Test
+ public void states() throws Exception {
+ executor.execute(runnable);
+ listener.onStart.assertAwait();
+ assertStateIs(Task.State.STARTING);
+
+ listener.onStart.complete();
+ runnable.run.assertAwait();
+ assertStateIs(Task.State.RUNNING);
+
+ runnable.run.complete();
+ listener.onStop.assertAwait();
+ assertStateIs(Task.State.STOPPING);
+
+ listener.onStop.complete();
+ assertAwaitQueueIsEmpty();
+ assertStateIs(Task.State.DONE);
+ }
+
+ private void assertStateIs(Task.State state) {
+ assertThat(forwarder.task.getState()).isEqualTo(state);
+ }
+
+ private int assertQueueBlockedOnExecution(Runnable runnable) {
+ int expectedSize = workQueue.getTasks().size() + 1;
+ executor.execute(runnable);
+ assertQueueSize(expectedSize);
+ return expectedSize;
+ }
+
+ private void assertQueueSize(int size) {
+ assertThat(workQueue.getTasks().size()).isEqualTo(size);
+ }
+
+ private void assertAwaitQueueIsEmpty() throws InterruptedException {
+ assertAwaitQueueSize(0);
+ }
+
+ /** Fails if the waiting time elapses before the count is reached, otherwise passes */
+ private void assertAwaitQueueSize(int size) throws InterruptedException {
+ long i = 0;
+ do {
+ TimeUnit.NANOSECONDS.sleep(10);
+ assertThat(i++).isLessThan(100);
+ } while (size != workQueue.getTasks().size());
+ }
+}
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
index c5e8574..00272112 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/entities/FileDiffOutputSerializerTest.java
@@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.entities.Patch.FileMode;
import com.google.gerrit.entities.Patch.PatchType;
import com.google.gerrit.server.patch.ComparisonType;
import com.google.gerrit.server.patch.filediff.Edit;
@@ -42,6 +43,8 @@
.comparisonType(ComparisonType.againstOtherPatchSet())
.oldPath(Optional.of("old_file_path.txt"))
.newPath(Optional.empty())
+ .oldMode(Optional.of(FileMode.REGULAR_FILE))
+ .newMode(Optional.of(FileMode.SYMLINK))
.changeType(ChangeType.DELETED)
.patchType(Optional.of(PatchType.UNIFIED))
.size(23)
diff --git a/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java b/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java
index f70c97a..451126c 100644
--- a/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java
+++ b/javatests/com/google/gerrit/server/index/change/FakeChangeIndex.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.index.SchemaUtil.schema;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Change;
import com.google.gerrit.index.QueryOptions;
import com.google.gerrit.index.Schema;
@@ -29,10 +30,19 @@
@Ignore
public class FakeChangeIndex implements ChangeIndex {
- static final Schema<ChangeData> V1 = schema(1, ChangeField.STATUS);
+ static final Schema<ChangeData> V1 =
+ schema(
+ 1,
+ ImmutableList.of(),
+ ImmutableList.of(ChangeField.STATUS_FIELD),
+ ImmutableList.of(ChangeField.STATUS_SPEC));
static final Schema<ChangeData> V2 =
- schema(2, ChangeField.STATUS, ChangeField.PATH, ChangeField.UPDATED);
+ schema(
+ 2,
+ ImmutableList.of(ChangeField.PATH, ChangeField.UPDATED),
+ ImmutableList.of(ChangeField.STATUS_FIELD),
+ ImmutableList.of(ChangeField.STATUS_SPEC));
private static class Source implements ChangeDataSource {
private final Predicate<ChangeData> p;
diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
index d69fe9e..262fb9d9 100644
--- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
@@ -55,7 +55,6 @@
@Test
@UseClockStep
- @SuppressWarnings("unchecked")
public void stopQueryIfNoMoreResults() throws Exception {
// create 2 visible changes
TestRepository<InMemoryRepositoryManager.Repo> testRepo = createProject("repo");
@@ -72,7 +71,8 @@
.add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
.update();
- AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex();
+ AbstractFakeIndex<?, ?, ?> idx =
+ (AbstractFakeIndex<?, ?, ?>) changeIndexCollection.getSearchIndex();
newQuery("status:new").withLimit(5).get();
// Since the limit of the query (i.e. 5) is more than the total number of changes (i.e. 4),
// only 1 index search is expected.
@@ -81,7 +81,6 @@
@Test
@UseClockStep
- @SuppressWarnings("unchecked")
public void noLimitQueryPaginates() throws Exception {
TestRepository<InMemoryRepositoryManager.Repo> testRepo = createProject("repo");
// create 4 changes
@@ -97,7 +96,8 @@
.add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, 2))
.update();
- AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex();
+ AbstractFakeIndex<?, ?, ?> idx =
+ (AbstractFakeIndex<?, ?, ?>) changeIndexCollection.getSearchIndex();
// 2 index searches are expected. The first index search will run with size 3 (i.e.
// the configured query-limit+1), and then we will paginate to get the remaining
diff --git a/plugins/yarn.lock b/plugins/yarn.lock
index e012bd1..ec05cad 100644
--- a/plugins/yarn.lock
+++ b/plugins/yarn.lock
@@ -10,9 +10,9 @@
"@babel/highlight" "^7.18.6"
"@babel/helper-validator-identifier@^7.18.6":
- version "7.18.6"
- resolved "https://registry.yarnpkg.com/@babel/helper-validator-identifier/-/helper-validator-identifier-7.18.6.tgz#9c97e30d31b2b8c72a1d08984f2ca9b574d7a076"
- integrity sha512-MmetCkz9ej86nJQV+sFCxoGGrUbU3q02kgLciwkrt9QqEB7cP39oKEY0PakknEO0Gu20SskMRi+AYZ3b1TpN9g==
+ version "7.19.1"
+ resolved "https://registry.yarnpkg.com/@babel/helper-validator-identifier/-/helper-validator-identifier-7.19.1.tgz#7eea834cf32901ffdc1a7ee555e2f9c27e249ca2"
+ integrity sha512-awrNfaMtnHUr653GgGEs++LlAvW6w+DcPrOliSMXWCKo597CwL5Acf/wWdNkf/tfEQE3mjkeD1YOVZOUV/od1w==
"@babel/highlight@^7.18.6":
version "7.18.6"
@@ -35,16 +35,11 @@
resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.7.0.tgz#ae3886b5c4ddc6a02659a11d577e1df0b6158727"
integrity sha512-8zeZClN1gur+Isrn02bRXJ0wUjYvK99jQxg36ZbDelrGDglXKddf8QQkZmaH9sYIRcCFDLlh5+ZlRUTcXTuDVA==
-"@lit/reactive-element@^1.0.0", "@lit/reactive-element@^1.4.0":
+"@lit/reactive-element@^1.0.0", "@lit/reactive-element@^1.3.0", "@lit/reactive-element@^1.4.0":
version "1.4.1"
resolved "https://registry.yarnpkg.com/@lit/reactive-element/-/reactive-element-1.4.1.tgz#3f587eec5708692135bc9e94cf396130604979f3"
integrity sha512-qDv4851VFSaBWzpS02cXHclo40jsbAjRXnebNXpm0uVg32kCneZPo9RYVQtrTNICtZ+1wAYHu1ZtxWSWMbKrBw==
-"@lit/reactive-element@^1.3.0":
- version "1.3.2"
- resolved "https://registry.yarnpkg.com/@lit/reactive-element/-/reactive-element-1.3.2.tgz#43e470537b6ec2c23510c07812616d5aa27a17cd"
- integrity sha512-A2e18XzPMrIh35nhIdE4uoqRzoIpEU5vZYuQN4S3Ee1zkGdYC27DP12pewbw/RLgPHzaE4kx/YqxMzebOpm0dA==
-
"@nodelib/fs.scandir@2.1.5":
version "2.1.5"
resolved "https://registry.yarnpkg.com/@nodelib/fs.scandir/-/fs.scandir-2.1.5.tgz#7619c2eb21b25483f6d167548b4cfd5a7488c3d5"
@@ -131,9 +126,9 @@
"@polymer/polymer" "^3.0.5"
"@polymer/polymer@^3.0.5", "@polymer/polymer@^3.4.1":
- version "3.4.1"
- resolved "https://registry.yarnpkg.com/@polymer/polymer/-/polymer-3.4.1.tgz#333bef25711f8411bb5624fb3eba8212ef8bee96"
- integrity sha512-KPWnhDZibtqKrUz7enIPOiO4ZQoJNOuLwqrhV2MXzIt3VVnUVJVG5ORz4Z2sgO+UZ+/UZnPD0jqY+jmw/+a9mQ==
+ version "3.5.1"
+ resolved "https://registry.yarnpkg.com/@polymer/polymer/-/polymer-3.5.1.tgz#4b5234e43b8876441022bcb91313ab3c4a29f0c8"
+ integrity sha512-JlAHuy+1qIC6hL1ojEUfIVD58fzTpJAoCxFwV5yr0mYTXV1H8bz5zy0+rC963Cgr9iNXQ4T9ncSjC2fkF9BQfw==
dependencies:
"@webcomponents/shadycss" "^1.9.1"
@@ -289,9 +284,9 @@
integrity sha512-Y4XFY5VJAuw0FgAqPNd6NNoV44jbq9Bz2L7Rh/J6jLTiHBSBJa9fxqQIvkIld4GsoDOcCbvzOUAbLPsSKKg+uA==
"@types/node@*":
- version "18.7.18"
- resolved "https://registry.yarnpkg.com/@types/node/-/node-18.7.18.tgz#633184f55c322e4fb08612307c274ee6d5ed3154"
- integrity sha512-m+6nTEOadJZuTPkKR/SYK3A2d7FZrgElol9UP1Kae90VVU4a6mxnPuLiIW1m4Cq4gZ/nWb9GrdVXJCoCazDAbg==
+ version "18.8.3"
+ resolved "https://registry.yarnpkg.com/@types/node/-/node-18.8.3.tgz#ce750ab4017effa51aed6a7230651778d54e327c"
+ integrity sha512-0os9vz6BpGwxGe9LOhgP/ncvYN5Tx1fNcd2TM3rD/aCGBkysb+ZWpXEocG24h6ZzOi13+VB8HndAQFezsSOw1w==
"@types/parse5@^6.0.1":
version "6.0.3"
@@ -1070,44 +1065,28 @@
vary "^1.1.2"
lit-element@^3.2.0:
- version "3.2.0"
- resolved "https://registry.yarnpkg.com/lit-element/-/lit-element-3.2.0.tgz#9c981c55dfd9a8f124dc863edb62cc529d434db7"
- integrity sha512-HbE7yt2SnUtg5DCrWt028oaU4D5F4k/1cntAFHTkzY8ZIa8N0Wmu92PxSxucsQSOXlODFrICkQ5x/tEshKi13g==
+ version "3.2.2"
+ resolved "https://registry.yarnpkg.com/lit-element/-/lit-element-3.2.2.tgz#d148ab6bf4c53a33f707a5168e087725499e5f2b"
+ integrity sha512-6ZgxBR9KNroqKb6+htkyBwD90XGRiqKDHVrW/Eh0EZ+l+iC+u+v+w3/BA5NGi4nizAVHGYvQBHUDuSmLjPp7NQ==
dependencies:
"@lit/reactive-element" "^1.3.0"
lit-html "^2.2.0"
-lit-html@^2.0.0, lit-html@^2.3.0:
- version "2.3.1"
- resolved "https://registry.yarnpkg.com/lit-html/-/lit-html-2.3.1.tgz#56f15104ea75c0a702904893e3409d0e89e2a2b9"
- integrity sha512-FyKH6LTW6aBdkfNhNSHyZTnLgJSTe5hMk7HFtc/+DcN1w74C215q8B+Cfxc2OuIEpBNcEKxgF64qL8as30FDHA==
+lit-html@^2.0.0, lit-html@^2.2.0, lit-html@^2.4.0:
+ version "2.4.0"
+ resolved "https://registry.yarnpkg.com/lit-html/-/lit-html-2.4.0.tgz#b510430f39a56ec959167ed1187241a4e3ab1574"
+ integrity sha512-G6qXu4JNUpY6aaF2VMfaszhO9hlWw0hOTRFDmuMheg/nDYGB+2RztUSOyrzALAbr8Nh0Y7qjhYkReh3rPnplVg==
dependencies:
"@types/trusted-types" "^2.0.2"
-lit-html@^2.2.0:
- version "2.2.6"
- resolved "https://registry.yarnpkg.com/lit-html/-/lit-html-2.2.6.tgz#e70679605420a34c4f3cbd0c483b2fb1fff781df"
- integrity sha512-xOKsPmq/RAKJ6dUeOxhmOYFjcjf0Q7aSdfBJgdJkOfCUnkmmJPxNrlZpRBeVe1Gg50oYWMlgm6ccAE/SpJgSdw==
- dependencies:
- "@types/trusted-types" "^2.0.2"
-
-lit@^2.0.0:
- version "2.3.1"
- resolved "https://registry.yarnpkg.com/lit/-/lit-2.3.1.tgz#2cf1c2042da1e44c7a7cc72dff2d72303fd26f48"
- integrity sha512-TejktDR4mqG3qB32Y8Lm5Lye3c8SUehqz7qRsxe1PqGYL6me2Ef+jeQAEqh20BnnGncv4Yxy2njEIT0kzK1WCw==
+lit@^2.0.0, lit@^2.2.3:
+ version "2.4.0"
+ resolved "https://registry.yarnpkg.com/lit/-/lit-2.4.0.tgz#e33a0f463e17408f6e7f71464e6a266e60a5bb77"
+ integrity sha512-fdgzxEtLrZFQU/BqTtxFQCLwlZd9bdat+ltzSFjvWkZrs7eBmeX0L5MHUMb3kYIkuS8Xlfnii/iI5klirF8/Xg==
dependencies:
"@lit/reactive-element" "^1.4.0"
lit-element "^3.2.0"
- lit-html "^2.3.0"
-
-lit@^2.2.3:
- version "2.2.6"
- resolved "https://registry.yarnpkg.com/lit/-/lit-2.2.6.tgz#4ef223e88517c000b0c01baf2e3535e61a75a5b5"
- integrity sha512-K2vkeGABfSJSfkhqHy86ujchJs3NR9nW1bEEiV+bXDkbiQ60Tv5GUausYN2mXigZn8lC1qXuc46ArQRKYmumZw==
- dependencies:
- "@lit/reactive-element" "^1.3.0"
- lit-element "^3.2.0"
- lit-html "^2.2.0"
+ lit-html "^2.4.0"
log-update@^4.0.0:
version "4.0.0"
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts
index 7fe070c..66ad38c 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts
@@ -30,7 +30,6 @@
import {ReviewInput} from '../../../types/common';
import {getAppContext} from '../../../services/app-context';
import {assertIsDefined} from '../../../utils/common-util';
-import {CURRENT} from '../../../utils/patch-set-util';
import {fireReload} from '../../../utils/event-util';
import {submitRequirementsStyles} from '../../../styles/gr-submit-requirements-styles';
import {
@@ -321,7 +320,11 @@
},
};
return this.restApiService
- .saveChangeReview(this.change._number, CURRENT, review)
+ .saveChangeReview(
+ this.change._number,
+ this.change.current_revision,
+ review
+ )
.then(() => {
fireReload(this, true);
});
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard_test.ts b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard_test.ts
index 2117ec5..4e78e8a 100644
--- a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard_test.ts
@@ -18,7 +18,7 @@
createSubmitRequirementResultInfo,
} from '../../../test/test-data-generators';
import {ParsedChangeInfo} from '../../../types/types';
-import {query, queryAndAssert} from '../../../test/test-utils';
+import {query, queryAndAssert, stubRestApi} from '../../../test/test-utils';
import {GrButton} from '../../shared/gr-button/gr-button';
import {ChangeStatus, SubmitRequirementResultInfo} from '../../../api/rest-api';
@@ -321,6 +321,22 @@
assert.isUndefined(query(element, '.quickApprove'));
});
+ test('uses patchset from change', async () => {
+ const saveChangeReview = stubRestApi('saveChangeReview').resolves();
+ const element = await fixture<GrSubmitRequirementHovercard>(
+ html`<gr-submit-requirement-hovercard
+ .requirement=${submitRequirement}
+ .change=${change}
+ .account=${account}
+ ></gr-submit-requirement-hovercard>`
+ );
+
+ queryAndAssert<GrButton>(element, '.quickApprove > gr-button').click();
+
+ assert.equal(saveChangeReview.callCount, 1);
+ assert.equal(saveChangeReview.firstCall.args[1], change.current_revision);
+ });
+
test('override button renders', async () => {
const submitRequirement: SubmitRequirementResultInfo = {
...createSubmitRequirementResultInfo(),
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index a35ca40..74589d3 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -315,11 +315,12 @@
// Note that router model view must be updated before view model state.
// So this check is slightly fragile, but should work.
if (this.view !== GerritView.CHANGE) return;
- const browserUrl = window.location.toString();
- const stateUrl = new URL(createChangeUrl(state), browserUrl).toString();
- if (browserUrl !== stateUrl) {
+ const browserUrl = new URL(window.location.toString());
+ const stateUrl = new URL(createChangeUrl(state), browserUrl);
+ stateUrl.hash = browserUrl.hash;
+ if (browserUrl.toString() !== stateUrl.toString()) {
page.replace(
- stateUrl,
+ stateUrl.toString(),
null,
/* init: */ false,
/* dispatch: */ false
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 78a1509..9dfbc04 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -73,6 +73,7 @@
import {Interaction} from '../../../constants/reporting';
import {HtmlPatched} from '../../../utils/lit-util';
import {createDiffUrl} from '../../../models/views/diff';
+import {createChangeUrl} from '../../../models/views/change';
declare global {
interface HTMLElementEventMap {
@@ -779,13 +780,20 @@
if (!comment) return;
assertIsDefined(this.changeNum, 'changeNum');
assertIsDefined(this.repoName, 'repoName');
- const url = generateAbsoluteUrl(
- createDiffUrl({
+ let url: string;
+ if (this.isPatchsetLevel()) {
+ url = createChangeUrl({
changeNum: this.changeNum,
project: this.repoName,
commentId: comment.id,
- })
- );
+ });
+ } else {
+ url = createDiffUrl({
+ changeNum: this.changeNum,
+ project: this.repoName,
+ commentId: comment.id,
+ });
+ }
assertIsDefined(url, 'url for comment');
copyToClipbard(generateAbsoluteUrl(url), 'Link');
}
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index b767a32..10e54c7 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -28,6 +28,8 @@
import {SinonStub} from 'sinon';
import {fixture, html, waitUntil, assert} from '@open-wc/testing';
import {GrButton} from '../gr-button/gr-button';
+import {SpecialFilePath} from '../../../constants/constants';
+import {GrIcon} from '../gr-icon/gr-icon';
const c1 = {
author: {name: 'Kermit'},
@@ -447,4 +449,35 @@
},
]);
});
+
+ test('patchset comments link to /comments URL', async () => {
+ const clipboardStub = sinon.stub(navigator.clipboard, 'writeText');
+ element.thread = {
+ ...createThread(c1),
+ path: SpecialFilePath.PATCHSET_LEVEL_COMMENTS,
+ };
+ await element.updateComplete;
+
+ queryAndAssert<GrIcon>(element, 'gr-icon.copy').click();
+
+ assert.equal(1, clipboardStub.callCount);
+ assert.equal(
+ clipboardStub.firstCall.args[0],
+ 'http://localhost:9876/c/test-repo-name/+/1/comments/the-root'
+ );
+ });
+
+ test('file comments link to /comment URL', async () => {
+ const clipboardStub = sinon.stub(navigator.clipboard, 'writeText');
+ element.thread = createThread(c1);
+ await element.updateComplete;
+
+ queryAndAssert<GrIcon>(element, 'gr-icon.copy').click();
+
+ assert.equal(1, clipboardStub.callCount);
+ assert.equal(
+ clipboardStub.firstCall.args[0],
+ 'http://localhost:9876/c/test-repo-name/+/1/comment/the-root/'
+ );
+ });
});
diff --git a/polygerrit-ui/app/models/checks/checks-model.ts b/polygerrit-ui/app/models/checks/checks-model.ts
index 2b92529..db208d5 100644
--- a/polygerrit-ui/app/models/checks/checks-model.ts
+++ b/polygerrit-ui/app/models/checks/checks-model.ts
@@ -151,7 +151,12 @@
};
}
-const FETCH_RESULT_TIMEOUT_MS = 10000;
+/**
+ * Android's Checks Plugin has a 15s timeout internally. So we are using
+ * something slightly larger, so that we get a proper error from the plugin,
+ * if they run into timeout issues.
+ */
+const FETCH_RESULT_TIMEOUT_MS = 16000;
/**
* Can be used in `reduce()` to collect all results from all runs from all
@@ -185,9 +190,11 @@
private checkToPluginMap = new Map<string, string>();
- private changeNum?: NumericChangeId;
+ // visible for testing
+ changeNum?: NumericChangeId;
- private latestPatchNum?: PatchSetNumber;
+ // visible for testing
+ latestPatchNum?: PatchSetNumber;
private readonly documentVisibilityChange$ = new BehaviorSubject(undefined);
@@ -384,6 +391,9 @@
this.reporting.time(Timing.CHECKS_LOAD);
this.subscriptions = [
this.changeModel.changeNum$.subscribe(x => (this.changeNum = x)),
+ this.changeModel.latestPatchNum$.subscribe(
+ x => (this.latestPatchNum = x)
+ ),
this.pluginsModel.checksPlugins$.subscribe(plugins => {
for (const plugin of plugins) {
this.register(plugin);
@@ -750,8 +760,10 @@
patchset === ChecksPatchset.LATEST
? this.changeModel.latestPatchNum$
: this.checksSelectedPatchsetNumber$,
- this.reloadSubjects[pluginName].pipe(throttleTime(1000)),
- timer(0, pollIntervalMs),
+ this.reloadSubjects[pluginName].pipe(
+ throttleTime(1000, undefined, {trailing: true, leading: true})
+ ),
+ pollIntervalMs === 0 ? from([0]) : timer(0, pollIntervalMs),
this.documentVisibilityChange$,
])
.pipe(
diff --git a/polygerrit-ui/app/models/checks/checks-model_test.ts b/polygerrit-ui/app/models/checks/checks-model_test.ts
index 83ed464..3489c5a 100644
--- a/polygerrit-ui/app/models/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/models/checks/checks-model_test.ts
@@ -7,6 +7,7 @@
import './checks-model';
import {ChecksModel, ChecksPatchset, ChecksProviderState} from './checks-model';
import {
+ Action,
Category,
CheckRun,
ChecksApiConfig,
@@ -22,6 +23,7 @@
import {assert} from '@open-wc/testing';
import {testResolver} from '../../test/common-test-setup';
import {changeViewModelToken} from '../views/change';
+import {NumericChangeId, PatchSetNumber} from '../../api/rest-api';
const PLUGIN_NAME = 'test-plugin';
@@ -42,8 +44,12 @@
},
];
-const CONFIG: ChecksApiConfig = {
- fetchPollingIntervalSeconds: 1000,
+const CONFIG_POLLING_5S: ChecksApiConfig = {
+ fetchPollingIntervalSeconds: 5,
+};
+
+const CONFIG_POLLING_NONE: ChecksApiConfig = {
+ fetchPollingIntervalSeconds: 0,
};
function createProvider(): ChecksProvider {
@@ -82,13 +88,67 @@
const provider = createProvider();
const fetchSpy = sinon.spy(provider, 'fetch');
- model.register({pluginName: 'test-plugin', provider, config: CONFIG});
+ model.register({
+ pluginName: 'test-plugin',
+ provider,
+ config: CONFIG_POLLING_NONE,
+ });
await waitUntil(() => change === undefined);
const testChange = createParsedChange();
model.changeModel.updateStateChange(testChange);
await waitUntil(() => change === testChange);
await waitUntilCalled(fetchSpy, 'fetch');
+
+ assert.equal(
+ model.latestPatchNum,
+ testChange.revisions[testChange.current_revision]
+ ._number as PatchSetNumber
+ );
+ assert.equal(model.changeNum, testChange._number);
+ });
+
+ test('reload throttle', async () => {
+ const clock = sinon.useFakeTimers();
+ let change: ParsedChangeInfo | undefined = undefined;
+ model.changeModel.change$.subscribe(c => (change = c));
+ const provider = createProvider();
+ const fetchSpy = sinon.spy(provider, 'fetch');
+
+ model.register({
+ pluginName: 'test-plugin',
+ provider,
+ config: CONFIG_POLLING_NONE,
+ });
+ await waitUntil(() => change === undefined);
+
+ const testChange = createParsedChange();
+ model.changeModel.updateStateChange(testChange);
+ await waitUntil(() => change === testChange);
+ clock.tick(1);
+ assert.equal(fetchSpy.callCount, 1);
+
+ // The second reload call will be processed, but only after a 1s throttle.
+ model.reload('test-plugin');
+ clock.tick(100);
+ assert.equal(fetchSpy.callCount, 1);
+ // 2000 ms is greater than the 1000 ms throttle time.
+ clock.tick(2000);
+ assert.equal(fetchSpy.callCount, 2);
+ });
+
+ test('triggerAction', async () => {
+ model.changeNum = 314 as NumericChangeId;
+ model.latestPatchNum = 13 as PatchSetNumber;
+ const action: Action = {
+ name: 'test action',
+ callback: () => undefined,
+ };
+ const spy = sinon.spy(action, 'callback');
+ model.triggerAction(action, undefined, 'none');
+ assert.isTrue(spy.calledOnce);
+ assert.equal(spy.lastCall.args[0], 314);
+ assert.equal(spy.lastCall.args[1], 13);
});
test('model.updateStateSetProvider', () => {
@@ -204,4 +264,58 @@
assert.lengthOf(current.runs[0].results!, 1);
assert.equal(current.runs[0].results![0].summary, 'new');
});
+
+ test('polls for changes', async () => {
+ const clock = sinon.useFakeTimers();
+ let change: ParsedChangeInfo | undefined = undefined;
+ model.changeModel.change$.subscribe(c => (change = c));
+ const provider = createProvider();
+ const fetchSpy = sinon.spy(provider, 'fetch');
+
+ model.register({
+ pluginName: 'test-plugin',
+ provider,
+ config: CONFIG_POLLING_5S,
+ });
+ await waitUntil(() => change === undefined);
+ clock.tick(1);
+ const testChange = createParsedChange();
+ model.changeModel.updateStateChange(testChange);
+ await waitUntil(() => change === testChange);
+ await waitUntilCalled(fetchSpy, 'fetch');
+ clock.tick(1);
+ const pollCount = fetchSpy.callCount;
+
+ // polling should continue while we wait
+ clock.tick(CONFIG_POLLING_5S.fetchPollingIntervalSeconds * 1000 * 2);
+
+ assert.isTrue(fetchSpy.callCount > pollCount);
+ });
+
+ test('does not poll when config specifies 0 seconds', async () => {
+ const clock = sinon.useFakeTimers();
+ let change: ParsedChangeInfo | undefined = undefined;
+ model.changeModel.change$.subscribe(c => (change = c));
+ const provider = createProvider();
+ const fetchSpy = sinon.spy(provider, 'fetch');
+
+ model.register({
+ pluginName: 'test-plugin',
+ provider,
+ config: CONFIG_POLLING_NONE,
+ });
+ await waitUntil(() => change === undefined);
+ clock.tick(1);
+ const testChange = createParsedChange();
+ model.changeModel.updateStateChange(testChange);
+ await waitUntil(() => change === testChange);
+ await waitUntilCalled(fetchSpy, 'fetch');
+ clock.tick(1);
+ const pollCount = fetchSpy.callCount;
+
+ // polling should not happen
+ clock.tick(60 * 1000);
+
+ assert.equal(fetchSpy.callCount, pollCount);
+ });
});
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts
index 826ec66..a2d3506 100644
--- a/polygerrit-ui/app/models/views/change.ts
+++ b/polygerrit-ui/app/models/views/change.ts
@@ -155,11 +155,18 @@
super(undefined);
this.state$.subscribe(s => {
if (s?.usp || s?.forceReload || s?.openReplyDialog) {
- this.updateState({
- usp: undefined,
- forceReload: undefined,
- openReplyDialog: undefined,
- });
+ // We had been running into issues with directly calling
+ // `this.updateState()` without `setTimeout()`, because we want to
+ // first allow all subscribers to process the first state update before
+ // creating another one. For some unknown reason some subscribers even
+ // got the state updates out of order.
+ setTimeout(() => {
+ this.updateState({
+ usp: undefined,
+ forceReload: undefined,
+ openReplyDialog: undefined,
+ });
+ }, 0);
}
});
}
diff --git a/polygerrit-ui/app/rollup.config.js b/polygerrit-ui/app/rollup.config.js
index 47492a3..be60a63 100644
--- a/polygerrit-ui/app/rollup.config.js
+++ b/polygerrit-ui/app/rollup.config.js
@@ -30,6 +30,7 @@
const resolve = requirePlugin('rollup-plugin-node-resolve');
const {terser} = requirePlugin('rollup-plugin-terser');
+const define = requirePlugin('rollup-plugin-define');
// @polymer/font-roboto-local uses import.meta.url value
// as a base path to fonts. We should substitute a correct javascript
@@ -75,5 +76,11 @@
extensions: ['.js'],
moduleDirectory: 'external/ui_npm/node_modules',
},
- }), importLocalFontMetaUrlResolver()],
+ }),
+ define({
+ replacements: {
+ 'process.env.NODE_ENV': JSON.stringify('production'),
+ },
+ }),
+ importLocalFontMetaUrlResolver()],
};
diff --git a/polygerrit-ui/app/rules.bzl b/polygerrit-ui/app/rules.bzl
index 1d5e577..9ab0f64 100644
--- a/polygerrit-ui/app/rules.bzl
+++ b/polygerrit-ui/app/rules.bzl
@@ -29,6 +29,7 @@
silent = True,
sourcemap = "hidden",
deps = [
+ "@tools_npm//rollup-plugin-define",
"@tools_npm//rollup-plugin-node-resolve",
],
)
@@ -42,6 +43,7 @@
silent = True,
sourcemap = "hidden",
deps = [
+ "@tools_npm//rollup-plugin-define",
"@tools_npm//rollup-plugin-node-resolve",
],
)
@@ -55,6 +57,7 @@
silent = True,
sourcemap = "hidden",
deps = [
+ "@tools_npm//rollup-plugin-define",
"@tools_npm//rollup-plugin-node-resolve",
],
)
diff --git a/polygerrit-ui/app/styles/themes/app-theme.ts b/polygerrit-ui/app/styles/themes/app-theme.ts
index 117e7cc..e148da9 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.ts
+++ b/polygerrit-ui/app/styles/themes/app-theme.ts
@@ -99,6 +99,8 @@
--purple-100: #e9d2fd;
--purple-50: #f3e8fd;
--purple-tonal: #523272;
+ --deep-purple-800: #4527a0;
+ --deep-purple-600: #5e35b1;
--pink-800: #b80672;
--pink-500: #f538a0;
--pink-50: #fde7f3;
diff --git a/polygerrit-ui/app/styles/themes/dark-theme.ts b/polygerrit-ui/app/styles/themes/dark-theme.ts
index d11e372..2330041 100644
--- a/polygerrit-ui/app/styles/themes/dark-theme.ts
+++ b/polygerrit-ui/app/styles/themes/dark-theme.ts
@@ -219,8 +219,8 @@
--dark-remove-highlight-color: #62110f;
--light-remove-highlight-color: #320404;
- --dark-rebased-add-highlight-color: rgba(11, 255, 155, 0.15);
- --light-rebased-add-highlight-color: #487165;
+ --dark-rebased-add-highlight-color: var(--deep-purple-800);
+ --light-rebased-add-highlight-color: var(--deep-purple-600);
--dark-rebased-remove-highlight-color: rgba(255, 139, 6, 0.15);
--light-rebased-remove-highlight-color: #2f3f2f;
diff --git a/proto/cache.proto b/proto/cache.proto
index 83c2ce2..9aadf0f 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -696,7 +696,7 @@
// Serialized form of
// com.google.gerrit.server.patch.filediff.FileDiffOutput
-// Next ID: 13
+// Next ID: 15
message FileDiffOutputProto {
// Next ID: 5
message Edit {
@@ -728,4 +728,6 @@
bytes new_commit = 10;
ComparisonType comparison_type = 11;
bool negative = 12;
+ string old_mode = 13; // ENUM as string
+ string new_mode = 14; // ENUM as string
}
diff --git a/tools/BUILD b/tools/BUILD
index 4e4e5f0..f2d887e 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -46,10 +46,11 @@
"-XepDisableWarningsInGeneratedCode",
# The XepDisableWarningsInGeneratedCode disables only warnings, but
# not errors. We should manually exclude all files generated by
- # AutoValue; such files always start $AutoValue_.....
+ # AutoValue; such files always start AutoValue_..., $AutoValue_...
+ # or $$AutoValue_...
# XepExcludedPaths is a regexp. If you need more paths - use | as
# separator.
- "-XepExcludedPaths:.*/\\\\$$AutoValue_.*\\.java",
+ "-XepExcludedPaths:.*/\\\\$$?\\\\$$?AutoValue_.*\\.java",
"-Xep:AlmostJavadoc:ERROR",
"-Xep:AlwaysThrows:ERROR",
"-Xep:AmbiguousMethodReference:ERROR",
@@ -68,7 +69,7 @@
"-Xep:AutoValueConstructorOrderChecker:ERROR",
"-Xep:AutoValueFinalMethods:ERROR",
"-Xep:AutoValueImmutableFields:ERROR",
- # "-Xep:AutoValueSubclassLeaked:WARN",
+ "-Xep:AutoValueSubclassLeaked:ERROR",
"-Xep:BadAnnotationImplementation:ERROR",
"-Xep:BadComparable:ERROR",
"-Xep:BadImport:ERROR",
diff --git a/tools/node_tools/package.json b/tools/node_tools/package.json
index fc00451..3765575 100644
--- a/tools/node_tools/package.json
+++ b/tools/node_tools/package.json
@@ -15,6 +15,7 @@
"polymer-bundler": "^4.0.10",
"rollup": "^2.3.4",
"rollup-plugin-commonjs": "^10.1.0",
+ "rollup-plugin-define": "^1.0.1",
"rollup-plugin-node-resolve": "^5.2.0",
"rollup-plugin-terser": "^5.1.3",
"typescript": "^4.7.2"
diff --git a/tools/node_tools/yarn.lock b/tools/node_tools/yarn.lock
index 3914e17..c6b3eab 100644
--- a/tools/node_tools/yarn.lock
+++ b/tools/node_tools/yarn.lock
@@ -216,6 +216,14 @@
resolved "https://registry.yarnpkg.com/@protobufjs/utf8/-/utf8-1.1.0.tgz#a777360b5b39a1a2e5106f8e858f2fd2d060c570"
integrity sha512-Vvn3zZrhQZkkBE8LSuW3em98c0FwgO4nxzv6OdSxPKJIEKY2bGbHn+mhGIPerzI4twdxaP8/0+06HBpwf345Lw==
+"@rollup/pluginutils@^4.0.0":
+ version "4.2.1"
+ resolved "https://registry.yarnpkg.com/@rollup/pluginutils/-/pluginutils-4.2.1.tgz#e6c6c3aba0744edce3fb2074922d3776c0af2a6d"
+ integrity sha512-iKnFXr7NkdZAIHiIWE+BX5ULi/ucVFYWD6TbAV+rZctiRTY2PL6tsIKhoIOaoskiWAkgu+VsbXgUVDNLHf+InQ==
+ dependencies:
+ estree-walker "^2.0.1"
+ picomatch "^2.2.2"
+
"@sindresorhus/is@^4.0.0":
version "4.6.0"
resolved "https://registry.yarnpkg.com/@sindresorhus/is/-/is-4.6.0.tgz#3c7c9c46e678feefe7a2e5bb609d3dbd665ffb3f"
@@ -576,6 +584,11 @@
resolved "https://registry.yarnpkg.com/array-back/-/array-back-3.1.0.tgz#b8859d7a508871c9a7b2cf42f99428f65e96bfb0"
integrity sha512-TkuxA4UCOvxuDK6NZYXCalszEzj+TLszyASooky+i742l9TqsOdYCMJJupxRic61hwquNtppB3hgcuq9SVSH1Q==
+ast-matcher@^1.1.1:
+ version "1.1.1"
+ resolved "https://registry.yarnpkg.com/ast-matcher/-/ast-matcher-1.1.1.tgz#95a6dc72318319507024fff438b7839e4e280813"
+ integrity sha512-wQPAp09kPFRQsOijM2Blfg4lH6B9MIhIUrhFtDdhD/1JFhPmfg2/+WAjViVYl3N7EwleHI+q/enTHjaDrv+wEw==
+
async@^2.0.1:
version "2.6.4"
resolved "https://registry.yarnpkg.com/async/-/async-2.6.4.tgz#706b7ff6084664cd7eae713f6f965433b5504221"
@@ -985,6 +998,11 @@
resolved "https://registry.yarnpkg.com/escape-string-regexp/-/escape-string-regexp-1.0.5.tgz#1b61c0562190a8dff6ae3bb2cf0200ca130b86d4"
integrity sha512-vbRorB5FUQWvla16U8R/qgaFIya2qGzwDrNmCZuYKrbdSUMG6I1ZCGQRefkRVhuOkIGVne7BQ35DSfo1qvJqFg==
+escape-string-regexp@^4.0.0:
+ version "4.0.0"
+ resolved "https://registry.yarnpkg.com/escape-string-regexp/-/escape-string-regexp-4.0.0.tgz#14ba83a5d373e3d311e5afca29cf5bfad965bf34"
+ integrity sha512-TtpcNJ3XAzx3Gq8sWRzJaVajRs0uVxA2YAkdb1jm2YkPz4G6egUFAyA3n5vtEIZefPk5Wa4UXbKuS5fKkJWdgA==
+
espree@^3.5.2:
version "3.5.4"
resolved "https://registry.yarnpkg.com/espree/-/espree-3.5.4.tgz#b0f447187c8a8bed944b815a660bddf5deb5d1a7"
@@ -998,6 +1016,11 @@
resolved "https://registry.yarnpkg.com/estree-walker/-/estree-walker-0.6.1.tgz#53049143f40c6eb918b23671d1fe3219f3a1b362"
integrity sha512-SqmZANLWS0mnatqbSfRP5g8OXZC12Fgg1IwNtLsyHDzJizORW4khDfjPqJZsemPWBB2uqykUah5YpQ6epsqC/w==
+estree-walker@^2.0.1:
+ version "2.0.2"
+ resolved "https://registry.yarnpkg.com/estree-walker/-/estree-walker-2.0.2.tgz#52f010178c2a4c117a7757cfe942adb7d2da4cac"
+ integrity sha512-Rfkk/Mp/DL7JVje3u18FxFujQlTNR2q6QfMSMB7AvCBx91NGj/ba3kCfza0f6dVDbw7YlRf/nDrn7pQrCCyQ/w==
+
esutils@^2.0.2:
version "2.0.3"
resolved "https://registry.yarnpkg.com/esutils/-/esutils-2.0.3.tgz#74d2eb4de0b8da1293711910d50775b9b710ef64"
@@ -1325,7 +1348,7 @@
dependencies:
vlq "^0.2.2"
-magic-string@^0.25.2:
+magic-string@^0.25.2, magic-string@^0.25.7:
version "0.25.9"
resolved "https://registry.yarnpkg.com/magic-string/-/magic-string-0.25.9.tgz#de7f9faf91ef8a1c91d02c2e5314c8277dbcdd1c"
integrity sha512-RmF0AsMzgt25qzqqLc1+MbHmhdx0ojF2Fvs4XnOqz2ZOBXzzkEwc/dJQZCYHAn7v1jbVOjAZfK8msRn4BxO4VQ==
@@ -1458,6 +1481,11 @@
resolved "https://registry.yarnpkg.com/pend/-/pend-1.2.0.tgz#7a57eb550a6783f9115331fcf4663d5c8e007a50"
integrity sha512-F3asv42UuXchdzt+xXqfW1OGlVBe+mxa2mqI0pg5yAHZPvFmY3Y6drSf/GQ1A86WgWEN9Kzh/WrgKa6iGcHXLg==
+picomatch@^2.2.2:
+ version "2.3.1"
+ resolved "https://registry.yarnpkg.com/picomatch/-/picomatch-2.3.1.tgz#3ba3833733646d9d3e4995946c1365a67fb07a42"
+ integrity sha512-JU3teHTNjmE2VCGFzuY8EXzCDVwEqB2a8fsIvwaStHhAWJEeVd1o1QD80CU6+ZdEXXSLbSsuLwJjkCBWqRQUVA==
+
plist@^2.0.1:
version "2.1.0"
resolved "https://registry.yarnpkg.com/plist/-/plist-2.1.0.tgz#57ccdb7a0821df21831217a3cad54e3e146a1025"
@@ -1652,6 +1680,16 @@
resolve "^1.11.0"
rollup-pluginutils "^2.8.1"
+rollup-plugin-define@^1.0.1:
+ version "1.0.1"
+ resolved "https://registry.yarnpkg.com/rollup-plugin-define/-/rollup-plugin-define-1.0.1.tgz#45b027cec9d2e30df71118efa156170e3846fd3d"
+ integrity sha512-SM/CKFpLvWq5xBEf84ff/ooT3KodXPVITCkRliyNccuq8SZMpzthN/Bp7JkWScbGTX5lo1SF3cjxKKDjnnFCuA==
+ dependencies:
+ "@rollup/pluginutils" "^4.0.0"
+ ast-matcher "^1.1.1"
+ escape-string-regexp "^4.0.0"
+ magic-string "^0.25.7"
+
rollup-plugin-node-resolve@^5.2.0:
version "5.2.0"
resolved "https://registry.yarnpkg.com/rollup-plugin-node-resolve/-/rollup-plugin-node-resolve-5.2.0.tgz#730f93d10ed202473b1fb54a5997a7db8c6d8523"
diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl
index 37d8b9c..d6f2ef63 100644
--- a/tools/nongoogle.bzl
+++ b/tools/nongoogle.bzl
@@ -135,8 +135,8 @@
maven_jar(
name = "error-prone-annotations",
- artifact = "com.google.errorprone:error_prone_annotations:2.10.0",
- sha1 = "9bc20b94d3ac42489cf6ce1e42509c86f6f861a1",
+ artifact = "com.google.errorprone:error_prone_annotations:2.15.0",
+ sha1 = "38c8485a652f808c8c149150da4e5c2b0bd17f9a",
)
FLOGGER_VERS = "0.7.4"
