Merge "Remove JCraft JSch dependency"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 90e0713..a56055a 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/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 25a1a00..1c00c95 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -37,6 +37,7 @@
} from '../../../utils/patch-set-util';
import {
CommentThread,
+ equalLocation,
isInBaseOfPatchRange,
isInRevisionOfPatchRange,
} from '../../../utils/comment-util';
@@ -738,15 +739,36 @@
_threadsChanged(threads: CommentThread[]) {
const rootIdToThreadEl = new Map<UrlEncodedCommentId, GrCommentThread>();
+ const unsavedThreadEls: GrCommentThread[] = [];
for (const threadEl of this.getThreadEls()) {
if (threadEl.rootId) {
rootIdToThreadEl.set(threadEl.rootId, threadEl);
+ } else {
+ // Unsaved thread els must have editing:true, just being defensive here.
+ if (threadEl.editing) unsavedThreadEls.push(threadEl);
}
}
const dontRemove = new Set<GrCommentThread>();
for (const thread of threads) {
- const existingThreadEl =
+ // Let's find an existing DOM element matching the thread. Normally this
+ // is as simple as matching the rootIds.
+ let existingThreadEl =
thread.rootId && rootIdToThreadEl.get(thread.rootId);
+ // But unsaved threads don't have rootIds. The incoming thread might be
+ // the saved version of the unsaved thread element. To verify that we
+ // check that the thread only has one comment and that their location is
+ // identical.
+ // TODO(brohlfs): This matching is not perfect. You could quickly create
+ // two new threads on the same line/range. Then this code just makes a
+ // random guess.
+ if (!existingThreadEl && thread.comments?.length === 1) {
+ for (const unsavedThreadEl of unsavedThreadEls) {
+ if (equalLocation(unsavedThreadEl.thread, thread)) {
+ existingThreadEl = unsavedThreadEl;
+ break;
+ }
+ }
+ }
if (existingThreadEl) {
existingThreadEl.thread = thread;
dontRemove.add(existingThreadEl);
@@ -759,6 +781,10 @@
// Remove all threads that are no longer existing.
for (const threadEl of this.getThreadEls()) {
if (dontRemove.has(threadEl)) continue;
+ // The user may have opened a couple of comment boxes for editing. They
+ // might be unsaved and thus not be reflected in `threads` yet, so let's
+ // keep them open.
+ if (threadEl.editing && threadEl.thread?.comments.length === 0) continue;
threadEl.remove();
}
const portedThreadsCount = threads.filter(thread => thread.ported).length;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index 6149c82..888dce3 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -1166,6 +1166,47 @@
assert.equal(threads.length, 2);
});
+ test('unsaved thread changes to draft', async () => {
+ element.patchRange = {
+ basePatchNum: 2,
+ patchNum: 3,
+ };
+ element.file = {basePath: 'file_renamed.txt', path: element.path};
+ element.threads = [];
+ await flush();
+
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ side: Side.RIGHT,
+ path: element.path,
+ lineNum: 13,
+ },
+ }));
+ await flush();
+ assert.equal(element.getThreadEls().length, 1);
+ const threadEl = element.getThreadEls()[0];
+ assert.equal(threadEl.thread.line, 13);
+ assert.isDefined(threadEl.unsavedComment);
+ assert.equal(threadEl.thread.comments.length, 0);
+
+ const draftThread = createCommentThread([{
+ path: element.path,
+ patch_set: 3,
+ line: 13,
+ __draft: true,
+ }]);
+ element.threads = [draftThread];
+ await flush();
+
+ // We expect that no additional thread element was created.
+ assert.equal(element.getThreadEls().length, 1);
+ // In fact the thread element must still be the same.
+ assert.equal(element.getThreadEls()[0], threadEl);
+ // But it must have been updated from unsaved to draft:
+ assert.isUndefined(threadEl.unsavedComment);
+ assert.equal(threadEl.thread.comments.length, 1);
+ });
+
test('thread should use new file path if first created ' +
'on patch set (left) but is base', async () => {
element.patchRange = {
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index de8a02d..7a4b6f2 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -69,6 +69,7 @@
import {classMap} from 'lit/directives/class-map';
import {ShortcutController} from '../../lit/shortcut-controller';
import {ValueChangedEvent} from '../../../types/events';
+import {notDeepEqual} from '../../../utils/deep-util';
const NEWLINE_PATTERN = /\n/g;
@@ -112,8 +113,17 @@
@queryAll('gr-comment')
commentElements?: NodeList;
- /** Required to be set by parent. */
- @property()
+ /**
+ * Required to be set by parent.
+ *
+ * Lit's `hasChanged` change detection defaults to just checking strict
+ * equality (===). Here it makes sense to install a proper `deepEqual`
+ * check, because of how the comments-model and ChangeComments are setup:
+ * Each thread object is recreated on the slightest model change. So when you
+ * have 100 comment threads and there is an update to one thread, then you
+ * want to avoid re-rendering the other 99 threads.
+ */
+ @property({hasChanged: notDeepEqual})
thread?: CommentThread;
/**
@@ -599,7 +609,10 @@
}
}
if (changed.has('editing')) {
- if (!this.editing) {
+ // changed.get('editing') contains the old value. We only want to trigger
+ // when changing from editing to non-editing (user has cancelled/saved).
+ // We do *not* want to trigger on first render (old value is `null`)
+ if (!this.editing && changed.get('editing') === true) {
this.unsavedComment = undefined;
if (this.thread?.comments.length === 0) {
this.remove();
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index ab08996..347e1e0 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -39,6 +39,7 @@
} from '../../../test/test-data-generators';
import {tap} from '@polymer/iron-test-helpers/mock-interactions';
import {SinonStub} from 'sinon';
+import {waitUntil} from '@open-wc/testing-helpers';
const basicFixture = fixtureFromElement('gr-comment-thread');
@@ -271,6 +272,32 @@
});
});
+ suite('self removal when empty thread changed to editing:false', () => {
+ let threadEl: GrCommentThread;
+
+ setup(async () => {
+ threadEl = basicFixture.instantiate();
+ threadEl.thread = createThread();
+ });
+
+ test('new thread el normally has a parent and an unsaved comment', async () => {
+ await waitUntil(() => threadEl.editing);
+ assert.isOk(threadEl.unsavedComment);
+ assert.isOk(threadEl.parentElement);
+ });
+
+ test('thread el removed after clicking CANCEL', async () => {
+ await waitUntil(() => threadEl.editing);
+
+ const commentEl = queryAndAssert(threadEl, 'gr-comment');
+ const buttonEl = queryAndAssert(commentEl, 'gr-button.cancel');
+ tap(buttonEl);
+
+ await waitUntil(() => !threadEl.editing);
+ assert.isNotOk(threadEl.parentElement);
+ });
+ });
+
test('comments are sorted correctly', () => {
const comments: CommentInfo[] = [
{
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 966a75c..49e9674 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -205,6 +205,21 @@
rangeInfoLost?: boolean; // if BE was unable to determine a range for this
}
+export function equalLocation(t1?: CommentThread, t2?: CommentThread) {
+ if (t1 === t2) return true;
+ if (t1 === undefined || t2 === undefined) return false;
+ return (
+ t1.path === t2.path &&
+ t1.patchNum === t2.patchNum &&
+ t1.commentSide === t2.commentSide &&
+ t1.line === t2.line &&
+ t1.range?.start_line === t2.range?.start_line &&
+ t1.range?.start_character === t2.range?.start_character &&
+ t1.range?.end_line === t2.range?.end_line &&
+ t1.range?.end_character === t2.range?.end_character
+ );
+}
+
export function getLastComment(thread: CommentThread): CommentInfo | undefined {
const len = thread.comments.length;
return thread.comments[len - 1];
diff --git a/polygerrit-ui/app/utils/deep-util.ts b/polygerrit-ui/app/utils/deep-util.ts
index ff19635..694a8d7 100644
--- a/polygerrit-ui/app/utils/deep-util.ts
+++ b/polygerrit-ui/app/utils/deep-util.ts
@@ -37,6 +37,10 @@
return false;
}
+export function notDeepEqual<T>(a: T, b: T): boolean {
+ return !deepEqual(a, b);
+}
+
/**
* @param obj Object
*/
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;
-}