Merge "Add Typescript test migration instruction to README.md"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 6b73482..c74b2c3 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5935,6 +5935,8 @@
If a user is added while already in the attention set, the
request is silently ignored.
+The user must be a reviewer, cc, uploader, or owner on the change.
+
.Request
----
POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/attention HTTP/1.0
@@ -7662,7 +7664,8 @@
`ready` and `work_in_progress` to be true.
|`add_to_attention_set` |optional|
list of link:#attention-set-input[AttentionSetInput] entities to add
-to the link:#attention-set[attention set].
+to the link:#attention-set[attention set]. Users that are not reviewers,
+ccs, owner, or uploader are silently ignored.
|`remove_from_attention_set` |optional|
list of link:#attention-set-input[AttentionSetInput] entities to remove
from the link:#attention-set[attention set].
diff --git a/Documentation/user-named-destinations.txt b/Documentation/user-named-destinations.txt
index 1b6f143..a1ab258 100644
--- a/Documentation/user-named-destinations.txt
+++ b/Documentation/user-named-destinations.txt
@@ -13,6 +13,7 @@
row in a destination file represents a single destination in the
named set. The left column represents the ref of the destination,
and the right column represents the project of the destination.
+The named destinations can be publicly accessible by other users.
Example destination file named `destinations/myreviews`:
diff --git a/Documentation/user-named-queries.txt b/Documentation/user-named-queries.txt
index e79b3da..c01f790 100644
--- a/Documentation/user-named-queries.txt
+++ b/Documentation/user-named-queries.txt
@@ -7,7 +7,8 @@
link:intro-user.html#user-refs[user's ref] in the `All-Users` project. The
user's queries file is a 2 column tab delimited file. The left
column represents the name of the query, and the right column
-represents the query expression represented by the name.
+represents the query expression represented by the name. The named queries
+can be publicly accessible by other users.
Example queries file:
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index ffe889f..5f18e9b 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -106,9 +106,11 @@
that was scraped out of the commit message.
[[destination]]
-destination:'NAME'::
+destination:'[name=]NAME[,user=USER]'::
+
-Changes which match the current user's destination named 'NAME'.
+Changes which match the specified USER's destination named 'NAME'. If 'USER'
+is unspecified, the current user is used. The named destinations can be
+publicly accessible by other users.
(see link:user-named-destinations.html[Named Destinations]).
[[owner]]
@@ -123,9 +125,11 @@
Changes originally submitted by a user in 'GROUP'.
[[query]]
-query:'NAME'::
+query:'[name=]NAME[,user=USER]'::
+
-Changes which match the current user's query named 'NAME'
+Changes which match the specified USER's query named 'NAME'. If 'USER'
+is unspecified, the current user is used. The named queries can be
+publicly accessible by other users.
(see link:user-named-queries.html[Named Queries]).
[[reviewer]]
@@ -304,6 +308,11 @@
`file:"^name[1-3].xml"`.
+
Slash ('/') is used path separator.
++
+More examples:
+* `-file:^path/.*` - changes that do not modify files from `path/`,
+* `file:{^~(path/.*)}` - changes that modify files not from `path/` (but may
+contain files from `path/`).
[[file]]
file:'NAME', f:'NAME'::
@@ -524,7 +533,7 @@
For example, added:>50 will be true for any change which adds at least 50
lines.
+
-Valid relations are >=, >, <=, <, or no relation, which will match if the
+Valid relations are >=, >, \<=, <, or no relation, which will match if the
number of lines is exactly equal.
[[commentby]]
@@ -576,7 +585,7 @@
For example, unresolved:>0 will be true for any change which has at least one unresolved
comment while unresolved:0 will be true for any change which has all comments resolved.
+
-Valid relations are >=, >, <=, <, or no relation, which will match if the number of unresolved
+Valid relations are >=, >, \<=, <, or no relation, which will match if the number of unresolved
comments is exactly equal.
== Argument Quoting
@@ -693,7 +702,7 @@
Matches changes with a +1 code review where the reviewer is in the
ldap/linux.workflow group.
-`label:Code-Review<=-1`::
+`label:Code-Review\<=-1`::
+
Matches changes with either a -1, -2, or any lower score.
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 8323cfd..0cfc6cb 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -244,7 +244,7 @@
Provider<ConsistencyChecker> checkerProvider,
ActionJson actionJson,
ChangeNotes.Factory notesFactory,
- LabelsJson.Factory labelsJsonFactory,
+ LabelsJson labelsJson,
RemoveReviewerControl removeReviewerControl,
TrackingFooters trackingFooters,
Metrics metrics,
@@ -261,7 +261,7 @@
this.checkerProvider = checkerProvider;
this.actionJson = actionJson;
this.notesFactory = notesFactory;
- this.labelsJson = labelsJsonFactory.create(options);
+ this.labelsJson = labelsJson;
this.removeReviewerControl = removeReviewerControl;
this.trackingFooters = trackingFooters;
this.metrics = metrics;
diff --git a/java/com/google/gerrit/server/change/LabelsJson.java b/java/com/google/gerrit/server/change/LabelsJson.java
index b1d154c..76992e8 100644
--- a/java/com/google/gerrit/server/change/LabelsJson.java
+++ b/java/com/google/gerrit/server/change/LabelsJson.java
@@ -42,17 +42,15 @@
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.VotingRangeInfo;
-import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.account.AccountLoader;
-import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
-import com.google.inject.assistedinject.Assisted;
+import com.google.inject.Singleton;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
@@ -68,28 +66,15 @@
/**
* Produces label-related entities, like {@link LabelInfo}s, which is serialized to JSON afterwards.
*/
+@Singleton
public class LabelsJson {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- public interface Factory {
- LabelsJson create(Iterable<ListChangesOption> options);
- }
-
- private final ApprovalsUtil approvalsUtil;
- private final ChangeNotes.Factory notesFactory;
private final PermissionBackend permissionBackend;
- private final boolean lazyLoad;
@Inject
- LabelsJson(
- ApprovalsUtil approvalsUtil,
- ChangeNotes.Factory notesFactory,
- PermissionBackend permissionBackend,
- @Assisted Iterable<ListChangesOption> options) {
- this.approvalsUtil = approvalsUtil;
- this.notesFactory = notesFactory;
+ LabelsJson(PermissionBackend permissionBackend) {
this.permissionBackend = permissionBackend;
- this.lazyLoad = containsAnyOf(Sets.immutableEnumSet(options), ChangeJson.REQUIRE_LAZY_LOAD);
}
/**
@@ -253,14 +238,10 @@
private Map<String, Short> currentLabels(Account.Id accountId, ChangeData cd) {
Map<String, Short> result = new HashMap<>();
- for (PatchSetApproval psa :
- approvalsUtil.byPatchSetUser(
- lazyLoad ? cd.notes() : notesFactory.createFromIndexedChange(cd.change()),
- cd.change().currentPatchSetId(),
- accountId,
- null,
- null)) {
- result.put(psa.label(), psa.value());
+ for (PatchSetApproval psa : cd.currentApprovals()) {
+ if (psa.accountId().equals(accountId)) {
+ result.put(psa.label(), psa.value());
+ }
}
return result;
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 2edf00c..6a25afd 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -111,7 +111,6 @@
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeKindCacheImpl;
import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory;
-import com.google.gerrit.server.change.LabelsJson;
import com.google.gerrit.server.change.MergeabilityCacheImpl;
import com.google.gerrit.server.change.ReviewerSuggestion;
import com.google.gerrit.server.change.RevisionJson;
@@ -273,7 +272,6 @@
factory(ChangeData.AssistedFactory.class);
factory(ChangeJson.AssistedFactory.class);
factory(ChangeIsVisibleToPredicate.Factory.class);
- factory(LabelsJson.Factory.class);
factory(MergeUtil.Factory.class);
factory(PatchScriptFactory.Factory.class);
factory(PatchScriptFactoryForAutoFix.Factory.class);
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 890a1b1..41263a4 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -141,17 +141,6 @@
return new ChangeNotes(args, newChange(project, changeId), true, null).load();
}
- /**
- * Create change notes for a change that was loaded from index. This method should only be used
- * when database access is harmful and potentially stale data from the index is acceptable.
- *
- * @param change change loaded from secondary index
- * @return change notes
- */
- public ChangeNotes createFromIndexedChange(Change change) {
- return new ChangeNotes(args, change, true, null);
- }
-
public ChangeNotes createForBatchUpdate(Change change, boolean shouldExist) {
return new ChangeNotes(args, change, shouldExist, null).load();
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index c3b0e79..6d75bd2 100644
--- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -65,6 +65,7 @@
import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.account.ServiceUserClassifier;
@@ -87,6 +88,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
+import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.ObjectId;
@@ -821,6 +823,22 @@
AttentionSetUtil.additionsOnly(getNotes().getAttentionSet()).stream()
.map(AttentionSetUpdate::account)
.collect(Collectors.toSet());
+
+ // Current reviewers/ccs are the reviewers/ccs before the update + the new reviewers/ccs - the
+ // deleted reviewers/ccs.
+ Set<Account.Id> currentReviewers =
+ Stream.concat(
+ getNotes().getReviewers().all().stream(),
+ reviewers.entrySet().stream()
+ .filter(r -> r.getValue().asReviewerState() != ReviewerState.REMOVED)
+ .map(r -> r.getKey()))
+ .collect(Collectors.toSet());
+ currentReviewers.removeAll(
+ reviewers.entrySet().stream()
+ .filter(r -> r.getValue().asReviewerState() == ReviewerState.REMOVED)
+ .map(r -> r.getKey())
+ .collect(ImmutableSet.toImmutableSet()));
+
for (AttentionSetUpdate attentionSetUpdate : plannedAttentionSetUpdates.values()) {
if (attentionSetUpdate.operation() == AttentionSetUpdate.Operation.ADD
&& currentUsersInAttentionSet.contains(attentionSetUpdate.account())) {
@@ -848,11 +866,27 @@
continue;
}
+ // Don't add accounts that are not active in the change to the attention set.
+ if (attentionSetUpdate.operation() == AttentionSetUpdate.Operation.ADD
+ && !isActiveOnChange(currentReviewers, attentionSetUpdate.account())) {
+ continue;
+ }
+
addFooter(msg, FOOTER_ATTENTION, noteUtil.attentionSetUpdateToJson(attentionSetUpdate));
}
}
/**
+ * Returns whether {@code accountId} is active on a change based on the {@code currentReviewers}.
+ * Activity is defined as being a part of the reviewers, an uploader, or an owner of a change.
+ */
+ private boolean isActiveOnChange(Set<Account.Id> currentReviewers, Account.Id accountId) {
+ return currentReviewers.contains(accountId)
+ || getChange().getOwner().equals(accountId)
+ || getNotes().getCurrentPatchSet().uploader().equals(accountId);
+ }
+
+ /**
* When set, default attention set rules are ignored (E.g, adding reviewers -> adds to attention
* set, etc).
*/
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 6f4ccb7..ff90c3f 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -199,6 +199,7 @@
public static final String FIELD_CHERRY_PICK_OF_CHANGE = "cherrypickofchange";
public static final String FIELD_CHERRY_PICK_OF_PATCHSET = "cherrypickofpatchset";
+ public static final String ARG_ID_NAME = "name";
public static final String ARG_ID_USER = "user";
public static final String ARG_ID_GROUP = "group";
public static final String ARG_ID_OWNER = "owner";
@@ -1224,9 +1225,36 @@
}
@Operator
- public Predicate<ChangeData> query(String name) throws QueryParseException {
+ public Predicate<ChangeData> query(String value) throws QueryParseException {
+ // [name=]<name>[,user=<user>] || [user=<user>,][name=]<name>
+ PredicateArgs inputArgs = new PredicateArgs(value);
+ String name = null;
+ Account.Id account = null;
+
try (Repository git = args.repoManager.openRepository(args.allUsersName)) {
- VersionedAccountQueries q = VersionedAccountQueries.forUser(self());
+ // [name=]<name>
+ if (inputArgs.keyValue.containsKey(ARG_ID_NAME)) {
+ name = inputArgs.keyValue.get(ARG_ID_NAME);
+ } else if (inputArgs.positional.size() == 1) {
+ name = Iterables.getOnlyElement(inputArgs.positional);
+ } else if (inputArgs.positional.size() > 1) {
+ throw new QueryParseException("Error parsing named query: " + value);
+ }
+
+ // [,user=<user>]
+ if (inputArgs.keyValue.containsKey(ARG_ID_USER)) {
+ Set<Account.Id> accounts = parseAccount(inputArgs.keyValue.get(ARG_ID_USER));
+ if (accounts != null && accounts.size() > 1) {
+ throw error(
+ String.format(
+ "\"%s\" resolves to multiple accounts", inputArgs.keyValue.get(ARG_ID_USER)));
+ }
+ account = (accounts == null ? self() : Iterables.getOnlyElement(accounts));
+ } else {
+ account = self();
+ }
+
+ VersionedAccountQueries q = VersionedAccountQueries.forUser(account);
q.load(args.allUsersName, git);
String query = q.getQueryList().getQuery(name);
if (query != null) {
@@ -1236,7 +1264,7 @@
throw new QueryParseException(
"Unknown named query (no " + args.allUsersName + " repo): " + name, e);
} catch (IOException | ConfigInvalidException e) {
- throw new QueryParseException("Error parsing named query: " + name, e);
+ throw new QueryParseException("Error parsing named query: " + value, e);
}
throw new QueryParseException("Unknown named query: " + name);
}
@@ -1248,19 +1276,46 @@
}
@Operator
- public Predicate<ChangeData> destination(String name) throws QueryParseException {
+ public Predicate<ChangeData> destination(String value) throws QueryParseException {
+ // [name=]<name>[,user=<user>] || [user=<user>,][name=]<name>
+ PredicateArgs inputArgs = new PredicateArgs(value);
+ String name = null;
+ Account.Id account = null;
+
try (Repository git = args.repoManager.openRepository(args.allUsersName)) {
- VersionedAccountDestinations d = VersionedAccountDestinations.forUser(self());
+ // [name=]<name>
+ if (inputArgs.keyValue.containsKey(ARG_ID_NAME)) {
+ name = inputArgs.keyValue.get(ARG_ID_NAME);
+ } else if (inputArgs.positional.size() == 1) {
+ name = Iterables.getOnlyElement(inputArgs.positional);
+ } else if (inputArgs.positional.size() > 1) {
+ throw new QueryParseException("Error parsing named destination: " + value);
+ }
+
+ // [,user=<user>]
+ if (inputArgs.keyValue.containsKey(ARG_ID_USER)) {
+ Set<Account.Id> accounts = parseAccount(inputArgs.keyValue.get(ARG_ID_USER));
+ if (accounts != null && accounts.size() > 1) {
+ throw error(
+ String.format(
+ "\"%s\" resolves to multiple accounts", inputArgs.keyValue.get(ARG_ID_USER)));
+ }
+ account = (accounts == null ? self() : Iterables.getOnlyElement(accounts));
+ } else {
+ account = self();
+ }
+
+ VersionedAccountDestinations d = VersionedAccountDestinations.forUser(account);
d.load(args.allUsersName, git);
Set<BranchNameKey> destinations = d.getDestinationList().getDestinations(name);
if (destinations != null && !destinations.isEmpty()) {
- return new DestinationPredicate(destinations, name);
+ return new DestinationPredicate(destinations, value);
}
} catch (RepositoryNotFoundException e) {
throw new QueryParseException(
"Unknown named destination (no " + args.allUsersName + " repo): " + name, e);
} catch (IOException | ConfigInvalidException e) {
- throw new QueryParseException("Error parsing named destination: " + name, e);
+ throw new QueryParseException("Error parsing named destination: " + value, e);
}
throw new QueryParseException("Unknown named destination: " + name);
}
diff --git a/java/com/google/gerrit/server/restapi/change/AddToAttentionSet.java b/java/com/google/gerrit/server/restapi/change/AddToAttentionSet.java
index a5be14f..cb1256c 100644
--- a/java/com/google/gerrit/server/restapi/change/AddToAttentionSet.java
+++ b/java/com/google/gerrit/server/restapi/change/AddToAttentionSet.java
@@ -42,6 +42,7 @@
public class AddToAttentionSet
implements RestCollectionModifyView<
ChangeResource, AttentionSetEntryResource, AttentionSetInput> {
+
private final BatchUpdate.Factory updateFactory;
private final AccountResolver accountResolver;
private final AddToAttentionSetOp.Factory opFactory;
@@ -72,8 +73,9 @@
public Response<AccountInfo> apply(ChangeResource changeResource, AttentionSetInput input)
throws Exception {
AttentionSetUtil.validateInput(input);
+ Account.Id attentionUserId =
+ AttentionSetUtil.resolveAccount(accountResolver, changeResource.getNotes(), input.user);
- Account.Id attentionUserId = accountResolver.resolve(input.user).asUnique().account().id();
if (serviceUserClassifier.isServiceUser(attentionUserId)) {
throw new BadRequestException(
String.format(
diff --git a/java/com/google/gerrit/server/restapi/change/AttentionSet.java b/java/com/google/gerrit/server/restapi/change/AttentionSet.java
index 45d78dc..f72fe64ec 100644
--- a/java/com/google/gerrit/server/restapi/change/AttentionSet.java
+++ b/java/com/google/gerrit/server/restapi/change/AttentionSet.java
@@ -17,14 +17,15 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ChildCollection;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.server.account.AccountResolver;
-import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException;
import com.google.gerrit.server.change.AttentionSetEntryResource;
import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.util.AttentionSetUtil;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -58,12 +59,10 @@
@Override
public AttentionSetEntryResource parse(ChangeResource changeResource, IdString idString)
- throws ResourceNotFoundException, AuthException, IOException, ConfigInvalidException {
- try {
- Account.Id accountId = accountResolver.resolve(idString.get()).asUnique().account().id();
- return new AttentionSetEntryResource(changeResource, accountId);
- } catch (UnresolvableAccountException e) {
- throw new ResourceNotFoundException(idString, e);
- }
+ throws ResourceNotFoundException, AuthException, IOException, ConfigInvalidException,
+ BadRequestException {
+ Account.Id accountId =
+ AttentionSetUtil.resolveAccount(accountResolver, changeResource.getNotes(), idString.get());
+ return new AttentionSetEntryResource(changeResource, accountId);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java b/java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java
index c4dd04e..7fe463e 100644
--- a/java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java
+++ b/java/com/google/gerrit/server/restapi/change/RemoveFromAttentionSet.java
@@ -30,6 +30,7 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.AttentionSetUtil;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import java.io.IOException;
@@ -69,13 +70,9 @@
}
input.user = Strings.nullToEmpty(input.user).trim();
if (!input.user.isEmpty()) {
- Account.Id attentionUserId = null;
- try {
- attentionUserId = accountResolver.resolve(input.user).asUnique().account().id();
- } catch (AccountResolver.UnresolvableAccountException ex) {
- throw new BadRequestException(
- "The user specified in the input body couldn't be found.", ex);
- }
+ Account.Id attentionUserId =
+ AttentionSetUtil.resolveAccount(
+ accountResolver, attentionResource.getChangeResource().getNotes(), input.user);
if (attentionUserId.get() != attentionResource.getAccountId().get()) {
throw new BadRequestException(
"The field \"user\" must be empty, or must match the user specified in the URL.");
diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index 65c0cda..a1bd678 100644
--- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -312,10 +312,14 @@
throws BadRequestException, IOException, PermissionBackendException,
UnprocessableEntityException, ConfigInvalidException {
AttentionSetUtil.validateInput(add);
- Account.Id attentionUserId =
- getAccountIdAndValidateUser(changeNotes, add.user, accountsChangedInCommit);
-
- addToAttentionSet(bu, changeNotes, attentionUserId, add.reason, false);
+ try {
+ Account.Id attentionUserId =
+ getAccountIdAndValidateUser(changeNotes, add.user, accountsChangedInCommit);
+ addToAttentionSet(bu, changeNotes, attentionUserId, add.reason, false);
+ } catch (AccountResolver.UnresolvableAccountException ex) {
+ // This happens only when the account doesn't exist. Silently ignore it. If we threw an error
+ // message here, then it would be possible to probe whether an account exists.
+ }
}
private void removeFromAttentionSet(
@@ -326,10 +330,14 @@
throws BadRequestException, IOException, PermissionBackendException,
UnprocessableEntityException, ConfigInvalidException {
AttentionSetUtil.validateInput(remove);
- Account.Id attentionUserId =
- getAccountIdAndValidateUser(changeNotes, remove.user, accountsChangedInCommit);
-
- removeFromAttentionSet(bu, changeNotes, attentionUserId, remove.reason, false);
+ try {
+ Account.Id attentionUserId =
+ getAccountIdAndValidateUser(changeNotes, remove.user, accountsChangedInCommit);
+ removeFromAttentionSet(bu, changeNotes, attentionUserId, remove.reason, false);
+ } catch (AccountResolver.UnresolvableAccountException ex) {
+ // This happens only when the account doesn't exist. Silently ignore it. If we threw an error
+ // message here, then it would be possible to probe whether an account exists.
+ }
}
private Account.Id getAccountId(ChangeNotes changeNotes, String user)
@@ -356,15 +364,21 @@
ChangeNotes changeNotes, String user, Set<Account.Id> accountsChangedInCommit)
throws ConfigInvalidException, IOException, PermissionBackendException,
UnprocessableEntityException, BadRequestException {
- Account.Id attentionUserId = getAccountId(changeNotes, user);
- if (accountsChangedInCommit.contains(attentionUserId)) {
- throw new BadRequestException(
- String.format(
- "%s can not be added/removed twice, and can not be added and "
- + "removed at the same time",
- user));
+ try {
+ Account.Id attentionUserId = getAccountId(changeNotes, user);
+ if (accountsChangedInCommit.contains(attentionUserId)) {
+ throw new BadRequestException(
+ String.format(
+ "%s can not be added/removed twice, and can not be added and "
+ + "removed at the same time",
+ user));
+ }
+ accountsChangedInCommit.add(attentionUserId);
+ return attentionUserId;
+ } catch (AccountResolver.UnresolvableAccountException ex) {
+ // This can only happen if this user can't see the account or the account doesn't exist.
+ // Silently modify the account's attention set anyway, if the account exists.
+ return accountResolver.resolveIgnoreVisibility(user).asUnique().account().id();
}
- accountsChangedInCommit.add(attentionUserId);
- return attentionUserId;
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index e77bfe7..790b2db 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -30,8 +30,10 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.SubmitTypeRecord;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.api.changes.SubmitInput;
+import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -66,12 +68,14 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.Arrays;
import java.util.Collection;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config;
@@ -373,8 +377,15 @@
public Collection<ChangeData> unmergeableChanges(ChangeSet cs) throws IOException {
Set<ChangeData> mergeabilityMap = new HashSet<>();
+ Set<ObjectId> outDatedPatchsets = new HashSet<>();
for (ChangeData change : cs.changes()) {
mergeabilityMap.add(change);
+ // Add all the patchsets commit ids except the current patchset.
+ outDatedPatchsets.addAll(
+ change.notes().getPatchSets().values().stream()
+ .map(p -> p.commitId())
+ .collect(Collectors.toSet()));
+ outDatedPatchsets.remove(change.currentPatchSet().commitId());
}
ListMultimap<BranchNameKey, ChangeData> cbb = cs.changesByBranch();
@@ -388,12 +399,17 @@
allParents.add(parent.getId());
}
}
-
for (ChangeData change : targetBranch) {
+
RevCommit commit = commits.get(change.getId());
boolean isMergeCommit = commit.getParentCount() > 1;
boolean isLastInChain = !allParents.contains(commit.getId());
-
+ if (Arrays.stream(commit.getParents()).anyMatch(c -> outDatedPatchsets.contains(c.getId()))
+ && !isCherryPickSubmit(change)) {
+ // Found a parent that depends on an outdated patchset and the submit strategy is not
+ // cherry-pick.
+ continue;
+ }
// Recheck mergeability rather than using value stored in the index,
// which may be stale.
// TODO(dborowitz): This is ugly; consider providing a way to not read
@@ -419,6 +435,11 @@
return mergeabilityMap;
}
+ private boolean isCherryPickSubmit(ChangeData changeData) {
+ SubmitTypeRecord submitTypeRecord = changeData.submitTypeRecord();
+ return submitTypeRecord.isOk() && submitTypeRecord.type == SubmitType.CHERRY_PICK;
+ }
+
private HashMap<Change.Id, RevCommit> findCommits(
Collection<ChangeData> changes, Project.NameKey project) throws IOException {
HashMap<Change.Id, RevCommit> commits = new HashMap<>();
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 45e544d2..01c7b75 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -353,7 +353,7 @@
}
if (record.requirements != null) {
record.requirements.stream()
- .map(SubmitRequirement::fallbackText)
+ .map(MergeOp::describeSubmitRequirement)
.forEach(blockingConditions::add);
}
return Joiner.on("; ").join(blockingConditions);
@@ -389,6 +389,10 @@
return Joiner.on("; ").join(labelResults);
}
+ private static String describeSubmitRequirement(SubmitRequirement submitRequirement) {
+ return String.format("Submit requirement not fulfilled: %s", submitRequirement.fallbackText());
+ }
+
private void checkSubmitRulesAndState(ChangeSet cs, boolean allowMerged)
throws ResourceConflictException {
checkArgument(
diff --git a/java/com/google/gerrit/server/submit/MergeSuperSet.java b/java/com/google/gerrit/server/submit/MergeSuperSet.java
index 93c78a8..67f2907 100644
--- a/java/com/google/gerrit/server/submit/MergeSuperSet.java
+++ b/java/com/google/gerrit/server/submit/MergeSuperSet.java
@@ -18,19 +18,16 @@
import static java.util.Objects.requireNonNull;
import com.google.common.base.Strings;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.logging.TraceContext;
-import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.plugincontext.PluginContext;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
@@ -55,8 +52,6 @@
* included.
*/
public class MergeSuperSet {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final ChangeData.Factory changeDataFactory;
private final Provider<InternalChangeQuery> queryProvider;
private final Provider<MergeOpRepoManager> repoManagerProvider;
@@ -64,7 +59,6 @@
private final PermissionBackend permissionBackend;
private final Config cfg;
private final ProjectCache projectCache;
- private final ChangeNotes.Factory notesFactory;
private MergeOpRepoManager orm;
private boolean closeOrm;
@@ -77,8 +71,7 @@
Provider<MergeOpRepoManager> repoManagerProvider,
DynamicItem<MergeSuperSetComputation> mergeSuperSetComputation,
PermissionBackend permissionBackend,
- ProjectCache projectCache,
- ChangeNotes.Factory notesFactory) {
+ ProjectCache projectCache) {
this.cfg = cfg;
this.changeDataFactory = changeDataFactory;
this.queryProvider = queryProvider;
@@ -86,7 +79,6 @@
this.mergeSuperSetComputation = mergeSuperSetComputation;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
- this.notesFactory = notesFactory;
}
public static boolean wholeTopicEnabled(Config config) {
@@ -212,24 +204,8 @@
if (!projectCache.get(cd.project()).map(ProjectState::statePermitsRead).orElse(false)) {
return false;
}
-
- ChangeNotes notes;
try {
- notes = cd.notes();
- } catch (NoSuchChangeException e) {
- // The change was found in the index but is missing in NoteDb.
- // This can happen in systems with multiple primary nodes when the replication of the index
- // documents is faster than the replication of the Git data.
- // Instead of failing, create the change notes from the index data so that the read permission
- // check can be performed successfully.
- logger.atWarning().log(
- "Got change %d of project %s from index, but couldn't find it in NoteDb",
- cd.getId().get(), cd.project().get());
- notes = notesFactory.createFromIndexedChange(cd.change());
- }
-
- try {
- permissionBackend.user(user).change(notes).check(ChangePermission.READ);
+ permissionBackend.user(user).change(cd).check(ChangePermission.READ);
return true;
} catch (AuthException e) {
return false;
diff --git a/java/com/google/gerrit/server/util/AttentionSetUtil.java b/java/com/google/gerrit/server/util/AttentionSetUtil.java
index 62cad3f..26c862d 100644
--- a/java/com/google/gerrit/server/util/AttentionSetUtil.java
+++ b/java/com/google/gerrit/server/util/AttentionSetUtil.java
@@ -16,11 +16,16 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.AttentionSetUpdate.Operation;
import com.google.gerrit.extensions.api.changes.AttentionSetInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import java.io.IOException;
import java.util.Collection;
+import org.eclipse.jgit.errors.ConfigInvalidException;
/** Common helpers for dealing with attention set data structures. */
public class AttentionSetUtil {
@@ -49,5 +54,45 @@
}
}
+ /**
+ * Returns the {@code Account.Id} of {@code user} if the user is active on the change, and exists.
+ * If the user doesn't exist or is not active on the change, the same exception is thrown to
+ * disallow probing for account existence based on exception type.
+ */
+ public static Account.Id resolveAccount(
+ AccountResolver accountResolver, ChangeNotes changeNotes, String user)
+ throws ConfigInvalidException, IOException, BadRequestException {
+ // We will throw this exception if the account doesn't exist, or if the account is not active.
+ // This is purposely the same exception so that users can't probe for account existence based on
+ // the thrown exception.
+ BadRequestException possibleExceptionForNotFoundOrInactiveAccount =
+ new BadRequestException(
+ String.format(
+ "%s doesn't exist or is not active on the change as an owner, uploader, "
+ + "reviewer, or cc so they can't be added to the attention set",
+ user));
+ Account.Id attentionUserId;
+ try {
+ attentionUserId = accountResolver.resolveIgnoreVisibility(user).asUnique().account().id();
+ } catch (AccountResolver.UnresolvableAccountException ex) {
+ possibleExceptionForNotFoundOrInactiveAccount.initCause(ex);
+ throw possibleExceptionForNotFoundOrInactiveAccount;
+ }
+ if (!isActiveOnTheChange(changeNotes, attentionUserId)) {
+ throw possibleExceptionForNotFoundOrInactiveAccount;
+ }
+ return attentionUserId;
+ }
+
+ /**
+ * Returns whether {@code attentionUserId} is active on a change. Activity is defined as being a
+ * part of the reviewers, an uploader, or an owner of a change.
+ */
+ private static boolean isActiveOnTheChange(ChangeNotes changeNotes, Account.Id attentionUserId) {
+ return changeNotes.getChange().getOwner().equals(attentionUserId)
+ || changeNotes.getCurrentPatchSet().uploader().equals(attentionUserId)
+ || changeNotes.getReviewers().all().stream().anyMatch(id -> id.equals(attentionUserId));
+ }
+
private AttentionSetUtil() {}
}
diff --git a/java/com/google/gerrit/server/util/PluginLogFile.java b/java/com/google/gerrit/server/util/PluginLogFile.java
index de8b3aa..8235623 100644
--- a/java/com/google/gerrit/server/util/PluginLogFile.java
+++ b/java/com/google/gerrit/server/util/PluginLogFile.java
@@ -40,8 +40,13 @@
public void start() {
AsyncAppender asyncAppender = systemLog.createAsyncAppender(logName, layout, true, true);
Logger logger = LogManager.getLogger(logName);
- logger.removeAppender(logName);
- logger.addAppender(asyncAppender);
+ if (logger.getAppender(logName) == null) {
+ synchronized (this) {
+ if (logger.getAppender(logName) == null) {
+ logger.addAppender(asyncAppender);
+ }
+ }
+ }
logger.setAdditivity(false);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index faef5aa..085d23d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -1439,6 +1439,12 @@
assertWithMessage("enabled bit on submit action").that(desc.isEnabled()).isTrue();
}
+ protected void assertSubmitDisabled(String changeId) throws Throwable {
+ RevisionResource rsrc = parseCurrentRevisionResource(changeId);
+ UiAction.Description desc = submitHandler.getDescription(rsrc);
+ assertWithMessage("enabled bit on submit action").that(desc.isEnabled()).isFalse();
+ }
+
protected void assertChangeMergedEvents(String... expected) throws Throwable {
eventRecorder.assertChangeMergedEvents(project.get(), "refs/heads/master", expected);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
index f77552d..9c496fa 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
@@ -20,6 +20,7 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.inject.Inject;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -117,4 +118,49 @@
assertThat(head.getParent(0)).isEqualTo(change1.getCommit());
assertThat(head.getParent(1)).isEqualTo(change2.getCommit());
}
+
+ @Test
+ public void dependencyOnOutdatedPatchSetPreventsMerge() throws Throwable {
+ // Create a change
+ PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
+ PushOneCommit.Result changeResult = change.to("refs/for/master");
+ PatchSet.Id patchSetId = changeResult.getPatchSetId();
+
+ // Create a successor change.
+ PushOneCommit change2 =
+ pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
+ PushOneCommit.Result change2Result = change2.to("refs/for/master");
+
+ // Create new patch set for first change.
+ testRepo.reset(changeResult.getCommit().name());
+ amendChange(changeResult.getChangeId());
+
+ // Approve both changes
+ approve(changeResult.getChangeId());
+ approve(change2Result.getChangeId());
+
+ // submit button is disabled.
+ assertSubmitDisabled(change2Result.getChangeId());
+
+ submitWithConflict(
+ change2Result.getChangeId(),
+ "Failed to submit 2 changes due to the following problems:\n"
+ + "Change "
+ + change2Result.getChange().getId()
+ + ": Depends on change that was not submitted."
+ + " Commit "
+ + change2Result.getCommit().name()
+ + " depends on commit "
+ + changeResult.getCommit().name()
+ + ", which is outdated patch set "
+ + patchSetId.get()
+ + " of change "
+ + changeResult.getChange().getId()
+ + ". The latest patch set is "
+ + changeResult.getPatchSetId().get()
+ + ".");
+
+ assertRefUpdatedEvents();
+ assertChangeMergedEvents();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
index 955dd7a..5eb19df 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
@@ -29,6 +29,7 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.ChangeStatus;
@@ -384,4 +385,49 @@
gApi.changes().id(change2.getChangeId()).current().rebase();
submit(change2.getChangeId());
}
+
+ @Test
+ public void dependencyOnOutdatedPatchSetPreventsRebase() throws Throwable {
+ // Create a change
+ PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
+ PushOneCommit.Result changeResult = change.to("refs/for/master");
+ PatchSet.Id patchSetId = changeResult.getPatchSetId();
+
+ // Create a successor change.
+ PushOneCommit change2 =
+ pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
+ PushOneCommit.Result change2Result = change2.to("refs/for/master");
+
+ // Create new patch set for first change.
+ testRepo.reset(changeResult.getCommit().name());
+ amendChange(changeResult.getChangeId());
+
+ // Approve both changes
+ approve(changeResult.getChangeId());
+ approve(change2Result.getChangeId());
+
+ // submit button is disabled.
+ assertSubmitDisabled(change2Result.getChangeId());
+
+ submitWithConflict(
+ change2Result.getChangeId(),
+ "Failed to submit 2 changes due to the following problems:\n"
+ + "Change "
+ + change2Result.getChange().getId()
+ + ": Depends on change that was not submitted."
+ + " Commit "
+ + change2Result.getCommit().name()
+ + " depends on commit "
+ + changeResult.getCommit().name()
+ + ", which is outdated patch set "
+ + patchSetId.get()
+ + " of change "
+ + changeResult.getChange().getId()
+ + ". The latest patch set is "
+ + changeResult.getPatchSetId().get()
+ + ".");
+
+ assertRefUpdatedEvents();
+ assertChangeMergedEvents();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index d0641e4..9e944a2 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -35,6 +35,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
+import com.google.gerrit.entities.AttentionSetUpdate.Operation;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
@@ -115,48 +116,48 @@
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
int accountId =
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "first"))._accountId;
- assertThat(accountId).isEqualTo(user.id().get());
+ change(r).addToAttentionSet(new AttentionSetInput(admin.email(), "first"))._accountId;
+ assertThat(accountId).isEqualTo(admin.id().get());
AttentionSetUpdate expectedAttentionSetUpdate =
AttentionSetUpdate.createFromRead(
- fakeClock.now(), user.id(), AttentionSetUpdate.Operation.ADD, "first");
+ fakeClock.now(), admin.id(), AttentionSetUpdate.Operation.ADD, "first");
assertThat(r.getChange().attentionSet()).containsExactly(expectedAttentionSetUpdate);
// Second add is ignored.
accountId =
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "second"))._accountId;
- assertThat(accountId).isEqualTo(user.id().get());
+ change(r).addToAttentionSet(new AttentionSetInput(admin.email(), "second"))._accountId;
+ assertThat(accountId).isEqualTo(admin.id().get());
assertThat(r.getChange().attentionSet()).containsExactly(expectedAttentionSetUpdate);
// Only one email since the second add was ignored.
String emailBody = Iterables.getOnlyElement(email.getMessages()).body();
assertThat(emailBody)
.contains(
- user.fullName()
- + " added themselves to the attention set of this change.\n The reason is: first.");
+ String.format(
+ "%s requires the attention of %s to this change.\n The reason is: first.",
+ user.fullName(), admin.fullName()));
}
@Test
public void addMultipleUsers() throws Exception {
PushOneCommit.Result r = createChange();
Instant timestamp1 = fakeClock.now();
- int accountId1 =
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "user"))._accountId;
- assertThat(accountId1).isEqualTo(user.id().get());
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
fakeClock.advance(Duration.ofSeconds(42));
Instant timestamp2 = fakeClock.now();
int accountId2 =
change(r)
- .addToAttentionSet(new AttentionSetInput(admin.id().toString(), "admin"))
+ .addToAttentionSet(new AttentionSetInput(admin.id().toString(), "manual update"))
._accountId;
assertThat(accountId2).isEqualTo(admin.id().get());
AttentionSetUpdate expectedAttentionSetUpdate1 =
AttentionSetUpdate.createFromRead(
- timestamp1, user.id(), AttentionSetUpdate.Operation.ADD, "user");
+ timestamp1, user.id(), AttentionSetUpdate.Operation.ADD, "Reviewer was added");
AttentionSetUpdate expectedAttentionSetUpdate2 =
AttentionSetUpdate.createFromRead(
- timestamp2, admin.id(), AttentionSetUpdate.Operation.ADD, "admin");
+ timestamp2, admin.id(), AttentionSetUpdate.Operation.ADD, "manual update");
assertThat(r.getChange().attentionSet())
.containsExactly(expectedAttentionSetUpdate1, expectedAttentionSetUpdate2);
}
@@ -164,7 +165,9 @@
@Test
public void removeUser() throws Exception {
PushOneCommit.Result r = createChange();
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "added"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
+ sender.clear();
requestScopeOperations.setApiUser(user.id());
fakeClock.advance(Duration.ofSeconds(42));
@@ -191,6 +194,9 @@
@Test
public void removeUserWithInvalidUserInput() throws Exception {
PushOneCommit.Result r = createChange();
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
+
BadRequestException exception =
assertThrows(
BadRequestException.class,
@@ -199,7 +205,9 @@
.attention(user.id().toString())
.remove(new AttentionSetInput("invalid user", "reason")));
assertThat(exception.getMessage())
- .isEqualTo("The user specified in the input body couldn't be found.");
+ .isEqualTo(
+ "invalid user doesn't exist or is not active on the change as an owner, "
+ + "uploader, reviewer, or cc so they can't be added to the attention set");
exception =
assertThrows(
@@ -214,16 +222,10 @@
}
@Test
- public void removeUnrelatedUser() throws Exception {
- PushOneCommit.Result r = createChange();
- change(r).attention(user.id().toString()).remove(new AttentionSetInput("foo"));
- assertThat(r.getChange().attentionSet()).isEmpty();
- }
-
- @Test
public void abandonRemovesUsers() throws Exception {
PushOneCommit.Result r = createChange();
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "user"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
change(r).addToAttentionSet(new AttentionSetInput(admin.email(), "admin"));
change(r).abandon();
@@ -244,7 +246,8 @@
@Test
public void workInProgressRemovesUsers() throws Exception {
PushOneCommit.Result r = createChange();
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "reason"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
change(r).setWorkInProgress();
@@ -258,13 +261,10 @@
public void submitRemovesUsersForAllSubmittedChanges() throws Exception {
PushOneCommit.Result r1 = createChange("refs/heads/master", "file1", "content");
- change(r1)
- .current()
- .review(ReviewInput.approve().addUserToAttentionSet(user.email(), "reason"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r1).current().review(ReviewInput.approve().reviewer(user.email()));
PushOneCommit.Result r2 = createChange("refs/heads/master", "file2", "content");
- change(r2)
- .current()
- .review(ReviewInput.approve().addUserToAttentionSet(user.email(), "reason"));
+ change(r2).current().review(ReviewInput.approve().reviewer(user.email()));
change(r2).current().submit();
@@ -286,11 +286,10 @@
@Test
public void robotSubmitsRemovesUsers() throws Exception {
- PushOneCommit.Result r1 = createChange("refs/heads/master", "file1", "content");
+ PushOneCommit.Result r = createChange("refs/heads/master", "file1", "content");
- change(r1)
- .current()
- .review(ReviewInput.approve().addUserToAttentionSet(user.email(), "reason"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
TestAccount robot =
accountCreator.create(
@@ -301,11 +300,12 @@
ServiceUserClassifier.SERVICE_USERS,
"Administrators");
requestScopeOperations.setApiUser(robot.id());
- change(r1).current().submit();
+ change(r).current().review(ReviewInput.approve());
+ change(r).current().submit();
// Attention set updates that relate to the admin (the person who replied) are filtered out.
AttentionSetUpdate attentionSet =
- Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r1, user));
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user));
assertThat(attentionSet).hasAccountIdThat().isEqualTo(user.id());
assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.REMOVE);
@@ -533,7 +533,9 @@
@Test
public void reviewersAreNotAddedForNoReasonBecauseOfAnUpdate() throws Exception {
PushOneCommit.Result r = createChange();
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "user"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
+
change(r).attention(user.id().toString()).remove(new AttentionSetInput("removed"));
HashtagsInput hashtagsInput = new HashtagsInput();
@@ -567,7 +569,8 @@
@Test
public void reviewRemovesManuallyRemovedUserFromAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "reason"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
requestScopeOperations.setApiUser(user.id());
ReviewInput reviewInput =
@@ -587,7 +590,8 @@
@Test
public void reviewWithManualAdditionToAttentionSetFailsWithoutReason() throws Exception {
PushOneCommit.Result r = createChange();
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "reason"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
ReviewInput reviewInput = ReviewInput.create().addUserToAttentionSet(user.email(), "");
@@ -611,7 +615,8 @@
@Test
public void reviewAddReviewerWhileRemovingFromAttentionSetJustRemovesUser() throws Exception {
PushOneCommit.Result r = createChange();
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "addition"));
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
ReviewInput reviewInput =
ReviewInput.create()
@@ -772,7 +777,15 @@
requestScopeOperations.setApiUser(user.id());
- change(r).addToAttentionSet(new AttentionSetInput(user.email(), "reason"));
+ // add the user to the attention set.
+ change(r)
+ .current()
+ .review(
+ ReviewInput.create()
+ .reviewer(user.email(), ReviewerState.CC, true)
+ .addUserToAttentionSet(user.email(), "reason"));
+
+ // add the user as reviewer but still be removed on reply.
ReviewInput reviewInput = ReviewInput.create().reviewer(user.email());
change(r).current().review(reviewInput);
@@ -1144,6 +1157,9 @@
@Test
public void repliesWhileAddingAsReviewerStillRemovesUser() throws Exception {
PushOneCommit.Result r = createChange();
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
+
change(r).addToAttentionSet(new AttentionSetInput(user.email(), "remove"));
requestScopeOperations.setApiUser(user.id());
@@ -1237,6 +1253,8 @@
accountCreator.create(
"robot1", "robot1@example.com", "Ro Bot", "Ro", ServiceUserClassifier.SERVICE_USERS);
PushOneCommit.Result r = createChange();
+ // Make the robot active on the change.
+ change(r).addReviewer(robot.email());
// Throw an error when adding a robot explicitly.
BadRequestException exception =
@@ -1335,13 +1353,15 @@
public void addUsersToAttentionSetInPrivateChanges() throws Exception {
PushOneCommit.Result r = createChange();
change(r).setPrivate(true);
- change(r).current().review(new ReviewInput().addUserToAttentionSet(user.email(), "reason"));
+
+ // implictly adds the user to the attention set when adding as reviewer
+ change(r).addReviewer(user.email());
AttentionSetUpdate attentionSet =
Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user));
assertThat(attentionSet).hasAccountIdThat().isEqualTo(user.id());
assertThat(attentionSet).hasOperationThat().isEqualTo(AttentionSetUpdate.Operation.ADD);
- assertThat(attentionSet).hasReasonThat().isEqualTo("reason");
+ assertThat(attentionSet).hasReasonThat().isEqualTo("Reviewer was added");
}
@Test
@@ -1532,6 +1552,156 @@
assertThat(sender.getMessages()).isNotEmpty();
}
+ @Test
+ public void cannotAddIrrelevantUserToAttentionSet() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> change(r).addToAttentionSet(new AttentionSetInput(user.email(), "reason")));
+ assertThat(exception.getMessage())
+ .isEqualTo(
+ String.format(
+ "%s doesn't exist or is not active on the change as an owner, uploader, reviewer, "
+ + "or cc so they can't be added to the attention set",
+ user.email()));
+ }
+
+ @Test
+ public void cannotAddNonExistingUserToAttentionSet() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> change(r).addToAttentionSet(new AttentionSetInput("INVALID USER", "reason")));
+ assertThat(exception.getMessage())
+ .isEqualTo(
+ "INVALID USER doesn't exist or is not active on the change as an owner,"
+ + " uploader, reviewer, or cc so they can't be added to the attention set");
+ }
+
+ @Test
+ public void cannotRemoveIrrelevantUserToAttentionSet() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> change(r).attention(user.email()).remove(new AttentionSetInput("reason")));
+ assertThat(exception.getMessage())
+ .isEqualTo(
+ String.format(
+ "%s doesn't exist or is not active on the change as an owner, uploader, reviewer, "
+ + "or cc so they can't be added to the attention set",
+ user.email()));
+ }
+
+ @Test
+ public void cannotRemoveIrrelevantUserToAttentionSetWithUserInInput() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () ->
+ change(r)
+ .attention(user.email())
+ .remove(new AttentionSetInput(user.email(), "reason")));
+ assertThat(exception.getMessage())
+ .isEqualTo(
+ String.format(
+ "%s doesn't exist or is not active on the change as an owner, uploader, reviewer, "
+ + "or cc so they can't be added to the attention set",
+ user.email()));
+ }
+
+ @Test
+ public void cannotRemoveNonExistingUser() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> change(r).attention("INVALID USER").remove(new AttentionSetInput("reason")));
+ assertThat(exception.getMessage())
+ .isEqualTo(
+ "INVALID USER doesn't exist or is not active on the change as an owner,"
+ + " uploader, reviewer, or cc so they can't be added to the attention set");
+ }
+
+ @Test
+ public void irrelevantUsersAddedToAttentionSetAreIgnoredOnReply() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ change(r).current().review(ReviewInput.create().addUserToAttentionSet(user.email(), "reason"));
+ assertThat(getAttentionSetUpdatesForUser(r, user)).isEmpty();
+ }
+
+ @Test
+ public void newReviewerCanBeAddedToTheAttentionSetManually() throws Exception {
+ PushOneCommit.Result r = createChange();
+ change(r)
+ .current()
+ .review(
+ ReviewInput.create()
+ .reviewer(user.email())
+ .addUserToAttentionSet(user.email(), "reason")
+ .blockAutomaticAttentionSetRules());
+ assertThat(Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user)).operation())
+ .isEqualTo(Operation.ADD);
+ }
+
+ @Test
+ public void newReviewerCanBeAddedToTheAttentionSetAutomatically() throws Exception {
+ PushOneCommit.Result r = createChange();
+ change(r).current().review(ReviewInput.create().reviewer(user.email()));
+ assertThat(Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user)).operation())
+ .isEqualTo(Operation.ADD);
+ }
+
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void onReplyCanAddInvisibleUsersToAttentionSetOnVisibleChanges() throws Exception {
+ PushOneCommit.Result r = createChange();
+ requestScopeOperations.setApiUser(user.id());
+
+ // admin is invisible to the user, but they can still add them to the attention set since they
+ // see the change.
+ change(r).current().review(ReviewInput.create().addUserToAttentionSet(admin.email(), "reason"));
+ assertThat(Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin)).operation())
+ .isEqualTo(Operation.ADD);
+ }
+
+ @Test
+ public void onReplyNonExistingUsersAreSilentlyIgnored() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ change(r)
+ .current()
+ .review(ReviewInput.create().addUserToAttentionSet("INVALID USER", "reason"));
+ assertThat(getAttentionSetUpdates(r.getChange().getId())).isEmpty();
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void canModifyAttentionSetForInvisibleUsersOnVisibleChanges() throws Exception {
+ PushOneCommit.Result r = createChange();
+ requestScopeOperations.setApiUser(user.id());
+
+ // admin is invisible to the user, but they can still add them to the attention set since they
+ // see the change.
+ change(r).addToAttentionSet(new AttentionSetInput(admin.email(), "reason"));
+ assertThat(Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin)).operation())
+ .isEqualTo(Operation.ADD);
+
+ // admin is invisible to the user, but they can still remove them to the attention set since
+ // they see the change.
+ change(r).attention(admin.id().toString()).remove(new AttentionSetInput("removed"));
+ assertThat(Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin)).operation())
+ .isEqualTo(Operation.REMOVE);
+ }
+
private List<AttentionSetUpdate> getAttentionSetUpdatesForUser(
PushOneCommit.Result r, TestAccount account) {
return getAttentionSetUpdates(r.getChange().getId()).stream()
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
index 6f519f1..5bcf995 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
@@ -25,6 +25,7 @@
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.FooterConstants;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -373,4 +374,27 @@
change2.getChangeId(),
headAfterFirstSubmit.name());
}
+
+ @Test
+ public void dependencyOnOutdatedPatchSetNotPreventingCherryPick() throws Throwable {
+ // Create a change
+ PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
+ PushOneCommit.Result changeResult = change.to("refs/for/master");
+ PatchSet.Id patchSetId = changeResult.getPatchSetId();
+
+ // Create a successor change.
+ PushOneCommit change2 =
+ pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
+ PushOneCommit.Result change2Result = change2.to("refs/for/master");
+
+ // Create new patch set for first change.
+ testRepo.reset(changeResult.getCommit().name());
+ amendChange(changeResult.getChangeId());
+
+ // Approve both changes
+ approve(changeResult.getChangeId());
+ approve(change2Result.getChangeId());
+
+ assertSubmittable(change2Result.getChangeId());
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
index 1912697..66eb48c 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ActionInfo;
@@ -170,4 +171,49 @@
assertRefUpdatedEvents(initialHead, headAfterSubmit);
assertChangeMergedEvents(id1, headAfterSubmit.name());
}
+
+ @Test
+ public void dependencyOnOutdatedPatchSetPreventsFastForward() throws Throwable {
+ // Create a change
+ PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
+ PushOneCommit.Result changeResult = change.to("refs/for/master");
+ PatchSet.Id patchSetId = changeResult.getPatchSetId();
+
+ // Create a successor change.
+ PushOneCommit change2 =
+ pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
+ PushOneCommit.Result change2Result = change2.to("refs/for/master");
+
+ // Create new patch set for first change.
+ testRepo.reset(changeResult.getCommit().name());
+ amendChange(changeResult.getChangeId());
+
+ // Approve both changes
+ approve(changeResult.getChangeId());
+ approve(change2Result.getChangeId());
+
+ // submit button is disabled.
+ assertSubmitDisabled(change2Result.getChangeId());
+
+ submitWithConflict(
+ change2Result.getChangeId(),
+ "Failed to submit 2 changes due to the following problems:\n"
+ + "Change "
+ + change2Result.getChange().getId()
+ + ": Depends on change that was not submitted."
+ + " Commit "
+ + change2Result.getCommit().name()
+ + " depends on commit "
+ + changeResult.getCommit().name()
+ + ", which is outdated patch set "
+ + patchSetId.get()
+ + " of change "
+ + changeResult.getChange().getId()
+ + ". The latest patch set is "
+ + changeResult.getPatchSetId().get()
+ + ".");
+
+ assertRefUpdatedEvents();
+ assertChangeMergedEvents();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
index 5fe741d..995de0d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
@@ -29,7 +29,6 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
-import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.changes.ChangeApi;
@@ -534,48 +533,6 @@
}
@Test
- public void dependencyOnOutdatedPatchSetPreventsMerge() throws Throwable {
- // Create a change
- PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
- PushOneCommit.Result changeResult = change.to("refs/for/master");
- PatchSet.Id patchSetId = changeResult.getPatchSetId();
-
- // Create a successor change.
- PushOneCommit change2 =
- pushFactory.create(user.newIdent(), testRepo, "feature", "b.txt", "bar");
- PushOneCommit.Result change2Result = change2.to("refs/for/master");
-
- // Create new patch set for first change.
- testRepo.reset(changeResult.getCommit().name());
- amendChange(changeResult.getChangeId());
-
- // Approve both changes
- approve(changeResult.getChangeId());
- approve(change2Result.getChangeId());
-
- submitWithConflict(
- change2Result.getChangeId(),
- "Failed to submit 2 changes due to the following problems:\n"
- + "Change "
- + change2Result.getChange().getId()
- + ": Depends on change that was not submitted."
- + " Commit "
- + change2Result.getCommit().name()
- + " depends on commit "
- + changeResult.getCommit().name()
- + ", which is outdated patch set "
- + patchSetId.get()
- + " of change "
- + changeResult.getChange().getId()
- + ". The latest patch set is "
- + changeResult.getPatchSetId().get()
- + ".");
-
- assertRefUpdatedEvents();
- assertChangeMergedEvents();
- }
-
- @Test
public void dependencyOnDeletedChangePreventsMerge() throws Throwable {
// Create a change
PushOneCommit change = pushFactory.create(user.newIdent(), testRepo, "fix", "a.txt", "foo");
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 44dd831..cc0b109 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -809,6 +809,8 @@
public void addAttentionStatusForMultipleUsers() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, changeOwner);
+ // put the user as cc to ensure that the user took part in this change.
+ update.putReviewer(otherUser.getAccount().id(), CC);
AttentionSetUpdate attentionSetUpdate0 =
AttentionSetUpdate.createForWrite(changeOwner.getAccountId(), Operation.ADD, "test");
AttentionSetUpdate attentionSetUpdate1 =
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 02f514a..1de548f 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -3055,6 +3055,14 @@
gApi.changes().id(change.getChangeId()).addToAttentionSet(input);
Account.Id user2Id =
accountManager.authenticate(AuthRequest.forUser("anotheruser")).getAccountId();
+
+ // Add the second user as cc to ensure that user took part of the change and can be added to the
+ // attention set.
+ AddReviewerInput addReviewerInput = new AddReviewerInput();
+ addReviewerInput.reviewer = user2Id.toString();
+ addReviewerInput.state = ReviewerState.CC;
+ gApi.changes().id(change.getChangeId()).addReviewer(addReviewerInput);
+
input = new AttentionSetInput(user2Id.toString(), "reason 2");
gApi.changes().id(change.getChangeId()).addToAttentionSet(input);
@@ -3085,6 +3093,7 @@
assertQuery("-assignee:" + user.getUserName().get(), change2);
}
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
@Test
public void userDestination() throws Exception {
TestRepository<Repo> repo1 = createProject("repo1");
@@ -3096,6 +3105,8 @@
.hasMessageThat()
.isEqualTo("Unknown named destination: foo");
+ Account.Id anotherUserId =
+ accountManager.authenticate(AuthRequest.forUser("anotheruser")).getAccountId();
String destination1 = "refs/heads/master\trepo1";
String destination2 = "refs/heads/master\trepo2";
String destination3 = "refs/heads/master\trepo1\nrefs/heads/master\trepo2";
@@ -3111,8 +3122,32 @@
allUsers.branch(refsUsers).commit().add("destinations/destination4", destination4).create();
allUsers.branch(refsUsers).commit().add("destinations/destination5", destination5).create();
+ String anotherRefsUsers = RefNames.refsUsers(anotherUserId);
+ allUsers
+ .branch(anotherRefsUsers)
+ .commit()
+ .add("destinations/destination6", destination1)
+ .create();
+ allUsers
+ .branch(anotherRefsUsers)
+ .commit()
+ .add("destinations/destination7", destination2)
+ .create();
+ allUsers
+ .branch(anotherRefsUsers)
+ .commit()
+ .add("destinations/destination8", destination3)
+ .create();
+ allUsers
+ .branch(anotherRefsUsers)
+ .commit()
+ .add("destinations/destination9", destination4)
+ .create();
+
Ref userRef = allUsers.getRepository().exactRef(refsUsers);
+ Ref anotherUserRef = allUsers.getRepository().exactRef(anotherRefsUsers);
assertThat(userRef).isNotNull();
+ assertThat(anotherUserRef).isNotNull();
}
assertQuery("destination:destination1", change1);
@@ -3120,38 +3155,87 @@
assertQuery("destination:destination3", change2, change1);
assertQuery("destination:destination4");
assertQuery("destination:destination5");
+ assertQuery("destination:destination6,user=" + anotherUserId, change1);
+ assertQuery("destination:name=destination6,user=" + anotherUserId, change1);
+ assertQuery("destination:user=" + anotherUserId + ",destination7", change2);
+ assertQuery("destination:user=" + anotherUserId + ",name=destination8", change2, change1);
+ assertQuery("destination:destination9,user=" + anotherUserId);
+
+ assertThatQueryException("destination:destination3,user=" + anotherUserId)
+ .hasMessageThat()
+ .isEqualTo("Unknown named destination: destination3");
+ assertThatQueryException("destination:destination3,user=test")
+ .hasMessageThat()
+ .isEqualTo("Account 'test' not found");
+
+ requestContext.setContext(newRequestContext(anotherUserId));
+ // account 1000000 is not visible to 'anotheruser' as they are not an admin
+ assertThatQueryException("destination:destination3,user=" + userId)
+ .hasMessageThat()
+ .isEqualTo("Account '1000000' not found");
}
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
@Test
public void userQuery() throws Exception {
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChange(repo));
Change change2 = insert(repo, newChangeForBranch(repo, "stable"));
+ Account.Id anotherUserId =
+ accountManager.authenticate(AuthRequest.forUser("anotheruser")).getAccountId();
String queryListText =
"query1\tproject:repo\n"
+ "query2\tproject:repo status:open\n"
+ "query3\tproject:repo branch:stable\n"
+ "query4\tproject:repo branch:other";
+ String anotherQueryListText =
+ "query5\tproject:repo\n"
+ + "query6\tproject:repo status:merged\n"
+ + "query7\tproject:repo branch:stable\n"
+ + "query8\tproject:repo branch:other";
try (TestRepository<Repo> allUsers =
new TestRepository<>(repoManager.openRepository(allUsersName));
- MetaDataUpdate md = metaDataUpdateFactory.create(allUsersName)) {
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsersName);
+ MetaDataUpdate anotherMd = metaDataUpdateFactory.create(allUsersName)) {
VersionedAccountQueries queries = VersionedAccountQueries.forUser(userId);
queries.load(md);
queries.setQueryList(queryListText);
queries.commit(md);
+ VersionedAccountQueries anotherQueries = VersionedAccountQueries.forUser(anotherUserId);
+ anotherQueries.load(anotherMd);
+ anotherQueries.setQueryList(anotherQueryListText);
+ anotherQueries.commit(anotherMd);
}
assertThatQueryException("query:foo").hasMessageThat().isEqualTo("Unknown named query: foo");
+ assertThatQueryException("query:query1,user=" + anotherUserId)
+ .hasMessageThat()
+ .isEqualTo("Unknown named query: query1");
+ assertThatQueryException("query:query1,user=test")
+ .hasMessageThat()
+ .isEqualTo("Account 'test' not found");
+
+ requestContext.setContext(newRequestContext(anotherUserId));
+ // account 1000000 is not visible to 'anotheruser' as they are not an admin
+ assertThatQueryException("query:query1,user=" + userId)
+ .hasMessageThat()
+ .isEqualTo("Account '1000000' not found");
+ requestContext.setContext(newRequestContext(userId));
assertQuery("query:query1", change2, change1);
assertQuery("query:query2", change2, change1);
+ assertQuery("query:name=query5,user=" + anotherUserId, change2, change1);
+ assertQuery("query:user=" + anotherUserId + ",name=query6");
gApi.changes().id(change1.getChangeId()).current().review(ReviewInput.approve());
gApi.changes().id(change1.getChangeId()).current().submit();
assertQuery("query:query2", change2);
assertQuery("query:query3", change2);
assertQuery("query:query4");
+ assertQuery("query:query6,user=" + anotherUserId, change1);
+ assertQuery("query:user=" + anotherUserId + ",query7", change2);
+ assertQuery("query:query8,user=" + anotherUserId);
}
@Test
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
index 69e9900..f8a5940 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts
@@ -594,8 +594,7 @@
if (
role === ChangeRole.AUTHOR &&
- rev.commit &&
- rev.commit.author &&
+ rev.commit?.author &&
change.owner.email !== rev.commit.author.email
) {
return rev.commit.author;
@@ -603,9 +602,11 @@
if (
role === ChangeRole.COMMITTER &&
- rev.commit &&
- rev.commit.committer &&
- change.owner.email !== rev.commit.committer.email
+ rev.commit?.committer &&
+ change.owner.email !== rev.commit.committer.email &&
+ !(
+ rev.uploader?.email && rev.uploader.email === rev.commit.committer.email
+ )
) {
return rev.commit.committer;
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.js b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.js
index 5f53641..5bdf105 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.js
@@ -210,13 +210,13 @@
test('_getNonOwnerRole that it does not return uploader', () => {
// Set the uploader email to be the same as the owner.
change.revisions.rev1.uploader._account_id = 1019328;
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.UPLOADER));
});
test('_getNonOwnerRole null for uploader with no current rev', () => {
delete change.current_revision;
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.UPLOADER));
});
@@ -235,33 +235,39 @@
suite('role=committer', () => {
test('_getNonOwnerRole for committer', () => {
+ change.revisions.rev1.uploader.email = 'ghh@def';
assert.deepEqual(
element._getNonOwnerRole(change, element._CHANGE_ROLE.COMMITTER),
{email: 'ghi@def'});
});
+ test('_getNonOwnerRole is null if committer is same as uploader', () => {
+ assert.isNotOk(element._getNonOwnerRole(change,
+ element._CHANGE_ROLE.COMMITTER));
+ });
+
test('_getNonOwnerRole that it does not return committer', () => {
// Set the committer email to be the same as the owner.
change.revisions.rev1.commit.committer.email = 'abc@def';
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.COMMITTER));
});
test('_getNonOwnerRole null for committer with no current rev', () => {
delete change.current_revision;
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.COMMITTER));
});
test('_getNonOwnerRole null for committer with no commit', () => {
delete change.revisions.rev1.commit;
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.COMMITTER));
});
test('_getNonOwnerRole null for committer with no committer', () => {
delete change.revisions.rev1.commit.committer;
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.COMMITTER));
});
});
@@ -276,25 +282,25 @@
test('_getNonOwnerRole that it does not return author', () => {
// Set the author email to be the same as the owner.
change.revisions.rev1.commit.author.email = 'abc@def';
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.AUTHOR));
});
test('_getNonOwnerRole null for author with no current rev', () => {
delete change.current_revision;
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.AUTHOR));
});
test('_getNonOwnerRole null for author with no commit', () => {
delete change.revisions.rev1.commit;
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.AUTHOR));
});
test('_getNonOwnerRole null for author with no author', () => {
delete change.revisions.rev1.commit.author;
- assert.isNull(element._getNonOwnerRole(change,
+ assert.isNotOk(element._getNonOwnerRole(change,
element._CHANGE_ROLE.AUTHOR));
});
});
@@ -425,6 +431,7 @@
element.change = {
current_revision: '456',
revisions: {456: revision('111')},
+ owner: {},
};
element.revision = revision('222');
assert.equal(element._currentParents[0].commit, '222');
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 8504df4..1119a20 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
@@ -151,6 +151,7 @@
import {GrButton} from '../../shared/gr-button/gr-button';
import {GrMessagesList} from '../gr-messages-list/gr-messages-list';
import {GrThreadList} from '../gr-thread-list/gr-thread-list';
+import {PORTING_COMMENTS_CHANGE_LATENCY_LABEL} from '../../../services/gr-reporting/gr-reporting';
const CHANGE_ID_ERROR = {
MISMATCH: 'mismatch',
@@ -2127,9 +2128,20 @@
this._robotCommentThreads = undefined;
if (!this._changeNum)
throw new Error('missing required changeNum property');
- return this.$.commentAPI
+
+ const portedCommentsPromise = this.$.commentAPI.getPortedComments(
+ this._changeNum
+ );
+ const commentsPromise = this.$.commentAPI
.loadAll(this._changeNum)
- .then(comments => this._recomputeComments(comments));
+ .then(comments => {
+ this.reporting.time(PORTING_COMMENTS_CHANGE_LATENCY_LABEL);
+ this._recomputeComments(comments);
+ });
+ Promise.all([portedCommentsPromise, commentsPromise]).then(() => {
+ this.reporting.timeEnd(PORTING_COMMENTS_CHANGE_LATENCY_LABEL);
+ });
+ return commentsPromise;
}
/**
diff --git a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
index eb8d4cb..27d9756 100644
--- a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.ts
@@ -188,7 +188,7 @@
if (patchNumEquals(rev._number, patchNum)) {
const parentLength =
rev.commit && rev.commit.parents ? rev.commit.parents.length : 0;
- return parentLength === 0;
+ return parentLength === 0 || parentLength > 1;
}
}
return false;
diff --git a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog_test.js b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog_test.js
index 213c202..7401026 100644
--- a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog_test.js
+++ b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog_test.js
@@ -174,21 +174,33 @@
test('_computeHidePatchFile', () => {
const patchNum = '1';
- const change1 = {
+ const changeWithNoParent = {
revisions: {
r1: {_number: 1, commit: {parents: []}},
},
};
- assert.isTrue(element._computeHidePatchFile(change1, patchNum));
+ assert.isTrue(element._computeHidePatchFile(changeWithNoParent, patchNum));
- const change2 = {
+ const changeWithOneParent = {
revisions: {
r1: {_number: 1, commit: {parents: [
{commit: 'p1'},
]}},
},
};
- assert.isFalse(element._computeHidePatchFile(change2, patchNum));
+ assert.isFalse(
+ element._computeHidePatchFile(changeWithOneParent, patchNum));
+
+ const changeWithMultipleParents = {
+ revisions: {
+ r1: {_number: 1, commit: {parents: [
+ {commit: 'p1'},
+ {commit: 'p2'},
+ ]}},
+ },
+ };
+ assert.isTrue(
+ element._computeHidePatchFile(changeWithMultipleParents, patchNum));
});
});
diff --git a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_html.ts b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_html.ts
index 974e55d..7b1fb7f 100644
--- a/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-label-scores/gr-label-scores_html.ts
@@ -34,7 +34,7 @@
display: table-row;
}
gr-label-score-row.no-access {
- display: var(--label-no-access-display, table-row);
+ display: none;
}
</style>
<div class="scoresTable">
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
index c26afe3..d785a2f 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.ts
@@ -39,7 +39,7 @@
import {MergeabilityComputationBehavior} from '../../../constants/constants';
// Possible static search options for auto complete, without negations.
-const SEARCH_OPERATORS = [
+const SEARCH_OPERATORS: ReadonlyArray<string> = [
'added:',
'age:',
'age:1week', // Give an example age
@@ -117,7 +117,7 @@
];
// All of the ops, with corresponding negations.
-const SEARCH_OPERATORS_WITH_NEGATIONS_SET = new Set(
+const SEARCH_OPERATORS_WITH_NEGATIONS_SET: ReadonlySet<string> = new Set(
SEARCH_OPERATORS.concat(SEARCH_OPERATORS.map(op => `-${op}`))
);
@@ -149,6 +149,8 @@
return htmlTemplate;
}
+ private searchOperators = new Set(SEARCH_OPERATORS_WITH_NEGATIONS_SET);
+
/**
* Fired when a search is committed
*
@@ -203,7 +205,7 @@
mergeability ===
MergeabilityComputationBehavior.REF_UPDATED_AND_CHANGE_REINDEX
) {
- // add 'is:mergeable' to SEARCH_OPERATORS_WITH_NEGATIONS_SET
+ // add 'is:mergeable' to searchOperators
this._addOperator('is:mergeable');
}
if (serverConfig) {
@@ -225,9 +227,9 @@
}
_addOperator(name: string, include_neg = true) {
- SEARCH_OPERATORS_WITH_NEGATIONS_SET.add(name);
+ this.searchOperators.add(name);
if (include_neg) {
- SEARCH_OPERATORS_WITH_NEGATIONS_SET.add(`-${name}`);
+ this.searchOperators.add(`-${name}`);
}
}
@@ -264,9 +266,9 @@
if (!this._inputVal) return;
const trimmedInput = this._inputVal.trim();
if (trimmedInput) {
- const predefinedOpOnlyQuery = [
- ...SEARCH_OPERATORS_WITH_NEGATIONS_SET,
- ].some(op => op.endsWith(':') && op === trimmedInput);
+ const predefinedOpOnlyQuery = [...this.searchOperators].some(
+ op => op.endsWith(':') && op === trimmedInput
+ );
if (predefinedOpOnlyQuery) {
return;
}
@@ -319,7 +321,7 @@
default:
return Promise.resolve(
- [...SEARCH_OPERATORS_WITH_NEGATIONS_SET]
+ [...this.searchOperators]
.filter(operator => operator.includes(input))
.map(operator => {
return {text: operator};
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
index 0514553..8dce63f 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -35,6 +35,7 @@
RobotCommentInfo,
UrlEncodedCommentId,
NumericChangeId,
+ RevisionId,
} from '../../../types/common';
import {hasOwnProperty} from '../../../utils/common-util';
import {CommentSide} from '../../../constants/constants';
@@ -611,6 +612,19 @@
);
}
+ getPortedComments(changeNum: NumericChangeId, revision?: RevisionId) {
+ if (!revision) revision = 'current';
+ return Promise.all([
+ this.$.restAPI.getPortedComments(changeNum, revision),
+ this.$.restAPI.getPortedDrafts(changeNum, revision),
+ ]).then(result => {
+ return {
+ portedComments: result[0],
+ portedDrafts: result[1],
+ };
+ });
+ }
+
/**
* Load all comments (with drafts and robot comments) for the given change
* number. The returned promise resolves when the comments have loaded, but
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
index d1f52ae..91fd137 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
@@ -18,6 +18,7 @@
import {GrDiffLine, GrDiffLineType, LineNumber} from '../gr-diff/gr-diff-line';
import {
GrDiffGroup,
+ GrDiffGroupRange,
GrDiffGroupType,
hideInContextControl,
rangeBySide,
@@ -64,6 +65,10 @@
};
}
+export interface ContentLoadNeededEventDetail {
+ lineRange: GrDiffGroupRange;
+}
+
export abstract class GrDiffBuilder {
private readonly _diff: DiffInfo;
@@ -93,7 +98,7 @@
this._numLinesLeft = this._diff.content
? this._diff.content.reduce((sum, chunk) => {
const left = chunk.a || chunk.ab;
- return sum + (left ? left.length : 0);
+ return sum + (left?.length || chunk.skip || 0);
}, 0)
: 0;
this._prefs = prefs;
@@ -311,8 +316,13 @@
const td = this._createElement('td');
const showPartialLinks = numLines > PARTIAL_CONTEXT_AMOUNT;
-
- if (showPartialLinks && leftStart > 1) {
+ const firstGroupIsSkipped = !!contextGroups[0].skip;
+ const lastGroupIsSkipped = !!contextGroups[contextGroups.length - 1].skip;
+ const showAboveButton =
+ showPartialLinks && leftStart > 1 && !firstGroupIsSkipped;
+ const showBelowButton =
+ showPartialLinks && leftEnd < this._numLinesLeft && !lastGroupIsSkipped;
+ if (showAboveButton) {
td.appendChild(
this._createContextButton(
ContextButtonType.ABOVE,
@@ -322,7 +332,6 @@
)
);
}
-
td.appendChild(
this._createContextButton(
ContextButtonType.ALL,
@@ -331,8 +340,7 @@
numLines
)
);
-
- if (showPartialLinks && leftEnd < this._numLinesLeft) {
+ if (showBelowButton) {
td.appendChild(
this._createContextButton(
ContextButtonType.BELOW,
@@ -342,7 +350,6 @@
)
);
}
-
return td;
}
@@ -359,7 +366,9 @@
let text = '';
let groups: GrDiffGroup[] = []; // The groups that replace this one if tapped.
+ let requiresLoad = false;
if (type === GrDiffBuilder.ContextButtonType.ALL) {
+ requiresLoad = contextGroups.find(c => !!c.skip) !== undefined;
const icon = this._createElement('iron-icon', 'showContext');
icon.setAttribute('icon', 'gr-icons:unfold-more');
button.appendChild(icon);
@@ -368,6 +377,10 @@
if (numLines > 1) {
text += 's';
}
+ if (requiresLoad) {
+ // Expanding content would require load of more data
+ text += ' (too large)';
+ }
groups.push(...contextGroups);
} else if (type === GrDiffBuilder.ContextButtonType.ABOVE) {
text = `+${context} above`;
@@ -380,15 +393,36 @@
textSpan.textContent = text;
button.appendChild(textSpan);
- button.addEventListener('tap', e => {
- const event = e as ContextEvent;
- event.detail = {
- groups,
- section,
- numLines,
- };
- // Let it bubble up the DOM tree.
- });
+ if (requiresLoad) {
+ button.addEventListener('tap', e => {
+ e.stopPropagation();
+ const firstRange = groups[0].lineRange;
+ const lastRange = groups[groups.length - 1].lineRange;
+ const lineRange = {
+ left: {start: firstRange.left.start, end: lastRange.left.end},
+ right: {start: firstRange.right.start, end: lastRange.right.end},
+ };
+ button.dispatchEvent(
+ new CustomEvent<ContentLoadNeededEventDetail>('content-load-needed', {
+ detail: {
+ lineRange,
+ },
+ bubbles: true,
+ composed: true,
+ })
+ );
+ });
+ } else {
+ button.addEventListener('tap', e => {
+ const event = e as ContextEvent;
+ event.detail = {
+ groups,
+ section,
+ numLines,
+ };
+ // Let it bubble up the DOM tree.
+ });
+ }
return button;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
index 39e12ca..08ea1a6 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
@@ -274,7 +274,7 @@
}
_isCollapsibleChunk(chunk: DiffContent) {
- return (chunk.ab || chunk.common) && !chunk.keyLocation;
+ return (chunk.ab || chunk.common || chunk.skip) && !chunk.keyLocation;
}
/**
@@ -326,7 +326,11 @@
}
_commonChunkLength(chunk: DiffContent) {
+ if (chunk.skip) {
+ return chunk.skip;
+ }
console.assert(!!chunk.ab || !!chunk.common);
+
console.assert(
!chunk.a || (!!chunk.b && chunk.a.length === chunk.b.length),
'common chunk needs same number of a and b lines: ',
@@ -354,13 +358,21 @@
offsetLeft: number,
offsetRight: number
): GrDiffGroup {
- const type = chunk.ab ? GrDiffGroupType.BOTH : GrDiffGroupType.DELTA;
+ const type =
+ chunk.ab || chunk.skip ? GrDiffGroupType.BOTH : GrDiffGroupType.DELTA;
const lines = this._linesFromChunk(chunk, offsetLeft, offsetRight);
const group = new GrDiffGroup(type, lines);
group.keyLocation = !!chunk.keyLocation;
group.dueToRebase = !!chunk.due_to_rebase;
group.dueToMove = !!chunk.due_to_move;
+ group.skip = chunk.skip;
group.ignoredWhitespaceOnly = !!chunk.common;
+ if (chunk.skip) {
+ group.lineRange = {
+ left: {start: offsetLeft, end: offsetLeft + chunk.skip - 1},
+ right: {start: offsetRight, end: offsetRight + chunk.skip - 1},
+ };
+ }
return group;
}
@@ -497,7 +509,7 @@
for (const chunk of chunks) {
// If it isn't a common chunk, append it as-is and update line numbers.
- if (!chunk.ab && !chunk.common) {
+ if (!chunk.ab && !chunk.skip && !chunk.common) {
if (chunk.a) {
leftLineNum += chunk.a.length;
}
@@ -522,7 +534,13 @@
leftLineNum += numLines;
rightLineNum += numLines;
- if (chunk.ab) {
+ if (chunk.skip) {
+ result.push({
+ ...chunk,
+ skip: chunk.skip,
+ keyLocation: false,
+ });
+ } else if (chunk.ab) {
result.push(
...this._splitAtChunkEnds(chunk.ab, chunkEnds).map(
({lines, keyLocation}) => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js
index 3a074bb..eed5900 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js
@@ -171,6 +171,56 @@
});
});
+ test('at the beginning with skip chunks', async () => {
+ element.context = 10;
+ const content = [
+ {ab: new Array(20)
+ .fill('all work and no play make jack a dull boy')},
+ {skip: 43900},
+ {ab: new Array(30)
+ .fill('some other content')},
+ {a: ['some other content']},
+ ];
+
+ await element.process(content);
+
+ const groups = element.groups;
+
+ // group[0] is the file group
+
+ const commonGroup = groups[1];
+
+ // Hidden context before
+ assert.equal(commonGroup.type, GrDiffGroupType.CONTEXT_CONTROL);
+ assert.instanceOf(commonGroup.contextGroups[0], GrDiffGroup);
+ assert.equal(commonGroup.contextGroups[0].lines.length, 20);
+ for (const l of commonGroup.contextGroups[0].lines) {
+ assert.equal(l.text, 'all work and no play make jack a dull boy');
+ }
+
+ // Skipped group
+ const skipGroup = commonGroup.contextGroups[1];
+ assert.equal(skipGroup.skip, 43900);
+ const expectedRange = {
+ left: {start: 21, end: 43920},
+ right: {start: 21, end: 43920},
+ };
+ assert.deepEqual(skipGroup.lineRange, expectedRange);
+
+ // Hidden context after
+ assert.equal(commonGroup.contextGroups[2].lines.length, 20);
+ for (const l of commonGroup.contextGroups[2].lines) {
+ assert.equal(l.text, 'some other content');
+ }
+
+ // Displayed lines
+ assert.equal(groups[2].type, GrDiffGroupType.BOTH);
+ assert.equal(groups[2].lines.length, 10);
+ for (const l of groups[2].lines) {
+ assert.equal(l.text, 'some other content');
+ }
+ });
+
test('at the beginning, smaller than context', () => {
element.context = 10;
const content = [
@@ -414,6 +464,55 @@
});
});
+ test('in the middle with skip chunks', async () => {
+ element.context = 10;
+ const content = [
+ {a: ['all work and no play make andybons a dull boy']},
+ {ab: new Array(20)
+ .fill('all work and no play make jill a dull girl')},
+ {skip: 60},
+ {ab: new Array(20)
+ .fill('all work and no play make jill a dull girl')},
+ {a: ['all work and no play make andybons a dull boy']},
+ ];
+
+ await element.process(content);
+
+ const groups = element.groups;
+
+ // group[0] is the file group
+ // group[1] is the chunk with a
+ // group[2] is the displayed part of ab before
+
+ const commonGroup = groups[3];
+
+ // Hidden context before
+ assert.equal(commonGroup.type, GrDiffGroupType.CONTEXT_CONTROL);
+ assert.instanceOf(commonGroup.contextGroups[0], GrDiffGroup);
+ assert.equal(commonGroup.contextGroups[0].lines.length, 10);
+ for (const l of commonGroup.contextGroups[0].lines) {
+ assert.equal(
+ l.text, 'all work and no play make jill a dull girl');
+ }
+
+ // Skipped group
+ const skipGroup = commonGroup.contextGroups[1];
+ assert.equal(skipGroup.skip, 60);
+ const expectedRange = {
+ left: {start: 22, end: 81},
+ right: {start: 21, end: 80},
+ };
+ assert.deepEqual(skipGroup.lineRange, expectedRange);
+
+ // Hidden context after
+ assert.equal(commonGroup.contextGroups[2].lines.length, 10);
+ for (const l of commonGroup.contextGroups[2].lines) {
+ assert.equal(
+ l.text, 'all work and no play make jill a dull girl');
+ }
+ // group[4] is the displayed part of the second ab
+ });
+
test('break up common diff chunks', () => {
element.keyLocations = {
left: {1: true},
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 4bde136..f749f0e 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
@@ -101,6 +101,7 @@
import {CommentMap} from '../../../utils/comment-util';
import {AppElementParams} from '../../gr-app-types';
import {CustomKeyboardEvent, OpenFixPreviewEvent} from '../../../types/events';
+import {PORTING_COMMENTS_DIFF_LATENCY_LABEL} from '../../../services/gr-reporting/gr-reporting';
const ERR_REVIEW_STATUS = 'Couldn’t change file review status.';
const MSG_LOADING_BLAME = 'Loading blame...';
@@ -1060,6 +1061,11 @@
return;
}
+ const portedCommentsPromise = this.$.commentAPI.getPortedComments(
+ value.changeNum,
+ value.patchNum || 'current'
+ );
+
const promises: Promise<unknown>[] = [];
promises.push(this._getDiffPreferences());
@@ -1080,10 +1086,14 @@
this._loading = true;
return Promise.all(promises)
.then(r => {
+ this.reporting.time(PORTING_COMMENTS_DIFF_LATENCY_LABEL);
this._loading = false;
this._initPatchRange();
this._initCommitRange();
this.$.diffHost.comments = this._commentsForDiff;
+ portedCommentsPromise.then(() => {
+ this.reporting.timeEnd(PORTING_COMMENTS_DIFF_LATENCY_LABEL);
+ });
const edit = r[4] as EditInfo | undefined;
if (edit) {
this.set(`_change.revisions.${edit.commit.commit}`, {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.ts
index 9f5cdf3..588b9d1 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group.ts
@@ -38,7 +38,7 @@
end: number | null;
}
-interface GrDiffGroupRange {
+export interface GrDiffGroupRange {
left: Range;
right: Range;
}
@@ -89,7 +89,13 @@
[before, hidden] = _splitCommonGroups(hidden, hiddenStart);
}
if (hiddenEnd) {
- [hidden, after] = _splitCommonGroups(hidden, hiddenEnd - hiddenStart);
+ let beforeLength = 0;
+ if (before.length > 0) {
+ const beforeStart = before[0].lineRange.left.start || 0;
+ const beforeEnd = before[before.length - 1].lineRange.left.end || 0;
+ beforeLength = beforeEnd - beforeStart + 1;
+ }
+ [hidden, after] = _splitCommonGroups(hidden, hiddenEnd - beforeLength);
}
} else {
[hidden, after] = [[], hidden];
@@ -106,6 +112,67 @@
}
/**
+ * Splits a group in two, defined by leftSplit and rightSplit. Primarily to be
+ * used in function _splitCommonGroups
+ * Groups with some lines before and some lines after the split will be split
+ * into two groups, which will be put into the first and second list.
+ *
+ * @param group The group to be split in two
+ * @param leftSplit The line number relative to the split on the left side
+ * @param rightSplit The line number relative to the split on the right side
+ * @return two new groups, one before the split and another after it
+ */
+function _splitGroupInTwo(
+ group: GrDiffGroup,
+ leftSplit: number,
+ rightSplit: number
+) {
+ let beforeSplit: GrDiffGroup | undefined;
+ let afterSplit: GrDiffGroup | undefined;
+ // split line is in the middle of a group, we need to break the group
+ // in lines before and after the split.
+ if (group.skip) {
+ // Currently we assume skip chunks "refuse" to be split. Expanding this
+ // group will in the future mean load more data - and therefore we want to
+ // fire an event when user wants to do it.
+ const closerToStartThanEnd =
+ leftSplit - (group.lineRange.left.start || 0) <
+ (group.lineRange.right.end || 0) - leftSplit;
+ if (closerToStartThanEnd) {
+ afterSplit = group;
+ } else {
+ beforeSplit = group;
+ }
+ } else {
+ const before = [];
+ const after = [];
+ for (const line of group.lines) {
+ if (
+ (line.beforeNumber && line.beforeNumber < leftSplit) ||
+ (line.afterNumber && line.afterNumber < rightSplit)
+ ) {
+ before.push(line);
+ } else {
+ after.push(line);
+ }
+ }
+ if (before.length) {
+ beforeSplit =
+ before.length === group.lines.length
+ ? group
+ : group.cloneWithLines(before);
+ }
+ if (after.length) {
+ afterSplit =
+ after.length === group.lines.length
+ ? group
+ : group.cloneWithLines(after);
+ }
+ }
+ return {beforeSplit, afterSplit};
+}
+
+/**
* Splits a list of common groups into two lists of groups.
*
* Groups where all lines are before or all lines are after the split will be
@@ -129,47 +196,28 @@
const beforeGroups = [];
const afterGroups = [];
for (const group of groups) {
- if (
+ const isCompletelyBefore =
(group.lineRange.left.end || 0) < leftSplit ||
- (group.lineRange.right.end || 0) < rightSplit
- ) {
- beforeGroups.push(group);
- continue;
- }
- if (
+ (group.lineRange.right.end || 0) < rightSplit;
+ const isCompletelyAfter =
leftSplit <= (group.lineRange.left.start || 0) ||
- rightSplit <= (group.lineRange.right.start || 0)
- ) {
+ rightSplit <= (group.lineRange.right.start || 0);
+ if (isCompletelyBefore) {
+ beforeGroups.push(group);
+ } else if (isCompletelyAfter) {
afterGroups.push(group);
- continue;
- }
-
- const before = [];
- const after = [];
- for (const line of group.lines) {
- if (
- (line.beforeNumber && line.beforeNumber < leftSplit) ||
- (line.afterNumber && line.afterNumber < rightSplit)
- ) {
- before.push(line);
- } else {
- after.push(line);
+ } else {
+ const {beforeSplit, afterSplit} = _splitGroupInTwo(
+ group,
+ leftSplit,
+ rightSplit
+ );
+ if (beforeSplit) {
+ beforeGroups.push(beforeSplit);
}
- }
-
- if (before.length) {
- beforeGroups.push(
- before.length === group.lines.length
- ? group
- : group.cloneWithLines(before)
- );
- }
- if (after.length) {
- afterGroups.push(
- after.length === group.lines.length
- ? group
- : group.cloneWithLines(after)
- );
+ if (afterSplit) {
+ afterGroups.push(afterSplit);
+ }
}
}
return [beforeGroups, afterGroups];
@@ -213,6 +261,8 @@
contextGroups: GrDiffGroup[] = [];
+ skip?: number;
+
/** Both start and end line are inclusive. */
lineRange: GrDiffGroupRange = {
left: {start: null, end: null},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group_test.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group_test.js
index 13fc4d1..3423834 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff-group_test.js
@@ -177,6 +177,35 @@
assert.deepEqual(collapsedGroups[3].lines, groups[2].lines.slice(1));
});
+ suite('with skip chunks', () => {
+ setup(() => {
+ const skipGroup = new GrDiffGroup(GrDiffGroupType.BOTH);
+ skipGroup.skip = 60;
+ skipGroup.lineRange = {
+ left: {start: 8, end: 67},
+ right: {start: 10, end: 69},
+ };
+ groups = [
+ new GrDiffGroup(GrDiffGroupType.BOTH, [
+ new GrDiffLine(GrDiffLineType.BOTH, 5, 7),
+ new GrDiffLine(GrDiffLineType.BOTH, 6, 8),
+ new GrDiffLine(GrDiffLineType.BOTH, 7, 9),
+ ]),
+ skipGroup,
+ new GrDiffGroup(GrDiffGroupType.BOTH, [
+ new GrDiffLine(GrDiffLineType.BOTH, 68, 70),
+ new GrDiffLine(GrDiffLineType.BOTH, 69, 71),
+ new GrDiffLine(GrDiffLineType.BOTH, 70, 72),
+ ]),
+ ];
+ });
+
+ test('refuses to split skip group when closer to before', () => {
+ const collapsedGroups = hideInContextControl(groups, 4, 10);
+ assert.deepEqual(groups, collapsedGroups);
+ });
+ });
+
test('groups unchanged if the hidden range is empty', () => {
assert.deepEqual(
hideInContextControl(groups, 0, 0), groups);
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
index 7d08560..1a89008 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
@@ -2795,6 +2795,43 @@
return this._changeBaseURL(changeNum, patchNum).then(url => url + endpoint);
}
+ getPortedComments(
+ changeNum: NumericChangeId,
+ revision: RevisionId
+ ): Promise<PathToCommentsInfoMap | undefined> {
+ // maintaining a custom error function so that errors do not surface in UI
+ const errFn: ErrorCallback = (response?: Response | null) => {
+ if (response)
+ console.info(`Fetching ported comments failed, ${response.status}`);
+ };
+ return this._getChangeURLAndFetch({
+ changeNum,
+ endpoint: '/ported_comments/',
+ revision,
+ errFn,
+ });
+ }
+
+ getPortedDrafts(
+ changeNum: NumericChangeId,
+ revision: RevisionId
+ ): Promise<PathToCommentsInfoMap | undefined> {
+ // maintaining a custom error function so that errors do not surface in UI
+ const errFn: ErrorCallback = (response?: Response | null) => {
+ if (response)
+ console.info(`Fetching ported drafts failed, ${response.status}`);
+ };
+ return this.getLoggedIn().then(loggedIn => {
+ if (!loggedIn) return {};
+ return this._getChangeURLAndFetch({
+ changeNum,
+ endpoint: '/ported_drafts/',
+ revision,
+ errFn,
+ });
+ });
+ }
+
saveDiffDraft(
changeNum: NumericChangeId,
patchNum: PatchSetNum,
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
index 505fd58..d75c186 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
@@ -21,6 +21,7 @@
import {GrReviewerUpdatesParser} from './gr-reviewer-updates-parser.js';
import {ListChangesOption} from '../../../utils/change-util.js';
import {appContext} from '../../../services/app-context.js';
+import {createChange} from '../../../test/test-data-generators.js';
const basicFixture = fixtureFromElement('gr-rest-api-interface');
@@ -1343,6 +1344,28 @@
assert.isTrue(handler.calledOnce);
});
+ test('ported comment errors do not trigger error dialog', () => {
+ const change = createChange();
+ const dispatchStub = sinon.stub(element._restApiHelper, 'dispatchEvent');
+ sinon.stub(element._restApiHelper, 'fetchJSON').returns(Promise.resolve({
+ ok: false}));
+
+ element.getPortedComments(change._number, 'current');
+
+ assert.isFalse(dispatchStub.called);
+ });
+
+ test('ported drafts are not requested user is not logged in', () => {
+ const change = createChange();
+ sinon.stub(element, 'getLoggedIn').returns(Promise.resolve(false));
+ const getChangeURLAndFetchStub = sinon.stub(element,
+ '_getChangeURLAndFetch');
+
+ element.getPortedDrafts(change._number, 'current');
+
+ assert.isFalse(getChangeURLAndFetchStub.called);
+ });
+
test('saveChangeStarred', async () => {
sinon.stub(element, 'getFromProjectLookup')
.returns(Promise.resolve('test'));
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
index 743e0f4..e6139e5 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
@@ -20,6 +20,10 @@
// TODO(dmfilippov): TS-fix-any use more specific type instead if possible
export type EventDetails = any;
+export const PORTING_COMMENTS_DIFF_LATENCY_LABEL = 'PortingCommentsDiffLatency';
+export const PORTING_COMMENTS_CHANGE_LATENCY_LABEL =
+ 'PortingCommentsChangeLatency';
+
export interface Timer {
reset(): this;
end(): this;
diff --git a/polygerrit-ui/app/services/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/services/gr-rest-api/gr-rest-api.ts
index 950619b..08dbb16 100644
--- a/polygerrit-ui/app/services/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/services/gr-rest-api/gr-rest-api.ts
@@ -457,6 +457,16 @@
cancelCondition?: CancelConditionCallback
): Promise<ChangeInfo | undefined | null>;
+ getPortedComments(
+ changeNum: NumericChangeId,
+ revision: RevisionId
+ ): Promise<PathToCommentsInfoMap | undefined>;
+
+ getPortedDrafts(
+ changeNum: NumericChangeId,
+ revision: RevisionId
+ ): Promise<PathToCommentsInfoMap | undefined>;
+
getDiffComments(
changeNum: NumericChangeId
): Promise<PathToCommentsInfoMap | undefined>;
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 1d1d32e..96cb02a 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -1184,6 +1184,11 @@
export type PathToCommentsInfoMap = {[path: string]: CommentInfo[]};
+export type PortedCommentsAndDrafts = {
+ portedComments?: PathToCommentsInfoMap;
+ portedDrafts?: PathToCommentsInfoMap;
+};
+
/**
* The CommentRange entity describes the range of an inline comment.
* https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#comment-range
@@ -1249,7 +1254,7 @@
edit_b?: number[][];
due_to_rebase?: boolean;
due_to_move?: boolean;
- skip?: string;
+ skip?: number;
common?: string;
keyLocation?: boolean;
}
