Merge "Fix auto-saving of new comment threads"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 3109ec7..cccbd7b 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -940,12 +940,6 @@
+
If direct updates are made to `All-Users`, this cache should be flushed.
-cache `"approvals"`::
-+
-Cache entries contain approvals for a given patch set. This includes
-approvals granted on this patch set as well as approvals copied from
-earlier patch sets.
-
cache `"adv_bases"`::
+
Used only for push over smart HTTP when branch level access controls
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index 468623b..ce433d7 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -41,7 +41,6 @@
import com.google.gerrit.server.account.Realm;
import com.google.gerrit.server.account.ServiceUserClassifierImpl;
import com.google.gerrit.server.account.externalids.ExternalIdCacheModule;
-import com.google.gerrit.server.approval.ApprovalCacheImpl;
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.cache.h2.H2CacheModule;
import com.google.gerrit.server.cache.mem.DefaultMemoryCacheModule;
@@ -177,7 +176,6 @@
modules.add(new GroupModule());
modules.add(new NoteDbModule());
modules.add(AccountCacheImpl.module());
- modules.add(ApprovalCacheImpl.module());
modules.add(ConflictsCacheImpl.module());
modules.add(DefaultPreferencesCacheImpl.module());
modules.add(GroupCacheImpl.module());
diff --git a/java/com/google/gerrit/server/approval/ApprovalCache.java b/java/com/google/gerrit/server/approval/ApprovalCache.java
deleted file mode 100644
index 5637249..0000000
--- a/java/com/google/gerrit/server/approval/ApprovalCache.java
+++ /dev/null
@@ -1,28 +0,0 @@
-// Copyright (C) 2021 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.approval;
-
-import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.entities.PatchSetApproval;
-import com.google.gerrit.server.notedb.ChangeNotes;
-
-/**
- * Cache that holds approvals per patch set and NoteDb state. This includes approvals copied forward
- * from older patch sets.
- */
-public interface ApprovalCache {
- /** Returns {@link PatchSetApproval}s for the given patch set. */
- Iterable<PatchSetApproval> get(ChangeNotes notes, PatchSet.Id psId);
-}
diff --git a/java/com/google/gerrit/server/approval/ApprovalCacheImpl.java b/java/com/google/gerrit/server/approval/ApprovalCacheImpl.java
deleted file mode 100644
index fd31da9..0000000
--- a/java/com/google/gerrit/server/approval/ApprovalCacheImpl.java
+++ /dev/null
@@ -1,133 +0,0 @@
-// Copyright (C) 2021 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.approval;
-
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
-import com.google.common.collect.ImmutableList;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.entities.PatchSetApproval;
-import com.google.gerrit.entities.Project;
-import com.google.gerrit.entities.converter.PatchSetApprovalProtoConverter;
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.proto.Entities;
-import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.cache.proto.Cache;
-import com.google.gerrit.server.cache.serialize.ObjectIdCacheSerializer;
-import com.google.gerrit.server.cache.serialize.ProtobufSerializer;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.inject.Inject;
-import com.google.inject.Module;
-import com.google.inject.Singleton;
-import com.google.inject.name.Named;
-import com.google.protobuf.ByteString;
-import java.util.concurrent.ExecutionException;
-
-/** Implementation of the {@link ApprovalCache} interface */
-public class ApprovalCacheImpl implements ApprovalCache {
- private static final String CACHE_NAME = "approvals";
-
- public static Module module() {
- return new CacheModule() {
- @Override
- protected void configure() {
- bind(ApprovalCache.class).to(ApprovalCacheImpl.class);
- persist(
- CACHE_NAME,
- Cache.PatchSetApprovalsKeyProto.class,
- Cache.AllPatchSetApprovalsProto.class)
- .version(2)
- .loader(Loader.class)
- .keySerializer(new ProtobufSerializer<>(Cache.PatchSetApprovalsKeyProto.parser()))
- .valueSerializer(new ProtobufSerializer<>(Cache.AllPatchSetApprovalsProto.parser()));
- }
- };
- }
-
- private final LoadingCache<Cache.PatchSetApprovalsKeyProto, Cache.AllPatchSetApprovalsProto>
- cache;
-
- @Inject
- ApprovalCacheImpl(
- @Named(CACHE_NAME)
- LoadingCache<Cache.PatchSetApprovalsKeyProto, Cache.AllPatchSetApprovalsProto> cache) {
- this.cache = cache;
- }
-
- @Override
- public Iterable<PatchSetApproval> get(ChangeNotes notes, PatchSet.Id psId) {
- try {
- return fromProto(
- cache.get(
- Cache.PatchSetApprovalsKeyProto.newBuilder()
- .setChangeId(notes.getChangeId().get())
- .setPatchSetId(psId.get())
- .setProject(notes.getProjectName().get())
- .setId(
- ByteString.copyFrom(
- ObjectIdCacheSerializer.INSTANCE.serialize(notes.getMetaId())))
- .build()));
- } catch (ExecutionException e) {
- throw new StorageException(e);
- }
- }
-
- @Singleton
- static class Loader
- extends CacheLoader<Cache.PatchSetApprovalsKeyProto, Cache.AllPatchSetApprovalsProto> {
- private final ApprovalInference approvalInference;
- private final ChangeNotes.Factory changeNotesFactory;
-
- @Inject
- Loader(ApprovalInference approvalInference, ChangeNotes.Factory changeNotesFactory) {
- this.approvalInference = approvalInference;
- this.changeNotesFactory = changeNotesFactory;
- }
-
- @Override
- public Cache.AllPatchSetApprovalsProto load(Cache.PatchSetApprovalsKeyProto key)
- throws Exception {
- Change.Id changeId = Change.id(key.getChangeId());
- return toProto(
- approvalInference.forPatchSet(
- changeNotesFactory.createChecked(
- Project.nameKey(key.getProject()),
- changeId,
- ObjectIdCacheSerializer.INSTANCE.deserialize(key.getId().toByteArray())),
- PatchSet.id(changeId, key.getPatchSetId()),
- null
- /* revWalk= */ ,
- null
- /* repoConfig= */ ));
- }
- }
-
- private static Iterable<PatchSetApproval> fromProto(Cache.AllPatchSetApprovalsProto proto) {
- ImmutableList.Builder<PatchSetApproval> builder = ImmutableList.builder();
- for (Entities.PatchSetApproval psa : proto.getApprovalList()) {
- builder.add(PatchSetApprovalProtoConverter.INSTANCE.fromProto(psa));
- }
- return builder.build();
- }
-
- private static Cache.AllPatchSetApprovalsProto toProto(Iterable<PatchSetApproval> autoValue) {
- Cache.AllPatchSetApprovalsProto.Builder builder = Cache.AllPatchSetApprovalsProto.newBuilder();
- for (PatchSetApproval psa : autoValue) {
- builder.addApproval(PatchSetApprovalProtoConverter.INSTANCE.toProto(psa));
- }
- return builder.build();
- }
-}
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java
index 5517ab1..fba2fcb 100644
--- a/java/com/google/gerrit/server/approval/ApprovalInference.java
+++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -40,7 +40,7 @@
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.DiffOptions;
-import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.approval.ApprovalContext;
@@ -51,7 +51,6 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.Collection;
-import java.util.Collections;
import java.util.Map;
import java.util.Optional;
import org.eclipse.jgit.lib.Config;
@@ -98,20 +97,11 @@
}
/**
- * Returns all approvals that apply to the given patch set. Honors direct and indirect (approval
- * on parents) approvals.
+ * Returns all approvals that apply to the given patch set. Honors copied approvals from previous
+ * patch-set.
*/
Iterable<PatchSetApproval> forPatchSet(
- ChangeNotes notes, PatchSet.Id psId, @Nullable RevWalk rw, @Nullable Config repoConfig) {
- PatchSet patchset = notes.getPatchSets().get(psId);
- if (patchset == null) {
- return Collections.emptyList();
- }
- return forPatchSet(notes, patchset, rw, repoConfig);
- }
-
- Iterable<PatchSetApproval> forPatchSet(
- ChangeNotes notes, PatchSet ps, @Nullable RevWalk rw, @Nullable Config repoConfig) {
+ ChangeNotes notes, PatchSet ps, RevWalk rw, Config repoConfig) {
ProjectState project;
try (TraceTimer traceTimer =
TraceContext.newTimer(
@@ -136,9 +126,9 @@
PatchSet.Id psId,
ChangeKind kind,
LabelType type,
- @Nullable Map<String, FileDiffOutput> baseVsCurrentDiff,
- @Nullable Map<String, FileDiffOutput> baseVsPriorDiff,
- @Nullable Map<String, FileDiffOutput> priorVsCurrentDiff) {
+ @Nullable Map<String, ModifiedFile> baseVsCurrentDiff,
+ @Nullable Map<String, ModifiedFile> baseVsPriorDiff,
+ @Nullable Map<String, ModifiedFile> priorVsCurrentDiff) {
int n = psa.key().patchSetId().get();
checkArgument(n != psId.get());
@@ -326,11 +316,14 @@
PatchSetApproval psa,
PatchSet patchSet,
LabelType type,
- ChangeKind changeKind) {
+ ChangeKind changeKind,
+ RevWalk revWalk,
+ Config repoConfig) {
if (!type.getCopyCondition().isPresent()) {
return false;
}
- ApprovalContext ctx = ApprovalContext.create(changeNotes, psa, patchSet, changeKind);
+ ApprovalContext ctx =
+ ApprovalContext.create(changeNotes, psa, patchSet, changeKind, revWalk, repoConfig);
try {
// Use a request context to run checks as an internal user with expanded visibility. This is
// so that the output of the copy condition does not depend on who is running the current
@@ -347,11 +340,7 @@
}
private Collection<PatchSetApproval> getForPatchSetWithoutNormalization(
- ChangeNotes notes,
- ProjectState project,
- PatchSet patchSet,
- @Nullable RevWalk rw,
- @Nullable Config repoConfig) {
+ ChangeNotes notes, ProjectState project, PatchSet patchSet, RevWalk rw, Config repoConfig) {
checkState(
project.getNameKey().equals(notes.getProjectName()),
"project must match %s, %s",
@@ -361,34 +350,23 @@
PatchSet.Id psId = patchSet.id();
// Add approvals on the given patch set to the result
Table<String, Account.Id, PatchSetApproval> resultByUser = HashBasedTable.create();
- ImmutableList<PatchSetApproval> approvalsForGivenPatchSet =
+ ImmutableList<PatchSetApproval> nonCopiedApprovalsForGivenPatchSet =
notes.load().getApprovals().get(patchSet.id());
- approvalsForGivenPatchSet.forEach(psa -> resultByUser.put(psa.label(), psa.accountId(), psa));
+ nonCopiedApprovalsForGivenPatchSet.forEach(
+ psa -> resultByUser.put(psa.label(), psa.accountId(), psa));
// Bail out immediately if this is the first patch set. Return only approvals granted on the
// given patch set.
if (psId.get() == 1) {
return resultByUser.values();
}
-
- // Call this algorithm recursively to check if the prior patch set had approvals. This has the
- // advantage that all caches - most importantly ChangeKindCache - have values cached for what we
- // need for this computation.
- // The way this algorithm is written is that any approval will be copied forward by one patch
- // set at a time if configs and change kind allow so. Once an approval is held back - for
- // example because the patch set is a REWORK - it will not be picked up again in a future
- // patch set.
Map.Entry<PatchSet.Id, PatchSet> priorPatchSet = notes.load().getPatchSets().lowerEntry(psId);
if (priorPatchSet == null) {
return resultByUser.values();
}
- Iterable<PatchSetApproval> priorApprovals =
- getForPatchSetWithoutNormalization(
- notes, project, priorPatchSet.getValue(), rw, repoConfig);
- if (!priorApprovals.iterator().hasNext()) {
- return resultByUser.values();
- }
+ ImmutableList<PatchSetApproval> priorApprovalsIncludingCopied =
+ notes.load().getApprovalsWithCopied().get(priorPatchSet.getKey());
// Add labels from the previous patch set to the result in case the label isn't already there
// and settings as well as change kind allow copying.
@@ -406,11 +384,11 @@
priorPatchSet.getValue().id().changeId(),
changeKind);
- Map<String, FileDiffOutput> baseVsCurrent = null;
- Map<String, FileDiffOutput> baseVsPrior = null;
- Map<String, FileDiffOutput> priorVsCurrent = null;
+ Map<String, ModifiedFile> baseVsCurrent = null;
+ Map<String, ModifiedFile> baseVsPrior = null;
+ Map<String, ModifiedFile> priorVsCurrent = null;
LabelTypes labelTypes = project.getLabelTypes();
- for (PatchSetApproval psa : priorApprovals) {
+ for (PatchSetApproval psa : priorApprovalsIncludingCopied) {
if (resultByUser.contains(psa.label(), psa.accountId())) {
continue;
}
@@ -419,10 +397,11 @@
if (baseVsCurrent == null
&& type.isPresent()
&& type.get().isCopyAllScoresIfListOfFilesDidNotChange()) {
- baseVsCurrent = listModifiedFiles(project, patchSet);
- baseVsPrior = listModifiedFiles(project, priorPatchSet.getValue());
+ baseVsCurrent = listModifiedFiles(project, patchSet, rw, repoConfig);
+ baseVsPrior = listModifiedFiles(project, priorPatchSet.getValue(), rw, repoConfig);
priorVsCurrent =
- listModifiedFiles(project, priorPatchSet.getValue().commitId(), patchSet.commitId());
+ listModifiedFiles(
+ project, priorPatchSet.getValue().commitId(), patchSet.commitId(), rw, repoConfig);
}
if (!type.isPresent()) {
logger.atFine().log(
@@ -445,7 +424,8 @@
baseVsCurrent,
baseVsPrior,
priorVsCurrent)
- && !canCopyBasedOnCopyCondition(notes, psa, patchSet, type.get(), changeKind)) {
+ && !canCopyBasedOnCopyCondition(
+ notes, psa, patchSet, type.get(), changeKind, rw, repoConfig)) {
continue;
}
resultByUser.put(psa.label(), psa.accountId(), psa.copyWithPatchSet(patchSet.id()));
@@ -457,14 +437,20 @@
* Gets the modified files between the two latest patch-sets. Can be used to compute difference in
* files between those two patch-sets .
*/
- private Map<String, FileDiffOutput> listModifiedFiles(ProjectState project, PatchSet ps) {
+ private Map<String, ModifiedFile> listModifiedFiles(
+ ProjectState project, PatchSet ps, RevWalk revWalk, Config repoConfig) {
try {
Integer parentNum =
listOfFilesUnchangedPredicate.isInitialCommit(project.getNameKey(), ps.commitId())
? 0
: 1;
- return diffOperations.listModifiedFilesAgainstParent(
- project.getNameKey(), ps.commitId(), parentNum, DiffOptions.DEFAULTS);
+ return diffOperations.loadModifiedFilesAgainstParent(
+ project.getNameKey(),
+ ps.commitId(),
+ parentNum,
+ DiffOptions.DEFAULTS,
+ revWalk,
+ repoConfig);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't copy"
@@ -478,11 +464,20 @@
* Gets the modified files between two commits corresponding to different patchsets of the same
* change.
*/
- private Map<String, FileDiffOutput> listModifiedFiles(
- ProjectState project, ObjectId sourceCommit, ObjectId targetCommit) {
+ private Map<String, ModifiedFile> listModifiedFiles(
+ ProjectState project,
+ ObjectId sourceCommit,
+ ObjectId targetCommit,
+ RevWalk revWalk,
+ Config repoConfig) {
try {
- return diffOperations.listModifiedFiles(
- project.getNameKey(), sourceCommit, targetCommit, DiffOptions.DEFAULTS);
+ return diffOperations.loadModifiedFiles(
+ project.getNameKey(),
+ sourceCommit,
+ targetCommit,
+ DiffOptions.DEFAULTS,
+ revWalk,
+ repoConfig);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't copy"
diff --git a/java/com/google/gerrit/server/approval/ApprovalsUtil.java b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
index c2e35d2..c3921f8 100644
--- a/java/com/google/gerrit/server/approval/ApprovalsUtil.java
+++ b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
@@ -26,7 +26,6 @@
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelId;
@@ -42,6 +41,7 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
+import com.google.gerrit.server.change.LabelNormalizer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
@@ -98,7 +98,7 @@
private final ApprovalInference approvalInference;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
- private final ApprovalCache approvalCache;
+ private final LabelNormalizer labelNormalizer;
@VisibleForTesting
@Inject
@@ -106,11 +106,11 @@
ApprovalInference approvalInference,
PermissionBackend permissionBackend,
ProjectCache projectCache,
- ApprovalCache approvalCache) {
+ LabelNormalizer labelNormalizer) {
this.approvalInference = approvalInference;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
- this.approvalCache = approvalCache;
+ this.labelNormalizer = labelNormalizer;
}
/**
@@ -339,30 +339,42 @@
}
}
- public ListMultimap<PatchSet.Id, PatchSetApproval> byChange(ChangeNotes notes) {
+ public ListMultimap<PatchSet.Id, PatchSetApproval> byChangeExcludingCopiedApprovals(
+ ChangeNotes notes) {
return notes.load().getApprovals();
}
- public Iterable<PatchSetApproval> byPatchSet(
- ChangeNotes notes, PatchSet.Id psId, @Nullable RevWalk rw, @Nullable Config repoConfig) {
- return approvalInference.forPatchSet(notes, psId, rw, repoConfig);
- }
-
- public Iterable<PatchSetApproval> byPatchSet(ChangeNotes notes, PatchSet patchSet) {
- return approvalInference.forPatchSet(notes, patchSet, /* rw= */ null, /* repoConfig= */ null);
- }
-
- public Iterable<PatchSetApproval> byPatchSet(ChangeNotes notes, PatchSet.Id psId) {
- return approvalCache.get(notes, psId);
- }
-
- public Iterable<PatchSetApproval> byPatchSetUser(
+ /**
+ * This method should only be used when we want to dynamically compute the approvals. Generally,
+ * the copied approvals are available in {@link ChangeNotes}. However, if the patch-set is just
+ * being created, we need to dynamically compute the approvals so that we can persist them in
+ * storage. The {@link RevWalk} and {@link Config} objects that are being used to create the new
+ * patch-set are required for this method. Here we also add those votes to the provided {@link
+ * ChangeUpdate} object.
+ */
+ public void persistCopiedApprovals(
ChangeNotes notes,
- PatchSet.Id psId,
- Account.Id accountId,
- @Nullable RevWalk rw,
- @Nullable Config repoConfig) {
- return filterApprovals(byPatchSet(notes, psId, rw, repoConfig), accountId);
+ PatchSet patchSet,
+ RevWalk revWalk,
+ Config repoConfig,
+ ChangeUpdate changeUpdate) {
+ approvalInference
+ .forPatchSet(notes, patchSet, revWalk, repoConfig)
+ .forEach(a -> changeUpdate.putCopiedApproval(a));
+ }
+
+ /**
+ * Gets {@link PatchSetApproval}s for a specified patch-set. The result includes copied votes but
+ * does not include deleted labels.
+ *
+ * @param notes changenotes of the change.
+ * @param psId patch-set id for the change and patch-set we want to get approvals.
+ * @return all approvals for the specified patch-set, including copied votes, not including
+ * deleted labels.
+ */
+ public Iterable<PatchSetApproval> byPatchSet(ChangeNotes notes, PatchSet.Id psId) {
+ List<PatchSetApproval> approvalsNotNormalized = notes.load().getApprovalsWithCopied().get(psId);
+ return labelNormalizer.normalize(notes, approvalsNotNormalized).getNormalized();
}
public Iterable<PatchSetApproval> byPatchSetUser(
@@ -375,8 +387,8 @@
return null;
}
try {
- // Submit approval is never copied, so bypass expensive byPatchSet call.
- return getSubmitter(c, byChange(notes).get(c));
+ // Submit approval is never copied.
+ return getSubmitter(c, byChangeExcludingCopiedApprovals(notes).get(c));
} catch (StorageException e) {
return null;
}
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index 1e40429..415383a 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -223,7 +223,7 @@
private Iterable<PatchSetApproval> approvals(ChangeContext ctx, Account.Id accountId) {
Iterable<PatchSetApproval> approvals;
- approvals = approvalsUtil.byChange(ctx.getNotes()).values();
+ approvals = ctx.getNotes().getApprovalsWithCopied().values();
return Iterables.filter(approvals, psa -> accountId.equals(psa.accountId()));
}
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index 209901d..cfdb576 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -104,7 +104,6 @@
private boolean allowClosed;
private boolean sendEmail = true;
private String topic;
- private boolean storeCopiedVotes = true;
// Fields set during some phase of BatchUpdate.Op.
private Change change;
@@ -204,17 +203,6 @@
return this;
}
- /**
- * We always want to store copied votes except when the change is getting submitted and a new
- * patch-set is created on submit (using submit strategies such as "REBASE_ALWAYS"). In such
- * cases, we already store the votes of the new patch-sets in SubmitStrategyOp#saveApprovals. We
- * should not also store the copied votes.
- */
- public PatchSetInserter setStoreCopiedVotes(boolean storeCopiedVotes) {
- this.storeCopiedVotes = storeCopiedVotes;
- return this;
- }
-
public Change getChange() {
checkState(change != null, "getChange() only valid after executing update");
return change;
@@ -298,11 +286,8 @@
}
}
- // Approvals that are being set in the new patch-set during this operation are not available yet
- // outside of the scope of this method. Only copied approvals are set here.
- if (storeCopiedVotes) {
- approvalsUtil.byPatchSet(ctx.getNotes(), patchSet).forEach(a -> update.putCopiedApproval(a));
- }
+ approvalsUtil.persistCopiedApprovals(
+ ctx.getNotes(), patchSet, ctx.getRevWalk(), ctx.getRepoView().getConfig(), update);
return true;
}
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 3e67cca..4952464 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -92,7 +92,6 @@
private boolean detailedCommitMessage;
private boolean postMessage = true;
private boolean sendEmail = true;
- private boolean storeCopiedVotes = true;
private boolean matchAuthorToCommitterDate = false;
private CodeReviewCommit rebasedCommit;
@@ -170,17 +169,6 @@
return this;
}
- /**
- * We always want to store copied votes except when the change is getting submitted and a new
- * patch-set is created on submit (using submit strategies such as "REBASE_ALWAYS"). In such
- * cases, we already store the votes of the new patch-sets in SubmitStrategyOp#saveApprovals. We
- * should not also store the copied votes.
- */
- public RebaseChangeOp setStoreCopiedVotes(boolean storeCopiedVotes) {
- this.storeCopiedVotes = storeCopiedVotes;
- return this;
- }
-
public RebaseChangeOp setSendEmail(boolean sendEmail) {
this.sendEmail = sendEmail;
return this;
@@ -231,10 +219,7 @@
.setFireRevisionCreated(fireRevisionCreated)
.setCheckAddPatchSetPermission(checkAddPatchSetPermission)
.setValidate(validate)
- .setSendEmail(sendEmail)
- // The votes are automatically copied and they don't count as copied votes. See
- // method's javadoc.
- .setStoreCopiedVotes(storeCopiedVotes);
+ .setSendEmail(sendEmail);
if (!rebasedCommit.getFilesWithGitConflicts().isEmpty()
&& !notes.getChange().isWorkInProgress()) {
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 02435d9..7d3ff12 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -108,7 +108,6 @@
import com.google.gerrit.server.account.externalids.ExternalIdCacheModule;
import com.google.gerrit.server.account.externalids.ExternalIdModule;
import com.google.gerrit.server.account.externalids.ExternalIdUpsertPreprocessor;
-import com.google.gerrit.server.approval.ApprovalCacheImpl;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.auth.AuthBackend;
import com.google.gerrit.server.auth.UniversalAuthBackend;
@@ -250,7 +249,6 @@
bind(RulesCache.class);
bind(BlameCache.class).to(BlameCacheImpl.class);
install(AccountCacheImpl.module());
- install(ApprovalCacheImpl.module());
install(BatchUpdate.module());
install(ChangeKindCacheImpl.module());
install(ChangeFinder.module());
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index a9ef70e..20ce991 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -345,9 +345,8 @@
update.putReviewer(ctx.getAccountId(), REVIEWER);
}
- // Approvals that are being set in the new patch-set during this operation are not available yet
- // outside of the scope of this method. Only copied approvals are set here.
- approvalsUtil.byPatchSet(ctx.getNotes(), newPatchSet).forEach(a -> update.putCopiedApproval(a));
+ approvalsUtil.persistCopiedApprovals(
+ ctx.getNotes(), newPatchSet, ctx.getRevWalk(), ctx.getRepoView().getConfig(), update);
mailMessage = insertChangeMessage(update, ctx, reviewMessage);
if (mergedByPushOp == null) {
@@ -458,12 +457,7 @@
// We optimize here and only retrieve current when approvals provided
if (!approvals.isEmpty()) {
for (PatchSetApproval a :
- approvalsUtil.byPatchSetUser(
- ctx.getNotes(),
- priorPatchSetId,
- ctx.getAccountId(),
- ctx.getRevWalk(),
- ctx.getRepoView().getConfig())) {
+ approvalsUtil.byPatchSetUser(ctx.getNotes(), priorPatchSetId, ctx.getAccountId())) {
if (a.isLegacySubmit()) {
continue;
}
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index 0710784..31e3948 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -378,8 +378,7 @@
// Get previous approvals from this user
Map<String, Short> approvals = new HashMap<>();
approvalsUtil
- .byPatchSetUser(
- notes, psId, ctx.getAccountId(), ctx.getRevWalk(), ctx.getRepoView().getConfig())
+ .byPatchSetUser(notes, psId, ctx.getAccountId())
.forEach(a -> approvals.put(a.label(), a.value()));
// Fire Gerrit event. Note that approvals can't be granted via email, so old and new approvals
// are always the same here.
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 2d9b014..bbfe8dd 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -391,13 +391,7 @@
return approvals;
}
- /**
- * This method is currently used only in tests. TODO(paiking): Use this method to fetch approvals
- * (including copied approvals) instead of computing copied approvals on demand. This will be used
- * by {@code ApprovalCache}.
- *
- * @return all approvals, including copied approvals.
- */
+ /** Gets all approvals, including copied approvals. */
public ImmutableListMultimap<PatchSet.Id, PatchSetApproval> getApprovalsWithCopied() {
if (approvalsWithCopied == null) {
approvalsWithCopied = ImmutableListMultimap.copyOf(state.approvals());
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index c554ca5..b8b8a2c 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -61,7 +61,7 @@
.weigher(Weigher.class)
.maximumWeight(10 << 20)
.diskLimit(-1)
- .version(2)
+ .version(3)
.keySerializer(Key.Serializer.INSTANCE)
.valueSerializer(ChangeNotesState.Serializer.INSTANCE);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 5cf3a64..a3a2b4b 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -40,11 +40,13 @@
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_WORK_IN_PROGRESS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.parseCommitMessageRange;
import static java.util.Comparator.comparing;
+import static java.util.Comparator.comparingInt;
import static java.util.stream.Collectors.joining;
import com.google.common.base.Enums;
import com.google.common.base.Splitter;
import com.google.common.collect.HashBasedTable;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableTable;
import com.google.common.collect.ListMultimap;
@@ -70,6 +72,7 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.entities.SubmitRecord.Label.Status;
import com.google.gerrit.entities.SubmitRequirementResult;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.server.AssigneeStatusUpdate;
@@ -83,6 +86,7 @@
import java.sql.Timestamp;
import java.time.Instant;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
@@ -95,6 +99,7 @@
import java.util.Set;
import java.util.TreeSet;
import java.util.function.Function;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.lib.ObjectId;
@@ -312,10 +317,81 @@
}
result.put(a.key().patchSetId(), a.build());
}
+ if (status != null && status.isClosed() && !isAnyApprovalCopied(result)) {
+ // If the change is closed, check if there are "submit records" with approvals that do not
+ // exist on the latest patch-set and copy them to the latest patch-set.
+ // We do not invoke this logic if any approval is copied. This is because prior to change
+ // https://gerrit-review.googlesource.com/c/gerrit/+/318135 we used to copy approvals
+ // dynamically (e.g. when requesting the change page). After that change, we started
+ // persisting copied votes in NoteDb, so we don't need to do this back-filling.
+ // Prior to that change (318135), we could've had changes with dynamically copied approvals
+ // that were merged in NoteDb but these approvals do not exist on the latest patch-set, so
+ // we need to back-fill these approvals.
+ PatchSet.Id latestPs = buildCurrentPatchSetId();
+ backFillMissingCopiedApprovalsFromSubmitRecords(result, latestPs).stream()
+ .forEach(a -> result.put(latestPs, a));
+ }
result.keySet().forEach(k -> result.get(k).sort(ChangeNotes.PSA_BY_TIME));
return result;
}
+ /**
+ * Returns patch-set approvals that do not exist on the latest patch-set but for which a submit
+ * record exists in NoteDb when the change was merged.
+ */
+ private List<PatchSetApproval> backFillMissingCopiedApprovalsFromSubmitRecords(
+ ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals, @Nullable PatchSet.Id latestPs) {
+ List<PatchSetApproval> copiedApprovals = new ArrayList<>();
+ if (latestPs == null) {
+ return copiedApprovals;
+ }
+ List<PatchSetApproval> approvalsOnLatestPs = allApprovals.get(latestPs);
+ ListMultimap<Account.Id, PatchSetApproval> approvalsByUser = getApprovalsByUser(allApprovals);
+ List<SubmitRecord.Label> submitRecordLabels =
+ submitRecords.stream()
+ .filter(r -> r.labels != null)
+ .flatMap(r -> r.labels.stream())
+ .filter(label -> Status.OK.equals(label.status) || Status.MAY.equals(label.status))
+ .collect(Collectors.toList());
+ for (SubmitRecord.Label recordLabel : submitRecordLabels) {
+ String labelName = recordLabel.label;
+ Account.Id appliedBy = recordLabel.appliedBy;
+ if (appliedBy == null || labelName == null) {
+ continue;
+ }
+ boolean existsAtLatestPs =
+ approvalsOnLatestPs.stream()
+ .anyMatch(a -> a.accountId().equals(appliedBy) && a.label().equals(labelName));
+ if (existsAtLatestPs) {
+ continue;
+ }
+ // Search for an approval for this label on the max previous patch-set and copy the approval.
+ Collection<PatchSetApproval> userApprovals =
+ approvalsByUser.get(appliedBy).stream()
+ .filter(approval -> approval.label().equals(labelName))
+ .collect(Collectors.toList());
+ if (userApprovals.isEmpty()) {
+ continue;
+ }
+ PatchSetApproval lastApproved =
+ Collections.max(userApprovals, comparingInt(a -> a.patchSetId().get()));
+ copiedApprovals.add(lastApproved.copyWithPatchSet(latestPs));
+ }
+ return copiedApprovals;
+ }
+
+ private boolean isAnyApprovalCopied(ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals) {
+ return allApprovals.values().stream().anyMatch(approval -> approval.copied());
+ }
+
+ private ListMultimap<Account.Id, PatchSetApproval> getApprovalsByUser(
+ ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals) {
+ return allApprovals.values().stream()
+ .collect(
+ ImmutableListMultimap.toImmutableListMultimap(
+ PatchSetApproval::accountId, Function.identity()));
+ }
+
private List<ReviewerStatusUpdate> buildReviewerUpdates() {
List<ReviewerStatusUpdate> result = new ArrayList<>();
HashMap<Account.Id, ReviewerStateInternal> lastState = new HashMap<>();
diff --git a/java/com/google/gerrit/server/patch/DiffOperations.java b/java/com/google/gerrit/server/patch/DiffOperations.java
index c84186d..a265337 100644
--- a/java/com/google/gerrit/server/patch/DiffOperations.java
+++ b/java/com/google/gerrit/server/patch/DiffOperations.java
@@ -18,10 +18,14 @@
import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import java.util.Map;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevWalk;
/**
* An interface for all file diff related operations. Clients should use this interface to request:
@@ -60,6 +64,29 @@
throws DiffNotAvailableException;
/**
+ * This method is similar to {@link #listModifiedFilesAgainstParent(NameKey, ObjectId, int,
+ * DiffOptions)} but loads the modified files directly instead of retrieving them from the diff
+ * cache.
+ *
+ * <p>A RevWalk and repoConfig are also supplied and are used to look up the commit IDs. This is
+ * useful in case one the commits is currently being created, that's why the {@code revWalk}
+ * parameter is needed.
+ *
+ * <p>Note that rename detection is disabled for this method.
+ *
+ * @return a map of file paths to {@link ModifiedFile}. The {@link ModifiedFile} contains the
+ * old/new file paths and the change type (added, deleted, etc...).
+ */
+ Map<String, ModifiedFile> loadModifiedFilesAgainstParent(
+ Project.NameKey project,
+ ObjectId newCommit,
+ int parentNum,
+ DiffOptions diffOptions,
+ RevWalk revWalk,
+ Config repoConfig)
+ throws DiffNotAvailableException;
+
+ /**
* Returns the list of added, deleted or modified files between two commits (patchsets). The
* commit message and merge list (for merge commits) are also returned.
*
@@ -77,6 +104,29 @@
throws DiffNotAvailableException;
/**
+ * This method is similar to {@link #listModifiedFilesAgainstParent(NameKey, ObjectId, int,
+ * DiffOptions)} but loads the modified files directly instead of retrieving them from the diff
+ * cache.
+ *
+ * <p>A RevWalk and repoConfig are also supplied and are used to look up the commit IDs. This is
+ * useful in case one the commits is currently being created, that's why the {@code revWalk}
+ * parameter is needed.
+ *
+ * <p>Note that rename detection is disabled for this method.
+ *
+ * @return a map of file paths to {@link ModifiedFile}. The {@link ModifiedFile} contains the
+ * old/new file paths and the change type (added, deleted, etc...).
+ */
+ Map<String, ModifiedFile> loadModifiedFiles(
+ Project.NameKey project,
+ ObjectId oldCommit,
+ ObjectId newCommit,
+ DiffOptions diffOptions,
+ RevWalk revWalk,
+ Config repoConfig)
+ throws DiffNotAvailableException;
+
+ /**
* Returns the diff for a single file between a patchset commit against its parent or the
* auto-merge commit. For deleted files, the {@code fileName} parameter should contain the old
* name of the file. This method will return {@link FileDiffOutput#empty(String, ObjectId,
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
index 3d230f8..012a91c 100644
--- a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -47,7 +47,15 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
+import org.eclipse.jgit.diff.DiffEntry;
+import org.eclipse.jgit.diff.DiffFormatter;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.util.io.DisabledOutputStream;
/**
* Provides different file diff operations. Uses the underlying Git/Gerrit caches to speed up the
@@ -57,6 +65,19 @@
public class DiffOperationsImpl implements DiffOperations {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final ImmutableMap<DiffEntry.ChangeType, Patch.ChangeType> changeTypeMap =
+ ImmutableMap.of(
+ DiffEntry.ChangeType.ADD,
+ Patch.ChangeType.ADDED,
+ DiffEntry.ChangeType.MODIFY,
+ Patch.ChangeType.MODIFIED,
+ DiffEntry.ChangeType.DELETE,
+ Patch.ChangeType.DELETED,
+ DiffEntry.ChangeType.RENAME,
+ Patch.ChangeType.RENAMED,
+ DiffEntry.ChangeType.COPY,
+ Patch.ChangeType.COPIED);
+
private static final int RENAME_SCORE = 60;
private static final DiffAlgorithm DEFAULT_DIFF_ALGORITHM =
DiffAlgorithm.HISTOGRAM_WITH_FALLBACK_MYERS;
@@ -103,6 +124,27 @@
}
@Override
+ public Map<String, ModifiedFile> loadModifiedFilesAgainstParent(
+ Project.NameKey project,
+ ObjectId newCommit,
+ int parentNum,
+ DiffOptions diffOptions,
+ RevWalk revWalk,
+ Config repoConfig)
+ throws DiffNotAvailableException {
+ try {
+ DiffParameters diffParams = computeDiffParameters(project, newCommit, parentNum);
+ return loadModifiedFilesWithoutCache(project, diffParams, revWalk, repoConfig);
+ } catch (IOException e) {
+ throw new DiffNotAvailableException(
+ String.format(
+ "Failed to evaluate the parent/base commit for commit '%s' with parentNum=%d",
+ newCommit, parentNum),
+ e);
+ }
+ }
+
+ @Override
public Map<String, FileDiffOutput> listModifiedFiles(
Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, DiffOptions diffOptions)
throws DiffNotAvailableException {
@@ -117,6 +159,25 @@
}
@Override
+ public Map<String, ModifiedFile> loadModifiedFiles(
+ Project.NameKey project,
+ ObjectId oldCommit,
+ ObjectId newCommit,
+ DiffOptions diffOptions,
+ RevWalk revWalk,
+ Config repoConfig)
+ throws DiffNotAvailableException {
+ DiffParameters params =
+ DiffParameters.builder()
+ .project(project)
+ .newCommit(newCommit)
+ .baseCommit(oldCommit)
+ .comparisonType(ComparisonType.againstOtherPatchSet())
+ .build();
+ return loadModifiedFilesWithoutCache(project, params, revWalk, repoConfig);
+ }
+
+ @Override
public FileDiffOutput getModifiedFileAgainstParent(
Project.NameKey project,
ObjectId newCommit,
@@ -329,6 +390,50 @@
.build();
}
+ /** Loads the modified file paths between two commits without inspecting the diff cache. */
+ private static Map<String, ModifiedFile> loadModifiedFilesWithoutCache(
+ Project.NameKey project, DiffParameters diffParams, RevWalk revWalk, Config repoConfig)
+ throws DiffNotAvailableException {
+ ObjectId newCommit = diffParams.newCommit();
+ ObjectId oldCommit = diffParams.baseCommit();
+ try {
+ ObjectReader reader = revWalk.getObjectReader();
+ List<DiffEntry> diffEntries;
+ try (DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
+ df.setReader(reader, repoConfig);
+ df.setDetectRenames(false);
+ diffEntries = df.scan(oldCommit.equals(ObjectId.zeroId()) ? null : oldCommit, newCommit);
+ }
+ return diffEntries.stream()
+ .map(
+ entry ->
+ ModifiedFile.builder()
+ .changeType(toChangeType(entry.getChangeType()))
+ .oldPath(getGitPath(entry.getOldPath()))
+ .newPath(getGitPath(entry.getNewPath()))
+ .build())
+ .collect(ImmutableMap.toImmutableMap(ModifiedFile::getDefaultPath, Function.identity()));
+ } catch (IOException e) {
+ throw new DiffNotAvailableException(
+ String.format(
+ "Failed to compute the modified files for project '%s',"
+ + " old commit '%s', new commit '%s'.",
+ project, oldCommit.name(), newCommit.name()),
+ e);
+ }
+ }
+
+ private static Optional<String> getGitPath(String path) {
+ return path.equals(DiffEntry.DEV_NULL) ? Optional.empty() : Optional.of(path);
+ }
+
+ private static Patch.ChangeType toChangeType(DiffEntry.ChangeType changeType) {
+ if (!changeTypeMap.containsKey(changeType)) {
+ throw new IllegalArgumentException("Unsupported type " + changeType);
+ }
+ return changeTypeMap.get(changeType);
+ }
+
@AutoValue
abstract static class DiffParameters {
abstract Project.NameKey project();
diff --git a/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java b/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
index f4e7ca3..3596a54 100644
--- a/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
+++ b/java/com/google/gerrit/server/patch/gitdiff/ModifiedFile.java
@@ -47,6 +47,10 @@
*/
public abstract Optional<String> newPath();
+ public String getDefaultPath() {
+ return newPath().isPresent() ? newPath().get() : oldPath().get();
+ }
+
public static Builder builder() {
return new AutoValue_ModifiedFile.Builder();
}
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalContext.java b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
index 4dedbb5..2839eb7 100644
--- a/java/com/google/gerrit/server/query/approval/ApprovalContext.java
+++ b/java/com/google/gerrit/server/query/approval/ApprovalContext.java
@@ -21,6 +21,8 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.server.notedb.ChangeNotes;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.revwalk.RevWalk;
/** Entity representing all required information to match predicates for copying approvals. */
@AutoValue
@@ -41,8 +43,19 @@
/** {@link ChangeKind} of the delta between the origin and target patch set. */
public abstract ChangeKind changeKind();
+ /** {@link RevWalk} of the repository for the current commit. */
+ public abstract RevWalk revWalk();
+
+ /** {@link RevWalk} of the repository for the current commit. */
+ public abstract Config repoConfig();
+
public static ApprovalContext create(
- ChangeNotes changeNotes, PatchSetApproval psa, PatchSet patchSet, ChangeKind changeKind) {
+ ChangeNotes changeNotes,
+ PatchSetApproval psa,
+ PatchSet patchSet,
+ ChangeKind changeKind,
+ RevWalk revWalk,
+ Config repoConfig) {
checkState(
psa.patchSetId().changeId().equals(patchSet.id().changeId()),
"approval and target must be the same change. got: %s, %s",
@@ -54,6 +67,7 @@
// As explained in the commit message of this change doing this check is only possible if there
// are no changes with gaps in patch set numbers. Since it's planned to fix-up old changes with
// gaps in patch set numbers, this todo is a reminder to add back the check once this is done.
- return new AutoValue_ApprovalContext(psa, patchSet, changeNotes, changeKind);
+ return new AutoValue_ApprovalContext(
+ psa, patchSet, changeNotes, changeKind, revWalk, repoConfig);
}
}
diff --git a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
index c35fa1c..2a72c49 100644
--- a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
+++ b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
@@ -24,7 +24,7 @@
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.DiffOptions;
-import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -59,24 +59,30 @@
Integer parentNum =
isInitialCommit(ctx.changeNotes().getProjectName(), targetPatchSet.commitId()) ? 0 : 1;
try {
- Map<String, FileDiffOutput> baseVsCurrent =
- diffOperations.listModifiedFilesAgainstParent(
+ Map<String, ModifiedFile> baseVsCurrent =
+ diffOperations.loadModifiedFilesAgainstParent(
ctx.changeNotes().getProjectName(),
targetPatchSet.commitId(),
parentNum,
- DiffOptions.DEFAULTS);
- Map<String, FileDiffOutput> baseVsPrior =
- diffOperations.listModifiedFilesAgainstParent(
+ DiffOptions.DEFAULTS,
+ ctx.revWalk(),
+ ctx.repoConfig());
+ Map<String, ModifiedFile> baseVsPrior =
+ diffOperations.loadModifiedFilesAgainstParent(
ctx.changeNotes().getProjectName(),
sourcePatchSet.commitId(),
parentNum,
- DiffOptions.DEFAULTS);
- Map<String, FileDiffOutput> priorVsCurrent =
- diffOperations.listModifiedFiles(
+ DiffOptions.DEFAULTS,
+ ctx.revWalk(),
+ ctx.repoConfig());
+ Map<String, ModifiedFile> priorVsCurrent =
+ diffOperations.loadModifiedFiles(
ctx.changeNotes().getProjectName(),
sourcePatchSet.commitId(),
targetPatchSet.commitId(),
- DiffOptions.DEFAULTS);
+ DiffOptions.DEFAULTS,
+ ctx.revWalk(),
+ ctx.repoConfig());
return match(baseVsCurrent, baseVsPrior, priorVsCurrent);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
@@ -92,9 +98,9 @@
* {@link ChangeType} matches for each modified file.
*/
public boolean match(
- Map<String, FileDiffOutput> baseVsCurrent,
- Map<String, FileDiffOutput> baseVsPrior,
- Map<String, FileDiffOutput> priorVsCurrent) {
+ Map<String, ModifiedFile> baseVsCurrent,
+ Map<String, ModifiedFile> baseVsPrior,
+ Map<String, ModifiedFile> priorVsCurrent) {
Set<String> allFiles = new HashSet<>();
allFiles.addAll(baseVsCurrent.keySet());
allFiles.addAll(baseVsPrior.keySet());
@@ -102,17 +108,17 @@
if (Patch.isMagic(file)) {
continue;
}
- FileDiffOutput fileDiffOutput1 = baseVsCurrent.get(file);
- FileDiffOutput fileDiffOutput2 = baseVsPrior.get(file);
+ ModifiedFile modifiedFile1 = baseVsCurrent.get(file);
+ ModifiedFile modifiedFile2 = baseVsPrior.get(file);
if (!priorVsCurrent.containsKey(file)) {
// If the file is not modified between prior and current patchsets, then scan safely skip
- // it. The file might has been modified due to rebase.
+ // it. The file might have been modified due to rebase.
continue;
}
- if (fileDiffOutput1 == null || fileDiffOutput2 == null) {
+ if (modifiedFile1 == null || modifiedFile2 == null) {
return false;
}
- if (!fileDiffOutput2.changeType().equals(fileDiffOutput1.changeType())) {
+ if (!modifiedFile2.changeType().equals(modifiedFile1.changeType())) {
return false;
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 397a8fa..f3a8a9d 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -774,7 +774,7 @@
if (!lazyload()) {
return ImmutableListMultimap.of();
}
- allApprovals = approvalsUtil.byChange(notes());
+ allApprovals = approvalsUtil.byChangeExcludingCopiedApprovals(notes());
}
return allApprovals;
}
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVote.java b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
index 4387524..b266031 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVote.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
@@ -197,9 +197,7 @@
Account.Id accountId = accountState.account().id();
- for (PatchSetApproval a :
- approvalsUtil.byPatchSetUser(
- ctx.getNotes(), psId, accountId, ctx.getRevWalk(), ctx.getRepoView().getConfig())) {
+ for (PatchSetApproval a : approvalsUtil.byPatchSetUser(ctx.getNotes(), psId, accountId)) {
if (!labelTypes.byLabel(a.labelId()).isPresent()) {
continue; // Ignore undefined labels.
} else if (!a.label().equals(label)) {
diff --git a/java/com/google/gerrit/server/restapi/change/Move.java b/java/com/google/gerrit/server/restapi/change/Move.java
index 8c21841..86ec6c8 100644
--- a/java/com/google/gerrit/server/restapi/change/Move.java
+++ b/java/com/google/gerrit/server/restapi/change/Move.java
@@ -260,11 +260,8 @@
* proposal: https://gerrit-review.googlesource.com/c/gerrit/+/129171
*/
private void updateApprovals(
- ChangeContext ctx, ChangeUpdate update, PatchSet.Id psId, Project.NameKey project)
- throws IOException {
- for (PatchSetApproval psa :
- approvalsUtil.byPatchSet(
- ctx.getNotes(), psId, ctx.getRevWalk(), ctx.getRepoView().getConfig())) {
+ ChangeContext ctx, ChangeUpdate update, PatchSet.Id psId, Project.NameKey project) {
+ for (PatchSetApproval psa : approvalsUtil.byPatchSet(ctx.getNotes(), psId)) {
ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
Optional<LabelType> type =
projectState.getLabelTypes(ctx.getNotes()).byLabel(psa.labelId());
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 5c252f4..24b0799 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -1579,12 +1579,7 @@
Map<String, PatchSetApproval> current = new HashMap<>();
for (PatchSetApproval a :
- approvalsUtil.byPatchSetUser(
- ctx.getNotes(),
- psId,
- user.getAccountId(),
- ctx.getRevWalk(),
- ctx.getRepoView().getConfig())) {
+ approvalsUtil.byPatchSetUser(ctx.getNotes(), psId, user.getAccountId())) {
if (a.isLegacySubmit()) {
continue;
}
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index 355d25f..cc3b75d 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -211,10 +211,7 @@
.setPostMessage(false)
.setSendEmail(false)
.setMatchAuthorToCommitterDate(
- args.project.is(BooleanProjectConfig.MATCH_AUTHOR_TO_COMMITTER_DATE))
- // The votes are automatically copied and they don't count as copied votes. See
- // method's javadoc.
- .setStoreCopiedVotes(/* storeCopiedVotes = */ false);
+ args.project.is(BooleanProjectConfig.MATCH_AUTHOR_TO_COMMITTER_DATE));
try {
rebaseOp.updateRepo(ctx);
} catch (MergeConflictException | NoSuchChangeException e) {
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 7d428eb..cd8ea4c 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -348,13 +348,10 @@
}
}
- private LabelNormalizer.Result approve(ChangeContext ctx, ChangeUpdate update)
- throws IOException {
+ private LabelNormalizer.Result approve(ChangeContext ctx, ChangeUpdate update) {
PatchSet.Id psId = update.getPatchSetId();
Map<PatchSetApproval.Key, PatchSetApproval> byKey = new HashMap<>();
- for (PatchSetApproval psa :
- args.approvalsUtil.byPatchSet(
- ctx.getNotes(), psId, ctx.getRevWalk(), ctx.getRepoView().getConfig())) {
+ for (PatchSetApproval psa : args.approvalsUtil.byPatchSet(ctx.getNotes(), psId)) {
byKey.put(psa.key(), psa);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
index 3888679..ff8a2d0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
@@ -34,7 +34,10 @@
import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
+import com.google.common.collect.MoreCollectors;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.ExtensionRegistry;
+import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
@@ -44,12 +47,15 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.RawInputUtil;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelFunction;
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.client.ChangeKind;
@@ -58,13 +64,18 @@
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.server.change.ChangeKindCacheImpl;
import com.google.gerrit.server.project.testing.TestLabels;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.Inject;
import com.google.inject.name.Named;
+import java.util.Arrays;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
+import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Before;
import org.junit.Test;
@@ -75,6 +86,7 @@
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ChangeOperations changeOperations;
@Inject private ChangeKindCreator changeKindCreator;
+ @Inject private ExtensionRegistry extensionRegistry;
@Inject
@Named("change_kind")
@@ -260,6 +272,113 @@
}
@Test
+ public void sticky_copiedToLatestPatchSetFromSubmitRecords() throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig().updateLabelType(LabelId.VERIFIED, b -> b.setFunction(LabelFunction.NO_BLOCK));
+ u.save();
+ }
+
+ // This test is covering the backfilling logic for changes which have been submitted, based on
+ // copied approvals, before Gerrit persisted copied votes as Copied-Label footers in NoteDb. It
+ // verifies that for such changes copied approvals are returned from the API even if the copied
+ // votes were not persisted as Copied-Label footers.
+ //
+ // In other words, this test verifies that given a change that was approved by a copied vote and
+ // then submitted and for which the copied approval is not persisted as a Copied-Label footer in
+ // NoteDb the copied approval is backfilled from the corresponding Submitted-With footer that
+ // got written to NoteDb on submit.
+ //
+ // Creating such a change would be possible by running the old Gerrit code from before Gerrit
+ // persisted copied labels as Copied-Label footers. However since this old Gerrit code is no
+ // longer available, the test needs to apply a trick to create a change in this state. It
+ // configures a fake submit rule, that pretends that an approval for a non-sticky label from an
+ // old patch set is still present on the current patch set and allows to submit the change.
+ // Since the label is non-sticky no Copied-Label footer is written for it. On submit the fake
+ // submit rule results in a Submitted-With footer that records the label as approved (although
+ // the label is actually not present on the current patch set). This is exactly the change state
+ // that we would have had by running the old code if submit was based on a copied label. As
+ // result of the backfilling logic we expect that this "copied" label (the label that is
+ // mentioned in the Submitted-With footer) is returned from the API.
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(new TestSubmitRule(user.id()))) {
+ // We want to add a vote on PS1, then not copy it to PS2, but include it in submit records
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Vote on patch-set 1
+ vote(admin, changeId, 2, 1);
+ vote(user, changeId, 1, -1);
+
+ // Upload patch-set 2. Change user's "Verified" vote on PS2.
+ changeOperations
+ .change(Change.id(r.getChange().getId().get()))
+ .newPatchset()
+ .file("new_file")
+ .content("content")
+ .commitMessage("Upload PS2")
+ .create();
+ vote(admin, changeId, 2, 1);
+ vote(user, changeId, 1, 1);
+
+ // Upload patch-set 3
+ changeOperations
+ .change(Change.id(r.getChange().getId().get()))
+ .newPatchset()
+ .file("another_file")
+ .content("content")
+ .commitMessage("Upload PS3")
+ .create();
+ vote(admin, changeId, 2, 1);
+
+ List<PatchSetApproval> patchSetApprovals =
+ notesFactory.create(project, r.getChange().getId()).getApprovalsWithCopied().values()
+ .stream()
+ .sorted(comparing(a -> a.patchSetId().get()))
+ .collect(toImmutableList());
+
+ // There's no verified approval on PS#3.
+ assertThat(
+ patchSetApprovals.stream()
+ .filter(
+ a ->
+ a.accountId().equals(user.id())
+ && a.label().equals(TestLabels.verified().getName())
+ && a.patchSetId().get() == 3)
+ .collect(Collectors.toList()))
+ .isEmpty();
+
+ // Submit the change. The TestSubmitRule will store a "submit record" containing a label
+ // voted by user, but the latest patch-set does not have an approval for this user, hence
+ // it will be copied if we request approvals after the change is merged.
+ requestScopeOperations.setApiUser(admin.id());
+ gApi.changes().id(changeId).current().submit();
+
+ patchSetApprovals =
+ notesFactory.create(project, r.getChange().getId()).getApprovalsWithCopied().values()
+ .stream()
+ .sorted(comparing(a -> a.patchSetId().get()))
+ .collect(toImmutableList());
+
+ // Get the copied approval for user on PS3 for the "Verified" label.
+ PatchSetApproval verifiedApproval =
+ patchSetApprovals.stream()
+ .filter(
+ a ->
+ a.accountId().equals(user.id())
+ && a.label().equals(TestLabels.verified().getName())
+ && a.patchSetId().get() == 3)
+ .collect(MoreCollectors.onlyElement());
+
+ assertCopied(
+ verifiedApproval,
+ /* psId= */ 3,
+ TestLabels.verified().getName(),
+ (short) 1,
+ /* copied= */ true);
+ }
+ }
+
+ @Test
public void stickyOnCopyValues() throws Exception {
TestAccount user2 = accountCreator.user2();
@@ -1265,6 +1384,35 @@
assertThat(nonCopiedSecondPatchsetRemovedVote.copied()).isFalse();
}
+ @Test
+ public void reviewerStickyVotingCanBeRemoved() throws Exception {
+ // Code-Review will be sticky.
+ String label = LabelId.CODE_REVIEW;
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig().updateLabelType(label, b -> b.setCopyAnyScore(true));
+ u.save();
+ }
+
+ PushOneCommit.Result r = createChange();
+
+ // Add a new vote by user
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(r.getChangeId()).current().review(ReviewInput.recommend());
+ requestScopeOperations.setApiUser(admin.id());
+
+ // Make a new patchset, keeping the Code-Review +1 vote.
+ amendChange(r.getChangeId());
+ assertVotes(detailedChange(r.getChangeId()), user, label, 1, null);
+
+ gApi.changes().id(r.getChangeId()).reviewer(user.email()).remove();
+
+ assertThat(r.getChange().notes().getApprovalsWithCopied()).isEmpty();
+
+ // Changes message has info about vote removed.
+ assertThat(Iterables.getLast(gApi.changes().id(r.getChangeId()).messages()).message)
+ .contains("Code-Review+1 by User");
+ }
+
private void assertChangeKindCacheContains(ObjectId prior, ObjectId next) {
ChangeKind kind =
changeKindCache.getIfPresent(ChangeKindCacheImpl.Key.create(prior, next, "recursive"));
@@ -1352,4 +1500,29 @@
assertThat(approval.value()).isEqualTo(value);
assertThat(approval.copied()).isEqualTo(copied);
}
+
+ /**
+ * Test submit rule that always return a passing record with a "Verified" label applied by {@link
+ * TestSubmitRule#userAccountId}.
+ */
+ private static class TestSubmitRule implements SubmitRule {
+ Account.Id userAccountId;
+
+ TestSubmitRule(Account.Id userAccountId) {
+ this.userAccountId = userAccountId;
+ }
+
+ @Override
+ public Optional<SubmitRecord> evaluate(ChangeData changeData) {
+ SubmitRecord record = new SubmitRecord();
+ record.ruleName = "testSubmitRule";
+ record.status = SubmitRecord.Status.OK;
+ SubmitRecord.Label label = new SubmitRecord.Label();
+ label.label = "Verified";
+ label.status = SubmitRecord.Label.Status.OK;
+ label.appliedBy = userAccountId;
+ record.labels = Arrays.asList(label);
+ return Optional.of(record);
+ }
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
index 537c7d8..97f072e 100644
--- a/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/query/ApprovalQueryIT.java
@@ -36,6 +36,8 @@
import com.google.gerrit.server.query.approval.ApprovalQueryBuilder;
import com.google.inject.Inject;
import java.util.Date;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Test;
public class ApprovalQueryIT extends AbstractDaemonTest {
@@ -250,7 +252,7 @@
}
private ApprovalContext contextForCodeReviewLabel(
- int value, PatchSet.Id psId, Account.Id approver) {
+ int value, PatchSet.Id psId, Account.Id approver) throws Exception {
ChangeNotes changeNotes = changeNotesFactory.create(project, psId.changeId());
PatchSet.Id newPsId = PatchSet.id(psId.changeId(), psId.get() + 1);
ChangeKind changeKind =
@@ -263,7 +265,15 @@
.key(PatchSetApproval.key(psId, approver, LabelId.create("Code-Review")))
.value(value)
.build();
- return ApprovalContext.create(
- changeNotes, approval, changeNotes.getPatchSets().get(newPsId), changeKind);
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk rw = new RevWalk(repo.newObjectReader())) {
+ return ApprovalContext.create(
+ changeNotes,
+ approval,
+ changeNotes.getPatchSets().get(newPsId),
+ changeKind,
+ rw,
+ repo.getConfig());
+ }
}
}
diff --git a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
index 9acb701..8b62628 100644
--- a/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
+++ b/javatests/com/google/gerrit/server/patch/DiffOperationsTest.java
@@ -19,10 +19,12 @@
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.testing.InMemoryModule;
import com.google.inject.Guice;
@@ -30,6 +32,7 @@
import com.google.inject.Injector;
import java.io.IOException;
import java.util.Map;
+import java.util.Optional;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
@@ -37,6 +40,7 @@
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.StoredConfig;
import org.eclipse.jgit.lib.TreeFormatter;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
@@ -112,6 +116,66 @@
assertThat(repo.getRefDatabase().exactRef(autoMergeRef)).isNotNull();
}
+ @Test
+ public void loadModifiedFiles() throws Exception {
+ ImmutableMap<String, String> oldFiles =
+ ImmutableMap.of(fileName1, fileContent1, fileName2, fileContent2);
+ ObjectId oldCommitId = createCommit(repo, null, oldFiles);
+
+ ImmutableMap<String, String> newFiles =
+ ImmutableMap.of(fileName1, fileContent1, fileName2, fileContent2 + "\nnew line here");
+ ObjectId newCommitId = createCommit(repo, oldCommitId, newFiles);
+
+ Repository repository = repoManager.openRepository(testProjectName);
+ ObjectReader objectReader = repository.newObjectReader();
+ RevWalk rw = new RevWalk(objectReader);
+ StoredConfig repoConfig = repository.getConfig();
+
+ // This call loads modified files directly without going through the diff cache.
+ Map<String, ModifiedFile> modifiedFiles =
+ diffOperations.loadModifiedFiles(
+ testProjectName, newCommitId, oldCommitId, DiffOptions.DEFAULTS, rw, repoConfig);
+
+ assertThat(modifiedFiles)
+ .containsExactly(
+ fileName2,
+ ModifiedFile.builder()
+ .changeType(ChangeType.MODIFIED)
+ .oldPath(Optional.of(fileName2))
+ .newPath(Optional.of(fileName2))
+ .build());
+ }
+
+ @Test
+ public void loadModifiedFilesAgainstParent() throws Exception {
+ ImmutableMap<String, String> oldFiles =
+ ImmutableMap.of(fileName1, fileContent1, fileName2, fileContent2);
+ ObjectId oldCommitId = createCommit(repo, null, oldFiles);
+
+ ImmutableMap<String, String> newFiles =
+ ImmutableMap.of(fileName1, fileContent1, fileName2, fileContent2 + "\nnew line here");
+ ObjectId newCommitId = createCommit(repo, oldCommitId, newFiles);
+
+ Repository repository = repoManager.openRepository(testProjectName);
+ ObjectReader objectReader = repository.newObjectReader();
+ RevWalk rw = new RevWalk(objectReader);
+ StoredConfig repoConfig = repository.getConfig();
+
+ // This call loads modified files directly without going through the diff cache.
+ Map<String, ModifiedFile> modifiedFiles =
+ diffOperations.loadModifiedFilesAgainstParent(
+ testProjectName, newCommitId, /* parentNum=*/ 0, DiffOptions.DEFAULTS, rw, repoConfig);
+
+ assertThat(modifiedFiles)
+ .containsExactly(
+ fileName2,
+ ModifiedFile.builder()
+ .changeType(ChangeType.MODIFIED)
+ .oldPath(Optional.of(fileName2))
+ .newPath(Optional.of(fileName2))
+ .build());
+ }
+
private ObjectId createMergeCommit(
Repository repo,
ImmutableMap<String, String> fileNameToContent,
diff --git a/plugins/replication b/plugins/replication
index b6f4011..d9c59a0 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit b6f4011070a8f0bf31fb1158ed12be71374eb47a
+Subproject commit d9c59a0be1c423706b2c4c74c955ebbeca5a894d
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 e397117..1d66aa5 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
@@ -180,7 +180,7 @@
fireReload,
fireTitleChange,
} from '../../../utils/event-util';
-import {GerritView, routerView$} from '../../../services/router/router-model';
+import {GerritView} from '../../../services/router/router-model';
import {
debounce,
DelayedTask,
@@ -604,6 +604,8 @@
// Private but used in tests.
readonly changeModel = getAppContext().changeModel;
+ private readonly routerModel = getAppContext().routerModel;
+
private readonly commentsModel = getAppContext().commentsModel;
private readonly shortcuts = getAppContext().shortcutsService;
@@ -632,7 +634,7 @@
})
);
this.subscriptions.push(
- routerView$.subscribe(view => {
+ this.routerModel.routerView$.subscribe(view => {
this.isViewCurrent = view === GerritView.CHANGE;
})
);
@@ -880,6 +882,13 @@
this._tabState = e.detail.tabState;
}
+ /**
+ * Currently there is a bug in this code where this.unresolvedOnly is only
+ * assigned the correct value when _onPaperTabClick is triggered which is
+ * only triggered when user explicitly clicks on the tab however the comments
+ * tab can also be opened via the url in which case the correct value to
+ * unresolvedOnly is never assigned.
+ */
_onPaperTabClick(e: MouseEvent) {
let target = e.target as HTMLElement | null;
let tabName: string | undefined;
@@ -891,7 +900,8 @@
} while (target);
if (tabName === PrimaryTab.COMMENT_THREADS) {
- // Show unresolved threads by default only if they are present
+ // Show unresolved threads by default
+ // Show resolved threads only if no unresolved threads exist
const hasUnresolvedThreads =
(this._commentThreads ?? []).filter(thread => isUnresolved(thread))
.length > 0;
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index 0e74cc6..e8cce2d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -17,6 +17,7 @@
import '../../../test/common-test-setup-karma';
import '../../edit/gr-edit-constants';
+import '../gr-thread-list/gr-thread-list';
import './gr-change-view';
import {
ChangeStatus,
@@ -107,6 +108,7 @@
import {FocusTarget, GrReplyDialog} from '../gr-reply-dialog/gr-reply-dialog';
import {GrOverlay} from '../../shared/gr-overlay/gr-overlay';
import {GrChangeStar} from '../../shared/gr-change-star/gr-change-star';
+import {GrThreadList} from '../gr-thread-list/gr-thread-list';
const fixture = fixtureFromElement('gr-change-view');
@@ -862,7 +864,43 @@
});
});
- suite('Findings comment tab', () => {
+ suite('Comments tab', () => {
+ setup(async () => {
+ element._changeNum = TEST_NUMERIC_CHANGE_ID;
+ element._change = {
+ ...createChangeViewChange(),
+ revisions: {
+ rev2: createRevision(2),
+ rev1: createRevision(1),
+ rev13: createRevision(13),
+ rev3: createRevision(3),
+ rev4: createRevision(4),
+ },
+ current_revision: 'rev4' as CommitId,
+ };
+ element._commentThreads = THREADS;
+ await flush();
+ const paperTabs = element.shadowRoot!.querySelector('#primaryTabs')!;
+ tap(paperTabs.querySelectorAll('paper-tab')[1]);
+ await flush();
+ });
+
+ test('commentId overrides unresolveOnly default', async () => {
+ const threadList = queryAndAssert<GrThreadList>(
+ element,
+ 'gr-thread-list'
+ );
+ assert.isTrue(element.unresolvedOnly);
+ assert.isNotOk(element.scrollCommentId);
+ assert.isTrue(threadList.unresolvedOnly);
+
+ element.scrollCommentId = 'abcd' as UrlEncodedCommentId;
+ await flush();
+ assert.isFalse(threadList.unresolvedOnly);
+ });
+ });
+
+ suite('Findings robot-comment tab', () => {
setup(async () => {
element._changeNum = TEST_NUMERIC_CHANGE_ID;
element._change = {
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
index 0349b3f..490162b 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
@@ -212,6 +212,7 @@
override willUpdate(changed: PropertyValues) {
if (changed.has('commentTabState')) this.onCommentTabStateUpdate();
+ if (changed.has('scrollCommentId')) this.onScrollCommentIdUpdate();
}
private onCommentTabStateUpdate() {
@@ -228,6 +229,14 @@
}
}
+ /**
+ * When user wants to scroll to a comment, render all comments so that the
+ * appropriate comment can be scrolled into view.
+ */
+ private onScrollCommentIdUpdate() {
+ if (this.scrollCommentId) this.handleAllComments();
+ }
+
static override get styles() {
return [
sharedStyles,
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 027c976..2a35494 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -66,7 +66,7 @@
AppElementParams,
} from '../../gr-app-types';
import {LocationChangeEventDetail} from '../../../types/events';
-import {GerritView, updateState} from '../../../services/router/router-model';
+import {GerritView} from '../../../services/router/router-model';
import {firePageError} from '../../../utils/event-util';
import {addQuotesWhen} from '../../../utils/string-util';
import {windowLocationReload} from '../../../utils/dom-util';
@@ -311,6 +311,8 @@
private readonly reporting = getAppContext().reportingService;
+ private readonly routerModel = getAppContext().routerModel;
+
private readonly restApiService = getAppContext().restApiService;
private readonly flagsService = getAppContext().flagsService;
@@ -323,11 +325,11 @@
}
_setParams(params: AppElementParams | GenerateUrlParameters) {
- updateState(
- params.view,
- 'changeNum' in params ? params.changeNum : undefined,
- 'patchNum' in params ? params.patchNum ?? undefined : undefined
- );
+ this.routerModel.updateState({
+ view: params.view,
+ changeNum: 'changeNum' in params ? params.changeNum : undefined,
+ patchNum: 'patchNum' in params ? params.patchNum ?? undefined : undefined,
+ });
this._appElement().params = params;
}
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 e527134..ff02d61 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
@@ -108,7 +108,7 @@
import {AppElementParams, AppElementDiffViewParam} from '../../gr-app-types';
import {EventType, OpenFixPreviewEvent} from '../../../types/events';
import {fireAlert, fireEvent, fireTitleChange} from '../../../utils/event-util';
-import {GerritView, routerView$} from '../../../services/router/router-model';
+import {GerritView} from '../../../services/router/router-model';
import {assertIsDefined} from '../../../utils/common-util';
import {addGlobalShortcut, Key, toggleClass} from '../../../utils/dom-util';
import {CursorMoveResult} from '../../../api/core';
@@ -356,6 +356,9 @@
private readonly restApiService = getAppContext().restApiService;
// Private but used in tests.
+ readonly routerModel = getAppContext().routerModel;
+
+ // Private but used in tests.
readonly userModel = getAppContext().userModel;
// Private but used in tests.
@@ -420,7 +423,7 @@
this.subscriptions.push(
combineLatest([
this.changeModel.currentPatchNum$,
- routerView$,
+ this.routerModel.routerView$,
this.changeModel.diffPath$,
this.userModel.diffPreferences$,
])
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index 4c4361d..b2d5a27 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -21,7 +21,7 @@
import {ChangeStatus, DiffViewMode, createDefaultDiffPrefs} from '../../../constants/constants.js';
import {stubRestApi, stubUsers, waitUntil} from '../../../test/test-utils.js';
import {ChangeComments} from '../gr-comment-api/gr-comment-api.js';
-import {GerritView, _testOnly_setState as setRouterModelState} from '../../../services/router/router-model.js';
+import {GerritView} from '../../../services/router/router-model.js';
import {
createChange,
createRevisions,
@@ -1206,7 +1206,7 @@
change: createChange(),
diffPath: '/COMMIT_MSG'});
- setRouterModelState({
+ element.routerModel.setState({
changeNum: TEST_NUMERIC_CHANGE_ID, view: GerritView.DIFF, patchNum: 2}
);
element._patchRange = {
@@ -1245,7 +1245,7 @@
change: createChange(),
diffPath: '/COMMIT_MSG'});
- setRouterModelState({
+ element.routerModel.setState({
changeNum: TEST_NUMERIC_CHANGE_ID, view: GerritView.DIFF,
patchNum: 22}
);
@@ -1271,7 +1271,7 @@
change: createChange(),
diffPath: '/COMMIT_MSG'});
- setRouterModelState({
+ element.routerModel.setState({
changeNum: TEST_NUMERIC_CHANGE_ID, view: GerritView.DIFF, patchNum: 2}
);
diff --git a/polygerrit-ui/app/embed/gr-diff-app-context-init.ts b/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
index 9b3e75d..38ed276 100644
--- a/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
+++ b/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
@@ -96,6 +96,9 @@
userModel: (_ctx: Partial<AppContext>) => {
throw new Error('userModel is not implemented');
},
+ routerModel: (_ctx: Partial<AppContext>) => {
+ throw new Error('routerModel is not implemented');
+ },
shortcutsService: (_ctx: Partial<AppContext>) => {
throw new Error('shortcutsService is not implemented');
},
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 46d2178..fbf5b0f 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -27,6 +27,7 @@
import {GrStorageService} from './storage/gr-storage_impl';
import {UserModel} from './user/user-model';
import {CommentsModel} from './comments/comments-model';
+import {RouterModel} from './router/router-model';
import {ShortcutsService} from './shortcuts/shortcuts-service';
import {BrowserModel} from './browser/browser-model';
import {assertIsDefined} from '../utils/common-util';
@@ -37,6 +38,7 @@
*/
export function createAppContext(): AppContext & Finalizable {
const appRegistry: Registry<AppContext> = {
+ routerModel: (_ctx: Partial<AppContext>) => new RouterModel(),
flagsService: (_ctx: Partial<AppContext>) =>
new FlagsServiceImplementation(),
reportingService: (ctx: Partial<AppContext>) => {
@@ -54,28 +56,41 @@
return new GrRestApiServiceImpl(ctx.authService!, ctx.flagsService!);
},
changeModel: (ctx: Partial<AppContext>) => {
- assertIsDefined(ctx.restApiService, 'restApiService');
- return new ChangeModel(ctx.restApiService!);
+ const routerModel = ctx.routerModel;
+ const restApiService = ctx.restApiService;
+ assertIsDefined(routerModel, 'routerModel');
+ assertIsDefined(restApiService, 'restApiService');
+ return new ChangeModel(routerModel, restApiService);
},
commentsModel: (ctx: Partial<AppContext>) => {
+ const routerModel = ctx.routerModel;
const changeModel = ctx.changeModel;
const restApiService = ctx.restApiService;
- const reporting = ctx.reportingService;
+ const reportingService = ctx.reportingService;
+ assertIsDefined(routerModel, 'routerModel');
assertIsDefined(changeModel, 'changeModel');
assertIsDefined(restApiService, 'restApiService');
- assertIsDefined(reporting, 'reportingService');
- return new CommentsModel(changeModel, restApiService, reporting);
+ assertIsDefined(reportingService, 'reportingService');
+ return new CommentsModel(
+ routerModel,
+ changeModel,
+ restApiService,
+ reportingService
+ );
},
checksModel: (ctx: Partial<AppContext>) => {
+ const routerModel = ctx.routerModel;
const changeModel = ctx.changeModel;
- const reporting = ctx.reportingService;
+ const reportingService = ctx.reportingService;
+ assertIsDefined(routerModel, 'routerModel');
assertIsDefined(changeModel, 'changeModel');
- assertIsDefined(reporting, 'reportingService');
- return new ChecksModel(changeModel, reporting);
+ assertIsDefined(reportingService, 'reportingService');
+ return new ChecksModel(routerModel, changeModel, reportingService);
},
jsApiService: (ctx: Partial<AppContext>) => {
- assertIsDefined(ctx.reportingService, 'reportingService');
- return new GrJsApiInterface(ctx.reportingService!);
+ const reportingService = ctx.reportingService;
+ assertIsDefined(reportingService, 'reportingService');
+ return new GrJsApiInterface(reportingService!);
},
storageService: (_ctx: Partial<AppContext>) => new GrStorageService(),
configModel: (ctx: Partial<AppContext>) => {
diff --git a/polygerrit-ui/app/services/app-context.ts b/polygerrit-ui/app/services/app-context.ts
index bea371f..367fee7 100644
--- a/polygerrit-ui/app/services/app-context.ts
+++ b/polygerrit-ui/app/services/app-context.ts
@@ -26,11 +26,13 @@
import {StorageService} from './storage/gr-storage';
import {UserModel} from './user/user-model';
import {CommentsModel} from './comments/comments-model';
+import {RouterModel} from './router/router-model';
import {ShortcutsService} from './shortcuts/shortcuts-service';
import {BrowserModel} from './browser/browser-model';
import {ConfigModel} from './config/config-model';
export interface AppContext {
+ routerModel: RouterModel;
flagsService: FlagsService;
reportingService: ReportingService;
eventEmitter: EventEmitterService;
diff --git a/polygerrit-ui/app/services/change/change-model.ts b/polygerrit-ui/app/services/change/change-model.ts
index 6a819f8..7055447 100644
--- a/polygerrit-ui/app/services/change/change-model.ts
+++ b/polygerrit-ui/app/services/change/change-model.ts
@@ -32,14 +32,13 @@
startWith,
switchMap,
} from 'rxjs/operators';
-import {routerPatchNum$, routerState$} from '../router/router-model';
+import {RouterModel} from '../router/router-model';
import {
computeAllPatchSets,
computeLatestPatchNum,
} from '../../utils/patch-set-util';
import {ParsedChangeInfo} from '../../types/types';
-import {routerChangeNum$} from '../router/router-model';
import {ChangeInfo} from '../../types/common';
import {RestApiService} from '../gr-rest-api/gr-rest-api';
import {Finalizable} from '../registry';
@@ -118,7 +117,7 @@
* out inconsistent state, e.g. router changeNum already updated, change not
* yet reset to undefined.
*/
- combineLatest([routerState$, this.changeState$])
+ combineLatest([this.routerModel.routerState$, this.changeState$])
.pipe(
filter(([routerState, changeState]) => {
const changeNum = changeState.change?._number;
@@ -128,7 +127,7 @@
distinctUntilChanged()
)
.pipe(
- withLatestFrom(routerPatchNum$, this.latestPatchNum$),
+ withLatestFrom(this.routerModel.routerPatchNum$, this.latestPatchNum$),
map(([_, routerPatchN, latestPatchN]) => routerPatchN || latestPatchN),
distinctUntilChanged()
);
@@ -142,9 +141,12 @@
'reload'
).pipe(startWith(undefined));
- constructor(readonly restApiService: RestApiService) {
+ constructor(
+ readonly routerModel: RouterModel,
+ readonly restApiService: RestApiService
+ ) {
this.subscriptions = [
- combineLatest([routerChangeNum$, this.reload$])
+ combineLatest([this.routerModel.routerChangeNum$, this.reload$])
.pipe(
map(([changeNum, _]) => changeNum),
switchMap(changeNum => {
diff --git a/polygerrit-ui/app/services/change/change-model_test.ts b/polygerrit-ui/app/services/change/change-model_test.ts
index 099a11f..0fb9712 100644
--- a/polygerrit-ui/app/services/change/change-model_test.ts
+++ b/polygerrit-ui/app/services/change/change-model_test.ts
@@ -28,10 +28,7 @@
import {CommitId, NumericChangeId, PatchSetNum} from '../../types/common';
import {ParsedChangeInfo} from '../../types/types';
import {getAppContext} from '../app-context';
-import {
- GerritView,
- _testOnly_setState as setRouterState,
-} from '../router/router-model';
+import {GerritView} from '../router/router-model';
import {ChangeState, LoadingStatus} from './change-model';
import {ChangeModel} from './change-model';
@@ -40,7 +37,10 @@
let knownChange: ParsedChangeInfo;
const testCompleted = new Subject<void>();
setup(() => {
- changeModel = new ChangeModel(getAppContext().restApiService);
+ changeModel = new ChangeModel(
+ getAppContext().routerModel,
+ getAppContext().restApiService
+ );
knownChange = {
...createChange(),
revisions: {
@@ -80,7 +80,10 @@
assert.equal(stub.callCount, 0);
assert.isUndefined(state?.change);
- setRouterState({view: GerritView.CHANGE, changeNum: knownChange._number});
+ changeModel.routerModel.setState({
+ view: GerritView.CHANGE,
+ changeNum: knownChange._number,
+ });
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADING);
assert.equal(stub.callCount, 1);
assert.isUndefined(state?.change);
@@ -101,7 +104,10 @@
changeModel.changeState$
.pipe(takeUntil(testCompleted))
.subscribe(s => (state = s));
- setRouterState({view: GerritView.CHANGE, changeNum: knownChange._number});
+ changeModel.routerModel.setState({
+ view: GerritView.CHANGE,
+ changeNum: knownChange._number,
+ });
promise.resolve(knownChange);
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADED);
@@ -127,7 +133,10 @@
changeModel.changeState$
.pipe(takeUntil(testCompleted))
.subscribe(s => (state = s));
- setRouterState({view: GerritView.CHANGE, changeNum: knownChange._number});
+ changeModel.routerModel.setState({
+ view: GerritView.CHANGE,
+ changeNum: knownChange._number,
+ });
promise.resolve(knownChange);
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADED);
@@ -138,7 +147,10 @@
_number: 123 as NumericChangeId,
};
promise = mockPromise<ParsedChangeInfo | undefined>();
- setRouterState({view: GerritView.CHANGE, changeNum: otherChange._number});
+ changeModel.routerModel.setState({
+ view: GerritView.CHANGE,
+ changeNum: otherChange._number,
+ });
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADING);
assert.equal(stub.callCount, 2);
assert.isUndefined(state?.change);
@@ -159,7 +171,10 @@
changeModel.changeState$
.pipe(takeUntil(testCompleted))
.subscribe(s => (state = s));
- setRouterState({view: GerritView.CHANGE, changeNum: knownChange._number});
+ changeModel.routerModel.setState({
+ view: GerritView.CHANGE,
+ changeNum: knownChange._number,
+ });
promise.resolve(knownChange);
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADED);
@@ -167,7 +182,10 @@
promise = mockPromise<ParsedChangeInfo | undefined>();
promise.resolve(undefined);
- setRouterState({view: GerritView.DASHBOARD, changeNum: undefined});
+ changeModel.routerModel.setState({
+ view: GerritView.CHANGE,
+ changeNum: undefined,
+ });
await waitUntil(() => state?.loadingStatus === LoadingStatus.NOT_LOADED);
assert.equal(stub.callCount, 2);
assert.isUndefined(state?.change);
@@ -176,7 +194,10 @@
promise = mockPromise<ParsedChangeInfo | undefined>();
promise.resolve(knownChange);
- setRouterState({view: GerritView.CHANGE, changeNum: knownChange._number});
+ changeModel.routerModel.setState({
+ view: GerritView.CHANGE,
+ changeNum: knownChange._number,
+ });
await waitUntil(() => state?.loadingStatus === LoadingStatus.LOADED);
assert.equal(stub.callCount, 3);
assert.equal(state?.change, knownChange);
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts
index 4024747..134afd8 100644
--- a/polygerrit-ui/app/services/checks/checks-model.ts
+++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -53,9 +53,9 @@
import {getCurrentRevision} from '../../utils/change-util';
import {getShaByPatchNum} from '../../utils/patch-set-util';
import {ReportingService} from '../gr-reporting/gr-reporting';
-import {routerPatchNum$} from '../router/router-model';
import {Execution} from '../../constants/reporting';
import {fireAlert, fireEvent} from '../../utils/event-util';
+import {RouterModel} from '../router/router-model';
/**
* The checks model maintains the state of checks for two patchsets: the latest
@@ -322,6 +322,7 @@
);
constructor(
+ readonly routerModel: RouterModel,
readonly changeModel: ChangeModel,
readonly reporting: ReportingService
) {
@@ -331,14 +332,14 @@
this.checkToPluginMap = map;
}),
combineLatest([
- routerPatchNum$,
+ this.routerModel.routerPatchNum$,
this.changeModel.latestPatchNum$,
]).subscribe(([routerPs, latestPs]) => {
this.latestPatchNum = latestPs;
if (latestPs === undefined) {
this.setPatchset(undefined);
} else if (typeof routerPs === 'number') {
- this.setPatchset(routerPs);
+ this.setPatchset(routerPs as PatchSetNumber);
} else {
this.setPatchset(latestPs);
}
diff --git a/polygerrit-ui/app/services/checks/checks-model_test.ts b/polygerrit-ui/app/services/checks/checks-model_test.ts
index 0d46289..bb90fe2 100644
--- a/polygerrit-ui/app/services/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/services/checks/checks-model_test.ts
@@ -46,6 +46,7 @@
setup(() => {
model = new ChecksModel(
+ getAppContext().routerModel,
getAppContext().changeModel,
getAppContext().reportingService
);
diff --git a/polygerrit-ui/app/services/comments/comments-model.ts b/polygerrit-ui/app/services/comments/comments-model.ts
index d46e08a..95a1030 100644
--- a/polygerrit-ui/app/services/comments/comments-model.ts
+++ b/polygerrit-ui/app/services/comments/comments-model.ts
@@ -37,7 +37,7 @@
} from '../../utils/comment-util';
import {deepEqual} from '../../utils/deep-util';
import {select} from '../../utils/observable-util';
-import {routerChangeNum$} from '../router/router-model';
+import {RouterModel} from '../router/router-model';
import {Finalizable} from '../registry';
import {combineLatest, Subscription} from 'rxjs';
import {fire, fireAlert, fireEvent} from '../../utils/event-util';
@@ -291,6 +291,7 @@
private discardedDrafts: DraftInfo[] = [];
constructor(
+ readonly routerModel: RouterModel,
readonly changeModel: ChangeModel,
readonly restApiService: RestApiService,
readonly reporting: ReportingService
@@ -305,7 +306,7 @@
this.changeModel.currentPatchNum$.subscribe(x => (this.patchNum = x))
);
this.subscriptions.push(
- routerChangeNum$.subscribe(changeNum => {
+ this.routerModel.routerChangeNum$.subscribe(changeNum => {
this.changeNum = changeNum;
this.setState({...initialState});
this.reloadAllComments();
diff --git a/polygerrit-ui/app/services/comments/comments-model_test.ts b/polygerrit-ui/app/services/comments/comments-model_test.ts
index f39cad8..a8f2118 100644
--- a/polygerrit-ui/app/services/comments/comments-model_test.ts
+++ b/polygerrit-ui/app/services/comments/comments-model_test.ts
@@ -30,10 +30,7 @@
} from '../../test/test-data-generators';
import {stubRestApi, waitUntil, waitUntilCalled} from '../../test/test-utils';
import {getAppContext} from '../app-context';
-import {
- GerritView,
- updateState as updateRouterState,
-} from '../router/router-model';
+import {GerritView} from '../router/router-model';
import {PathToCommentsInfoMap} from '../../types/common';
suite('comments model tests', () => {
@@ -76,6 +73,7 @@
test('loads comments', async () => {
const model = new CommentsModel(
+ getAppContext().routerModel,
getAppContext().changeModel,
getAppContext().restApiService,
getAppContext().reportingService
@@ -102,7 +100,10 @@
model.portedComments$.subscribe(c => (portedComments = c ?? {}))
);
- updateRouterState(GerritView.CHANGE, TEST_NUMERIC_CHANGE_ID);
+ model.routerModel.updateState({
+ view: GerritView.CHANGE,
+ changeNum: TEST_NUMERIC_CHANGE_ID,
+ });
model.changeModel.updateStateChange(createParsedChange());
await waitUntilCalled(diffCommentsSpy, 'diffCommentsSpy');
diff --git a/polygerrit-ui/app/services/router/router-model.ts b/polygerrit-ui/app/services/router/router-model.ts
index f549e859..73dee78 100644
--- a/polygerrit-ui/app/services/router/router-model.ts
+++ b/polygerrit-ui/app/services/router/router-model.ts
@@ -15,9 +15,10 @@
* limitations under the License.
*/
-import {NumericChangeId, PatchSetNum} from '../../types/common';
import {BehaviorSubject, Observable} from 'rxjs';
import {distinctUntilChanged, map} from 'rxjs/operators';
+import {Finalizable} from '../registry';
+import {NumericChangeId, PatchSetNum} from '../../types/common';
export enum GerritView {
ADMIN = 'admin',
@@ -42,57 +43,44 @@
patchNum?: PatchSetNum;
}
-// TODO: Figure out how to best enforce immutability of all states. Use Immer?
-// Use DeepReadOnly?
-const initialState: RouterState = {};
+export class RouterModel implements Finalizable {
+ private readonly privateState$ = new BehaviorSubject<RouterState>({});
-const privateState$ = new BehaviorSubject<RouterState>(initialState);
+ readonly routerView$: Observable<GerritView | undefined>;
-export function _testOnly_resetState() {
- // We cannot assign a new subject to privateState$, because all the selectors
- // have already subscribed to the original subject. So we have to emit the
- // initial state on the existing subject.
- privateState$.next({...initialState});
+ readonly routerChangeNum$: Observable<NumericChangeId | undefined>;
+
+ readonly routerPatchNum$: Observable<PatchSetNum | undefined>;
+
+ constructor() {
+ this.routerView$ = this.privateState$.pipe(
+ map(state => state.view),
+ distinctUntilChanged()
+ );
+ this.routerChangeNum$ = this.privateState$.pipe(
+ map(state => state.changeNum),
+ distinctUntilChanged()
+ );
+ this.routerPatchNum$ = this.privateState$.pipe(
+ map(state => state.patchNum),
+ distinctUntilChanged()
+ );
+ }
+
+ finalize() {}
+
+ setState(state: RouterState) {
+ this.privateState$.next(state);
+ }
+
+ updateState(partial: Partial<RouterState>) {
+ this.privateState$.next({
+ ...this.privateState$.getValue(),
+ ...partial,
+ });
+ }
+
+ get routerState$(): Observable<RouterState> {
+ return this.privateState$;
+ }
}
-
-export function _testOnly_setState(state: RouterState) {
- privateState$.next(state);
-}
-
-export function _testOnly_getState() {
- return privateState$.getValue();
-}
-
-// Re-exporting as Observable so that you can only subscribe, but not emit.
-export const routerState$: Observable<RouterState> = privateState$;
-
-// Must only be used by the router service or whatever is in control of this
-// model.
-// TODO: Consider keeping params of type AppElementParams entirely in the state
-export function updateState(
- view?: GerritView,
- changeNum?: NumericChangeId,
- patchNum?: PatchSetNum
-) {
- privateState$.next({
- ...privateState$.getValue(),
- view,
- changeNum,
- patchNum,
- });
-}
-
-export const routerView$ = routerState$.pipe(
- map(state => state.view),
- distinctUntilChanged()
-);
-
-export const routerChangeNum$ = routerState$.pipe(
- map(state => state.changeNum),
- distinctUntilChanged()
-);
-
-export const routerPatchNum$ = routerState$.pipe(
- map(state => state.patchNum),
- distinctUntilChanged()
-);
diff --git a/polygerrit-ui/app/services/user/user-model.ts b/polygerrit-ui/app/services/user/user-model.ts
index 9107b31d..441c09d 100644
--- a/polygerrit-ui/app/services/user/user-model.ts
+++ b/polygerrit-ui/app/services/user/user-model.ts
@@ -85,7 +85,7 @@
preference => preference.diff_view ?? DiffViewMode.SIDE_BY_SIDE
);
- private readonly subscriptions: Subscription[] = [];
+ private subscriptions: Subscription[] = [];
get userState$(): Observable<UserState> {
return this.privateState$;
@@ -135,7 +135,7 @@
for (const s of this.subscriptions) {
s.unsubscribe();
}
- this.subscriptions.splice(0, this.subscriptions.length);
+ this.subscriptions = [];
}
updatePreferences(prefs: Partial<PreferencesInfo>) {
diff --git a/polygerrit-ui/app/test/common-test-setup.ts b/polygerrit-ui/app/test/common-test-setup.ts
index b8a5e49..94e3f69 100644
--- a/polygerrit-ui/app/test/common-test-setup.ts
+++ b/polygerrit-ui/app/test/common-test-setup.ts
@@ -45,8 +45,6 @@
import {_testOnly_allTasks} from '../utils/async-util';
import {cleanUpStorage} from '../services/storage/gr-storage_mock';
-import {_testOnly_resetState as resetRouterState} from '../services/router/router-model';
-
declare global {
interface Window {
assert: typeof chai.assert;
@@ -111,8 +109,6 @@
// tests.
initGlobalVariables(appContext);
- resetRouterState();
-
const shortcuts = appContext.shortcutsService;
assert.isTrue(shortcuts._testOnly_isEmpty());
const selection = document.getSelection();
diff --git a/polygerrit-ui/app/test/test-app-context-init.ts b/polygerrit-ui/app/test/test-app-context-init.ts
index 01d776e..5f5507b 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -30,12 +30,14 @@
import {GrJsApiInterface} from '../elements/shared/gr-js-api-interface/gr-js-api-interface-element';
import {UserModel} from '../services/user/user-model';
import {CommentsModel} from '../services/comments/comments-model';
+import {RouterModel} from '../services/router/router-model';
import {ShortcutsService} from '../services/shortcuts/shortcuts-service';
import {BrowserModel} from '../services/browser/browser-model';
import {ConfigModel} from '../services/config/config-model';
export function createTestAppContext(): AppContext & Finalizable {
const appRegistry: Registry<AppContext> = {
+ routerModel: (_ctx: Partial<AppContext>) => new RouterModel(),
flagsService: (_ctx: Partial<AppContext>) =>
new FlagsServiceImplementation(),
reportingService: (_ctx: Partial<AppContext>) => grReportingMock,
@@ -46,23 +48,36 @@
},
restApiService: (_ctx: Partial<AppContext>) => grRestApiMock,
changeModel: (ctx: Partial<AppContext>) => {
- assertIsDefined(ctx.restApiService, 'restApiService');
- return new ChangeModel(ctx.restApiService!);
+ const routerModel = ctx.routerModel;
+ const restApiService = ctx.restApiService;
+ assertIsDefined(routerModel, 'routerModel');
+ assertIsDefined(restApiService, 'restApiService');
+ return new ChangeModel(routerModel, restApiService);
},
commentsModel: (ctx: Partial<AppContext>) => {
- assertIsDefined(ctx.changeModel, 'changeModel');
- assertIsDefined(ctx.restApiService, 'restApiService');
- assertIsDefined(ctx.reportingService, 'reportingService');
+ const routerModel = ctx.routerModel;
+ const changeModel = ctx.changeModel;
+ const restApiService = ctx.restApiService;
+ const reportingService = ctx.reportingService;
+ assertIsDefined(routerModel, 'routerModel');
+ assertIsDefined(changeModel, 'changeModel');
+ assertIsDefined(restApiService, 'restApiService');
+ assertIsDefined(reportingService, 'reportingService');
return new CommentsModel(
- ctx.changeModel!,
- ctx.restApiService!,
- ctx.reportingService!
+ routerModel,
+ changeModel,
+ restApiService,
+ reportingService
);
},
checksModel: (ctx: Partial<AppContext>) => {
- assertIsDefined(ctx.changeModel, 'changeModel');
- assertIsDefined(ctx.reportingService, 'reportingService');
- return new ChecksModel(ctx.changeModel!, ctx.reportingService!);
+ const routerModel = ctx.routerModel;
+ const changeModel = ctx.changeModel;
+ const reportingService = ctx.reportingService;
+ assertIsDefined(routerModel, 'routerModel');
+ assertIsDefined(changeModel, 'changeModel');
+ assertIsDefined(reportingService, 'reportingService');
+ return new ChecksModel(routerModel, changeModel, reportingService);
},
jsApiService: (ctx: Partial<AppContext>) => {
assertIsDefined(ctx.reportingService, 'reportingService');
diff --git a/proto/cache.proto b/proto/cache.proto
index 16e5e95..750bea1 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -718,18 +718,3 @@
ComparisonType comparison_type = 11;
bool negative = 12;
}
-
-// Serialized form of com.google.gerrit.server.approval.ApprovalCacheImpl.Key.
-// Next ID: 5
-message PatchSetApprovalsKeyProto {
- string project = 1;
- int32 change_id = 2;
- int32 patch_set_id = 3;
- bytes id = 4;
-}
-
-// Repeated version of PatchSetApprovalProto
-// Next ID: 2
-message AllPatchSetApprovalsProto {
- repeated devtools.gerritcodereview.PatchSetApproval approval = 1;
-}
