Notedb: Implement patch set groups

This field is mutable, which adds the complication that we might
parse a new value for groups in a later commit, before parsing the
Commit field that allows us to populate the patch set maps. Use a
sentinel RevId to handle this case internally.

Change-Id: I4d168f078241660c8e72ff709655081d99ac2b66

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -47,6 +47,7 @@ import com.google.gerrit.server.AnonymousUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.OutputFormat;
+import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.config.AllProjectsName;
@@ -170,6 +171,9 @@ public abstract class AbstractDaemonTest {
   @Inject
   protected ChangeData.Factory changeDataFactory;
 
+  @Inject
+  protected PatchSetUtil psUtil;
+
   protected TestRepository<InMemoryRepository> testRepo;
   protected GerritServer server;
   protected TestAccount admin;
@@ -634,4 +638,8 @@ public abstract class AbstractDaemonTest {
   protected PatchSet getPatchSet(PatchSet.Id psId) throws OrmException {
     return changeDataFactory.create(db, psId.getParentKey()).patchSet(psId);
   }
+
+  protected IdentifiedUser user(TestAccount testAccount) {
+    return identifiedUserFactory.create(Providers.of(db), testAccount.getId());
+  }
 }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java

@@ -22,6 +22,7 @@ import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.RestSession;
+import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -29,6 +30,8 @@ import com.google.gerrit.server.change.GetRelated.ChangeAndCommit;
 import com.google.gerrit.server.change.GetRelated.RelatedInfo;
 import com.google.gerrit.server.edit.ChangeEditModifier;
 import com.google.gerrit.server.edit.ChangeEditUtil;
+import com.google.gerrit.server.git.BatchUpdate;
+import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -47,6 +50,9 @@ public class GetRelatedIT extends AbstractDaemonTest {
   @Inject
   private ChangeEditModifier editModifier;
 
+  @Inject
+  private BatchUpdate.Factory updateFactory;
+
   @Test
   public void getRelatedNoResult() throws Exception {
     PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
@@ -566,9 +572,7 @@ public class GetRelatedIT extends AbstractDaemonTest {
     }
 
     // Pretend PS1,1 was pushed before the groups field was added.
-    PatchSet ps1_1 = getPatchSet(psId1_1);
+    setGroups(psId1_1, null);
-    ps1_1.setGroups(null);
-    db.patchSets().update(ImmutableList.of(ps1_1));
     indexer.index(changeDataFactory.create(db, psId1_1.getParentKey()));
 
     // PS1,1 has no groups, so disappeared from related changes.
@@ -625,6 +629,22 @@ public class GetRelatedIT extends AbstractDaemonTest {
     return result;
   }
 
+  private void setGroups(final PatchSet.Id psId,
+      final Iterable<String> groups) throws Exception {
+    try (BatchUpdate bu = updateFactory.create(
+        db, project, user(user), TimeUtil.nowTs())) {
+      bu.addOp(psId.getParentKey(), new BatchUpdate.Op() {
+        @Override
+        public boolean updateChange(ChangeContext ctx) throws OrmException {
+          PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
+          psUtil.setGroups(ctx.getDb(), ctx.getUpdate(psId), ps, groups);
+          return true;
+        }
+      });
+      bu.execute();
+    }
+  }
+
   private void assertRelated(PatchSet.Id psId, ChangeAndCommit... expected)
       throws Exception {
     List<ChangeAndCommit> actual = getRelated(psId);

gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java

@@ -118,4 +118,11 @@ public class PatchSetUtil {
       update.setPatchSetId(psId);
     }
   }
+
+  public void setGroups(ReviewDb db, ChangeUpdate update, PatchSet ps,
+      Iterable<String> groups) throws OrmException {
+    ps.setGroups(groups);
+    update.setGroups(groups);
+    db.patchSets().update(Collections.singleton(ps));
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -2423,29 +2423,27 @@ public class ReceiveCommits {
     }
 
     private void updateGroups(RequestState state)
-        throws OrmException, IOException {
+        throws RestApiException, UpdateException {
-      ReviewDb db = state.db;
+      try (ObjectInserter oi = repo.newObjectInserter();
-      PatchSet ps = db.patchSets().atomicUpdate(psId,
+          BatchUpdate bu = batchUpdateFactory.create(state.db,
-          new AtomicUpdate<PatchSet>() {
+              magicBranch.dest.getParentKey(), user, TimeUtil.nowTs())) {
+        bu.addOp(psId.getParentKey(), new BatchUpdate.Op() {
           @Override
-            public PatchSet update(PatchSet ps) {
+          public boolean updateChange(ChangeContext ctx) throws OrmException {
+            PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
             List<String> oldGroups = ps.getGroups();
             if (oldGroups == null) {
               if (groups == null) {
-                  return null;
+                return false;
               }
             } else if (Sets.newHashSet(oldGroups).equals(groups)) {
-                return null;
+              return false;
             }
-              ps.setGroups(groups);
+            psUtil.setGroups(ctx.getDb(), ctx.getUpdate(psId), ps, groups);
-              return ps;
+            return true;
           }
         });
-      if (ps != null) {
+        bu.execute();
-        Change change = db.changes().get(psId.getParentKey());
-        if (change != null) {
-          indexer.index(db, change);
-        }
       }
     }
 
@@ -2454,7 +2452,7 @@ public class ReceiveCommits {
       ListenableFuture<Void> future = changeUpdateExector.submit(
           requestScopePropagator.wrap(new Callable<Void>() {
         @Override
-        public Void call() throws OrmException, IOException {
+        public Void call() throws Exception {
           try (RequestState state = requestState(caller)) {
             updateGroups(state);
           }

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

@@ -30,6 +30,7 @@ public class ChangeNoteUtil {
 
   static final FooterKey FOOTER_BRANCH = new FooterKey("Branch");
   static final FooterKey FOOTER_COMMIT = new FooterKey("Commit");
+  static final FooterKey FOOTER_GROUPS = new FooterKey("Groups");
   static final FooterKey FOOTER_HASHTAGS = new FooterKey("Hashtags");
   static final FooterKey FOOTER_LABEL = new FooterKey("Label");
   static final FooterKey FOOTER_PATCH_SET = new FooterKey("Patch-set");

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

@@ -16,6 +16,7 @@ package com.google.gerrit.server.notedb;
 
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_COMMIT;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_GROUPS;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET;
@@ -84,6 +85,11 @@ import java.util.Set;
 import java.util.TreeMap;
 
 class ChangeNotesParser implements AutoCloseable {
+  // Sentinel RevId indicating a mutable field on a patch set was parsed, but
+  // the parser does not yet know its commit SHA-1.
+  private static final RevId PARTIAL_PATCH_SET =
+      new RevId("INVALID PARTIAL PATCH SET");
+
   final Map<Account.Id, ReviewerStateInternal> reviewers;
   final List<Account.Id> allPastReviewers;
   final List<SubmitRecord> submitRecords;
@@ -226,6 +232,7 @@ class ChangeNotesParser implements AutoCloseable {
     if (currRev != null) {
       parsePatchSet(psId, currRev, accountId, ts);
     }
+    parseGroups(psId, commit);
 
     if (submitRecords.isEmpty()) {
       // Only parse the most recent set of submit records; any older ones are
@@ -299,18 +306,36 @@ class ChangeNotesParser implements AutoCloseable {
 
   private void parsePatchSet(PatchSet.Id psId, ObjectId rev,
       Account.Id accountId, Timestamp ts) throws ConfigInvalidException {
-    if (patchSets.containsKey(psId)) {
+    PatchSet ps = patchSets.get(psId);
+    if (ps == null) {
+      ps = new PatchSet(psId);
+      patchSets.put(psId, ps);
+    } else if (ps.getRevision() != PARTIAL_PATCH_SET) {
       throw new ConfigInvalidException(
           String.format(
               "Multiple revisions parsed for patch set %s: %s and %s",
               psId.get(), patchSets.get(psId).getRevision(), rev.name()));
-    } else {
+    }
-      PatchSet ps = new PatchSet(psId);
     ps.setRevision(new RevId(rev.name()));
     ps.setUploader(accountId);
     ps.setCreatedOn(ts);
-      patchSets.put(psId, ps);
   }
+
+  private void parseGroups(PatchSet.Id psId, RevCommit commit)
+      throws ConfigInvalidException {
+    String groupsStr = parseOneFooter(commit, FOOTER_GROUPS);
+    if (groupsStr == null) {
+      return;
+    }
+    PatchSet ps = patchSets.get(psId);
+    if (ps == null) {
+      ps = new PatchSet(psId);
+      ps.setRevision(PARTIAL_PATCH_SET);
+      patchSets.put(psId, ps);
+    } else if (ps.getGroups() != null) {
+      return;
+    }
+    ps.setGroups(PatchSet.splitGroups(groupsStr));
   }
 
   private void parseHashtags(RevCommit commit) throws ConfigInvalidException {
@@ -634,7 +659,14 @@ class ChangeNotesParser implements AutoCloseable {
     }
   }
 
-  private void updatePatchSetStates() {
+  private void updatePatchSetStates() throws ConfigInvalidException {
+    for (PatchSet ps : patchSets.values()) {
+      if (ps.getRevision() == PARTIAL_PATCH_SET) {
+        throw parseException("No %s found for patch set %s",
+            FOOTER_COMMIT, ps.getPatchSetId());
+      }
+    }
+
     Set<PatchSet.Id> deleted =
         Sets.newHashSetWithExpectedSize(patchSetStates.size());
     for (Map.Entry<PatchSet.Id, PatchSetState> e : patchSetStates.entrySet()) {

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

@@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_COMMIT;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_GROUPS;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET;
@@ -112,6 +113,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   private String changeMessage;
   private ChangeNotes notes;
   private PatchSetState psState;
+  private Iterable<String> groups;
 
   private final ChangeDraftUpdate.Factory draftUpdateFactory;
   private ChangeDraftUpdate draftUpdate;
@@ -390,6 +392,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     this.psState = psState;
   }
 
+  public void setGroups(Iterable<String> groups) {
+    this.groups = groups;
+  }
+
   /** @return the tree id for the updated tree */
   private ObjectId storeCommentsInNotes() throws OrmException, IOException {
     ChangeNotes notes = ctl.getNotes().load();
@@ -495,8 +501,13 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       addFooter(msg, FOOTER_COMMIT, commit.name());
     }
 
+    Joiner comma = Joiner.on(',');
     if (hashtags != null) {
-      addFooter(msg, FOOTER_HASHTAGS, Joiner.on(",").join(hashtags));
+      addFooter(msg, FOOTER_HASHTAGS, comma.join(hashtags));
+    }
+
+    if (groups != null) {
+      addFooter(msg, FOOTER_GROUPS, comma.join(groups));
     }
 
     for (Map.Entry<Account.Id, ReviewerStateInternal> e : reviewers.entrySet()) {
@@ -579,7 +590,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         && hashtags == null
         && topic == null
         && commit == null
-        && psState == null;
+        && psState == null
+        && groups == null;
   }
 
   private static StringBuilder addFooter(StringBuilder sb, FooterKey footer) {

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java

@@ -319,6 +319,32 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
         + "Subject: Some subject of a change\n");
   }
 
+  @Test
+  public void parsePatchSetGroups() throws Exception {
+    assertParseSucceeds("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+        + "Subject: Change subject\n"
+        + "Groups: a,b,c\n");
+    // No patch set commit parsed on which we can set groups.
+    assertParseFails("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Subject: Change subject\n"
+        + "Groups: a,b,c\n");
+    assertParseFails("Update change\n"
+        + "\n"
+        + "Patch-set: 1\n"
+        + "Branch: refs/heads/master\n"
+        + "Commit: abcd1234abcd1234abcd1234abcd1234abcd1234\n"
+        + "Subject: Change subject\n"
+        + "Groups: a,b,c\n"
+        + "Groups: d,e,f\n");
+  }
+
   private RevCommit writeCommit(String body) throws Exception {
     return writeCommit(body, ChangeNoteUtil.newIdent(
         changeOwner.getAccount(), TimeUtil.nowTs(), serverIdent,

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -658,6 +658,36 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(notes.getComments()).isEmpty();
   }
 
+  @Test
+  public void patchSetGroups() throws Exception {
+    Change c = newChange();
+    PatchSet.Id psId1 = c.currentPatchSetId();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getPatchSets().get(psId1).getGroups()).isNull();
+
+    // ps1
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setGroups(ImmutableList.of("a", "b"));
+    update.commit();
+    notes = newNotes(c);
+    assertThat(notes.getPatchSets().get(psId1).getGroups())
+      .containsExactly("a", "b").inOrder();
+
+    // ps2
+    incrementPatchSet(c);
+    PatchSet.Id psId2 = c.currentPatchSetId();
+    update = newUpdate(c, changeOwner);
+    update.setCommit(rw, tr.commit().message("PS2").create());
+    update.setGroups(ImmutableList.of("d"));
+    update.commit();
+    notes = newNotes(c);
+    assertThat(notes.getPatchSets().get(psId2).getGroups())
+      .containsExactly("d");
+    assertThat(notes.getPatchSets().get(psId1).getGroups())
+      .containsExactly("a", "b").inOrder();
+  }
+
   @Test
   public void emptyExceptSubject() throws Exception {
     ChangeUpdate update = newUpdate(newChange(), changeOwner);