Instead of deleting patch-set-approval set vote to zero

Problem: some reviewers get removed from change after it gets submitted.

On submitting a change we used to delete those patch-set-approvals where
the reviewer didn't have permission to vote in that label or the label
didn't exist any more.

On the other side, when a reviewer is added to a change this is recorded
as patch-set-approval on some label, with zero vote. If the chosen label
happens to be one where the reviewer doesn't have permission to vote,
this patch-set-approval will be deleted on submit and the reviewer will
be removed from the change.

Even if the added reviewer did have permission to vote in the chosen label,
the label could get deleted before the change is submitted. Again, the
reviewer will be removed from the change.

Instead of deleting patch-set-approvals set the vote to zero so that we
don't accidentally remove reviewers.

NOTE: this issue doesn't occur when using NoteDb. In NoteDb we keep
track of reviewers explicitly and this is why for NoteDb we still want
to delete these approvals.

Bug: Issue 4163
Change-Id: I4d4f8d7b55c12a0484257192e7d0ed8d2f71ebcc

1 files changed, 16 insertions, 1 deletions

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index 9a95b520d3..ad1576b173 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -17,6 +17,7 @@ package com.google.gerrit.server.git;
 import static com.google.common.base.Preconditions.checkState;
 import static org.eclipse.jgit.lib.RefDatabase.ALL;
 
+import com.google.common.base.Function;
 import com.google.common.base.Optional;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ArrayListMultimap;
@@ -1117,7 +1118,7 @@ public class MergeOp {
     logDebug("Adding submit label " + submit);
 
     db.patchSetApprovals().upsert(normalized.getNormalized());
-    db.patchSetApprovals().delete(normalized.deleted());
+    db.patchSetApprovals().update(zero(normalized.deleted()));
 
     try {
       return saveToBatch(control, update, normalized, timestamp);
@@ -1126,6 +1127,20 @@ public class MergeOp {
     }
   }
 
+  private static Iterable<PatchSetApproval> zero(
+      Iterable<PatchSetApproval> approvals) {
+    return Iterables.transform(approvals,
+        new Function<PatchSetApproval, PatchSetApproval>() {
+          @Override
+          public PatchSetApproval apply(PatchSetApproval in) {
+            PatchSetApproval copy = new PatchSetApproval(in.getPatchSetId(), in);
+            copy.setValue((short) 0);
+            return copy;
+          }
+        });
+  }
+
+
   private BatchMetaDataUpdate saveToBatch(ChangeControl ctl,
       ChangeUpdate callerUpdate, LabelNormalizer.Result normalized,
       Timestamp timestamp) throws IOException {