Merge "Don't wrap manually in responsive modes."
diff --git a/java/com/google/gerrit/entities/LabelTypes.java b/java/com/google/gerrit/entities/LabelTypes.java
index 1c38c59..55a9976 100644
--- a/java/com/google/gerrit/entities/LabelTypes.java
+++ b/java/com/google/gerrit/entities/LabelTypes.java
@@ -20,6 +20,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
public class LabelTypes {
protected List<LabelType> labelTypes;
@@ -36,12 +37,12 @@
return labelTypes;
}
- public LabelType byLabel(LabelId labelId) {
- return byLabel().get(labelId.get().toLowerCase());
+ public Optional<LabelType> byLabel(LabelId labelId) {
+ return Optional.ofNullable(byLabel().get(labelId.get().toLowerCase()));
}
- public LabelType byLabel(String labelName) {
- return byLabel().get(labelName.toLowerCase());
+ public Optional<LabelType> byLabel(String labelName) {
+ return Optional.ofNullable(byLabel().get(labelName.toLowerCase()));
}
private Map<String, LabelType> byLabel() {
diff --git a/java/com/google/gerrit/entities/Project.java b/java/com/google/gerrit/entities/Project.java
index ef3cbeb..617b827 100644
--- a/java/com/google/gerrit/entities/Project.java
+++ b/java/com/google/gerrit/entities/Project.java
@@ -150,6 +150,7 @@
return builder;
}
+ @Nullable
public String getName() {
return getNameKey() != null ? getNameKey().get() : null;
}
@@ -183,7 +184,7 @@
@Override
public final String toString() {
- return Optional.of(getName()).orElse("<null>");
+ return Optional.ofNullable(getName()).orElse("<null>");
}
public abstract Builder toBuilder();
diff --git a/java/com/google/gerrit/server/PatchSetUtil.java b/java/com/google/gerrit/server/PatchSetUtil.java
index d60bc8f..326ddf4 100644
--- a/java/com/google/gerrit/server/PatchSetUtil.java
+++ b/java/com/google/gerrit/server/PatchSetUtil.java
@@ -151,8 +151,10 @@
ApprovalsUtil approvalsUtil = approvalsUtilProvider.get();
for (PatchSetApproval ap : approvalsUtil.byPatchSet(notes, change.currentPatchSetId())) {
- LabelType type = projectState.getLabelTypes(notes).byLabel(ap.label());
- if (type != null && ap.value() == 1 && type.getFunction() == LabelFunction.PATCH_SET_LOCK) {
+ Optional<LabelType> type = projectState.getLabelTypes(notes).byLabel(ap.label());
+ if (type.isPresent()
+ && ap.value() == 1
+ && type.get().getFunction() == LabelFunction.PATCH_SET_LOCK) {
return true;
}
}
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java
index 1efbd37..8d409e5 100644
--- a/java/com/google/gerrit/server/approval/ApprovalInference.java
+++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -52,6 +52,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
+import java.util.Optional;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -397,14 +398,14 @@
if (resultByUser.contains(psa.label(), psa.accountId())) {
continue;
}
- LabelType type = labelTypes.byLabel(psa.labelId());
+ Optional<LabelType> type = labelTypes.byLabel(psa.labelId());
// Only compute modified files if there is a relevant label, since this is expensive.
if (modifiedFiles == null
- && type != null
- && type.isCopyAllScoresIfListOfFilesDidNotChange()) {
+ && type.isPresent()
+ && type.get().isCopyAllScoresIfListOfFilesDidNotChange()) {
modifiedFiles = listModifiedFiles(project, ps, priorPatchSet);
}
- if (type == null) {
+ if (!type.isPresent()) {
logger.atFine().log(
"approval %d on label %s of patch set %d of change %d cannot be copied"
+ " to patch set %d because the label no longer exists on project %s",
@@ -416,8 +417,8 @@
project.getName());
continue;
}
- if (!canCopyBasedOnBooleanLabelConfigs(project, psa, ps.id(), kind, type, modifiedFiles)
- && !canCopyBasedOnCopyCondition(notes, psa, ps.id(), type, kind)) {
+ if (!canCopyBasedOnBooleanLabelConfigs(project, psa, ps.id(), kind, type.get(), modifiedFiles)
+ && !canCopyBasedOnCopyCondition(notes, psa, ps.id(), type.get(), kind)) {
continue;
}
resultByUser.put(psa.label(), psa.accountId(), psa.copyWithPatchSet(ps.id()));
diff --git a/java/com/google/gerrit/server/approval/ApprovalsUtil.java b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
index b1e85e9..a1cdd99 100644
--- a/java/com/google/gerrit/server/approval/ApprovalsUtil.java
+++ b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
@@ -61,6 +61,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -299,8 +300,12 @@
List<PatchSetApproval> cells = new ArrayList<>(approvals.size());
Date ts = update.getWhen();
for (Map.Entry<String, Short> vote : approvals.entrySet()) {
- LabelType lt = labelTypes.byLabel(vote.getKey());
- cells.add(newApproval(ps.id(), user, lt.getLabelId(), vote.getValue(), ts).build());
+ Optional<LabelType> lt = labelTypes.byLabel(vote.getKey());
+ if (!lt.isPresent()) {
+ throw new BadRequestException(
+ String.format("label \"%s\" is not a configured label", vote.getKey()));
+ }
+ cells.add(newApproval(ps.id(), user, lt.get().getLabelId(), vote.getValue(), ts).build());
}
for (PatchSetApproval psa : cells) {
update.putApproval(psa.label(), psa.value());
@@ -310,11 +315,11 @@
public static void checkLabel(LabelTypes labelTypes, String name, Short value)
throws BadRequestException {
- LabelType label = labelTypes.byLabel(name);
- if (label == null) {
+ Optional<LabelType> label = labelTypes.byLabel(name);
+ if (!label.isPresent()) {
throw new BadRequestException(String.format("label \"%s\" is not a configured label", name));
}
- if (label.getValue(value) == null) {
+ if (label.get().getValue(value) == null) {
throw new BadRequestException(
String.format("label \"%s\": %d is not a valid value", name, value));
}
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonExperimentImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonExperimentImpl.java
index 3f7ce68..81f014d 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonExperimentImpl.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonExperimentImpl.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.change;
+import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
@@ -33,7 +34,9 @@
* FileInfoJsonNewImpl}.
*/
public class FileInfoJsonExperimentImpl implements FileInfoJson {
- private final String NEW_DIFF_CACHE_FEATURE = "GerritBackendRequestFeature__use_new_diff_cache";
+ @VisibleForTesting
+ public static final String NEW_DIFF_CACHE_FEATURE =
+ "GerritBackendRequestFeature__use_new_diff_cache";
private final FileInfoJsonOldImpl oldImpl;
private final FileInfoJsonNewImpl newImpl;
diff --git a/java/com/google/gerrit/server/change/LabelNormalizer.java b/java/com/google/gerrit/server/change/LabelNormalizer.java
index 30343d4..b5527d7 100644
--- a/java/com/google/gerrit/server/change/LabelNormalizer.java
+++ b/java/com/google/gerrit/server/change/LabelNormalizer.java
@@ -33,6 +33,7 @@
import com.google.inject.Singleton;
import java.util.Collection;
import java.util.List;
+import java.util.Optional;
/**
* Normalizes votes on labels according to project config.
@@ -101,12 +102,12 @@
unchanged.add(psa);
continue;
}
- LabelType label = labelTypes.byLabel(psa.labelId());
- if (label == null) {
+ Optional<LabelType> label = labelTypes.byLabel(psa.labelId());
+ if (!label.isPresent()) {
deleted.add(psa);
continue;
}
- PatchSetApproval copy = applyTypeFloor(label, psa);
+ PatchSetApproval copy = applyTypeFloor(label.get(), psa);
if (copy.value() != psa.value()) {
updated.add(copy);
} else {
diff --git a/java/com/google/gerrit/server/change/LabelsJson.java b/java/com/google/gerrit/server/change/LabelsJson.java
index acff03c..5ce121b 100644
--- a/java/com/google/gerrit/server/change/LabelsJson.java
+++ b/java/com/google/gerrit/server/change/LabelsJson.java
@@ -57,6 +57,7 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
@@ -103,9 +104,9 @@
for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels != null) {
for (SubmitRecord.Label r : rec.labels) {
- LabelType type = labelTypes.byLabel(r.label);
- if (type != null && (!isMerged || type.isAllowPostSubmit())) {
- toCheck.put(type.getName(), type);
+ Optional<LabelType> type = labelTypes.byLabel(r.label);
+ if (type.isPresent() && (!isMerged || type.get().isAllowPostSubmit())) {
+ toCheck.put(type.get().getName(), type.get());
}
}
}
@@ -120,18 +121,18 @@
continue;
}
for (SubmitRecord.Label r : rec.labels) {
- LabelType type = labelTypes.byLabel(r.label);
- if (type == null || (isMerged && !type.isAllowPostSubmit())) {
+ Optional<LabelType> type = labelTypes.byLabel(r.label);
+ if (!type.isPresent() || (isMerged && !type.get().isAllowPostSubmit())) {
continue;
}
- for (LabelValue v : type.getValues()) {
- boolean ok = can.contains(new LabelPermission.WithValue(type, v));
+ for (LabelValue v : type.get().getValues()) {
+ boolean ok = can.contains(new LabelPermission.WithValue(type.get(), v));
if (isMerged) {
if (labels == null) {
labels = currentLabels(filterApprovalsBy, cd);
}
- short prev = labels.getOrDefault(type.getName(), (short) 0);
+ short prev = labels.getOrDefault(type.get().getName(), (short) 0);
ok &= v.getValue() >= prev;
}
if (ok) {
@@ -176,21 +177,21 @@
setAllApprovals(accountLoader, cd, labels);
}
for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
- LabelType type = labelTypes.byLabel(e.getKey());
- if (type == null) {
+ Optional<LabelType> type = labelTypes.byLabel(e.getKey());
+ if (!type.isPresent()) {
continue;
}
if (standard) {
for (PatchSetApproval psa : cd.currentApprovals()) {
- if (type.matches(psa)) {
+ if (type.get().matches(psa)) {
short val = psa.value();
Account.Id accountId = psa.accountId();
- setLabelScores(accountLoader, type, e.getValue(), val, accountId);
+ setLabelScores(accountLoader, type.get(), e.getValue(), val, accountId);
}
}
}
if (detailed) {
- setLabelValues(type, e.getValue());
+ setLabelValues(type.get(), e.getValue());
}
}
return labels;
@@ -261,9 +262,9 @@
MultimapBuilder.hashKeys().hashSetValues().build();
for (PatchSetApproval a : cd.currentApprovals()) {
allUsers.add(a.accountId());
- LabelType type = labelTypes.byLabel(a.labelId());
- if (type != null) {
- labelNames.add(type.getName());
+ Optional<LabelType> type = labelTypes.byLabel(a.labelId());
+ if (type.isPresent()) {
+ labelNames.add(type.get().getName());
// Not worth the effort to distinguish between votable/non-votable for 0
// values on closed changes, since they can't vote anyway.
current.put(a.accountId(), a);
@@ -292,8 +293,8 @@
if (detailed) {
labels.entrySet().stream()
- .filter(e -> labelTypes.byLabel(e.getKey()) != null)
- .forEach(e -> setLabelValues(labelTypes.byLabel(e.getKey()), e.getValue()));
+ .filter(e -> labelTypes.byLabel(e.getKey()).isPresent())
+ .forEach(e -> setLabelValues(labelTypes.byLabel(e.getKey()).get(), e.getValue()));
}
for (Account.Id accountId : allUsers) {
@@ -308,16 +309,16 @@
}
}
for (PatchSetApproval psa : current.get(accountId)) {
- LabelType type = labelTypes.byLabel(psa.labelId());
- if (type == null) {
+ Optional<LabelType> type = labelTypes.byLabel(psa.labelId());
+ if (!type.isPresent()) {
continue;
}
short val = psa.value();
- ApprovalInfo info = byLabel.get(type.getName());
+ ApprovalInfo info = byLabel.get(type.get().getName());
if (info != null) {
info.value = Integer.valueOf(val);
- info.permittedVotingRange = pvr.getOrDefault(type.getName(), null);
+ info.permittedVotingRange = pvr.getOrDefault(type.get().getName(), null);
info.date = psa.granted();
info.tag = psa.tag().orElse(null);
if (psa.postSubmit()) {
@@ -328,7 +329,7 @@
continue;
}
- setLabelScores(accountLoader, type, labels.get(type.getName()), val, accountId);
+ setLabelScores(accountLoader, type.get(), labels.get(type.get().getName()), val, accountId);
}
}
return labels;
@@ -428,24 +429,24 @@
PermissionBackend.ForChange perm = permissionBackend.absentUser(accountId).change(cd);
Map<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(accountId, cd));
for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
- LabelType lt = labelTypes.byLabel(e.getKey());
- if (lt == null) {
+ Optional<LabelType> lt = labelTypes.byLabel(e.getKey());
+ if (!lt.isPresent()) {
// Ignore submit record for undefined label; likely the submit rule
// author didn't intend for the label to show up in the table.
continue;
}
Integer value;
- VotingRangeInfo permittedVotingRange = pvr.getOrDefault(lt.getName(), null);
+ VotingRangeInfo permittedVotingRange = pvr.getOrDefault(lt.get().getName(), null);
String tag = null;
Timestamp date = null;
- PatchSetApproval psa = current.get(accountId, lt.getName());
+ PatchSetApproval psa = current.get(accountId, lt.get().getName());
if (psa != null) {
value = Integer.valueOf(psa.value());
if (value == 0) {
// This may be a dummy approval that was inserted when the reviewer
// was added. Explicitly check whether the user can vote on this
// label.
- value = perm.test(new LabelPermission(lt)) ? 0 : null;
+ value = perm.test(new LabelPermission(lt.get())) ? 0 : null;
}
tag = psa.tag().orElse(null);
date = psa.granted();
@@ -456,7 +457,7 @@
// Either the user cannot vote on this label, or they were added as a
// reviewer but have not responded yet. Explicitly check whether the
// user can vote on this label.
- value = perm.test(new LabelPermission(lt)) ? 0 : null;
+ value = perm.test(new LabelPermission(lt.get())) ? 0 : null;
}
addApproval(
e.getValue().label(),
diff --git a/java/com/google/gerrit/server/change/ReviewerJson.java b/java/com/google/gerrit/server/change/ReviewerJson.java
index d5b74a8..6189708 100644
--- a/java/com/google/gerrit/server/change/ReviewerJson.java
+++ b/java/com/google/gerrit/server/change/ReviewerJson.java
@@ -38,6 +38,7 @@
import com.google.inject.Singleton;
import java.util.Collection;
import java.util.List;
+import java.util.Optional;
import java.util.TreeMap;
@Singleton
@@ -107,10 +108,8 @@
out.approvals = new TreeMap<>(labelTypes.nameComparator());
for (PatchSetApproval ca : approvals) {
- LabelType at = labelTypes.byLabel(ca.labelId());
- if (at != null) {
- out.approvals.put(at.getName(), formatValue(ca.value()));
- }
+ Optional<LabelType> at = labelTypes.byLabel(ca.labelId());
+ at.ifPresent(lt -> out.approvals.put(lt.getName(), formatValue(ca.value())));
}
// Add dummy approvals for all permitted labels for the user even if they
@@ -125,13 +124,13 @@
}
for (SubmitRecord.Label label : rec.labels) {
String name = label.label;
- LabelType type = labelTypes.byLabel(name);
- if (out.approvals.containsKey(name) || type == null) {
+ Optional<LabelType> type = labelTypes.byLabel(name);
+ if (out.approvals.containsKey(name) || !type.isPresent()) {
continue;
}
try {
- perm.check(new LabelPermission(type));
+ perm.check(new LabelPermission(type.get()));
out.approvals.put(name, formatValue((short) 0));
} catch (AuthException e) {
// Do nothing.
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index 3f988a3..1bb694a 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/java/com/google/gerrit/server/events/EventFactory.java
@@ -71,6 +71,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
@@ -535,10 +536,8 @@
a.grantedOn = approval.granted().getTime() / 1000L;
a.oldValue = null;
- LabelType lt = labelTypes.byLabel(approval.labelId());
- if (lt != null) {
- a.description = lt.getName();
- }
+ Optional<LabelType> lt = labelTypes.byLabel(approval.labelId());
+ lt.ifPresent(l -> a.description = l.getName());
return a;
}
diff --git a/java/com/google/gerrit/server/events/StreamEventsApiListener.java b/java/com/google/gerrit/server/events/StreamEventsApiListener.java
index 439f53e..edd1928 100644
--- a/java/com/google/gerrit/server/events/StreamEventsApiListener.java
+++ b/java/com/google/gerrit/server/events/StreamEventsApiListener.java
@@ -23,7 +23,6 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelTypes;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
@@ -202,10 +201,7 @@
a.oldValue = Short.toString(oldApprovals.get(approval.getKey()));
}
}
- LabelType lt = labelTypes.byLabel(approval.getKey());
- if (lt != null) {
- a.description = lt.getName();
- }
+ labelTypes.byLabel(approval.getKey()).ifPresent(lt -> a.description = lt.getName());
if (approval.getValue() != null) {
a.value = Short.toString(approval.getValue());
}
diff --git a/java/com/google/gerrit/server/git/MergeUtil.java b/java/com/google/gerrit/server/git/MergeUtil.java
index 1da14f8..a5ea24d 100644
--- a/java/com/google/gerrit/server/git/MergeUtil.java
+++ b/java/com/google/gerrit/server/git/MergeUtil.java
@@ -600,11 +600,11 @@
} else if (isVerified(a.labelId())) {
tag = "Tested-by";
} else {
- final LabelType lt = project.getLabelTypes().byLabel(a.labelId());
- if (lt == null) {
+ final Optional<LabelType> lt = project.getLabelTypes().byLabel(a.labelId());
+ if (!lt.isPresent()) {
continue;
}
- tag = lt.getName();
+ tag = lt.get().getName();
}
if (!contains(footers, new FooterKey(tag), identbuf.toString())) {
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index f00b48eb..b2a31b9 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -465,10 +465,10 @@
continue;
}
- LabelType lt = projectState.getLabelTypes().byLabel(a.labelId());
- if (lt != null) {
- current.put(lt.getName(), a);
- }
+ projectState
+ .getLabelTypes()
+ .byLabel(a.labelId())
+ .ifPresent(l -> current.put(l.getName(), a));
}
}
return current;
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 810cd4d..1ee12fe 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -605,7 +605,7 @@
for (PatchSetApproval a : cd.currentApprovals()) {
if (a.value() != 0 && !a.isLegacySubmit()) {
allApprovals.add(formatLabel(a.label(), a.value(), a.accountId()));
- LabelType labelType = cd.getLabelTypes().byLabel(a.labelId());
+ Optional<LabelType> labelType = cd.getLabelTypes().byLabel(a.labelId());
allApprovals.addAll(getMaxMinAnyLabels(a.label(), a.value(), labelType, a.accountId()));
if (owners && cd.change().getOwner().equals(a.accountId())) {
allApprovals.add(formatLabel(a.label(), a.value(), ChangeQueryBuilder.OWNER_ACCOUNT_ID));
@@ -622,13 +622,15 @@
}
private static List<String> getMaxMinAnyLabels(
- String label, short labelVal, LabelType labelType, @Nullable Account.Id accountId) {
+ String label, short labelVal, Optional<LabelType> labelType, @Nullable Account.Id accountId) {
List<String> labels = new ArrayList<>();
- if (labelVal == labelType.getMaxPositive()) {
- labels.add(formatLabel(label, MagicLabelValue.MAX.name(), accountId));
- }
- if (labelVal == labelType.getMaxNegative()) {
- labels.add(formatLabel(label, MagicLabelValue.MIN.name(), accountId));
+ if (labelType.isPresent()) {
+ if (labelVal == labelType.get().getMaxPositive()) {
+ labels.add(formatLabel(label, MagicLabelValue.MAX.name(), accountId));
+ }
+ if (labelVal == labelType.get().getMaxNegative()) {
+ labels.add(formatLabel(label, MagicLabelValue.MIN.name(), accountId));
+ }
}
labels.add(formatLabel(label, MagicLabelValue.ANY.name(), accountId));
return labels;
diff --git a/java/com/google/gerrit/server/mail/send/MergedSender.java b/java/com/google/gerrit/server/mail/send/MergedSender.java
index 6af2345..56528df 100644
--- a/java/com/google/gerrit/server/mail/send/MergedSender.java
+++ b/java/com/google/gerrit/server/mail/send/MergedSender.java
@@ -79,14 +79,14 @@
Table<Account.Id, String, PatchSetApproval> pos = HashBasedTable.create();
Table<Account.Id, String, PatchSetApproval> neg = HashBasedTable.create();
for (PatchSetApproval ca : args.approvalsUtil.byPatchSet(changeData.notes(), patchSet.id())) {
- LabelType lt = labelTypes.byLabel(ca.labelId());
- if (lt == null) {
+ Optional<LabelType> lt = labelTypes.byLabel(ca.labelId());
+ if (!lt.isPresent()) {
continue;
}
if (ca.value() > 0) {
- pos.put(ca.accountId(), lt.getName(), ca);
+ pos.put(ca.accountId(), lt.get().getName(), ca);
} else if (ca.value() < 0) {
- neg.put(ca.accountId(), lt.getName(), ca);
+ neg.put(ca.accountId(), lt.get().getName(), ca);
}
}
diff --git a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
index e33b261..62cfa47 100644
--- a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
+++ b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
@@ -18,6 +18,7 @@
import com.google.gerrit.common.data.PatchScript;
import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.PatchSet;
@@ -43,6 +44,7 @@
import java.io.IOException;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.stream.Collectors;
import org.eclipse.jgit.diff.Edit;
import org.eclipse.jgit.lib.Config;
@@ -242,10 +244,9 @@
if (!patchSetApproval.label().equals(LabelId.CODE_REVIEW)) {
continue;
}
- if (!projectState
- .getLabelTypes(notes)
- .byLabel(patchSetApproval.labelId())
- .isMaxPositive(patchSetApproval)) {
+ Optional<LabelType> lt =
+ projectState.getLabelTypes(notes).byLabel(patchSetApproval.labelId());
+ if (!lt.isPresent() || !lt.get().isMaxPositive(patchSetApproval)) {
continue;
}
if (patchSetApproval.patchSetId().get() > maxPatchSetId.get()) {
diff --git a/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java b/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
index b779bf7..e4fd728 100644
--- a/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/diff/ModifiedFilesCacheImpl.java
@@ -18,11 +18,14 @@
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ListMultimap;
import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.DiffNotAvailableException;
@@ -37,6 +40,7 @@
import com.google.inject.TypeLiteral;
import com.google.inject.name.Named;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.stream.Stream;
@@ -82,7 +86,7 @@
.valueSerializer(GitModifiedFilesCacheImpl.ValueSerializer.INSTANCE)
.maximumWeight(10 << 20)
.weigher(ModifiedFilesWeigher.class)
- .version(1)
+ .version(2)
.loader(ModifiedFilesLoader.class);
}
};
@@ -139,7 +143,7 @@
.bTree(bTree)
.renameScore(key.renameScore())
.build();
- List<ModifiedFile> modifiedFiles = gitCache.get(gitKey);
+ List<ModifiedFile> modifiedFiles = mergeRewrittenEntries(gitCache.get(gitKey));
if (key.aCommit().equals(ObjectId.zeroId())) {
return ImmutableList.copyOf(modifiedFiles);
}
@@ -202,5 +206,61 @@
// value as the set of file paths shouldn't contain it.
return touchedFilePaths.contains(oldFilePath) || touchedFilePaths.contains(newFilePath);
}
+
+ /**
+ * Return the {@code modifiedFiles} input list while merging {@link ChangeType#ADDED} and {@link
+ * ChangeType#DELETED} entries for the same file into a single {@link ChangeType#REWRITE} entry.
+ *
+ * <p>Background: In some cases, JGit returns two diff entries (ADDED + DELETED) for the same
+ * file path. This happens e.g. when a file's mode is changed between patchsets, for example
+ * converting a symlink file to a regular file. We identify this case and return a single
+ * modified file with changeType = {@link ChangeType#REWRITE}.
+ */
+ private static List<ModifiedFile> mergeRewrittenEntries(List<ModifiedFile> modifiedFiles) {
+ List<ModifiedFile> result = new ArrayList<>();
+
+ // Handle ADDED and DELETED entries separately.
+ ListMultimap<String, ModifiedFile> byPath = ArrayListMultimap.create();
+ modifiedFiles.stream()
+ .filter(ModifiedFilesLoader::isAddedOrDeleted)
+ .forEach(
+ f -> {
+ if (f.oldPath().isPresent()) {
+ byPath.get(f.oldPath().get()).add(f);
+ }
+ if (f.newPath().isPresent()) {
+ byPath.get(f.newPath().get()).add(f);
+ }
+ });
+ for (String path : byPath.keySet()) {
+ List<ModifiedFile> entries = byPath.get(path);
+ if (entries.size() == 1) {
+ result.add(entries.get(0));
+ } else if (entries.size() == 2) {
+ result.add(getAddedEntry(entries).toBuilder().changeType(ChangeType.REWRITE).build());
+ } else {
+ // JGit error. Not expected to happen.
+ logger.atWarning().log(
+ "Found %d ADDED and DELETED entries for the same file path: %s."
+ + " Adding the first entry only to the result.",
+ entries.size(), entries);
+ result.add(entries.get(0));
+ }
+ }
+
+ // Add the remaining non ADDED/DELETED entries to the result
+ modifiedFiles.stream().filter(f -> !isAddedOrDeleted(f)).forEach(result::add);
+ return result;
+ }
+
+ private static boolean isAddedOrDeleted(ModifiedFile f) {
+ return f.changeType() == ChangeType.ADDED || f.changeType() == ChangeType.DELETED;
+ }
+
+ private static ModifiedFile getAddedEntry(List<ModifiedFile> modifiedFiles) {
+ return modifiedFiles.get(0).changeType() == ChangeType.ADDED
+ ? modifiedFiles.get(0)
+ : modifiedFiles.get(1);
+ }
}
}
diff --git a/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java b/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
index 9512094..f4e7ca3 100644
--- a/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
+++ b/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
@@ -51,6 +51,8 @@
return new AutoValue_ModifiedFile.Builder();
}
+ public abstract Builder toBuilder();
+
/** Computes this object's weight, which is its size in bytes. */
public int weight() {
int weight = 1; // the changeType field
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
index 2f2d29b..a502a46 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiff.java
@@ -187,6 +187,10 @@
return result;
}
+ public String getDefaultPath() {
+ return oldPath().isPresent() ? oldPath().get() : newPath().get();
+ }
+
public static Builder builder() {
return new AutoValue_GitFileDiff.Builder();
}
diff --git a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
index 2ce6925..77b8938 100644
--- a/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
+++ b/java/com/google/gerrit/server/patch/gitfilediff/GitFileDiffCacheImpl.java
@@ -24,7 +24,11 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.MultimapBuilder;
+import com.google.common.collect.Multimaps;
import com.google.common.collect.Streams;
+import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.server.cache.CacheModule;
@@ -41,6 +45,7 @@
import com.google.inject.Singleton;
import com.google.inject.name.Named;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;
@@ -105,6 +110,9 @@
private final LoadingCache<GitFileDiffCacheKey, GitFileDiff> cache;
+ private static final ImmutableSet<Patch.ChangeType> ADDED_AND_DELETED =
+ ImmutableSet.of(Patch.ChangeType.ADDED, Patch.ChangeType.DELETED);
+
@Inject
public GitFileDiffCacheImpl(
@Named(GIT_DIFF) LoadingCache<GitFileDiffCacheKey, GitFileDiff> cache) {
@@ -163,7 +171,7 @@
}
@Override
- public GitFileDiff load(GitFileDiffCacheKey key) throws IOException {
+ public GitFileDiff load(GitFileDiffCacheKey key) throws IOException, DiffNotAvailableException {
try (TraceTimer timer =
TraceContext.newTimer(
"Loading a single key from git file diff cache",
@@ -177,7 +185,8 @@
@Override
public Map<GitFileDiffCacheKey, GitFileDiff> loadAll(
- Iterable<? extends GitFileDiffCacheKey> keys) throws IOException {
+ Iterable<? extends GitFileDiffCacheKey> keys)
+ throws IOException, DiffNotAvailableException {
try (TraceTimer timer =
TraceContext.newTimer("Loading multiple keys from git file diff cache")) {
ImmutableMap.Builder<GitFileDiffCacheKey, GitFileDiff> result =
@@ -215,13 +224,14 @@
*/
private Map<GitFileDiffCacheKey, GitFileDiff> loadAllImpl(
Repository repo, ObjectReader reader, DiffOptions options, List<GitFileDiffCacheKey> keys)
- throws IOException {
+ throws IOException, DiffNotAvailableException {
ImmutableMap.Builder<GitFileDiffCacheKey, GitFileDiff> result =
ImmutableMap.builderWithExpectedSize(keys.size());
Map<GitFileDiffCacheKey, String> filePaths =
keys.stream().collect(Collectors.toMap(identity(), GitFileDiffCacheKey::newFilePath));
DiffFormatter formatter = createDiffFormatter(options, repo, reader);
- Map<String, DiffEntry> diffEntries = loadDiffEntries(formatter, options, filePaths.values());
+ ListMultimap<String, DiffEntry> diffEntries =
+ loadDiffEntries(formatter, options, filePaths.values());
for (GitFileDiffCacheKey key : filePaths.keySet()) {
String newFilePath = filePaths.get(key);
if (!diffEntries.containsKey(newFilePath)) {
@@ -233,14 +243,25 @@
newFilePath));
continue;
}
- DiffEntry diffEntry = diffEntries.get(newFilePath);
- GitFileDiff gitFileDiff = createGitFileDiff(diffEntry, formatter, key);
- result.put(key, gitFileDiff);
+ List<DiffEntry> entries = diffEntries.get(newFilePath);
+ if (entries.size() == 1) {
+ result.put(key, createGitFileDiff(entries.get(0), formatter, key));
+ } else {
+ // Handle when JGit returns two {Added, Deleted} entries for the same file. This happens,
+ // for example, when a file's mode is changed between patchsets (e.g. converting a
+ // symlink to a regular file). We combine both diff entries into a single entry with
+ // {changeType = Rewrite}.
+ List<GitFileDiff> gitDiffs = new ArrayList<>();
+ for (DiffEntry entry : diffEntries.get(newFilePath)) {
+ gitDiffs.add(createGitFileDiff(entry, formatter, key));
+ }
+ result.put(key, createRewriteEntry(gitDiffs));
+ }
}
return result.build();
}
- private static Map<String, DiffEntry> loadDiffEntries(
+ private static ListMultimap<String, DiffEntry> loadDiffEntries(
DiffFormatter diffFormatter, DiffOptions diffOptions, Collection<String> filePaths)
throws IOException {
Set<String> filePathsSet = ImmutableSet.copyOf(filePaths);
@@ -251,7 +272,11 @@
return diffEntries.stream()
.filter(d -> filePathsSet.contains(pathExtractor.apply(d)))
- .collect(Collectors.toMap(d -> pathExtractor.apply(d), identity()));
+ .collect(
+ Multimaps.toMultimap(
+ d -> pathExtractor.apply(d),
+ identity(),
+ MultimapBuilder.treeKeys().arrayListValues()::build));
}
private static DiffFormatter createDiffFormatter(
@@ -334,6 +359,39 @@
}
}
+ /**
+ * Create a single {@link GitFileDiff} with {@link Patch.ChangeType} equals {@link
+ * Patch.ChangeType#REWRITE}, assuming the input list contains two entries with types {@link
+ * Patch.ChangeType#ADDED} and {@link Patch.ChangeType#DELETED}.
+ *
+ * @param gitDiffs input list of exactly two {@link GitFileDiff} for same file path.
+ * @return a single {@link GitFileDiff} with change type equals {@link Patch.ChangeType#REWRITE}.
+ * @throws DiffNotAvailableException if input list contains git diffs with change types other than
+ * {ADDED, DELETED}. This is a JGit error.
+ */
+ private static GitFileDiff createRewriteEntry(List<GitFileDiff> gitDiffs)
+ throws DiffNotAvailableException {
+ if (gitDiffs.size() != 2) {
+ throw new DiffNotAvailableException(
+ String.format(
+ "JGit error: found %d dff entries for same file path %s",
+ gitDiffs.size(), gitDiffs.get(0).getDefaultPath()));
+ }
+ if (!ImmutableSet.of(gitDiffs.get(0).changeType(), gitDiffs.get(1).changeType())
+ .equals(ADDED_AND_DELETED)) {
+ // This is an illegal state. JGit is not supposed to return this, so we throw an exception.
+ throw new DiffNotAvailableException(
+ String.format(
+ "JGit error: unexpected change types %s and %s for same file path %s",
+ gitDiffs.get(0).changeType(),
+ gitDiffs.get(1).changeType(),
+ gitDiffs.get(0).getDefaultPath()));
+ }
+ GitFileDiff addedEntry =
+ gitDiffs.get(0).changeType() == Patch.ChangeType.ADDED ? gitDiffs.get(0) : gitDiffs.get(1);
+ return addedEntry.toBuilder().changeType(Patch.ChangeType.REWRITE).build();
+ }
+
/** An entity representing the options affecting the diff computation. */
@AutoValue
abstract static class DiffOptions {
diff --git a/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java b/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java
index 2924e6e..326620d 100644
--- a/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java
+++ b/java/com/google/gerrit/server/query/approval/MagicValuePredicate.java
@@ -23,6 +23,7 @@
import com.google.inject.assistedinject.Assisted;
import java.util.Collection;
import java.util.Objects;
+import java.util.Optional;
/** Predicate that matches patch set approvals we want to copy based on the value. */
public class MagicValuePredicate extends ApprovalPredicate {
@@ -47,19 +48,23 @@
@Override
public boolean match(ApprovalContext ctx) {
+ Optional<LabelType> lt =
+ getLabelType(ctx.changeNotes().getProjectName(), ctx.patchSetApproval().labelId());
short pValue;
switch (value) {
case ANY:
return true;
case MIN:
- pValue =
- getLabelType(ctx.changeNotes().getProjectName(), ctx.patchSetApproval().labelId())
- .getMaxNegative();
+ if (!lt.isPresent()) {
+ return false;
+ }
+ pValue = lt.get().getMaxNegative();
break;
case MAX:
- pValue =
- getLabelType(ctx.changeNotes().getProjectName(), ctx.patchSetApproval().labelId())
- .getMaxPositive();
+ if (!lt.isPresent()) {
+ return false;
+ }
+ pValue = lt.get().getMaxPositive();
break;
default:
throw new IllegalArgumentException("unrecognized label value: " + value);
@@ -67,7 +72,7 @@
return pValue == ctx.patchSetApproval().value();
}
- private LabelType getLabelType(Project.NameKey project, LabelId labelId) {
+ private Optional<LabelType> getLabelType(Project.NameKey project, LabelId labelId) {
return projectCache
.get(project)
.orElseThrow(() -> new IllegalStateException(project + " absent"))
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
index 30d5e2f..ade615c 100644
--- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -91,8 +91,8 @@
}
protected static LabelType type(LabelTypes types, String toFind) {
- if (types.byLabel(toFind) != null) {
- return types.byLabel(toFind);
+ if (types.byLabel(toFind).isPresent()) {
+ return types.byLabel(toFind).get();
}
for (LabelType lt : types.getLabelTypes()) {
diff --git a/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java b/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
index e3c58e47..2c56322 100644
--- a/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/MagicLabelPredicate.java
@@ -87,8 +87,8 @@
}
protected static LabelType type(LabelTypes types, String toFind) {
- if (types.byLabel(toFind) != null) {
- return types.byLabel(toFind);
+ if (types.byLabel(toFind).isPresent()) {
+ return types.byLabel(toFind).get();
}
for (LabelType lt : types.getLabelTypes()) {
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVote.java b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
index 84424a8..2c358d0 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVote.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
@@ -194,7 +194,7 @@
for (PatchSetApproval a :
approvalsUtil.byPatchSetUser(
ctx.getNotes(), psId, accountId, ctx.getRevWalk(), ctx.getRepoView().getConfig())) {
- if (labelTypes.byLabel(a.labelId()) == null) {
+ if (!labelTypes.byLabel(a.labelId()).isPresent()) {
continue; // Ignore undefined labels.
} else if (!a.label().equals(label)) {
// Populate map for non-matching labels, needed by VoteDeleted.
diff --git a/java/com/google/gerrit/server/restapi/change/Move.java b/java/com/google/gerrit/server/restapi/change/Move.java
index 9263971..8c21841 100644
--- a/java/com/google/gerrit/server/restapi/change/Move.java
+++ b/java/com/google/gerrit/server/restapi/change/Move.java
@@ -64,6 +64,7 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.Optional;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -265,11 +266,13 @@
approvalsUtil.byPatchSet(
ctx.getNotes(), psId, ctx.getRevWalk(), ctx.getRepoView().getConfig())) {
ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
- LabelType type = projectState.getLabelTypes(ctx.getNotes()).byLabel(psa.labelId());
+ Optional<LabelType> type =
+ projectState.getLabelTypes(ctx.getNotes()).byLabel(psa.labelId());
// Only keep veto votes, defined as votes where:
// 1- the label function allows minimum values to block submission.
// 2- the vote holds the minimum value.
- if (type == null || (type.isMaxNegative(psa) && type.getFunction().isBlock())) {
+ if (!type.isPresent()
+ || (type.get().isMaxNegative(psa) && type.get().getFunction().isBlock())) {
continue;
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 6816361..4dbb6ee 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -22,7 +22,6 @@
import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.nio.charset.StandardCharsets.UTF_8;
-import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
@@ -502,8 +501,8 @@
Iterator<Map.Entry<String, Short>> itr = in.labels.entrySet().iterator();
while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next();
- LabelType type = labelTypes.byLabel(ent.getKey());
- if (type == null) {
+ Optional<LabelType> type = labelTypes.byLabel(ent.getKey());
+ if (!type.isPresent()) {
logger.atFine().log("label %s not found", ent.getKey());
if (strictLabels) {
throw new BadRequestException(
@@ -518,15 +517,15 @@
logger.atFine().log(
"skipping on behalf of permission check for label %s"
+ " because caller is an internal user",
- type.getName());
+ type.get().getName());
} else {
try {
- perm.check(new LabelPermission.WithValue(ON_BEHALF_OF, type, ent.getValue()));
+ perm.check(new LabelPermission.WithValue(ON_BEHALF_OF, type.get(), ent.getValue()));
} catch (AuthException e) {
throw new AuthException(
String.format(
"not permitted to modify label \"%s\" on behalf of \"%s\"",
- type.getName(), in.onBehalfOf),
+ type.get().getName(), in.onBehalfOf),
e);
}
}
@@ -558,8 +557,8 @@
Iterator<Map.Entry<String, Short>> itr = labels.entrySet().iterator();
while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next();
- LabelType lt = labelTypes.byLabel(ent.getKey());
- if (lt == null) {
+ Optional<LabelType> lt = labelTypes.byLabel(ent.getKey());
+ if (!lt.isPresent()) {
logger.atFine().log("label %s not found", ent.getKey());
if (strictLabels) {
throw new BadRequestException(
@@ -576,7 +575,7 @@
continue;
}
- if (lt.getValue(ent.getValue()) == null) {
+ if (lt.get().getValue(ent.getValue()) == null) {
logger.atFine().log("label value %s not found", ent.getValue());
if (strictLabels) {
throw new BadRequestException(
@@ -590,10 +589,10 @@
short val = ent.getValue();
try {
- perm.check(new LabelPermission.WithValue(lt, val));
+ perm.check(new LabelPermission.WithValue(lt.get(), val));
} catch (AuthException e) {
throw new AuthException(
- String.format("Applying label \"%s\": %d is restricted", lt.getName(), val), e);
+ String.format("Applying label \"%s\": %d is restricted", lt.get().getName(), val), e);
}
}
}
@@ -1356,7 +1355,10 @@
ChangeUpdate update = ctx.getUpdate(psId);
for (Map.Entry<String, Short> ent : allApprovals.entrySet()) {
String name = ent.getKey();
- LabelType lt = requireNonNull(labelTypes.byLabel(name), name);
+ LabelType lt =
+ labelTypes
+ .byLabel(name)
+ .orElseThrow(() -> new IllegalStateException("no label config for " + name));
PatchSetApproval c = current.remove(lt.getName());
String normName = lt.getName();
@@ -1448,7 +1450,10 @@
List<String> disallowed = new ArrayList<>(labelTypes.getLabelTypes().size());
for (PatchSetApproval psa : del) {
- LabelType lt = requireNonNull(labelTypes.byLabel(psa.label()));
+ LabelType lt =
+ labelTypes
+ .byLabel(psa.label())
+ .orElseThrow(() -> new IllegalStateException("no label config for " + psa.label()));
String normName = lt.getName();
if (!lt.isAllowPostSubmit()) {
disallowed.add(normName);
@@ -1460,7 +1465,10 @@
}
for (PatchSetApproval psa : ups) {
- LabelType lt = requireNonNull(labelTypes.byLabel(psa.label()));
+ LabelType lt =
+ labelTypes
+ .byLabel(psa.label())
+ .orElseThrow(() -> new IllegalStateException("no label config for " + psa.label()));
String normName = lt.getName();
if (!lt.isAllowPostSubmit()) {
disallowed.add(normName);
@@ -1508,9 +1516,9 @@
continue;
}
- LabelType lt = labelTypes.byLabel(a.labelId());
- if (lt != null) {
- current.put(lt.getName(), a);
+ Optional<LabelType> lt = labelTypes.byLabel(a.labelId());
+ if (lt.isPresent()) {
+ current.put(lt.get().getName(), a);
} else {
del.add(a);
}
diff --git a/java/gerrit/PRED__load_commit_labels_1.java b/java/gerrit/PRED__load_commit_labels_1.java
index 5ee292ff..9a656b8 100644
--- a/java/gerrit/PRED__load_commit_labels_1.java
+++ b/java/gerrit/PRED__load_commit_labels_1.java
@@ -16,6 +16,7 @@
import com.googlecode.prolog_cafe.lang.StructureTerm;
import com.googlecode.prolog_cafe.lang.SymbolTerm;
import com.googlecode.prolog_cafe.lang.Term;
+import java.util.Optional;
/** Exports list of {@code commit_label( label('Code-Review', 2), user(12345789) )}. */
class PRED__load_commit_labels_1 extends Predicate.P1 {
@@ -38,13 +39,14 @@
LabelTypes types = cd.getLabelTypes();
for (PatchSetApproval a : cd.currentApprovals()) {
- LabelType t = types.byLabel(a.labelId());
- if (t == null) {
+ Optional<LabelType> t = types.byLabel(a.labelId());
+ if (!t.isPresent()) {
continue;
}
StructureTerm labelTerm =
- new StructureTerm(sym_label, SymbolTerm.intern(t.getName()), new IntegerTerm(a.value()));
+ new StructureTerm(
+ sym_label, SymbolTerm.intern(t.get().getName()), new IntegerTerm(a.value()));
StructureTerm userTerm = new StructureTerm(sym_user, new IntegerTerm(a.accountId().get()));
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index a01b340..5f3b702 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -50,6 +50,7 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.webui.EditWebLink;
+import com.google.gerrit.server.change.FileInfoJsonExperimentImpl;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.inject.Inject;
@@ -109,7 +110,8 @@
intraline = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "intraline", false);
useNewDiffCacheListFiles =
- baseConfig.getBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", false);
+ Arrays.asList(baseConfig.getStringList("experiments", null, "enabled"))
+ .contains(FileInfoJsonExperimentImpl.NEW_DIFF_CACHE_FEATURE);
useNewDiffCacheGetDiff =
baseConfig.getBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", false);
@@ -2836,11 +2838,7 @@
}
@Test
- public void symlinkConvertedToRegularFileIsIdentifiedAsAdded() throws Exception {
- // TODO(ghareeb): fix this test for the new diff cache implementation
- assume().that(useNewDiffCacheListFiles).isFalse();
- assume().that(useNewDiffCacheGetDiff).isFalse();
-
+ public void symlinkConvertedToRegularFileIsIdentifiedAsRewritten() throws Exception {
String target = "file.txt";
String symlink = "link.lnk";
@@ -2868,23 +2866,39 @@
gApi.changes().id(result.getChangeId()).current().files(initialRev);
assertThat(changedFiles.keySet()).containsExactly("/COMMIT_MSG", symlink);
- assertThat(changedFiles.get(symlink).status).isEqualTo('W'); // Rewrite
+
+ // Both old and new diff caches agree that the state is rewritten
+ assertThat(changedFiles.get(symlink).status).isEqualTo('W'); // Rewritten
DiffInfo diffInfo =
gApi.changes().id(result.getChangeId()).current().file(symlink).diff(initialRev);
- // The diff logic identifies two entries for the file:
- // 1. One entry as 'DELETED' for the symlink.
- // 2. Another entry as 'ADDED' for the new regular file.
- // Since the diff logic returns a single entry, we prioritize returning the 'ADDED' entry in
- // this case so that the client is able to see the new content that was added to the file.
- assertThat(diffInfo.changeType).isEqualTo(ChangeType.ADDED);
- assertThat(diffInfo.content).hasSize(1);
- assertThat(diffInfo)
- .content()
- .element(0)
- .linesOfB()
- .containsExactly("Content of the new file named 'symlink'");
+ // TODO(ghareeb): Remove the else branch when the new diff cache is rolled out as default.
+ if (useNewDiffCacheGetDiff) {
+ // File diff in New diff cache: change type is correctly identified as REWRITTEN
+ assertThat(diffInfo.changeType).isEqualTo(ChangeType.REWRITE);
+ assertThat(diffInfo.content).hasSize(2);
+ assertThat(diffInfo)
+ .content()
+ .element(0)
+ .linesOfB()
+ .containsExactly("Content of the new file named 'symlink'");
+ assertThat(diffInfo).content().element(1).linesOfA().containsExactly("file.txt");
+ } else {
+ // File diff in old diff cache: The diff logic identifies two entries for the file:
+ // 1. One entry as 'DELETED' for the symlink.
+ // 2. Another entry as 'ADDED' for the new regular file.
+ // Since the diff logic returns a single entry, the implementation prioritizes the 'ADDED'
+ // entry in this case so that the user is able to see the new content that was added to the
+ // file.
+ assertThat(diffInfo.changeType).isEqualTo(ChangeType.ADDED);
+ assertThat(diffInfo.content).hasSize(1);
+ assertThat(diffInfo)
+ .content()
+ .element(0)
+ .linesOfB()
+ .containsExactly("Content of the new file named 'symlink'");
+ }
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java
index ec0bcc6..714bd78 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java
@@ -14,6 +14,7 @@
package com.google.gerrit.acceptance.api.revision;
+import com.google.gerrit.server.change.FileInfoJsonExperimentImpl;
import com.google.gerrit.testing.ConfigSuite;
import org.eclipse.jgit.lib.Config;
@@ -26,7 +27,8 @@
@ConfigSuite.Default
public static Config newDiffCacheConfig() {
Config config = new Config();
- config.setBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", true);
+ config.setString(
+ "experiments", null, "enabled", FileInfoJsonExperimentImpl.NEW_DIFF_CACHE_FEATURE);
return config;
}
}
diff --git a/javatests/com/google/gerrit/server/schema/SchemaCreatorImplTest.java b/javatests/com/google/gerrit/server/schema/SchemaCreatorImplTest.java
index fc6b412..9cba362 100644
--- a/javatests/com/google/gerrit/server/schema/SchemaCreatorImplTest.java
+++ b/javatests/com/google/gerrit/server/schema/SchemaCreatorImplTest.java
@@ -72,7 +72,7 @@
@Test
public void createSchema_Label_CodeReview() throws Exception {
- LabelType codeReview = getLabelTypes().byLabel("Code-Review");
+ LabelType codeReview = getLabelTypes().byLabel("Code-Review").get();
assertThat(codeReview).isNotNull();
assertThat(codeReview.getName()).isEqualTo("Code-Review");
assertThat(codeReview.getDefaultValue()).isEqualTo(0);
diff --git a/plugins/reviewnotes b/plugins/reviewnotes
index 35e6449..a28ae59 160000
--- a/plugins/reviewnotes
+++ b/plugins/reviewnotes
@@ -1 +1 @@
-Subproject commit 35e6449a517691a880c94e7467bc07360f8e6666
+Subproject commit a28ae590486934690e4e0a95d7eb75f8b60644a6
diff --git a/plugins/webhooks b/plugins/webhooks
index 9fc9c2d..73f9dc7 160000
--- a/plugins/webhooks
+++ b/plugins/webhooks
@@ -1 +1 @@
-Subproject commit 9fc9c2d4e69f7e2701cbcd873977d3312a231a81
+Subproject commit 73f9dc72bd52f5d64853db31e711717a995f0a46
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
index b576896..0f8752d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
@@ -43,6 +43,9 @@
@property({type: Boolean})
saveOnChange = false;
+ @property({type: Boolean})
+ showTooltipBelow = false;
+
private readonly restApiService = appContext.restApiService;
/** @override */
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_html.ts
index 9943b58..3ebb58f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_html.ts
@@ -34,6 +34,7 @@
id="sideBySideBtn"
link=""
has-tooltip=""
+ position-below="[[showTooltipBelow]]"
class$="[[_computeSideBySideSelected(mode)]]"
title="Side-by-side diff"
aria-pressed="[[isSideBySideSelected(mode)]]"
@@ -45,6 +46,7 @@
id="unifiedBtn"
link=""
has-tooltip=""
+ position-below="[[showTooltipBelow]]"
title="Unified diff"
class$="[[_computeUnifiedSelected(mode)]]"
aria-pressed="[[isUnifiedSelected(mode)]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
index 8d69007d..743f905 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
@@ -343,6 +343,7 @@
id="modeSelect"
save-on-change="[[!_diffPrefsDisabled]]"
mode="{{changeViewState.diffMode}}"
+ show-tooltip-below=""
></gr-diff-mode-selector>
</div>
<span
@@ -355,6 +356,7 @@
link=""
class="prefsButton"
has-tooltip=""
+ position-below=""
title="Diff preferences"
on-click="_handlePrefsTap"
><iron-icon icon="gr-icons:settings"></iron-icon
diff --git a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
index 66214b4..1a340b9 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
@@ -198,7 +198,7 @@
.account="${account}"
.change="${change}"
?highlight-attention=${highlightAttention}
- .voteable-text=${this.voteableText}
+ .voteableText=${this.voteableText}
></gr-hovercard-account>`
: ''}
${hasAttention
diff --git a/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.js b/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.ts
similarity index 60%
rename from polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.js
rename to polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.ts
index df8632f..bb70855 100644
--- a/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.ts
@@ -15,19 +15,26 @@
* limitations under the License.
*/
-import '../../../test/common-test-setup-karma.js';
-import './gr-avatar.js';
-import {getPluginLoader} from '../gr-js-api-interface/gr-plugin-loader.js';
-import {appContext} from '../../../services/app-context.js';
+import '../../../test/common-test-setup-karma';
+import './gr-avatar';
+import {GrAvatar} from './gr-avatar';
+import {getPluginLoader} from '../gr-js-api-interface/gr-plugin-loader';
+import {appContext} from '../../../services/app-context';
+import {AvatarInfo} from '../../../types/common';
+import {
+ createAccountWithEmail,
+ createAccountWithId,
+} from '../../../test/test-data-generators';
const basicFixture = fixtureFromElement('gr-avatar');
suite('gr-avatar tests', () => {
- let element;
- const defaultAvatars = [
+ let element: GrAvatar;
+ const defaultAvatars: AvatarInfo[] = [
{
url: 'https://cdn.example.com/s12-p/photo.jpg',
height: 12,
+ width: 0,
},
];
@@ -36,68 +43,74 @@
});
test('account without avatar', () => {
- assert.equal(
- element._buildAvatarURL({
- _account_id: 123,
- }),
- '');
+ assert.equal(element._buildAvatarURL(createAccountWithId(123)), '');
});
test('methods', () => {
assert.equal(
- element._buildAvatarURL({
- _account_id: 123,
- avatars: defaultAvatars,
- }),
- '/accounts/123/avatar?s=16');
+ element._buildAvatarURL({
+ ...createAccountWithId(123),
+ avatars: defaultAvatars,
+ }),
+ '/accounts/123/avatar?s=16'
+ );
assert.equal(
- element._buildAvatarURL({
- email: 'test@example.com',
- avatars: defaultAvatars,
- }),
- '/accounts/test%40example.com/avatar?s=16');
+ element._buildAvatarURL({
+ ...createAccountWithEmail('test@example.com'),
+ avatars: defaultAvatars,
+ }),
+ '/accounts/test%40example.com/avatar?s=16'
+ );
assert.equal(
- element._buildAvatarURL({
- name: 'John Doe',
- avatars: defaultAvatars,
- }),
- '/accounts/John%20Doe/avatar?s=16');
+ element._buildAvatarURL({
+ name: 'John Doe',
+ avatars: defaultAvatars,
+ }),
+ '/accounts/John%20Doe/avatar?s=16'
+ );
assert.equal(
- element._buildAvatarURL({
- username: 'John_Doe',
- avatars: defaultAvatars,
- }),
- '/accounts/John_Doe/avatar?s=16');
+ element._buildAvatarURL({
+ username: 'John_Doe',
+ avatars: defaultAvatars,
+ }),
+ '/accounts/John_Doe/avatar?s=16'
+ );
assert.equal(
- element._buildAvatarURL({
- _account_id: 123,
- avatars: [
- {
- url: 'https://cdn.example.com/s12-p/photo.jpg',
- height: 12,
- },
- {
- url: 'https://cdn.example.com/s16-p/photo.jpg',
- height: 16,
- },
- {
- url: 'https://cdn.example.com/s100-p/photo.jpg',
- height: 100,
- },
- ],
- }),
- 'https://cdn.example.com/s16-p/photo.jpg');
+ element._buildAvatarURL({
+ ...createAccountWithId(123),
+ avatars: [
+ {
+ url: 'https://cdn.example.com/s12-p/photo.jpg',
+ height: 12,
+ width: 0,
+ },
+ {
+ url: 'https://cdn.example.com/s16-p/photo.jpg',
+ height: 16,
+ width: 0,
+ },
+ {
+ url: 'https://cdn.example.com/s100-p/photo.jpg',
+ height: 100,
+ width: 0,
+ },
+ ] as AvatarInfo[],
+ }),
+ 'https://cdn.example.com/s16-p/photo.jpg'
+ );
assert.equal(
- element._buildAvatarURL({
- _account_id: 123,
- avatars: [
- {
- url: 'https://cdn.example.com/s95-p/photo.jpg',
- height: 95,
- },
- ],
- }),
- '/accounts/123/avatar?s=16');
+ element._buildAvatarURL({
+ ...createAccountWithId(123),
+ avatars: [
+ {
+ url: 'https://cdn.example.com/s95-p/photo.jpg',
+ height: 95,
+ width: 0,
+ },
+ ] as AvatarInfo[],
+ }),
+ '/accounts/123/avatar?s=16'
+ );
assert.equal(element._buildAvatarURL(undefined), '');
});
@@ -114,7 +127,7 @@
element.imageSize = 64;
element.account = {
- _account_id: 123,
+ ...createAccountWithId(123),
avatars: defaultAvatars,
};
flush();
@@ -131,14 +144,14 @@
assert.isFalse(element.hasAttribute('hidden'));
assert.isTrue(
- element.style.backgroundImage.includes(
- '/accounts/123/avatar?s=64'));
+ element.style.backgroundImage.includes('/accounts/123/avatar?s=64')
+ );
});
});
});
suite('plugin has avatars', () => {
- let element;
+ let element: GrAvatar;
setup(() => {
stub('gr-avatar', '_getConfig').callsFake(() =>
@@ -166,7 +179,7 @@
});
suite('config not set', () => {
- let element;
+ let element: GrAvatar;
setup(() => {
stub('gr-avatar', '_getConfig').callsFake(() => Promise.resolve({}));
@@ -180,7 +193,7 @@
element.imageSize = 64;
element.account = {
- _account_id: 123,
+ ...createAccountWithId(123),
avatars: defaultAvatars,
};
// Emulate plugins loaded.
@@ -195,4 +208,3 @@
});
});
});
-
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
index 0193197..ddca5c5 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
@@ -183,7 +183,9 @@
// include pre or all regular lines but stop at next new line
while (
this._isPreFormat(lines[nextI]) ||
- (this._isRegularLine(lines[nextI]) && lines[nextI].length)
+ (this._isRegularLine(lines[nextI]) &&
+ !this._isWhitespaceLine(lines[nextI]) &&
+ lines[nextI].length)
) {
nextI++;
}
@@ -255,13 +257,17 @@
}
_isPreFormat(line: string) {
- return line && /^[ \t]/.test(line);
+ return line && /^[ \t]/.test(line) && !this._isWhitespaceLine(line);
}
_isList(line: string) {
return line && /^[-*] /.test(line);
}
+ _isWhitespaceLine(line: string) {
+ return line && /^\s+$/.test(line);
+ }
+
_makeLinkedText(content = '', isPre?: boolean) {
const text = document.createElement('gr-linked-text');
text.config = this.config;
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.js b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.js
index 3e05f11..8464af7 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.js
@@ -297,6 +297,14 @@
assertBlock(result, 1, 'paragraph', 'B');
});
+ test('pre format 5', () => {
+ const comment = ' Q\n <R>\n S\n \nB';
+ const result = element._computeBlocks(comment);
+ assert.lengthOf(result, 2);
+ assertBlock(result, 0, 'pre', ' Q\n <R>\n S');
+ assertBlock(result, 1, 'paragraph', ' \nB');
+ });
+
test('quote 1', () => {
const comment = '> I\'m happy\n > with quotes!\n\nSee above.';
const result = element._computeBlocks(comment);
diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.ts b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.ts
index 2812b47..287ed1b 100644
--- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.ts
+++ b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.ts
@@ -14,12 +14,10 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import '../../../styles/shared-styles';
-import {PolymerElement} from '@polymer/polymer/polymer-element';
-import {htmlTemplate} from './gr-linked-text_html';
import {GrLinkTextParser, LinkTextParserConfig} from './link-text-parser';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
-import {customElement, property, observe} from '@polymer/decorators';
+import {GrLitElement} from '../../lit/gr-lit-element';
+import {css, customElement, html, property, query} from 'lit-element';
declare global {
interface HTMLElementTagNameMap {
@@ -27,17 +25,10 @@
}
}
-export interface GrLinkedText {
- $: {
- output: HTMLSpanElement;
- };
-}
-
@customElement('gr-linked-text')
-export class GrLinkedText extends PolymerElement {
- static get template() {
- return htmlTemplate;
- }
+export class GrLinkedText extends GrLitElement {
+ @query('#output')
+ outputElement?: HTMLSpanElement;
@property({type: Boolean})
removeZeroWidthSpace?: boolean;
@@ -46,61 +37,63 @@
@property({type: String})
content: string | null = null;
- @property({type: Boolean, reflectToAttribute: true})
+ @property({type: Boolean, reflect: true})
pre = false;
- @property({type: Boolean, reflectToAttribute: true})
+ @property({type: Boolean, reflect: true})
disabled = false;
@property({type: Object})
config?: LinkTextParserConfig;
- @observe('content')
- _contentChanged(content: string | null) {
- // In the case where the config may not be set (perhaps due to the
- // request for it still being in flight), set the content anyway to
- // prevent waiting on the config to display the text.
+ static get styles() {
+ return [
+ css`
+ :host {
+ display: block;
+ }
+ :host([pre]) span {
+ white-space: var(--linked-text-white-space, pre-wrap);
+ word-wrap: var(--linked-text-word-wrap, break-word);
+ }
+ :host([disabled]) a {
+ color: inherit;
+ text-decoration: none;
+ pointer-events: none;
+ }
+ a {
+ color: var(--link-color);
+ }
+ `,
+ ];
+ }
+
+ render() {
if (!this.config) {
return;
}
- this.$.output.textContent = content;
+ return html`<span id="output">${this.content}</span>`;
}
- /**
- * Because either the source text or the linkification config has changed,
- * the content should be re-parsed.
- *
- * @param content The raw, un-linkified source string to parse.
- * @param config The server config specifying commentLink patterns
- */
- @observe('content', 'config')
- _contentOrConfigChanged(
- content: string | null,
- config?: LinkTextParserConfig
- ) {
- if (!config) {
- return;
- }
-
+ updated() {
+ if (!this.outputElement || !this.config) return;
+ this.outputElement.textContent = '';
// TODO(TS): mapCommentlinks always has value, remove
if (!GerritNav.mapCommentlinks) return;
- config = GerritNav.mapCommentlinks(config);
- const output = this.$.output;
- output.textContent = '';
+ const config = GerritNav.mapCommentlinks(this.config);
const parser = new GrLinkTextParser(
config,
(text: string | null, href: string | null, fragment?: DocumentFragment) =>
this._handleParseResult(text, href, fragment),
this.removeZeroWidthSpace
);
- parser.parse(content);
-
+ parser.parse(this.content);
// Ensure that external links originating from HTML commentlink configs
// open in a new tab. @see Issue 5567
// Ensure links to the same host originating from commentlink configs
// open in the same tab. When target is not set - default is _self
// @see Issue 4616
- output.querySelectorAll('a').forEach(anchor => {
+ this.outputElement.querySelectorAll('a').forEach(anchor => {
if (anchor.hostname === window.location.hostname) {
anchor.removeAttribute('target');
} else {
@@ -124,7 +117,8 @@
href: string | null,
fragment?: DocumentFragment
) {
- const output = this.$.output;
+ const output = this.outputElement;
+ if (!output) return;
if (href) {
const a = document.createElement('a');
a.setAttribute('href', href);
diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_html.ts b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_html.ts
deleted file mode 100644
index 0d44bc8..0000000
--- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_html.ts
+++ /dev/null
@@ -1,38 +0,0 @@
-/**
- * @license
- * Copyright (C) 2020 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-import {html} from '@polymer/polymer/lib/utils/html-tag';
-
-export const htmlTemplate = html`
- <style>
- :host {
- display: block;
- }
- :host([pre]) span {
- white-space: var(--linked-text-white-space, pre-wrap);
- word-wrap: var(--linked-text-word-wrap, break-word);
- }
- :host([disabled]) a {
- color: inherit;
- text-decoration: none;
- pointer-events: none;
- }
- a {
- color: var(--link-color);
- }
- </style>
- <span id="output"></span>
-`;
diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.ts b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.ts
index b2cdba1..c97c168 100644
--- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.ts
@@ -85,10 +85,11 @@
window.CANONICAL_PATH = originalCanonicalPath;
});
- test('URL pattern was parsed and linked.', () => {
+ test('URL pattern was parsed and linked.', async () => {
// Regular inline link.
const url = 'https://bugs.chromium.org/p/gerrit/issues/detail?id=3650';
element.content = url;
+ await flush();
const linkEl = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
assert.equal(linkEl.target, '_blank');
@@ -97,9 +98,10 @@
assert.equal(linkEl.textContent, url);
});
- test('Bug pattern was parsed and linked', () => {
+ test('Bug pattern was parsed and linked', async () => {
// "Issue/Bug" pattern.
element.content = 'Issue 3650';
+ await flush();
let linkEl = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
@@ -109,6 +111,7 @@
assert.equal(linkEl.textContent, 'Issue 3650');
element.content = 'Bug 3650';
+ await flush();
linkEl = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
assert.equal(linkEl.target, '_blank');
@@ -117,10 +120,10 @@
assert.equal(linkEl.textContent, 'Bug 3650');
});
- test('Pattern with same prefix as link was correctly parsed', () => {
+ test('Pattern with same prefix as link was correctly parsed', async () => {
// Pattern starts with the same prefix (`http`) as the url.
element.content = 'httpexample 3650';
-
+ await flush();
assert.equal(queryAndAssert(element, '#output').childNodes.length, 1);
const linkEl = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
@@ -130,12 +133,12 @@
assert.equal(linkEl.textContent, 'httpexample 3650');
});
- test('Change-Id pattern was parsed and linked', () => {
+ test('Change-Id pattern was parsed and linked', async () => {
// "Change-Id:" pattern.
const changeID = 'I11d6a37f5e9b5df0486f6c922d8836dfa780e03e';
const prefix = 'Change-Id: ';
element.content = prefix + changeID;
-
+ await flush();
const textNode = queryAndAssert(element, '#output').childNodes[0];
const linkEl = queryAndAssert(element, '#output')
.childNodes[1] as HTMLAnchorElement;
@@ -147,14 +150,14 @@
assert.equal(linkEl.textContent, changeID);
});
- test('Change-Id pattern was parsed and linked with base url', () => {
+ test('Change-Id pattern was parsed and linked with base url', async () => {
window.CANONICAL_PATH = '/r';
// "Change-Id:" pattern.
const changeID = 'I11d6a37f5e9b5df0486f6c922d8836dfa780e03e';
const prefix = 'Change-Id: ';
element.content = prefix + changeID;
-
+ await flush();
const textNode = queryAndAssert(element, '#output').childNodes[0];
const linkEl = queryAndAssert(element, '#output')
.childNodes[1] as HTMLAnchorElement;
@@ -166,8 +169,9 @@
assert.equal(linkEl.textContent, changeID);
});
- test('Multiple matches', () => {
+ test('Multiple matches', async () => {
element.content = 'Issue 3650\nIssue 3450';
+ await flush();
const linkEl1 = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
const linkEl2 = queryAndAssert(element, '#output')
@@ -188,7 +192,7 @@
assert.equal(linkEl2.textContent, 'Issue 3450');
});
- test('Change-Id pattern parsed before bug pattern', () => {
+ test('Change-Id pattern parsed before bug pattern', async () => {
// "Change-Id:" pattern.
const changeID = 'I11d6a37f5e9b5df0486f6c922d8836dfa780e03e';
const prefix = 'Change-Id: ';
@@ -200,7 +204,7 @@
const bugUrl = 'https://bugs.chromium.org/p/gerrit/issues/detail?id=3650';
element.content = prefix + changeID + bug;
-
+ await flush();
const textNode = queryAndAssert(element, '#output').childNodes[0];
const changeLinkEl = queryAndAssert(element, '#output')
.childNodes[1] as HTMLAnchorElement;
@@ -218,8 +222,9 @@
assert.equal(bugLinkEl.textContent, 'Issue 3650');
});
- test('html field in link config', () => {
+ test('html field in link config', async () => {
element.content = 'google:do a barrel roll';
+ await flush();
const linkEl = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
assert.equal(
@@ -229,52 +234,58 @@
assert.equal(linkEl.textContent, 'do a barrel roll');
});
- test('removing hash from links', () => {
+ test('removing hash from links', async () => {
element.content = 'hash:foo';
+ await flush();
const linkEl = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
assert.isTrue(linkEl.href.endsWith('/awesomesauce'));
assert.equal(linkEl.textContent, 'foo');
});
- test('html with base url', () => {
+ test('html with base url', async () => {
window.CANONICAL_PATH = '/r';
element.content = 'test foo';
+ await flush();
const linkEl = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
assert.isTrue(linkEl.href.endsWith('/r/awesomesauce'));
assert.equal(linkEl.textContent, 'foo');
});
- test('a is not at start', () => {
+ test('a is not at start', async () => {
window.CANONICAL_PATH = '/r';
element.content = 'a test foo';
+ await flush();
const linkEl = queryAndAssert(element, '#output')
.childNodes[1] as HTMLAnchorElement;
assert.isTrue(linkEl.href.endsWith('/r/awesomesauce'));
assert.equal(linkEl.textContent, 'foo');
});
- test('hash html with base url', () => {
+ test('hash html with base url', async () => {
window.CANONICAL_PATH = '/r';
element.content = 'hash:foo';
+ await flush();
const linkEl = queryAndAssert(element, '#output')
.childNodes[0] as HTMLAnchorElement;
assert.isTrue(linkEl.href.endsWith('/r/awesomesauce'));
assert.equal(linkEl.textContent, 'foo');
});
- test('disabled config', () => {
+ test('disabled config', async () => {
element.content = 'foo:baz';
+ await flush();
assert.equal(queryAndAssert(element, '#output').innerHTML, 'foo:baz');
});
- test('R=email labels link correctly', () => {
+ test('R=email labels link correctly', async () => {
element.removeZeroWidthSpace = true;
element.content = 'R=\u200Btest@google.com';
+ await flush();
assert.equal(
queryAndAssert(element, '#output').textContent,
'R=test@google.com'
@@ -285,9 +296,10 @@
);
});
- test('CC=email labels link correctly', () => {
+ test('CC=email labels link correctly', async () => {
element.removeZeroWidthSpace = true;
element.content = 'CC=\u200Btest@google.com';
+ await flush();
assert.equal(
queryAndAssert(element, '#output').textContent,
'CC=test@google.com'
@@ -298,36 +310,42 @@
);
});
- test('only {http,https,mailto} protocols are linkified', () => {
+ test('only {http,https,mailto} protocols are linkified', async () => {
element.content = 'xx mailto:test@google.com yy';
+ await flush();
let links = queryAndAssert(element, '#output').querySelectorAll('a');
assert.equal(links.length, 1);
assert.equal(links[0].getAttribute('href'), 'mailto:test@google.com');
assert.equal(links[0].innerHTML, 'mailto:test@google.com');
element.content = 'xx http://google.com yy';
+ await flush();
links = queryAndAssert(element, '#output').querySelectorAll('a');
assert.equal(links.length, 1);
assert.equal(links[0].getAttribute('href'), 'http://google.com');
assert.equal(links[0].innerHTML, 'http://google.com');
element.content = 'xx https://google.com yy';
+ await flush();
links = queryAndAssert(element, '#output').querySelectorAll('a');
assert.equal(links.length, 1);
assert.equal(links[0].getAttribute('href'), 'https://google.com');
assert.equal(links[0].innerHTML, 'https://google.com');
element.content = 'xx ssh://google.com yy';
+ await flush();
links = queryAndAssert(element, '#output').querySelectorAll('a');
assert.equal(links.length, 0);
element.content = 'xx ftp://google.com yy';
+ await flush();
links = queryAndAssert(element, '#output').querySelectorAll('a');
assert.equal(links.length, 0);
});
- test('links without leading whitespace are linkified', () => {
+ test('links without leading whitespace are linkified', async () => {
element.content = 'xx abcmailto:test@google.com yy';
+ await flush();
assert.equal(
queryAndAssert(element, '#output').innerHTML.substr(0, 6),
'xx abc'
@@ -338,6 +356,7 @@
assert.equal(links[0].innerHTML, 'mailto:test@google.com');
element.content = 'xx defhttp://google.com yy';
+ await flush();
assert.equal(
queryAndAssert(element, '#output').innerHTML.substr(0, 6),
'xx def'
@@ -348,6 +367,7 @@
assert.equal(links[0].innerHTML, 'http://google.com');
element.content = 'xx qwehttps://google.com yy';
+ await flush();
assert.equal(
queryAndAssert(element, '#output').innerHTML.substr(0, 6),
'xx qwe'
@@ -359,6 +379,7 @@
// Non-latin character
element.content = 'xx абвhttps://google.com yy';
+ await flush();
assert.equal(
queryAndAssert(element, '#output').innerHTML.substr(0, 6),
'xx абв'
@@ -369,15 +390,17 @@
assert.equal(links[0].innerHTML, 'https://google.com');
element.content = 'xx ssh://google.com yy';
+ await flush();
links = queryAndAssert(element, '#output').querySelectorAll('a');
assert.equal(links.length, 0);
element.content = 'xx ftp://google.com yy';
+ await flush();
links = queryAndAssert(element, '#output').querySelectorAll('a');
assert.equal(links.length, 0);
});
- test('overlapping links', () => {
+ test('overlapping links', async () => {
element.config = {
b1: {
match: '(B:\\s*)(\\d+)',
@@ -389,7 +412,8 @@
},
};
element.content = '- B: 123, 45';
- const links = element.root!.querySelectorAll('a');
+ await flush();
+ const links = element.shadowRoot!.querySelectorAll('a');
assert.equal(links.length, 2);
assert.equal(
@@ -403,12 +427,4 @@
assert.equal(links[1].href, 'ftp://foo/45');
assert.equal(links[1].textContent, '45');
});
-
- test('_contentOrConfigChanged called with config', () => {
- const contentStub = sinon.stub(element, '_contentChanged');
- const contentConfigStub = sinon.stub(element, '_contentOrConfigChanged');
- element.content = 'some text';
- assert.isTrue(contentStub.called);
- assert.isTrue(contentConfigStub.called);
- });
});
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 0f2608e..96c05ee 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -43,6 +43,7 @@
ActionNameToActionInfoMap,
ApprovalInfo,
AuthInfo,
+ AvatarInfo,
BasePatchSetNum,
BranchName,
BrandType,
@@ -124,6 +125,7 @@
ActionNameToActionInfoMap,
ApprovalInfo,
AuthInfo,
+ AvatarInfo,
BasePatchSetNum,
BranchName,
BrandType,
