ChangeRebuilderImpl: Omit empty group list from update

This allows us to properly convert old patch sets that never had
groups applied. Without this change, we put an empty Groups footer,
which results in the groups list [""] after NoteDb conversion, which
doesn't match the null list prior to conversion. Add a case to
ChangeRebuilderIT to verify.

Change-Id: I2a54c89271965f97fd454b036e78d11229b1a3d7

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java

@@ -22,11 +22,13 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.api.changes.DraftInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -39,6 +41,7 @@ import com.google.gerrit.server.notedb.NoteDbChangeState;
 import com.google.gerrit.server.schema.DisabledChangesReviewDbWrapper;
 import com.google.gerrit.testutil.NoteDbChecker;
 import com.google.gerrit.testutil.NoteDbMode;
+import com.google.gerrit.testutil.TestChanges;
 import com.google.gerrit.testutil.TestTimeUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -50,6 +53,7 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.sql.Timestamp;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.concurrent.TimeUnit;
@@ -106,6 +110,24 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     checker.rebuildAndCheckChanges(id);
   }
 
+  @Test
+  public void patchSetWithNullGroups() throws Exception {
+    Timestamp ts = TimeUtil.nowTs();
+    @SuppressWarnings("deprecation")
+    Change c = TestChanges.newChange(project, user.getId(), db.nextChangeId());
+    c.setCreatedOn(ts);
+    c.setLastUpdatedOn(ts);
+    PatchSet ps = TestChanges.newPatchSet(
+        c.currentPatchSetId(), "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
+        user.getId());
+    ps.setCreatedOn(ts);
+    db.changes().insert(Collections.singleton(c));
+    db.patchSets().insert(Collections.singleton(ps));
+
+    assertThat(ps.getGroups()).isEmpty();
+    checker.rebuildAndCheckChanges(c.getId());
+  }
+
   @Test
   public void draftComment() throws Exception {
     PushOneCommit.Result r = createChange();

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java

@@ -560,7 +560,10 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
         update.setSubjectForCommit("Create patch set " + ps.getPatchSetId());
       }
       setRevision(update, ps);
+      List<String> groups = ps.getGroups();
+      if (!groups.isEmpty()) {
         update.setGroups(ps.getGroups());
+      }
       if (ps.isDraft()) {
         update.setPatchSetState(PatchSetState.DRAFT);
       }