Merge "Move PROJECT_DASHBOARD_ROUTE into view model"
diff --git a/Documentation/config-submit-requirements.txt b/Documentation/config-submit-requirements.txt
index ba20cea..5ff9425 100644
--- a/Documentation/config-submit-requirements.txt
+++ b/Documentation/config-submit-requirements.txt
@@ -246,6 +246,15 @@
name contains the file pattern, or the edits of the file diff contain the edit
pattern.
+[[operator_label]]
+label:labelName=+1,user=non_contributor::
++
+Submit requirements support an additional `user=non_contributor` argument for
+labels that returns true if the change has a label vote matching the specified
+value and the vote is applied from a gerrit account that's not the uploader,
+author or committer of the latest patchset. See the documentation for the labels
+operator in the link:user-search.html[user search] page.
+
[[unsupported_operators]]
=== Unsupported Operators
diff --git a/Documentation/js_licenses.txt b/Documentation/js_licenses.txt
index 114aa3a..f685af9 100644
--- a/Documentation/js_licenses.txt
+++ b/Documentation/js_licenses.txt
@@ -603,39 +603,6 @@
----
-[[ba-linkify]]
-ba-linkify
-
-* ba-linkify
-
-[[ba-linkify_license]]
-----
-Copyright (c) 2009 "Cowboy" Ben Alman
-
-Permission is hereby granted, free of charge, to any person
-obtaining a copy of this software and associated documentation
-files (the "Software"), to deal in the Software without
-restriction, including without limitation the rights to use,
-copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the
-Software is furnished to do so, subject to the following
-conditions:
-
-The above copyright notice and this permission notice shall be
-included in all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
-EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
-OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
-NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
-HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
-WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
-FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
-OTHER DEALINGS IN THE SOFTWARE.
-
-----
-
-
[[codemirror-minified]]
codemirror-minified
diff --git a/Documentation/licenses.txt b/Documentation/licenses.txt
index f8ca85b..be41d0c 100644
--- a/Documentation/licenses.txt
+++ b/Documentation/licenses.txt
@@ -3507,39 +3507,6 @@
----
-[[ba-linkify]]
-ba-linkify
-
-* ba-linkify
-
-[[ba-linkify_license]]
-----
-Copyright (c) 2009 "Cowboy" Ben Alman
-
-Permission is hereby granted, free of charge, to any person
-obtaining a copy of this software and associated documentation
-files (the "Software"), to deal in the Software without
-restriction, including without limitation the rights to use,
-copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the
-Software is furnished to do so, subject to the following
-conditions:
-
-The above copyright notice and this permission notice shall be
-included in all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
-EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
-OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
-NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
-HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
-WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
-FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
-OTHER DEALINGS IN THE SOFTWARE.
-
-----
-
-
[[codemirror-minified]]
codemirror-minified
diff --git a/contrib/git-gc-preserve b/contrib/git-gc-preserve
index a886721..33c8f5b 100755
--- a/contrib/git-gc-preserve
+++ b/contrib/git-gc-preserve
@@ -49,10 +49,10 @@
[2] https://git.eclipse.org/r/c/jgit/jgit/+/122288
CONFIGURATION
- "gc.prunepreserved": if set to "true" preserved packs from the last gc run
+ "pack.prunepreserved": if set to "true" preserved packs from the last gc run
are pruned before current packs are preserved.
- "gc.preserveoldpacks": if set to "true" current packs will be hard linked
+ "pack.preserveoldpacks": if set to "true" current packs will be hard linked
to objects/pack/preserved before git gc is executed. JGit will
fallback to the preserved packs in this directory in case it comes
across missing objects which might be caused by a concurrent run of
@@ -84,9 +84,9 @@
exec 9>&-
}
-# prune preserved packs if gc.prunepreserved == true
+# prune preserved packs if pack.prunepreserved == true
prune_preserved() { # repo
- configured=$(git --git-dir="$1" config --get gc.prunepreserved)
+ configured=$(git --git-dir="$1" config --get pack.prunepreserved)
if [ "$configured" != "true" ]; then
return 0
fi
@@ -99,9 +99,9 @@
fi
}
-# preserve packs if gc.preserveoldpacks == true
+# preserve packs if pack.preserveoldpacks == true
preserve_packs() { # repo
- configured=$(git --git-dir="$1" config --get gc.preserveoldpacks)
+ configured=$(git --git-dir="$1" config --get pack.preserveoldpacks)
if [ "$configured" != "true" ]; then
return 0
fi
diff --git a/java/com/google/gerrit/entities/RefNames.java b/java/com/google/gerrit/entities/RefNames.java
index 9745fc5..66ffa42 100644
--- a/java/com/google/gerrit/entities/RefNames.java
+++ b/java/com/google/gerrit/entities/RefNames.java
@@ -185,6 +185,21 @@
return ref.startsWith(REFS_CHANGES);
}
+ /** True if the provided ref is in {@code refs/sequences/*}. */
+ public static boolean isSequenceRef(String ref) {
+ return ref.startsWith(REFS_SEQUENCES);
+ }
+
+ /** True if the provided ref is in {@code refs/tags/*}. */
+ public static boolean isTagRef(String ref) {
+ return ref.startsWith(REFS_TAGS);
+ }
+
+ /** True if the provided ref is {@link REFS_EXTERNAL_IDS}. */
+ public static boolean isExternalIdRef(String ref) {
+ return REFS_EXTERNAL_IDS.equals(ref);
+ }
+
public static String refsGroups(AccountGroup.UUID groupUuid) {
return REFS_GROUPS + shardUuid(groupUuid.get());
}
@@ -330,6 +345,21 @@
return REFS_CONFIG.equals(ref);
}
+ /** Whether the ref is the version branch, i.e. {@code refs/meta/version}. */
+ public static boolean isVersionRef(String ref) {
+ return REFS_VERSION.equals(ref);
+ }
+
+ /** Whether the ref is an auto-merge ref. */
+ public static boolean isAutoMergeRef(String ref) {
+ return ref.startsWith(REFS_CACHE_AUTOMERGE);
+ }
+
+ /** Whether the ref is an reject commit ref, i.e. {@code refs/meta/reject-commits} */
+ public static boolean isRejectCommitsRef(String ref) {
+ return REFS_REJECT_COMMITS.equals(ref);
+ }
+
/**
* Whether the ref is managed by Gerrit. Covers all Gerrit-internal refs like refs/cache-automerge
* and refs/meta as well as refs/changes. Does not cover user-created refs like branches or custom
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 738eab3..f6fc8db 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -225,9 +225,11 @@
public static final String ARG_ID_GROUP = "group";
public static final String ARG_ID_OWNER = "owner";
public static final String ARG_ID_NON_UPLOADER = "non_uploader";
+ public static final String ARG_ID_NON_CONTRIBUTOR = "non_contributor";
public static final String ARG_COUNT = "count";
public static final Account.Id OWNER_ACCOUNT_ID = Account.id(0);
public static final Account.Id NON_UPLOADER_ACCOUNT_ID = Account.id(-1);
+ public static final Account.Id NON_CONTRIBUTOR_ACCOUNT_ID = Account.id(-2);
public static final String OPERATOR_MERGED_BEFORE = "mergedbefore";
public static final String OPERATOR_MERGED_AFTER = "mergedafter";
@@ -485,13 +487,13 @@
}
}
- private final Arguments args;
+ protected final Arguments args;
protected Map<String, String> hasOperandAliases = Collections.emptyMap();
private Map<Account.Id, DestinationList> destinationListByAccount = new HashMap<>();
private static final Splitter RULE_SPLITTER = Splitter.on("=");
private static final Splitter PLUGIN_SPLITTER = Splitter.on("_");
- private static final Splitter LABEL_SPLITTER = Splitter.on(",");
+ protected static final Splitter LABEL_SPLITTER = Splitter.on(",");
@Inject
protected ChangeQueryBuilder(Arguments args) {
@@ -1038,6 +1040,8 @@
accounts = Collections.singleton(OWNER_ACCOUNT_ID);
} else if (value.equals(ARG_ID_NON_UPLOADER)) {
accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
+ accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
} else {
accounts = parseAccount(value);
}
@@ -1072,6 +1076,8 @@
accounts = Collections.singleton(OWNER_ACCOUNT_ID);
} else if (value.equals(ARG_ID_NON_UPLOADER)) {
accounts = Collections.singleton(NON_UPLOADER_ACCOUNT_ID);
+ } else if (value.equals(ARG_ID_NON_CONTRIBUTOR)) {
+ accounts = Collections.singleton(NON_CONTRIBUTOR_ACCOUNT_ID);
} else {
accounts = parseAccount(value);
}
@@ -1106,9 +1112,16 @@
}
}
+ validateLabelArgs(accounts);
return new LabelPredicate(args, name, accounts, group, count, countOp);
}
+ protected void validateLabelArgs(Set<Account.Id> accounts) throws QueryParseException {
+ if (accounts != null && accounts.contains(NON_CONTRIBUTOR_ACCOUNT_ID)) {
+ throw new QueryParseException("non_contributor arg is not allowed in change queries");
+ }
+ }
+
/** Assert that keys {@code k1} and {@code k2} do not exist in {@code labelArgs} together. */
private void assertDisjunctive(PredicateArgs labelArgs, String k1, String k2)
throws QueryParseException {
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
index 5662e4d..83dd5ba 100644
--- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.query.change;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
@@ -23,6 +24,8 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -30,9 +33,15 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData.StorageConstraint;
+import java.io.IOException;
+import java.util.List;
import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
public class EqualsLabelPredicate extends ChangeIndexPostFilterPredicate {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ protected final AccountResolver accountResolver;
protected final ProjectCache projectCache;
protected final PermissionBackend permissionBackend;
protected final IdentifiedUser.GenericFactory userFactory;
@@ -61,6 +70,7 @@
@Nullable Integer count) {
super(ChangeField.LABEL_SPEC, ChangeField.formatLabel(label, expVal, account, count));
this.permissionBackend = args.permissionBackend;
+ this.accountResolver = args.accountResolver;
this.projectCache = args.projectCache;
this.userFactory = args.userFactory;
this.count = count;
@@ -155,6 +165,14 @@
&& cd.currentPatchSet().uploader().equals(approver)) {
return false;
}
+
+ if (account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID)) {
+ if ((cd.currentPatchSet().uploader().equals(approver)
+ || matchAccount(cd.getCommitter().getEmailAddress(), approver)
+ || matchAccount(cd.getAuthor().getEmailAddress(), approver))) {
+ return false;
+ }
+ }
}
IdentifiedUser reviewer = userFactory.create(approver);
@@ -176,9 +194,24 @@
}
}
+ /**
+ * Returns true if the {@code email} parameter belongs to the account identified by the {@code
+ * accountId} parameter.
+ */
+ private boolean matchAccount(String email, Account.Id accountId) {
+ try {
+ List<AccountState> accountsList = accountResolver.resolve(email).asList();
+ return accountsList.stream().anyMatch(c -> c.account().id().equals(accountId));
+ } catch (ConfigInvalidException | IOException e) {
+ logger.atWarning().withCause(e).log("Failed to resolve account %s", email);
+ }
+ return false;
+ }
+
private boolean isMagicUser() {
return account.equals(ChangeQueryBuilder.OWNER_ACCOUNT_ID)
- || account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID);
+ || account.equals(ChangeQueryBuilder.NON_UPLOADER_ACCOUNT_ID)
+ || account.equals(ChangeQueryBuilder.NON_CONTRIBUTOR_ACCOUNT_ID);
}
@Override
diff --git a/java/com/google/gerrit/server/query/change/LabelPredicate.java b/java/com/google/gerrit/server/query/change/LabelPredicate.java
index 2a5a47d..d89940d 100644
--- a/java/com/google/gerrit/server/query/change/LabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/LabelPredicate.java
@@ -23,6 +23,7 @@
import com.google.gerrit.index.query.RangeUtil;
import com.google.gerrit.index.query.RangeUtil.Range;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
@@ -36,6 +37,7 @@
protected static final int MAX_COUNT = 5; // inclusive
protected static class Args {
+ protected final AccountResolver accountResolver;
protected final ProjectCache projectCache;
protected final PermissionBackend permissionBackend;
protected final IdentifiedUser.GenericFactory userFactory;
@@ -46,6 +48,7 @@
protected final PredicateArgs.Operator countOp;
protected Args(
+ AccountResolver accountResolver,
ProjectCache projectCache,
PermissionBackend permissionBackend,
IdentifiedUser.GenericFactory userFactory,
@@ -54,6 +57,7 @@
AccountGroup.UUID group,
@Nullable Integer count,
@Nullable PredicateArgs.Operator countOp) {
+ this.accountResolver = accountResolver;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.userFactory = userFactory;
@@ -89,6 +93,7 @@
super(
predicates(
new Args(
+ a.accountResolver,
a.projectCache,
a.permissionBackend,
a.userFactory,
diff --git a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
index 5632c14..cb92ddd 100644
--- a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.query.change;
import com.google.common.base.Splitter;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.index.SchemaFieldDefs.SchemaField;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryBuilder;
@@ -30,6 +31,7 @@
import com.google.inject.Inject;
import java.util.List;
import java.util.Locale;
+import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@@ -179,6 +181,9 @@
return fileEditsPredicateFactory.create(FileEditsArgs.create(filePattern, contentPattern));
}
+ @Override
+ protected void validateLabelArgs(Set<Account.Id> accountIds) throws QueryParseException {}
+
private static void validateRegularExpression(String pattern, String errorMessage)
throws QueryParseException {
try {
diff --git a/java/com/google/gerrit/server/update/context/RefUpdateContext.java b/java/com/google/gerrit/server/update/context/RefUpdateContext.java
new file mode 100644
index 0000000..d1c5ff8
--- /dev/null
+++ b/java/com/google/gerrit/server/update/context/RefUpdateContext.java
@@ -0,0 +1,170 @@
+// Copyright (C) 2023 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.server.update.context;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.common.collect.ImmutableList;
+import java.util.ArrayDeque;
+import java.util.Deque;
+
+/**
+ * Passes additional information about an operation to the {@link BatchRefUpdate#execute} method.
+ *
+ * <p>To pass the additional information {@link RefUpdateContext}, wraps a code into an open
+ * RefUpdateContext, e.g.:
+ *
+ * <pre>{@code
+ * try(RefUpdateContext ctx = RefUpdateContext.open(RefUpdateType.CHANGE_MODIFICATION)) {
+ * ...
+ * // some code which modifies a ref using BatchRefUpdate.execute method
+ * }
+ * }</pre>
+ *
+ * When the {@link BatchRefUpdate#execute} method is executed, it can get all opened contexts and
+ * use it for an additional actions, e.g. it can put it in the reflog.
+ *
+ * <p>The information provided by this class is used internally in google.
+ *
+ * <p>The InMemoryRepositoryManager file makes some validation to ensure that RefUpdateContext is
+ * used correctly within the code (see thee validateRefUpdateContext method).
+ *
+ * <p>The class includes only operations from open-source gerrit and can be extended (see {@link
+ * TestActionRefUpdateContext} for example how to extend it).
+ */
+public class RefUpdateContext implements AutoCloseable {
+ private static final ThreadLocal<Deque<RefUpdateContext>> current = new ThreadLocal<>();
+
+ /**
+ * List of possible ref-update types.
+ *
+ * <p>Items in this enum are not fine-grained; different actions are shared the same type (e.g.
+ * {@link #CHANGE_MODIFICATION} includes posting comments, change edits and attention set update).
+ *
+ * <p>It is expected, that each type of operation should include only specific ref(s); check the
+ * validateRefUpdateContext in InMemoryRepositoryManager for relation between RefUpdateType and
+ * ref name.
+ */
+ public enum RefUpdateType {
+ /**
+ * Indicates that the context is implemented as a descendant of the {@link RefUpdateContext} .
+ *
+ * <p>The {@link #getUpdateType()} returns this type for all descendant of {@link
+ * RefUpdateContext}. This type is never returned if the context is exactly {@link
+ * RefUpdateContext}.
+ */
+ OTHER,
+ /**
+ * A ref is updated as a part of change-related operation.
+ *
+ * <p>This covers multiple different cases - creating and uploading changes and patchsets,
+ * comments operations, change edits, etc...
+ */
+ CHANGE_MODIFICATION,
+ /** A ref is updated during merge-change operation. */
+ MERGE_CHANGE,
+ /** A ref is updated as a part of a repo sequence operation. */
+ REPO_SEQ,
+ /** A ref is updated as a part of a repo initialization. */
+ INIT_REPO,
+ /** A ref is udpated as a part of gpg keys modification. */
+ GPG_KEYS_MODIFICATION,
+ /** A ref is updated as a part of group(s) update */
+ GROUPS_UPDATE,
+ /** A ref is updated as a part of account(s) update. */
+ ACCOUNTS_UPDATE,
+ /** A ref is updated as a part of direct push. */
+ DIRECT_PUSH,
+ /** A ref is updated as a part of explicit branch update operation. */
+ BRANCH_MODIFICATION,
+ /** A ref is updated as a part of explicit tag update operation. */
+ TAG_MODIFICATION,
+ /**
+ * A tag is updated as a part of an offline operation.
+ *
+ * <p>Offline operation - an operation which is executed separately from the gerrit server and
+ * can't be triggered by any gerrit API. E.g. schema update.
+ */
+ OFFLINE_OPERATION,
+ /** A tag is updated as a part of an update-superproject flow. */
+ UPDATE_SUPERPROJECT,
+ /** A ref is updated as a part of explicit HEAD update operation. */
+ HEAD_MODIFICATION,
+ /** A ref is updated as a part of versioned meta data change. */
+ VERSIONED_META_DATA_CHANGE,
+ /** A ref is updated as a part of commit-ban operation. */
+ BAN_COMMIT
+ }
+
+ /** Opens a provided context. */
+ protected static <T extends RefUpdateContext> T open(T ctx) {
+ getCurrent().addLast(ctx);
+ return ctx;
+ }
+
+ /** Opens a context of a give type. */
+ public static RefUpdateContext open(RefUpdateType updateType) {
+ checkArgument(updateType != RefUpdateType.OTHER, "The OTHER type is for internal use only.");
+ return open(new RefUpdateContext(updateType));
+ }
+
+ /** Returns the list of opened contexts; the first element is the outermost context. */
+ public static ImmutableList<RefUpdateContext> getOpenedContexts() {
+ return ImmutableList.copyOf(getCurrent());
+ }
+
+ /** Checks if there is an open context of the given type. */
+ public static boolean hasOpen(RefUpdateType type) {
+ return getCurrent().stream().anyMatch(ctx -> ctx.getUpdateType() == type);
+ }
+
+ private final RefUpdateType updateType;
+
+ private RefUpdateContext(RefUpdateType updateType) {
+ this.updateType = updateType;
+ }
+
+ protected RefUpdateContext() {
+ this(RefUpdateType.OTHER);
+ }
+
+ protected static final Deque<RefUpdateContext> getCurrent() {
+ Deque<RefUpdateContext> result = current.get();
+ if (result == null) {
+ result = new ArrayDeque<>();
+ current.set(result);
+ }
+ return result;
+ }
+
+ /**
+ * Returns the type of {@link RefUpdateContext}.
+ *
+ * <p>For descendants, always return {@link RefUpdateType#OTHER}
+ */
+ public final RefUpdateType getUpdateType() {
+ return updateType;
+ }
+
+ /** Closes the current context. */
+ @Override
+ public void close() {
+ Deque<RefUpdateContext> openedContexts = getCurrent();
+ checkState(
+ openedContexts.peekLast() == this, "The current context is different from this context.");
+ openedContexts.removeLast();
+ }
+}
diff --git a/java/com/google/gerrit/testing/BUILD b/java/com/google/gerrit/testing/BUILD
index e5234fe..fb9e64e 100644
--- a/java/com/google/gerrit/testing/BUILD
+++ b/java/com/google/gerrit/testing/BUILD
@@ -5,7 +5,10 @@
testonly = True,
srcs = glob(
["**/*.java"],
- exclude = ["AssertableExecutorService.java"],
+ exclude = [
+ "AssertableExecutorService.java",
+ "TestActionRefUpdateContext.java",
+ ],
),
visibility = ["//visibility:public"],
exports = [
@@ -40,6 +43,7 @@
"//java/com/google/gerrit/server/restapi",
"//java/com/google/gerrit/server/schema",
"//java/com/google/gerrit/server/util/time",
+ "//java/com/google/gerrit/testing:test-ref-update-context",
"//lib:guava",
"//lib:h2",
"//lib:jgit",
@@ -66,3 +70,14 @@
"//lib/truth",
],
)
+
+java_library(
+ name = "test-ref-update-context",
+ testonly = True,
+ srcs = ["TestActionRefUpdateContext.java"],
+ visibility = ["//visibility:public"],
+ deps = [
+ "//java/com/google/gerrit/server",
+ "//lib/errorprone:annotations",
+ ],
+)
diff --git a/java/com/google/gerrit/testing/InMemoryRepositoryManager.java b/java/com/google/gerrit/testing/InMemoryRepositoryManager.java
index 2051ae3..0f70103 100644
--- a/java/com/google/gerrit/testing/InMemoryRepositoryManager.java
+++ b/java/com/google/gerrit/testing/InMemoryRepositoryManager.java
@@ -14,20 +14,55 @@
package com.google.gerrit.testing;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.ACCOUNTS_UPDATE;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.BAN_COMMIT;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.BRANCH_MODIFICATION;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.CHANGE_MODIFICATION;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.DIRECT_PUSH;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.GPG_KEYS_MODIFICATION;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.GROUPS_UPDATE;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.HEAD_MODIFICATION;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.INIT_REPO;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.MERGE_CHANGE;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.OFFLINE_OPERATION;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.REPO_SEQ;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.TAG_MODIFICATION;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.VERSIONED_META_DATA_CHANGE;
+import static java.util.stream.Collectors.toList;
+
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Sets;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.Project.NameKey;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.gpg.PublicKeyStore;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.RepositoryCaseMismatchException;
+import com.google.gerrit.server.update.context.RefUpdateContext;
+import com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType;
import com.google.inject.Inject;
+import java.util.AbstractMap.SimpleImmutableEntry;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.NavigableSet;
+import java.util.Optional;
+import java.util.function.Predicate;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase;
+import org.eclipse.jgit.internal.storage.dfs.DfsReftableBatchRefUpdate;
import org.eclipse.jgit.internal.storage.dfs.DfsRepository;
import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.ProgressMonitor;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
/** Repository manager that uses in-memory repositories. */
public class InMemoryRepositoryManager implements GitRepositoryManager {
@@ -56,6 +91,141 @@
setPerformsAtomicTransactions(true);
}
+ /** Validates that a given ref is updated within the expected context. */
+ private static class RefUpdateContextValidator {
+ /**
+ * A configured singleton for ref context validation.
+ *
+ * <p>Each ref must match no more than 1 special ref from the list below. If ref is not
+ * matched to any special ref predicate, then it is checked against the standard rules - check
+ * the code of the {@link #validateRefUpdateContext} for details.
+ */
+ public static final RefUpdateContextValidator INSTANCE =
+ new RefUpdateContextValidator()
+ .addSpecialRef(RefNames::isSequenceRef, REPO_SEQ)
+ .addSpecialRef(RefNames.HEAD::equals, HEAD_MODIFICATION)
+ .addSpecialRef(RefNames::isRefsChanges, CHANGE_MODIFICATION, MERGE_CHANGE)
+ .addSpecialRef(RefNames::isAutoMergeRef, CHANGE_MODIFICATION)
+ .addSpecialRef(RefNames::isRefsEdit, CHANGE_MODIFICATION, MERGE_CHANGE)
+ .addSpecialRef(RefNames::isTagRef, TAG_MODIFICATION)
+ .addSpecialRef(RefNames::isRejectCommitsRef, BAN_COMMIT)
+ .addSpecialRef(
+ name -> RefNames.isRefsUsers(name) && !RefNames.isRefsEdit(name),
+ VERSIONED_META_DATA_CHANGE,
+ ACCOUNTS_UPDATE,
+ MERGE_CHANGE)
+ .addSpecialRef(
+ RefNames::isConfigRef,
+ VERSIONED_META_DATA_CHANGE,
+ BRANCH_MODIFICATION,
+ MERGE_CHANGE)
+ .addSpecialRef(RefNames::isExternalIdRef, VERSIONED_META_DATA_CHANGE, ACCOUNTS_UPDATE)
+ .addSpecialRef(PublicKeyStore.REFS_GPG_KEYS::equals, GPG_KEYS_MODIFICATION)
+ .addSpecialRef(RefNames::isRefsDraftsComments, CHANGE_MODIFICATION)
+ .addSpecialRef(RefNames::isRefsStarredChanges, CHANGE_MODIFICATION)
+ // A user can create a change for updating a group and then merge it.
+ // The GroupsIT#pushToGroupBranchForReviewForNonAllUsersRepoAndSubmit test verifies
+ // this scenario.
+ .addSpecialRef(RefNames::isGroupRef, GROUPS_UPDATE, MERGE_CHANGE);
+
+ private List<Entry<Predicate<String>, ImmutableList<RefUpdateType>>> specialRefs =
+ new ArrayList<>();
+
+ private RefUpdateContextValidator() {}
+
+ public void validateRefUpdateContext(ReceiveCommand cmd) {
+ if (TestActionRefUpdateContext.isOpen()
+ || RefUpdateContext.hasOpen(OFFLINE_OPERATION)
+ || RefUpdateContext.hasOpen(INIT_REPO)
+ || RefUpdateContext.hasOpen(DIRECT_PUSH)) {
+ // The action can touch any refs in these contexts.
+ return;
+ }
+
+ String refName = cmd.getRefName();
+
+ Optional<ImmutableList<RefUpdateType>> allowedRefUpdateTypes =
+ RefUpdateContextValidator.INSTANCE.getAllowedRefUpdateTypes(refName);
+
+ if (allowedRefUpdateTypes.isPresent()) {
+ checkState(
+ allowedRefUpdateTypes.get().stream().anyMatch(RefUpdateContext::hasOpen)
+ || isTestRepoCall(),
+ "Special ref '%s' is updated outside of the expected operation. Wrap code in the correct RefUpdateContext or fix allowed update types",
+ refName);
+ return;
+ }
+ // It is not one of the special ref - update is possible only within specific contexts.
+ checkState(
+ RefUpdateContext.hasOpen(MERGE_CHANGE)
+ || RefUpdateContext.hasOpen(RefUpdateType.BRANCH_MODIFICATION)
+ || RefUpdateContext.hasOpen(RefUpdateType.UPDATE_SUPERPROJECT)
+ || isTestRepoCall(),
+ "Ordinary ref '%s' is updated outside of the expected operation. Wrap code in the correct RefUpdateContext or add the ref as a special ref.",
+ refName);
+ }
+
+ private RefUpdateContextValidator addSpecialRef(
+ Predicate<String> refNamePredicate, RefUpdateType... validRefUpdateTypes) {
+ specialRefs.add(
+ new SimpleImmutableEntry<Predicate<String>, ImmutableList<RefUpdateType>>(
+ refNamePredicate, ImmutableList.copyOf(validRefUpdateTypes)));
+ return this;
+ }
+
+ private Optional<ImmutableList<RefUpdateType>> getAllowedRefUpdateTypes(String refName) {
+ List<ImmutableList<RefUpdateType>> allowedTypes =
+ specialRefs.stream()
+ .filter(entry -> entry.getKey().test(refName))
+ .map(Entry::getValue)
+ .collect(toList());
+ checkState(
+ allowedTypes.size() <= 1,
+ "refName matches more than 1 predicate. Please fix the specialRefs list, so each reference has no more than one match.");
+ if (allowedTypes.size() == 0) {
+ return Optional.empty();
+ }
+ return Optional.of(allowedTypes.get(0));
+ }
+
+ /**
+ * Returns true if a ref is updated using one of the method in {@link
+ * org.eclipse.jgit.junit.TestRepository}.
+ *
+ * <p>The {@link org.eclipse.jgit.junit.TestRepository} used only in tests and allows to
+ * change refs directly. Wrapping each usage in a test context requires a lot of modification,
+ * so instead we allow any ref updates, which are made using through this class.
+ */
+ private boolean isTestRepoCall() {
+ return Arrays.stream(Thread.currentThread().getStackTrace())
+ .anyMatch(elem -> elem.getClassName().equals("org.eclipse.jgit.junit.TestRepository"));
+ }
+ }
+
+ // The following line will be uncommented in the upcoming changes, after adding
+ // RefUpdateContext to the code.
+ static final boolean VALIDATE_REF_UPDATE_CONTEXT = false;
+
+ @Override
+ protected MemRefDatabase createRefDatabase() {
+ return new MemRefDatabase() {
+ @Override
+ public BatchRefUpdate newBatchUpdate() {
+ DfsObjDatabase odb = getRepository().getObjectDatabase();
+ return new DfsReftableBatchRefUpdate(this, odb) {
+ @Override
+ public void execute(RevWalk rw, ProgressMonitor pm, List<String> options) {
+ if (VALIDATE_REF_UPDATE_CONTEXT) {
+ getCommands().stream()
+ .forEach(RefUpdateContextValidator.INSTANCE::validateRefUpdateContext);
+ }
+ super.execute(rw, pm, options);
+ }
+ };
+ }
+ };
+ }
+
@Override
public Description getDescription() {
return (Description) super.getDescription();
diff --git a/java/com/google/gerrit/testing/TestActionRefUpdateContext.java b/java/com/google/gerrit/testing/TestActionRefUpdateContext.java
new file mode 100644
index 0000000..23ec9aa
--- /dev/null
+++ b/java/com/google/gerrit/testing/TestActionRefUpdateContext.java
@@ -0,0 +1,73 @@
+// Copyright (C) 2023 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.testing;
+
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.gerrit.server.update.context.RefUpdateContext;
+
+/**
+ * Marks ref updates as a test actions.
+ *
+ * <p>This class should be used in tests only to wrap a portion of test code which directly modifies
+ * references. Usage:
+ *
+ * <pre>{@code
+ * import static ...TestActionRefUpdateContext.openTestRefUpdateContext();
+ *
+ * try(RefUpdateContext ctx=openTestRefUpdateContext()) {
+ * // Some test code, which modifies a reference.
+ * }
+ * }</pre>
+ *
+ * or
+ *
+ * <pre>{@code
+ * import static ...TestActionRefUpdateContext.testRefAction;
+ *
+ * testRefAction(() -> {doSomethingWithRef()});
+ * T result = testRefAction(() -> { return doSomethingWithRef()});
+ * }</pre>
+ */
+public final class TestActionRefUpdateContext extends RefUpdateContext {
+ public static boolean isOpen() {
+ return getCurrent().stream().anyMatch(ctx -> ctx instanceof TestActionRefUpdateContext);
+ }
+
+ public static TestActionRefUpdateContext openTestRefUpdateContext() {
+ return open(new TestActionRefUpdateContext());
+ }
+
+ @CanIgnoreReturnValue
+ public static <V, E extends Exception> V testRefAction(CallableWithException<V, E> c) throws E {
+ try (RefUpdateContext ctx = openTestRefUpdateContext()) {
+ return c.call();
+ }
+ }
+
+ public static <E extends Exception> void testRefAction(RunnableWithException<E> c) throws E {
+ try (RefUpdateContext ctx = openTestRefUpdateContext()) {
+ c.run();
+ }
+ }
+
+ public interface CallableWithException<V, E extends Exception> {
+ V call() throws E;
+ }
+
+ @FunctionalInterface
+ public interface RunnableWithException<E extends Exception> {
+ void run() throws E;
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
index 56e23a4..2fe7038 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
@@ -17,6 +17,8 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.project.testing.TestLabels.codeReview;
import static com.google.gerrit.server.project.testing.TestLabels.label;
import static com.google.gerrit.server.project.testing.TestLabels.value;
import static org.eclipse.jgit.lib.Constants.HEAD;
@@ -26,6 +28,8 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.Sandboxed;
+import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.UseTimezone;
import com.google.gerrit.acceptance.VerifyNoPiiInChangeNotes;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
@@ -34,15 +38,21 @@
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.entities.SubmitRequirementExpressionResult;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.server.project.SubmitRequirementsEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.merge.MergeStrategy;
import org.eclipse.jgit.merge.ThreeWayMerger;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -294,6 +304,162 @@
"unexpected base value format");
}
+ @Test
+ public void nonContributorLabelVote_match() throws Exception {
+ requestScopeOperations.setApiUser(user.id());
+ TestRepository<InMemoryRepository> clonedRepo = cloneProject(project, user);
+ PushOneCommit.Result r1 =
+ pushFactory
+ .create(user.newIdent(), clonedRepo, "Subject", "file.txt", "text")
+ .to("refs/for/master");
+
+ Change.Id cId = r1.getChange().getId();
+
+ ChangeInfo changeInfo = gApi.changes().id(r1.getChangeId()).get();
+
+ // Assert on uploader, committer and author
+ assertUploader(changeInfo, user.email());
+ assertCommitter(changeInfo, user.email());
+ assertAuthor(changeInfo, user.email());
+
+ // Vote from admin (a.k.a. non uploader/committer/author) matches
+ requestScopeOperations.setApiUser(admin.id());
+ approve(cId.toString());
+ assertMatching("label:Code-Review=+2,user=non_contributor", cId);
+ // Also make sure magic label votes and > operator work
+ assertMatching("label:Code-Review=MAX,user=non_contributor", cId);
+ assertMatching("label:Code-Review>+1,user=non_contributor", cId);
+ }
+
+ @Test
+ public void nonContributorLabelVote_voteFromUploader_doesNotMatch() throws Exception {
+ PushOneCommit.Result r1 = createNormalCommit(user.newIdent(), "refs/for/master", "file1");
+
+ ChangeInfo changeInfo = gApi.changes().id(r1.getChangeId()).get();
+ assertUploader(changeInfo, admin.email());
+
+ // Vote from admin (a.k.a. uploader) does not match
+ requestScopeOperations.setApiUser(admin.id());
+ approve(r1.getChangeId());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
+ }
+
+ @Test
+ @Sandboxed
+ public void nonContributorLabelVote_voteFromAuthor_doesNotMatch() throws Exception {
+ Account.Id authorId =
+ accountOperations
+ .newAccount()
+ .fullname("author")
+ .preferredEmail("authoremail@example.com")
+ .create();
+ Account.Id committerId =
+ accountOperations
+ .newAccount()
+ .fullname("committer")
+ .preferredEmail("committeremail@example.com")
+ .create();
+
+ Change.Id changeId =
+ changeOperations.newChange().author(authorId).committer(committerId).create();
+ ChangeInfo changeInfo = gApi.changes().id(changeId.get()).get();
+ assertAuthor(changeInfo, "authoremail@example.com");
+
+ allowLabelPermission(
+ codeReview().getName(), RefNames.REFS_HEADS + "*", REGISTERED_USERS, -2, +2);
+
+ // Vote from author does not match
+ requestScopeOperations.setApiUser(authorId);
+ approve(changeId.toString());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", changeId);
+ }
+
+ @Test
+ public void nonContributorLabelVote_voteFromCommitter_doesNotMatch() throws Exception {
+ Account.Id authorId =
+ accountOperations
+ .newAccount()
+ .fullname("author")
+ .preferredEmail("authoremail@example.com")
+ .create();
+ Account.Id committerId =
+ accountOperations
+ .newAccount()
+ .fullname("committer")
+ .preferredEmail("committeremail@example.com")
+ .create();
+
+ Change.Id changeId =
+ changeOperations.newChange().author(authorId).committer(committerId).create();
+ ChangeInfo changeInfo = gApi.changes().id(changeId.get()).get();
+ assertCommitter(changeInfo, "committeremail@example.com");
+
+ allowLabelPermission(
+ codeReview().getName(), RefNames.REFS_HEADS + "*", REGISTERED_USERS, -2, +2);
+
+ // Vote from committer does not match
+ requestScopeOperations.setApiUser(committerId);
+ approve(changeId.toString());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", changeId);
+ }
+
+ @Test
+ public void nonContributorLabelVote_uploaderAndAuthorDifferent() throws Exception {
+ TestRepository<InMemoryRepository> clonedRepo = cloneProject(project, admin);
+ PushOneCommit.Result r1 =
+ pushFactory
+ .create(user.newIdent(), clonedRepo, "Subject", "file.txt", "text")
+ .to("refs/for/master");
+
+ requestScopeOperations.setApiUser(admin.id());
+ ChangeInfo changeInfo = gApi.changes().id(r1.getChangeId()).get();
+ assertUploader(changeInfo, admin.email());
+ assertAuthor(changeInfo, user.email());
+
+ allowLabelPermission(
+ codeReview().getName(), RefNames.REFS_HEADS + "*", REGISTERED_USERS, -2, +2);
+
+ // Vote from admin (a.k.a. uploader) does not match
+ requestScopeOperations.setApiUser(user.id());
+ approve(r1.getChangeId());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
+
+ // Vote from user (a.k.a. author) does not match
+ requestScopeOperations.setApiUser(admin.id());
+ approve(r1.getChangeId());
+ assertNotMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
+
+ // Vote from user2 (a.k.a. non-author and non-uploader) matches
+ TestAccount user2 = accountCreator.create();
+ requestScopeOperations.setApiUser(user2.id());
+ approve(r1.getChangeId());
+ assertMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
+ }
+
+ private static void assertUploader(ChangeInfo changeInfo, String email) {
+ assertThat(changeInfo.revisions.get(changeInfo.currentRevision).uploader.email)
+ .isEqualTo(email);
+ }
+
+ private static void assertCommitter(ChangeInfo changeInfo, String email) {
+ assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.committer.email)
+ .isEqualTo(email);
+ }
+
+ private static void assertAuthor(ChangeInfo changeInfo, String email) {
+ assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.author.email)
+ .isEqualTo(email);
+ }
+
+ private void allowLabelPermission(
+ String labelName, String refPattern, AccountGroup.UUID group, int minVote, int maxVote) {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel(labelName).ref(refPattern).group(group).range(minVote, maxVote))
+ .update();
+ }
+
private PushOneCommit.Result createGitSubmoduleCommit(String ref) throws Exception {
return pushFactory
.create(admin.newIdent(), testRepo, "subject", ImmutableMap.of())
@@ -302,6 +468,13 @@
.to(ref);
}
+ private PushOneCommit.Result createNormalCommit(
+ PersonIdent personIdent, String ref, String fileName) throws Exception {
+ return pushFactory
+ .create(personIdent, testRepo, "subject", ImmutableMap.of(fileName, fileName))
+ .to(ref);
+ }
+
private PushOneCommit.Result createNormalCommit(String ref, String fileName) throws Exception {
return pushFactory
.create(admin.newIdent(), testRepo, "subject", ImmutableMap.of(fileName, fileName))
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 96a8dea..0533f3f 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1435,6 +1435,15 @@
// "count" and "group" args cannot be used simultaneously.
assertThrows(
BadRequestException.class, () -> assertQuery("label:Code-Review=+1,group=gerrit,count=2"));
+
+ // "non_contributor arg for the label operator is not allowed in change queries
+ thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> assertQuery("label:Code-Review=+2,user=non_contributor"));
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo("non_contributor arg is not allowed in change queries");
}
@Test
diff --git a/javatests/com/google/gerrit/server/update/context/BUILD b/javatests/com/google/gerrit/server/update/context/BUILD
new file mode 100644
index 0000000..e580595
--- /dev/null
+++ b/javatests/com/google/gerrit/server/update/context/BUILD
@@ -0,0 +1,14 @@
+load("//tools/bzl:junit.bzl", "junit_tests")
+
+junit_tests(
+ name = "update_context_tests",
+ size = "small",
+ srcs = glob(["*.java"]),
+ deps = [
+ "//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/testing:gerrit-test-util",
+ "//java/com/google/gerrit/testing:test-ref-update-context",
+ "//lib/truth",
+ "//lib/truth:truth-java8-extension",
+ ],
+)
diff --git a/javatests/com/google/gerrit/server/update/context/RefUpdateContextTest.java b/javatests/com/google/gerrit/server/update/context/RefUpdateContextTest.java
new file mode 100644
index 0000000..178d67d
--- /dev/null
+++ b/javatests/com/google/gerrit/server/update/context/RefUpdateContextTest.java
@@ -0,0 +1,93 @@
+// Copyright (C) 2023 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.server.update.context;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.CHANGE_MODIFICATION;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.GROUPS_UPDATE;
+import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.INIT_REPO;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.After;
+import org.junit.Test;
+
+public class RefUpdateContextTest {
+ @After
+ public void tearDown() {
+ // Each test should close all opened context to avoid interference with other tests.
+ assertThat(RefUpdateContext.getOpenedContexts()).isEmpty();
+ }
+
+ @Test
+ public void contextNotOpen() {
+ assertThat(RefUpdateContext.getOpenedContexts()).isEmpty();
+ assertThat(RefUpdateContext.hasOpen(INIT_REPO)).isFalse();
+ assertThat(RefUpdateContext.hasOpen(GROUPS_UPDATE)).isFalse();
+ }
+
+ @Test
+ public void singleContext_openedAndClosedCorrectly() {
+ try (RefUpdateContext ctx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
+ ImmutableList<RefUpdateContext> openedContexts = RefUpdateContext.getOpenedContexts();
+ assertThat(openedContexts).hasSize(1);
+ assertThat(openedContexts.get(0).getUpdateType()).isEqualTo(CHANGE_MODIFICATION);
+ assertThat(RefUpdateContext.hasOpen(CHANGE_MODIFICATION)).isTrue();
+ assertThat(RefUpdateContext.hasOpen(INIT_REPO)).isFalse();
+ }
+
+ assertThat(RefUpdateContext.getOpenedContexts()).isEmpty();
+ assertThat(RefUpdateContext.hasOpen(CHANGE_MODIFICATION)).isFalse();
+ }
+
+ @Test
+ public void nestedContext_openedAndClosedCorrectly() {
+ try (RefUpdateContext ctx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
+ try (RefUpdateContext nestedCtx = RefUpdateContext.open(INIT_REPO)) {
+ ImmutableList<RefUpdateContext> nestedOpenedContexts = RefUpdateContext.getOpenedContexts();
+ assertThat(nestedOpenedContexts).hasSize(2);
+ assertThat(nestedOpenedContexts.get(0).getUpdateType()).isEqualTo(CHANGE_MODIFICATION);
+ assertThat(nestedOpenedContexts.get(1).getUpdateType()).isEqualTo(INIT_REPO);
+ assertThat(RefUpdateContext.hasOpen(CHANGE_MODIFICATION)).isTrue();
+ assertThat(RefUpdateContext.hasOpen(INIT_REPO)).isTrue();
+ assertThat(RefUpdateContext.hasOpen(GROUPS_UPDATE)).isFalse();
+ }
+ ImmutableList<RefUpdateContext> openedContexts = RefUpdateContext.getOpenedContexts();
+ assertThat(openedContexts).hasSize(1);
+ assertThat(openedContexts.get(0).getUpdateType()).isEqualTo(CHANGE_MODIFICATION);
+ assertThat(RefUpdateContext.hasOpen(CHANGE_MODIFICATION)).isTrue();
+ assertThat(RefUpdateContext.hasOpen(INIT_REPO)).isFalse();
+ assertThat(RefUpdateContext.hasOpen(GROUPS_UPDATE)).isFalse();
+ }
+
+ assertThat(RefUpdateContext.getOpenedContexts()).isEmpty();
+ assertThat(RefUpdateContext.hasOpen(CHANGE_MODIFICATION)).isFalse();
+ assertThat(RefUpdateContext.hasOpen(INIT_REPO)).isFalse();
+ assertThat(RefUpdateContext.hasOpen(GROUPS_UPDATE)).isFalse();
+ }
+
+ @Test
+ public void incorrectCloseOrder_exceptionThrown() {
+ try (RefUpdateContext ctx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
+ try (RefUpdateContext nestedCtx = RefUpdateContext.open(INIT_REPO)) {
+ assertThrows(Exception.class, () -> ctx.close());
+ ImmutableList<RefUpdateContext> openedContexts = RefUpdateContext.getOpenedContexts();
+ assertThat(openedContexts).hasSize(2);
+ assertThat(RefUpdateContext.hasOpen(CHANGE_MODIFICATION)).isTrue();
+ assertThat(RefUpdateContext.hasOpen(INIT_REPO)).isTrue();
+ }
+ }
+ }
+}
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 72a6a3a..721d650 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -115,7 +115,6 @@
import {GrFileList} from '../gr-file-list/gr-file-list';
import {EditRevisionInfo, ParsedChangeInfo} from '../../../types/types';
import {
- CloseFixPreviewEvent,
EditableContentSaveEvent,
EventType,
OpenFixPreviewEvent,
@@ -590,8 +589,9 @@
this.addEventListener('editable-content-cancel', () =>
this.handleCommitMessageCancel()
);
- this.addEventListener('open-fix-preview', e => this.onOpenFixPreview(e));
- this.addEventListener('close-fix-preview', e => this.onCloseFixPreview(e));
+ this.addEventListener(EventType.OPEN_FIX_PREVIEW, e =>
+ this.onOpenFixPreview(e)
+ );
this.addEventListener(EventType.SHOW_TAB, e => this.setActiveTab(e));
this.addEventListener('reload', e => {
@@ -1675,10 +1675,6 @@
this.applyFixDialog.open(e);
}
- private onCloseFixPreview(e: CloseFixPreviewEvent) {
- if (e.detail.fixApplied) fireReload(this);
- }
-
// Private but used in tests.
handleToggleDiffMode() {
if (this.diffViewMode === DiffViewMode.SIDE_BY_SIDE) {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index d4defcb..6eaf7ae 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -258,10 +258,6 @@
// Private but used in tests.
@state()
- displayLine?: boolean;
-
- // Private but used in tests.
- @state()
showSizeBars = true;
// For merge commits vs Auto Merge, an extra file row is shown detailing the
@@ -733,9 +729,6 @@
this.shortcutsController.addAbstract(Shortcut.TOGGLE_LEFT_PANE, _ =>
this.handleToggleLeftPane()
);
- this.shortcutsController.addGlobal({key: Key.ESC}, _ =>
- this.handleEscKey()
- );
this.shortcutsController.addAbstract(
Shortcut.EXPAND_ALL_COMMENT_THREADS,
_ => {}
@@ -1079,7 +1072,6 @@
<gr-diff-host
?noAutoRender=${true}
?showLoadFailure=${true}
- .displayLine=${this.displayLine}
.changeNum=${this.changeNum}
.change=${this.change}
.patchRange=${this.patchRange}
@@ -2032,7 +2024,6 @@
e.stopPropagation();
if (this.filesExpanded === FilesExpandedState.ALL) {
this.diffCursor?.moveDown();
- this.displayLine = true;
} else {
this.fileCursor.next({circular: true});
this.selectedIndex = this.fileCursor.index;
@@ -2052,7 +2043,6 @@
e.stopPropagation();
if (this.filesExpanded === FilesExpandedState.ALL) {
this.diffCursor?.moveUp();
- this.displayLine = true;
} else {
this.fileCursor.previous({circular: true});
this.selectedIndex = this.fileCursor.index;
@@ -2504,11 +2494,6 @@
return undefined;
}
- // Private but used in tests.
- handleEscKey() {
- this.displayLine = false;
- }
-
/**
* Compute size bar layout values from the file list.
* Private but used in tests.
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
index 247b7c3..c79be96 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
@@ -2299,22 +2299,6 @@
assert.isTrue(setUrlStub.calledOnce);
});
- test('displayLine', () => {
- element.filesExpanded = FilesExpandedState.ALL;
-
- element.displayLine = false;
- element.handleCursorNext(new KeyboardEvent('keydown'));
- assert.isTrue(element.displayLine);
-
- element.displayLine = false;
- element.handleCursorPrev(new KeyboardEvent('keydown'));
- assert.isTrue(element.displayLine);
-
- element.displayLine = true;
- element.handleEscKey();
- assert.isFalse(element.displayLine);
- });
-
suite('editMode behavior', () => {
test('reviewed checkbox', async () => {
reviewFileStub.restore();
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
index 5e5d7a9..1608c22 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
@@ -20,7 +20,6 @@
import {PROVIDED_FIX_ID} from '../../../utils/comment-util';
import {OpenFixPreviewEvent} from '../../../types/events';
import {getAppContext} from '../../../services/app-context';
-import {fireCloseFixPreview} from '../../../utils/event-util';
import {DiffLayer, ParsedChangeInfo} from '../../../types/types';
import {GrButton} from '../../shared/gr-button/gr-button';
import {TokenHighlightLayer} from '../../../embed/diff/gr-diff-builder/token-highlight-layer';
@@ -38,6 +37,7 @@
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
import {anyLineTooLong} from '../../../embed/diff/gr-diff/gr-diff-utils';
import {changeModelToken} from '../../../models/change/change-model';
+import {fireReload} from '../../../utils/event-util';
interface FilePreview {
filepath: string;
@@ -94,6 +94,9 @@
@state()
isOwner = false;
+ @state()
+ onCloseFixPreviewCallbacks: ((fixapplied: boolean) => void)[] = [];
+
private readonly restApiService = getAppContext().restApiService;
private readonly getUserModel = resolve(this, userModelToken);
@@ -245,6 +248,7 @@
open(e: OpenFixPreviewEvent) {
this.patchNum = e.detail.patchNum;
this.fixSuggestions = e.detail.fixSuggestions;
+ this.onCloseFixPreviewCallbacks = e.detail.onCloseFixPreviewCallbacks;
assert(this.fixSuggestions.length > 0, 'no fix in the event');
this.selectedFixIdx = 0;
this.applyFixModal?.showModal();
@@ -330,8 +334,9 @@
this.currentPreviews = [];
this.isApplyFixLoading = false;
- fireCloseFixPreview(this, fixApplied);
+ this.onCloseFixPreviewCallbacks.forEach(fn => fn(fixApplied));
this.applyFixModal?.close();
+ if (fixApplied) fireReload(this);
}
private computeTooltip() {
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
index 9a1b488..9284fb2 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog_test.ts
@@ -17,11 +17,7 @@
} from '../../../test/test-data-generators';
import {createDefaultDiffPrefs} from '../../../constants/constants';
import {DiffInfo} from '../../../types/diff';
-import {
- CloseFixPreviewEventDetail,
- EventType,
- OpenFixPreviewEventDetail,
-} from '../../../types/events';
+import {EventType, OpenFixPreviewEventDetail} from '../../../types/events';
import {GrButton} from '../../shared/gr-button/gr-button';
import {fixture, html, assert} from '@open-wc/testing';
import {SinonStub} from 'sinon';
@@ -37,11 +33,13 @@
createFixSuggestionInfo('fix_1'),
createFixSuggestionInfo('fix_2'),
],
+ onCloseFixPreviewCallbacks: [],
};
const ONE_FIX: OpenFixPreviewEventDetail = {
patchNum: 2 as PatchSetNum,
fixSuggestions: [createFixSuggestionInfo('fix_1')],
+ onCloseFixPreviewCallbacks: [],
};
function getConfirmButton(): GrButton {
@@ -261,11 +259,7 @@
element.currentFix = createFixSuggestionInfo('123');
const closeFixPreviewEventSpy = sinon.spy();
- // Element is recreated after each test, removeEventListener isn't required
- element.addEventListener(
- EventType.CLOSE_FIX_PREVIEW,
- closeFixPreviewEventSpy
- );
+ element.onCloseFixPreviewCallbacks.push(closeFixPreviewEventSpy);
await element.handleApplyFix(new CustomEvent('confirm'));
@@ -278,14 +272,7 @@
assert.isTrue(setUrlStub.called);
assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/2..edit');
- sinon.assert.calledOnceWithExactly(
- closeFixPreviewEventSpy,
- new CustomEvent<CloseFixPreviewEventDetail>(EventType.CLOSE_FIX_PREVIEW, {
- detail: {
- fixApplied: true,
- },
- })
- );
+ sinon.assert.calledOnceWithExactly(closeFixPreviewEventSpy, true);
// reset gr-apply-fix-dialog and close
assert.equal(element.currentFix, undefined);
assert.equal(element.currentPreviews.length, 0);
@@ -326,11 +313,7 @@
element.currentFix = createFixSuggestionInfo('fix_123');
const closeFixPreviewEventSpy = sinon.spy();
- // Element is recreated after each test, removeEventListener isn't required
- element.addEventListener(
- EventType.CLOSE_FIX_PREVIEW,
- closeFixPreviewEventSpy
- );
+ element.onCloseFixPreviewCallbacks.push(closeFixPreviewEventSpy);
let expectedError;
await element.handleApplyFix(new CustomEvent('click')).catch(e => {
@@ -343,19 +326,8 @@
test('onCancel fires close with correct parameters', () => {
const closeFixPreviewEventSpy = sinon.spy();
- // Element is recreated after each test, removeEventListener isn't required
- element.addEventListener(
- EventType.CLOSE_FIX_PREVIEW,
- closeFixPreviewEventSpy
- );
+ element.onCloseFixPreviewCallbacks.push(closeFixPreviewEventSpy);
element.onCancel(new CustomEvent('cancel'));
- sinon.assert.calledOnceWithExactly(
- closeFixPreviewEventSpy,
- new CustomEvent<CloseFixPreviewEventDetail>(EventType.CLOSE_FIX_PREVIEW, {
- detail: {
- fixApplied: false,
- },
- })
- );
+ sinon.assert.calledOnceWithExactly(closeFixPreviewEventSpy, false);
});
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 23dce97..fd0e5d2 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -171,9 +171,6 @@
@property({type: String})
projectName?: RepoName;
- @property({type: Boolean})
- displayLine = false;
-
@state()
private _isImageDiff = false;
@@ -522,7 +519,6 @@
.noAutoRender=${this.noAutoRender}
.path=${this.path}
.prefs=${this.prefs}
- .displayLine=${this.displayLine}
.isImageDiff=${this.isImageDiff}
.noRenderOnPrefsChange=${this.noRenderOnPrefsChange}
.renderPrefs=${this.renderPrefs}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
index be03afd..7a32c27 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.ts
@@ -774,14 +774,6 @@
assert.equal(element.diffElement.prefs, value);
});
- test('passes in displayLine', async () => {
- const value = true;
- element.displayLine = value;
- await element.updateComplete;
- assertIsDefined(element.diffElement);
- assert.equal(element.diffElement.displayLine, value);
- });
-
test('passes in hidden', async () => {
const value = true;
element.hidden = value;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index c7d6948..19186ec 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -56,7 +56,7 @@
import {OpenFixPreviewEvent, ValueChangedEvent} from '../../../types/events';
import {fireAlert, fireEvent, fireTitleChange} from '../../../utils/event-util';
import {assertIsDefined, queryAndAssert} from '../../../utils/common-util';
-import {Key, toggleClass, whenVisible} from '../../../utils/dom-util';
+import {toggleClass, whenVisible} from '../../../utils/dom-util';
import {CursorMoveResult} from '../../../api/core';
import {throttleWrap} from '../../../utils/async-util';
import {filter, take, switchMap} from 'rxjs/operators';
@@ -344,10 +344,6 @@
);
listen(Shortcut.EXPAND_ALL_COMMENT_THREADS, _ => {}); // docOnly
listen(Shortcut.COLLAPSE_ALL_COMMENT_THREADS, _ => {}); // docOnly
- this.shortcutsController.addGlobal({key: Key.ESC}, _ => {
- assertIsDefined(this.diffHost, 'diffHost');
- this.diffHost.displayLine = false;
- });
}
private setupSubscriptions() {
@@ -1067,7 +1063,6 @@
private handlePrevLine() {
assertIsDefined(this.diffHost, 'diffHost');
- this.diffHost.displayLine = true;
this.cursor?.moveUp();
}
@@ -1102,7 +1097,6 @@
private handleNextLine() {
assertIsDefined(this.diffHost, 'diffHost');
- this.diffHost.displayLine = true;
this.cursor?.moveDown();
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
index 889e9dd..6507046 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
@@ -61,7 +61,6 @@
import {GrDiffModeSelector} from '../../../embed/diff/gr-diff-mode-selector/gr-diff-mode-selector';
import {fixture, html, assert} from '@open-wc/testing';
import {EventType} from '../../../types/events';
-import {Key} from '../../../utils/dom-util';
import {GrButton} from '../../shared/gr-button/gr-button';
import {testResolver} from '../../../test/common-test-setup';
import {UserModel, userModelToken} from '../../../models/user/user-model';
@@ -459,15 +458,6 @@
element.diffHost.diffElement.viewMode,
DiffViewMode.SIDE_BY_SIDE
);
- assert.isTrue(element.diffHost.diffElement.displayLine);
-
- pressKey(element, Key.ESC);
- await element.updateComplete;
- assert.equal(
- element.diffHost.diffElement.viewMode,
- DiffViewMode.SIDE_BY_SIDE
- );
- assert.isFalse(element.diffHost.diffElement.displayLine);
const setReviewedStub = sinon.stub(element, 'setReviewed');
const handleToggleSpy = sinon.spy(element, 'handleToggleFileReviewed');
diff --git a/polygerrit-ui/app/elements/gr-app-global-var-init.ts b/polygerrit-ui/app/elements/gr-app-global-var-init.ts
index da9c0c9..d66fec0 100644
--- a/polygerrit-ui/app/elements/gr-app-global-var-init.ts
+++ b/polygerrit-ui/app/elements/gr-app-global-var-init.ts
@@ -14,11 +14,27 @@
import {GrAnnotation} from '../embed/diff/gr-diff-highlight/gr-annotation';
import {GrPluginActionContext} from './shared/gr-js-api-interface/gr-plugin-action-context';
import {AppContext, injectAppContext} from '../services/app-context';
-import {Finalizable} from '../services/registry';
import {PluginLoader} from './shared/gr-js-api-interface/gr-plugin-loader';
+import {
+ initVisibilityReporter,
+ initPerformanceReporter,
+ initErrorReporter,
+ initWebVitals,
+} from '../services/gr-reporting/gr-reporting_impl';
+import {Finalizable} from '../services/registry';
-export function initGlobalVariables(appContext: AppContext & Finalizable) {
+export function initGlobalVariables(
+ appContext: AppContext & Finalizable,
+ initializeReporting: boolean
+) {
injectAppContext(appContext);
+ if (initializeReporting) {
+ const reportingService = appContext.reportingService;
+ initVisibilityReporter(reportingService);
+ initPerformanceReporter(reportingService);
+ initWebVitals(reportingService);
+ initErrorReporter(reportingService);
+ }
window.GrAnnotation = GrAnnotation;
window.GrPluginActionContext = GrPluginActionContext;
}
diff --git a/polygerrit-ui/app/elements/gr-app.ts b/polygerrit-ui/app/elements/gr-app.ts
index 645f94a..ded0626 100644
--- a/polygerrit-ui/app/elements/gr-app.ts
+++ b/polygerrit-ui/app/elements/gr-app.ts
@@ -37,12 +37,6 @@
createAppDependencies,
Creator,
} from '../services/app-context-init';
-import {
- initVisibilityReporter,
- initPerformanceReporter,
- initErrorReporter,
- initWebVitals,
-} from '../services/gr-reporting/gr-reporting_impl';
import {html, LitElement} from 'lit';
import {customElement} from 'lit/decorators.js';
import {
@@ -50,14 +44,9 @@
serviceWorkerInstallerToken,
} from '../services/service-worker-installer';
import {pluginLoaderToken} from './shared/gr-js-api-interface/gr-plugin-loader';
+import {getAppContext} from '../services/app-context';
-const appContext = createAppContext();
-initGlobalVariables(appContext);
-const reportingService = appContext.reportingService;
-initVisibilityReporter(reportingService);
-initPerformanceReporter(reportingService);
-initWebVitals(reportingService);
-initErrorReporter(reportingService);
+initGlobalVariables(createAppContext(), true);
installPolymerResin(safeTypesBridge);
@@ -97,7 +86,7 @@
};
for (const [token, creator] of createAppDependencies(
- appContext,
+ getAppContext(),
resolver
)) {
injectDependency(token, creator);
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 1ac89d6..c844d42 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -1053,6 +1053,11 @@
replacement
),
patchNum: this.comment.patch_set,
+ onCloseFixPreviewCallbacks: [
+ fixApplied => {
+ if (fixApplied) this.handleAppliedFix();
+ },
+ ],
};
}
if (isRobot(this.comment) && this.comment.fix_suggestions.length > 0) {
@@ -1065,6 +1070,7 @@
};
}),
patchNum: this.comment.patch_set,
+ onCloseFixPreviewCallbacks: [],
};
}
throw new Error('unable to create preview fix event');
@@ -1132,6 +1138,18 @@
fire(this, 'reply-to-comment', eventDetail);
}
+ private handleAppliedFix() {
+ const message = this.comment?.message;
+ assert(!!message, 'empty message');
+ const eventDetail: ReplyToCommentEventDetail = {
+ content: 'Fix applied.',
+ userWantsToEdit: false,
+ unresolved: false,
+ };
+ // Handled by <gr-comment-thread>.
+ fire(this, 'reply-to-comment', eventDetail);
+ }
+
private async handleShowFix() {
// Handled top-level in the diff and change view components.
fire(this, 'open-fix-preview', await this.createFixPreview());
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
index 3187ada..fcebeea 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
@@ -69,7 +69,7 @@
});
test('does not apply rewrites within links', async () => {
- element.content = 'google.com/LinkRewriteMe';
+ element.content = 'http://google.com/LinkRewriteMe';
await element.updateComplete;
assert.shadowDom.equal(
@@ -81,7 +81,7 @@
rel="noopener"
target="_blank"
>
- google.com/LinkRewriteMe
+ http://google.com/LinkRewriteMe
</a>
</pre>
`
@@ -149,20 +149,25 @@
});
test('renders text with links and rewrites', async () => {
- element.content = `text with plain link: google.com
- \ntext with config link: LinkRewriteMe
- \ntext with complex link: A Link 12`;
+ element.content = `
+ text with plain link: http://google.com
+ text with config link: LinkRewriteMe
+ text with complex link: A Link 12`;
await element.updateComplete;
assert.shadowDom.equal(
element,
/* HTML */ `
<pre class="plaintext">
- text with plain link:
- <a href="http://google.com" rel="noopener" target="_blank">
- google.com
- </a>
- text with config link:
+ text with plain link:
+ <a
+ href="http://google.com"
+ rel="noopener"
+ target="_blank"
+ >
+ http://google.com
+ </a>
+ text with config link:
<a
href="http://google.com/LinkRewriteMe"
rel="noopener"
@@ -212,7 +217,7 @@
});
test('renders text with links and rewrites', async () => {
element.content = `text
- \ntext with plain link: google.com
+ \ntext with plain link: http://google.com
\ntext with config link: LinkRewriteMe
\ntext without a link: NotA Link 15 cats
\ntext with complex link: A Link 12`;
@@ -227,7 +232,7 @@
<p>
text with plain link:
<a href="http://google.com" rel="noopener" target="_blank">
- google.com
+ http://google.com
</a>
</p>
<p>
@@ -259,7 +264,7 @@
test('does not render if too long', async () => {
element.content = `text
- text with plain link: google.com
+ text with plain link: http://google.com
text with config link: LinkRewriteMe
text without a link: NotA Link 15 cats
text with complex link: A Link 12`;
@@ -272,8 +277,14 @@
<pre class="plaintext">
text
text with plain link:
- <a href="http://google.com" rel="noopener" target="_blank">google.com</a>
- text with config link:
+ <a
+ href="http://google.com"
+ rel="noopener"
+ target="_blank"
+ >
+ http://google.com
+ </a>
+ text with config link:
<a
href="http://google.com/LinkRewriteMe"
rel="noopener"
@@ -302,7 +313,7 @@
\n#### h4-heading
\n##### h5-heading
\n###### h6-heading
- \n# heading with plain link: google.com
+ \n# heading with plain link: http://google.com
\n# heading with config link: LinkRewriteMe`;
await element.updateComplete;
@@ -320,7 +331,7 @@
<h1>
heading with plain link:
<a href="http://google.com" rel="noopener" target="_blank">
- google.com
+ http://google.com
</a>
</h1>
<h1>
@@ -480,7 +491,7 @@
test('renders block quotes with links and rewrites', async () => {
element.content = `> block quote
- \n> block quote with plain link: google.com
+ \n> block quote with plain link: http://google.com
\n> block quote with config link: LinkRewriteMe`;
await element.updateComplete;
@@ -496,7 +507,7 @@
<p>
block quote with plain link:
<a href="http://google.com" rel="noopener" target="_blank">
- google.com
+ http://google.com
</a>
</p>
</blockquote>
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
index a0526ed..54bc46d 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -141,9 +141,6 @@
renderPrefs?: RenderPreferences;
@property({type: Boolean})
- displayLine = false;
-
- @property({type: Boolean})
isImageDiff?: boolean;
@property({type: Boolean, reflect: true})
@@ -1081,7 +1078,6 @@
unified: this.viewMode === DiffViewMode.UNIFIED,
sideBySide: this.viewMode === DiffViewMode.SIDE_BY_SIDE,
canComment: this.loggedIn,
- displayLine: this.displayLine,
};
return html`
<div class=${classMap(cssClasses)} @click=${this.handleTap}>
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
index ce1393d..2855ae6 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff_test.ts
@@ -3141,18 +3141,6 @@
assert.isFalse(element.classList.contains('no-left'));
});
- test('view does not start with displayLine classList', () => {
- const container = queryAndAssert(element, '.diffContainer');
- assert.isFalse(container.classList.contains('displayLine'));
- });
-
- test('displayLine class added when displayLine is true', async () => {
- element.displayLine = true;
- await element.updateComplete;
- const container = queryAndAssert(element, '.diffContainer');
- assert.isTrue(container.classList.contains('displayLine'));
- });
-
suite('binary diffs', () => {
test('render binary diff', async () => {
element.prefs = {
diff --git a/polygerrit-ui/app/models/checks/checks-util.ts b/polygerrit-ui/app/models/checks/checks-util.ts
index ba43eb4..da05bbe 100644
--- a/polygerrit-ui/app/models/checks/checks-util.ts
+++ b/polygerrit-ui/app/models/checks/checks-util.ts
@@ -121,6 +121,7 @@
const eventDetail: OpenFixPreviewEventDetail = {
patchNum: result.patchset as PatchSetNumber,
fixSuggestions,
+ onCloseFixPreviewCallbacks: [],
};
return {
name: 'Show Fix',
diff --git a/polygerrit-ui/app/node_modules_licenses/licenses.ts b/polygerrit-ui/app/node_modules_licenses/licenses.ts
index c65e7a31..155dd6c 100644
--- a/polygerrit-ui/app/node_modules_licenses/licenses.ts
+++ b/polygerrit-ui/app/node_modules_licenses/licenses.ts
@@ -329,14 +329,6 @@
license: SharedLicenses.Polymer2018,
},
{
- name: 'ba-linkify',
- license: {
- name: 'ba-linkify',
- type: LicenseTypes.Mit,
- packageLicenseFile: 'LICENSE-MIT',
- },
- },
- {
name: 'codemirror-minified',
license: {
name: 'codemirror-minified',
diff --git a/polygerrit-ui/app/package.json b/polygerrit-ui/app/package.json
index 0df1eda..41f4043 100644
--- a/polygerrit-ui/app/package.json
+++ b/polygerrit-ui/app/package.json
@@ -34,7 +34,6 @@
"@types/resize-observer-browser": "^0.1.5",
"@webcomponents/shadycss": "^1.10.2",
"@webcomponents/webcomponentsjs": "^1.3.3",
- "ba-linkify": "^1.0.1",
"codemirror-minified": "^5.65.0",
"highlight.js": "^11.5.0",
"highlightjs-closure-templates": "https://github.com/highlightjs/highlightjs-closure-templates",
@@ -50,4 +49,4 @@
},
"license": "Apache-2.0",
"private": true
-}
\ No newline at end of file
+}
diff --git a/polygerrit-ui/app/test/common-test-setup.ts b/polygerrit-ui/app/test/common-test-setup.ts
index aed58d8..365bb16 100644
--- a/polygerrit-ui/app/test/common-test-setup.ts
+++ b/polygerrit-ui/app/test/common-test-setup.ts
@@ -6,7 +6,7 @@
// TODO(dmfilippov): remove bundled-polymer.js imports when the following issue
// https://github.com/Polymer/polymer-resin/issues/9 is resolved.
import '../scripts/bundled-polymer';
-import {AppContext} from '../services/app-context';
+import {getAppContext} from '../services/app-context';
import {Finalizable} from '../services/registry';
import {
createTestAppContext,
@@ -60,7 +60,6 @@
});
let testSetupTimestampMs = 0;
-let appContext: AppContext & Finalizable;
const injectedDependencies: Map<
DependencyToken<unknown>,
@@ -101,11 +100,10 @@
// If the following asserts fails - then window.stub is
// overwritten by some other code.
assert.equal(getCleanupsCount(), 0);
- appContext = createTestAppContext();
- initGlobalVariables(appContext);
+ initGlobalVariables(createTestAppContext(), false);
- finalizers.push(appContext);
- const dependencies = createTestDependencies(appContext, testResolver);
+ finalizers.push(getAppContext());
+ const dependencies = createTestDependencies(getAppContext(), testResolver);
for (const [token, provider] of dependencies) {
injectDependency(token, provider);
}
@@ -124,7 +122,7 @@
// `awaitPluginsLoaded` will rely on that to kick off,
// in testing, we want to kick start this earlier.
testResolver(pluginLoaderToken).loadPlugins([]);
- testOnlyResetGrRestApiSharedObjects(appContext.authService);
+ testOnlyResetGrRestApiSharedObjects(getAppContext().authService);
});
export function removeRequestDependencyListener() {
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index 2cc6742..e2612b0 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -26,7 +26,6 @@
MOVED_LINK_CLICKED = 'moved-link-clicked',
NETWORK_ERROR = 'network-error',
OPEN_FIX_PREVIEW = 'open-fix-preview',
- CLOSE_FIX_PREVIEW = 'close-fix-preview',
PAGE_ERROR = 'page-error',
RELOAD = 'reload',
REPLY = 'reply',
@@ -63,7 +62,6 @@
'line-cursor-moved-out': LineNumberEvent;
'moved-link-clicked': MovedLinkClickedEvent;
'open-fix-preview': OpenFixPreviewEvent;
- 'close-fix-preview': CloseFixPreviewEvent;
'reply-to-comment': ReplyToCommentEvent;
/* prettier-ignore */
'reload': ReloadEvent;
@@ -156,13 +154,10 @@
export interface OpenFixPreviewEventDetail {
patchNum: PatchSetNum;
fixSuggestions: FixSuggestionInfo[];
+ onCloseFixPreviewCallbacks: ((fixapplied: boolean) => void)[];
}
export type OpenFixPreviewEvent = CustomEvent<OpenFixPreviewEventDetail>;
-export interface CloseFixPreviewEventDetail {
- fixApplied: boolean;
-}
-export type CloseFixPreviewEvent = CustomEvent<CloseFixPreviewEventDetail>;
export interface ReplyToCommentEventDetail {
content: string;
userWantsToEdit: boolean;
diff --git a/polygerrit-ui/app/utils/event-util.ts b/polygerrit-ui/app/utils/event-util.ts
index 714955b..49d5382 100644
--- a/polygerrit-ui/app/utils/event-util.ts
+++ b/polygerrit-ui/app/utils/event-util.ts
@@ -103,10 +103,6 @@
fire(target, EventType.SHOW_TAB, detail);
}
-export function fireCloseFixPreview(target: EventTarget, fixApplied: boolean) {
- fire(target, EventType.CLOSE_FIX_PREVIEW, {fixApplied});
-}
-
export function fireReload(target: EventTarget, clearPatchset?: boolean) {
fire(target, EventType.RELOAD, {clearPatchset: !!clearPatchset});
}
diff --git a/polygerrit-ui/app/utils/link-util.ts b/polygerrit-ui/app/utils/link-util.ts
index ec1e7e7..ccafa5a 100644
--- a/polygerrit-ui/app/utils/link-util.ts
+++ b/polygerrit-ui/app/utils/link-util.ts
@@ -3,7 +3,6 @@
* Copyright 2022 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import 'ba-linkify/ba-linkify';
import {CommentLinkInfo, CommentLinks} from '../types/common';
import {getBaseUrl} from './url-util';
@@ -16,21 +15,8 @@
base: string,
repoCommentLinks: CommentLinks
): string {
- const parts: string[] = [];
- window.linkify(insertZeroWidthSpace(base), {
- callback: (text, href) => {
- if (href) {
- parts.push(removeZeroWidthSpace(createLinkTemplate(href, text)));
- } else {
- const rewriteResults = getRewriteResultsFromConfig(
- text,
- repoCommentLinks
- );
- parts.push(removeZeroWidthSpace(applyRewrites(text, rewriteResults)));
- }
- },
- });
- return parts.join('');
+ const rewriteResults = getRewriteResultsFromConfig(base, repoCommentLinks);
+ return applyRewrites(base, rewriteResults);
}
/**
@@ -47,6 +33,11 @@
commentLinkInfo =>
commentLinkInfo.enabled !== false && commentLinkInfo.link !== undefined
);
+ // Always linkify URLs starting with https?://
+ enabledRewrites.push({
+ match: '(https?://\\S+[\\w/])',
+ link: '$1',
+ });
return enabledRewrites.flatMap(rewrite => {
const regexp = new RegExp(rewrite.match, 'g');
const partialResults: RewriteResult[] = [];
@@ -141,22 +132,6 @@
);
}
-/**
- * Some tools are known to look for reviewers/CCs by finding lines such as
- * "R=foo@gmail.com, bar@gmail.com". However, "=" is technically a valid email
- * character, so ba-linkify interprets the entire string "R=foo@gmail.com" as an
- * email address. To fix this, we insert a zero width space character \u200B
- * before linking that prevents ba-linkify from associating the prefix with the
- * email. After linking we remove the zero width space.
- */
-function insertZeroWidthSpace(base: string) {
- return base.replace(/^(R=|CC=)/g, '$&\u200B');
-}
-
-function removeZeroWidthSpace(base: string) {
- return base.replace(/\u200B/g, '');
-}
-
function createLinkTemplate(
href: string,
displayText: string,
diff --git a/polygerrit-ui/app/utils/link-util_test.ts b/polygerrit-ui/app/utils/link-util_test.ts
index f5c13e8..52b8288 100644
--- a/polygerrit-ui/app/utils/link-util_test.ts
+++ b/polygerrit-ui/app/utils/link-util_test.ts
@@ -12,6 +12,17 @@
}
suite('link rewrites', () => {
+ test('default linking', () => {
+ assert.equal(
+ linkifyUrlsAndApplyRewrite('http://www.google.com', {}),
+ link('http://www.google.com', 'http://www.google.com')
+ );
+ assert.equal(
+ linkifyUrlsAndApplyRewrite('https://www.google.com', {}),
+ link('https://www.google.com', 'https://www.google.com')
+ );
+ });
+
test('without text', () => {
assert.equal(
linkifyUrlsAndApplyRewrite('foo', {
@@ -63,18 +74,6 @@
`${link('foo', 'foo.gov')} ${link('foo', 'foo.gov')}`
);
});
-
- test('does not apply within normal links', () => {
- assert.equal(
- linkifyUrlsAndApplyRewrite('google.com', {
- ogle: {
- match: 'ogle',
- link: 'gerritcodereview.com',
- },
- }),
- link('google.com', 'http://google.com')
- );
- });
});
test('for overlapping rewrites prefer the latest ending', () => {
@@ -165,45 +164,4 @@
)}`
);
});
-
- suite('normal links', () => {
- test('links urls', () => {
- const googleLink = link('google.com', 'http://google.com');
- const mapsLink = link('maps.google.com', 'http://maps.google.com');
-
- assert.equal(
- linkifyUrlsAndApplyRewrite('google.com, maps.google.com', {}),
- `${googleLink}, ${mapsLink}`
- );
- });
-
- test('links emails without including R= prefix', () => {
- const fooEmail = link('foo@gmail.com', 'mailto:foo@gmail.com');
- const barEmail = link('bar@gmail.com', 'mailto:bar@gmail.com');
- assert.equal(
- linkifyUrlsAndApplyRewrite('R=foo@gmail.com, bar@gmail.com', {}),
- `R=${fooEmail}, ${barEmail}`
- );
- });
-
- test('links emails without including CC= prefix', () => {
- const fooEmail = link('foo@gmail.com', 'mailto:foo@gmail.com');
- const barEmail = link('bar@gmail.com', 'mailto:bar@gmail.com');
- assert.equal(
- linkifyUrlsAndApplyRewrite('CC=foo@gmail.com, bar@gmail.com', {}),
- `CC=${fooEmail}, ${barEmail}`
- );
- });
-
- test('links emails maintains R= and CC= within addresses', () => {
- const fooBarBazEmail = link(
- 'fooR=barCC=baz@gmail.com',
- 'mailto:fooR=barCC=baz@gmail.com'
- );
- assert.equal(
- linkifyUrlsAndApplyRewrite('fooR=barCC=baz@gmail.com', {}),
- fooBarBazEmail
- );
- });
- });
});
diff --git a/polygerrit-ui/app/yarn.lock b/polygerrit-ui/app/yarn.lock
index 2edce7e..7f8168e 100644
--- a/polygerrit-ui/app/yarn.lock
+++ b/polygerrit-ui/app/yarn.lock
@@ -488,11 +488,6 @@
delegates "^1.0.0"
readable-stream "^2.0.6"
-ba-linkify@^1.0.1:
- version "1.0.1"
- resolved "https://registry.yarnpkg.com/ba-linkify/-/ba-linkify-1.0.1.tgz#664cf5744947c6e8611f1fbbaf7d9f315f982f4c"
- integrity sha1-Zkz1dElHxuhhHx+7r32fMV+YL0w=
-
balanced-match@^1.0.0:
version "1.0.2"
resolved "https://registry.yarnpkg.com/balanced-match/-/balanced-match-1.0.2.tgz#e83e3a7e3f300b34cb9d87f615fa0cbf357690ee"
