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;
-}