Merge changes I10e17e09,I15a68a95,I76efb029,I08e33593

* changes:
  Compare full GroupBundles in GroupRebuilder tests
  GroupBundle: Store sets instead of lists
  GroupBundle: Check columns like we do for ChangeBundle
  Implement group audit log in NoteDb

java/com/google/gerrit/reviewdb/client/AccountGroup.java

@@ -286,4 +286,25 @@ public final class AccountGroup {
     return Objects.hash(
         name, groupId, description, visibleToAll, groupUUID, ownerGroupUUID, createdOn);
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName()
+        + "{"
+        + "name="
+        + name
+        + ", groupId="
+        + groupId
+        + ", description="
+        + description
+        + ", visibleToAll="
+        + visibleToAll
+        + ", groupUUID="
+        + groupUUID
+        + ", ownerGroupUUID="
+        + ownerGroupUUID
+        + ", createdOn="
+        + createdOn
+        + "}";
+  }
 }

java/com/google/gerrit/reviewdb/client/AccountGroupById.java

@@ -88,4 +88,9 @@ public final class AccountGroupById {
   public int hashCode() {
     return key.hashCode();
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName() + "{key=" + key + "}";
+  }
 }

java/com/google/gerrit/reviewdb/client/AccountGroupByIdAud.java

@@ -142,4 +142,19 @@ public final class AccountGroupByIdAud {
   public int hashCode() {
     return Objects.hash(key, addedBy, removedBy, removedOn);
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName()
+        + "{"
+        + "key="
+        + key
+        + ", addedBy="
+        + addedBy
+        + ", removedBy="
+        + removedBy
+        + ", removedOn="
+        + removedOn
+        + "}";
+  }
 }

java/com/google/gerrit/reviewdb/client/AccountGroupMember.java

@@ -84,4 +84,9 @@ public final class AccountGroupMember {
   public int hashCode() {
     return key.hashCode();
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName() + "{key=" + key + "}";
+  }
 }

java/com/google/gerrit/reviewdb/client/AccountGroupMemberAudit.java

@@ -147,4 +147,19 @@ public final class AccountGroupMemberAudit {
   public int hashCode() {
     return Objects.hash(key, addedBy, removedBy, removedOn);
   }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName()
+        + "{"
+        + "key="
+        + key
+        + ", addedBy="
+        + addedBy
+        + ", removedBy="
+        + removedBy
+        + ", removedOn="
+        + removedOn
+        + "}";
+  }
 }

java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java

@@ -14,8 +14,16 @@
 
 package com.google.gerrit.reviewdb.server;
 
+import static com.google.common.base.Preconditions.checkState;
+
 import com.google.common.collect.Ordering;
+import com.google.common.collect.Sets;
+import com.google.gwtorm.client.Column;
 import com.google.gwtorm.client.IntKey;
+import java.lang.reflect.Field;
+import java.util.Arrays;
+import java.util.Set;
+import java.util.TreeSet;
 
 /** Static utilities for ReviewDb types. */
 public class ReviewDbUtil {
@@ -51,5 +59,22 @@ public class ReviewDbUtil {
     return db;
   }
 
+  public static void checkColumns(Class<?> clazz, Integer... expected) {
+    Set<Integer> ids = new TreeSet<>();
+    for (Field f : clazz.getDeclaredFields()) {
+      Column col = f.getAnnotation(Column.class);
+      if (col != null) {
+        ids.add(col.id());
+      }
+    }
+    Set<Integer> expectedIds = Sets.newTreeSet(Arrays.asList(expected));
+    checkState(
+        ids.equals(expectedIds),
+        "Unexpected column set for %s: %s != %s",
+        clazz.getSimpleName(),
+        ids,
+        expectedIds);
+  }
+
   private ReviewDbUtil() {}
 }

java/com/google/gerrit/server/git/CommitUtil.java

@@ -14,14 +14,22 @@
 
 package com.google.gerrit.server.git;
 
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.server.CommonConverters;
+import java.io.IOException;
 import java.util.ArrayList;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 
 /** Static utilities for working with {@link RevCommit}s. */
 public class CommitUtil {
-  public static CommitInfo toCommitInfo(RevCommit commit) {
+  public static CommitInfo toCommitInfo(RevCommit commit) throws IOException {
+    return toCommitInfo(commit, null);
+  }
+
+  public static CommitInfo toCommitInfo(RevCommit commit, @Nullable RevWalk walk)
+      throws IOException {
     CommitInfo info = new CommitInfo();
     info.commit = commit.getName();
     info.author = CommonConverters.toGitPerson(commit.getAuthorIdent());
@@ -30,7 +38,7 @@ public class CommitUtil {
     info.message = commit.getFullMessage();
     info.parents = new ArrayList<>(commit.getParentCount());
     for (int i = 0; i < commit.getParentCount(); i++) {
-      RevCommit p = commit.getParent(i);
+      RevCommit p = walk == null ? commit.getParent(i) : walk.parseCommit(commit.getParent(i));
       CommitInfo parentInfo = new CommitInfo();
       parentInfo.commit = p.getName();
       parentInfo.subject = p.getShortMessage();

java/com/google/gerrit/server/group/GetAuditLog.java

@@ -36,10 +36,12 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
 
 @Singleton
 public class GetAuditLog implements RestReadView<GroupResource> {
@@ -68,7 +70,8 @@ public class GetAuditLog implements RestReadView<GroupResource> {
 
   @Override
   public List<? extends GroupAuditEventInfo> apply(GroupResource rsrc)
-      throws AuthException, MethodNotAllowedException, OrmException {
+      throws AuthException, MethodNotAllowedException, OrmException, IOException,
+          ConfigInvalidException {
     GroupDescription.Internal group =
         rsrc.asInternalGroup().orElseThrow(MethodNotAllowedException::new);
     if (!rsrc.getControl().isOwner()) {
@@ -123,8 +126,9 @@ public class GetAuditLog implements RestReadView<GroupResource> {
 
     accountLoader.fill();
 
-    // sort by date in reverse order so that the newest audit event comes first
+    // sort by date and then reverse so that the newest audit event comes first
-    Collections.sort(auditEvents, comparing((GroupAuditEventInfo a) -> a.date).reversed());
+    Collections.sort(auditEvents, comparing((GroupAuditEventInfo a) -> a.date));
+    Collections.reverse(auditEvents);
 
     return auditEvents;
   }

java/com/google/gerrit/server/group/db/AuditLogReader.java

@@ -0,0 +1,274 @@
+// Copyright (C) 2017 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.group.db;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.MultimapBuilder;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.AccountGroupByIdAud;
+import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.GerritServerId;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.NoteDbUtil;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.FooterLine;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevSort;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.util.RawParseUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** NoteDb reader for group audit log. */
+@Singleton
+class AuditLogReader {
+  private static final Logger log = LoggerFactory.getLogger(AuditLogReader.class);
+
+  private final String serverId;
+  private final GitRepositoryManager repoManager;
+  private final AllUsersName allUsers;
+
+  @Inject
+  AuditLogReader(
+      @GerritServerId String serverId, GitRepositoryManager repoManager, AllUsersName allUsers) {
+    this.serverId = serverId;
+    this.repoManager = repoManager;
+    this.allUsers = allUsers;
+  }
+
+  // Having separate methods for reading the two types of audit records mirrors the split in
+  // ReviewDb. Once ReviewDb is gone, the audit record interface becomes more flexible and we can
+  // revisit this, e.g. to do only a single walk, or even change the record types.
+
+  ImmutableList<AccountGroupMemberAudit> getMembersAudit(AccountGroup.UUID uuid)
+      throws IOException, ConfigInvalidException {
+    return new MembersAuditLogParser().parseAuditLog(uuid);
+  }
+
+  ImmutableList<AccountGroupByIdAud> getSubgroupsAudit(AccountGroup.UUID uuid)
+      throws IOException, ConfigInvalidException {
+    return new SubgroupsAuditLogParser().parseAuditLog(uuid);
+  }
+
+  private Optional<ParsedCommit> parse(AccountGroup.UUID uuid, RevCommit c) {
+    Optional<Account.Id> authorId = NoteDbUtil.parseIdent(c.getAuthorIdent(), serverId);
+    if (!authorId.isPresent()) {
+      // Only report audit events from identified users, since this is a non-nullable field in
+      // ReviewDb. May be revisited after groups are fully migrated to NoteDb.
+      return Optional.empty();
+    }
+
+    List<Account.Id> addedMembers = new ArrayList<>();
+    List<AccountGroup.UUID> addedSubgroups = new ArrayList<>();
+    List<Account.Id> removedMembers = new ArrayList<>();
+    List<AccountGroup.UUID> removedSubgroups = new ArrayList<>();
+
+    for (FooterLine line : c.getFooterLines()) {
+      if (line.matches(GroupConfig.FOOTER_ADD_MEMBER)) {
+        parseAccount(uuid, c, line).ifPresent(addedMembers::add);
+      } else if (line.matches(GroupConfig.FOOTER_REMOVE_MEMBER)) {
+        parseAccount(uuid, c, line).ifPresent(removedMembers::add);
+      } else if (line.matches(GroupConfig.FOOTER_ADD_GROUP)) {
+        parseGroup(uuid, c, line).ifPresent(addedSubgroups::add);
+      } else if (line.matches(GroupConfig.FOOTER_REMOVE_GROUP)) {
+        parseGroup(uuid, c, line).ifPresent(removedSubgroups::add);
+      }
+    }
+    return Optional.of(
+        new AutoValue_AuditLogReader_ParsedCommit(
+            authorId.get(),
+            new Timestamp(c.getAuthorIdent().getWhen().getTime()),
+            ImmutableList.copyOf(addedMembers),
+            ImmutableList.copyOf(removedMembers),
+            ImmutableList.copyOf(addedSubgroups),
+            ImmutableList.copyOf(removedSubgroups)));
+  }
+
+  private Optional<Account.Id> parseAccount(AccountGroup.UUID uuid, RevCommit c, FooterLine line) {
+    Optional<Account.Id> result =
+        Optional.ofNullable(RawParseUtils.parsePersonIdent(line.getValue()))
+            .flatMap(ident -> NoteDbUtil.parseIdent(ident, serverId));
+    if (!result.isPresent()) {
+      logInvalid(uuid, c, line);
+    }
+    return result;
+  }
+
+  private static Optional<AccountGroup.UUID> parseGroup(
+      AccountGroup.UUID uuid, RevCommit c, FooterLine line) {
+    PersonIdent ident = RawParseUtils.parsePersonIdent(line.getValue());
+    if (ident == null) {
+      logInvalid(uuid, c, line);
+      return Optional.empty();
+    }
+    return Optional.of(new AccountGroup.UUID(ident.getEmailAddress()));
+  }
+
+  private static void logInvalid(AccountGroup.UUID uuid, RevCommit c, FooterLine line) {
+    log.debug(
+        "Invalid footer line in commit {} while parsing audit log for group {}: {}",
+        c.name(),
+        uuid,
+        line);
+  }
+
+  private abstract class AuditLogParser<T> {
+    final ImmutableList<T> parseAuditLog(AccountGroup.UUID uuid)
+        throws IOException, ConfigInvalidException {
+      try (Repository repo = repoManager.openRepository(allUsers);
+          RevWalk rw = new RevWalk(repo)) {
+        Ref ref = repo.exactRef(RefNames.refsGroups(uuid));
+        if (ref == null) {
+          return ImmutableList.of();
+        }
+
+        // TODO(dborowitz): This re-walks all commits just to find createdOn, which we don't need.
+        AccountGroup.Id groupId =
+            GroupConfig.loadForGroup(repo, uuid).getLoadedGroup().get().getId();
+
+        rw.reset();
+        rw.markStart(rw.parseCommit(ref.getObjectId()));
+        rw.setRetainBody(true);
+        rw.sort(RevSort.COMMIT_TIME_DESC, true);
+        rw.sort(RevSort.REVERSE, true);
+
+        ImmutableList.Builder<T> result = ImmutableList.builder();
+        RevCommit c;
+        while ((c = rw.next()) != null) {
+          parse(uuid, c).ifPresent(pc -> visit(groupId, pc, result));
+        }
+        return result.build();
+      }
+    }
+
+    protected abstract void visit(
+        AccountGroup.Id groupId, ParsedCommit pc, ImmutableList.Builder<T> result);
+  }
+
+  private class MembersAuditLogParser extends AuditLogParser<AccountGroupMemberAudit> {
+    private ListMultimap<MemberKey, AccountGroupMemberAudit> audits =
+        MultimapBuilder.hashKeys().linkedListValues().build();
+
+    @Override
+    protected void visit(
+        AccountGroup.Id groupId,
+        ParsedCommit pc,
+        ImmutableList.Builder<AccountGroupMemberAudit> result) {
+      for (Account.Id id : pc.addedMembers()) {
+        MemberKey key = MemberKey.create(groupId, id);
+        AccountGroupMemberAudit audit =
+            new AccountGroupMemberAudit(
+                new AccountGroupMemberAudit.Key(id, groupId, pc.when()), pc.authorId());
+        audits.put(key, audit);
+        result.add(audit);
+      }
+      for (Account.Id id : pc.removedMembers()) {
+        List<AccountGroupMemberAudit> adds = audits.get(MemberKey.create(groupId, id));
+        if (!adds.isEmpty()) {
+          AccountGroupMemberAudit audit = adds.remove(0);
+          audit.removed(pc.authorId(), pc.when());
+        } else {
+          // Match old behavior of DbGroupMemberAuditListener and add a "legacy" add/remove pair.
+          AccountGroupMemberAudit audit =
+              new AccountGroupMemberAudit(
+                  new AccountGroupMemberAudit.Key(id, groupId, pc.when()), pc.authorId());
+          audit.removedLegacy();
+          result.add(audit);
+        }
+      }
+    }
+  }
+
+  private class SubgroupsAuditLogParser extends AuditLogParser<AccountGroupByIdAud> {
+    private ListMultimap<SubgroupKey, AccountGroupByIdAud> audits =
+        MultimapBuilder.hashKeys().linkedListValues().build();
+
+    @Override
+    protected void visit(
+        AccountGroup.Id groupId,
+        ParsedCommit pc,
+        ImmutableList.Builder<AccountGroupByIdAud> result) {
+      for (AccountGroup.UUID uuid : pc.addedSubgroups()) {
+        SubgroupKey key = SubgroupKey.create(groupId, uuid);
+        AccountGroupByIdAud audit =
+            new AccountGroupByIdAud(
+                new AccountGroupByIdAud.Key(groupId, uuid, pc.when()), pc.authorId());
+        audits.put(key, audit);
+        result.add(audit);
+      }
+      for (AccountGroup.UUID uuid : pc.removedSubgroups()) {
+        List<AccountGroupByIdAud> adds = audits.get(SubgroupKey.create(groupId, uuid));
+        if (!adds.isEmpty()) {
+          AccountGroupByIdAud audit = adds.remove(0);
+          audit.removed(pc.authorId(), pc.when());
+        } else {
+          // Unlike members, DbGroupMemberAuditListener didn't insert an add/remove pair here.
+        }
+      }
+    }
+  }
+
+  @AutoValue
+  abstract static class MemberKey {
+    static MemberKey create(AccountGroup.Id groupId, Account.Id memberId) {
+      return new AutoValue_AuditLogReader_MemberKey(groupId, memberId);
+    }
+
+    abstract AccountGroup.Id groupId();
+
+    abstract Account.Id memberId();
+  }
+
+  @AutoValue
+  abstract static class SubgroupKey {
+    static SubgroupKey create(AccountGroup.Id groupId, AccountGroup.UUID subgroupUuid) {
+      return new AutoValue_AuditLogReader_SubgroupKey(groupId, subgroupUuid);
+    }
+
+    abstract AccountGroup.Id groupId();
+
+    abstract AccountGroup.UUID subgroupUuid();
+  }
+
+  @AutoValue
+  abstract static class ParsedCommit {
+    abstract Account.Id authorId();
+
+    abstract Timestamp when();
+
+    abstract ImmutableList<Account.Id> addedMembers();
+
+    abstract ImmutableList<Account.Id> removedMembers();
+
+    abstract ImmutableList<AccountGroup.UUID> addedSubgroups();
+
+    abstract ImmutableList<AccountGroup.UUID> removedSubgroups();
+  }
+}

java/com/google/gerrit/server/group/db/GroupBundle.java

@@ -14,11 +14,11 @@
 
 package com.google.gerrit.server.group.db;
 
-import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.reviewdb.server.ReviewDbUtil.checkColumns;
 
 import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.AccountGroupById;
@@ -28,6 +28,11 @@ import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Repository;
 
 /**
  * A bundle of all entities rooted at a single {@link AccountGroup} entity.
@@ -37,17 +42,84 @@ import com.google.gwtorm.server.OrmException;
  */
 @AutoValue
 public abstract class GroupBundle {
-  public static GroupBundle fromReviewDb(ReviewDb db, AccountGroup.Id id) throws OrmException {
+  static {
-    AccountGroup group = db.accountGroups().get(id);
+    // Initialization-time checks that the column set hasn't changed since the
-    if (group == null) {
+    // last time this file was updated.
-      throw new OrmException("Group " + id + " not found");
+    checkColumns(AccountGroup.NameKey.class, 1);
+    checkColumns(AccountGroup.UUID.class, 1);
+    checkColumns(AccountGroup.Id.class, 1);
+    checkColumns(AccountGroup.class, 1, 2, 4, 7, 9, 10, 11);
+
+    checkColumns(AccountGroupById.Key.class, 1, 2);
+    checkColumns(AccountGroupById.class, 1);
+
+    checkColumns(AccountGroupByIdAud.Key.class, 1, 2, 3);
+    checkColumns(AccountGroupByIdAud.class, 1, 2, 3, 4);
+
+    checkColumns(AccountGroupMember.Key.class, 1, 2);
+    checkColumns(AccountGroupMember.class, 1);
+
+    checkColumns(AccountGroupMemberAudit.Key.class, 1, 2, 3);
+    checkColumns(AccountGroupMemberAudit.class, 1, 2, 3, 4);
+  }
+
+  @Singleton
+  public static class Factory {
+    private final AuditLogReader auditLogReader;
+
+    @Inject
+    Factory(AuditLogReader auditLogReader) {
+      this.auditLogReader = auditLogReader;
+    }
+
+    public GroupBundle fromReviewDb(ReviewDb db, AccountGroup.Id id) throws OrmException {
+      AccountGroup group = db.accountGroups().get(id);
+      if (group == null) {
+        throw new OrmException("Group " + id + " not found");
+      }
+      return create(
+          group,
+          db.accountGroupMembers().byGroup(id),
+          db.accountGroupMembersAudit().byGroup(id),
+          db.accountGroupById().byGroup(id),
+          db.accountGroupByIdAud().byGroup(id));
+    }
+
+    public GroupBundle fromNoteDb(Repository repo, AccountGroup.UUID uuid)
+        throws ConfigInvalidException, IOException {
+      GroupConfig groupConfig = GroupConfig.loadForGroup(repo, uuid);
+      InternalGroup internalGroup = groupConfig.getLoadedGroup().get();
+      AccountGroup.Id groupId = internalGroup.getId();
+
+      AccountGroup accountGroup =
+          new AccountGroup(
+              internalGroup.getNameKey(),
+              internalGroup.getId(),
+              internalGroup.getGroupUUID(),
+              internalGroup.getCreatedOn());
+      accountGroup.setDescription(internalGroup.getDescription());
+      accountGroup.setOwnerGroupUUID(internalGroup.getOwnerGroupUUID());
+      accountGroup.setVisibleToAll(internalGroup.isVisibleToAll());
+
+      return create(
+          accountGroup,
+          internalGroup
+              .getMembers()
+              .stream()
+              .map(
+                  accountId ->
+                      new AccountGroupMember(new AccountGroupMember.Key(accountId, groupId)))
+              .collect(toImmutableSet()),
+          auditLogReader.getMembersAudit(uuid),
+          internalGroup
+              .getSubgroups()
+              .stream()
+              .map(
+                  subgroupUuid ->
+                      new AccountGroupById(new AccountGroupById.Key(groupId, subgroupUuid)))
+              .collect(toImmutableSet()),
+          auditLogReader.getSubgroupsAudit(uuid));
     }
-    return create(
-        group,
-        db.accountGroupMembers().byGroup(id),
-        db.accountGroupMembersAudit().byGroup(id),
-        db.accountGroupById().byGroup(id),
-        db.accountGroupByIdAud().byGroup(id));
   }
 
   public static GroupBundle create(
@@ -79,13 +151,13 @@ public abstract class GroupBundle {
 
   public abstract AccountGroup group();
 
-  public abstract ImmutableList<AccountGroupMember> members();
+  public abstract ImmutableSet<AccountGroupMember> members();
 
-  public abstract ImmutableList<AccountGroupMemberAudit> memberAudit();
+  public abstract ImmutableSet<AccountGroupMemberAudit> memberAudit();
 
-  public abstract ImmutableList<AccountGroupById> byId();
+  public abstract ImmutableSet<AccountGroupById> byId();
 
-  public abstract ImmutableList<AccountGroupByIdAud> byIdAudit();
+  public abstract ImmutableSet<AccountGroupByIdAud> byIdAudit();
 
   public abstract Builder toBuilder();
 
@@ -97,8 +169,8 @@ public abstract class GroupBundle {
     return toBuilder()
         .group(newGroup)
         .memberAudit(
-            memberAudit().stream().map(GroupBundle::roundToSecond).collect(toImmutableList()))
+            memberAudit().stream().map(GroupBundle::roundToSecond).collect(toImmutableSet()))
-        .byIdAudit(byIdAudit().stream().map(GroupBundle::roundToSecond).collect(toImmutableList()))
+        .byIdAudit(byIdAudit().stream().map(GroupBundle::roundToSecond).collect(toImmutableSet()))
         .build();
   }
 

java/com/google/gerrit/server/group/db/GroupConfig.java

@@ -44,12 +44,19 @@ import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.FooterKey;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevSort;
 
 // TODO(aliceks): Add Javadoc descriptions to this file.
 public class GroupConfig extends VersionedMetaData {
   public static final String GROUP_CONFIG_FILE = "group.config";
+
+  static final FooterKey FOOTER_ADD_MEMBER = new FooterKey("Add");
+  static final FooterKey FOOTER_REMOVE_MEMBER = new FooterKey("Remove");
+  static final FooterKey FOOTER_ADD_GROUP = new FooterKey("Add-group");
+  static final FooterKey FOOTER_REMOVE_GROUP = new FooterKey("Remove-group");
+
   private static final String MEMBERS_FILE = "members";
   private static final String SUBGROUPS_FILE = "subgroups";
   private static final Pattern LINE_SEPARATOR_PATTERN = Pattern.compile("\\R");
@@ -332,12 +339,12 @@ public class GroupConfig extends VersionedMetaData {
         Sets.difference(oldMembers, newMembers)
             .stream()
             .map(accountNameEmailRetriever)
-            .map("Remove: "::concat);
+            .map((FOOTER_REMOVE_MEMBER.getName() + ": ")::concat);
     Stream<String> addedMembers =
         Sets.difference(newMembers, oldMembers)
             .stream()
             .map(accountNameEmailRetriever)
-            .map("Add: "::concat);
+            .map((FOOTER_ADD_MEMBER.getName() + ": ")::concat);
     return Stream.concat(removedMembers, addedMembers);
   }
 
@@ -347,12 +354,12 @@ public class GroupConfig extends VersionedMetaData {
         Sets.difference(oldSubgroups, newSubgroups)
             .stream()
             .map(groupNameRetriever)
-            .map("Remove-group: "::concat);
+            .map((FOOTER_REMOVE_GROUP.getName() + ": ")::concat);
     Stream<String> addedMembers =
         Sets.difference(newSubgroups, oldSubgroups)
             .stream()
             .map(groupNameRetriever)
-            .map("Add-group: "::concat);
+            .map((FOOTER_ADD_GROUP.getName() + ": ")::concat);
     return Stream.concat(removedMembers, addedMembers);
   }
 }

java/com/google/gerrit/server/group/db/Groups.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.group.db;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static java.util.Comparator.comparing;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -39,6 +40,7 @@ import com.google.gwtorm.server.ResultSet;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.stream.Stream;
@@ -63,15 +65,18 @@ public class Groups {
   private final GroupsMigration groupsMigration;
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsersName;
+  private final AuditLogReader auditLogReader;
 
   @Inject
   public Groups(
       GroupsMigration groupsMigration,
       GitRepositoryManager repoManager,
-      AllUsersName allUsersName) {
+      AllUsersName allUsersName,
+      AuditLogReader auditLogReader) {
     this.groupsMigration = groupsMigration;
     this.repoManager = repoManager;
     this.allUsersName = allUsersName;
+    this.auditLogReader = auditLogReader;
   }
 
   /**
@@ -311,18 +316,23 @@ public class Groups {
    * @param groupUuid the UUID of the group
    * @return the audit records, in arbitrary order; empty if the group does not exist
    * @throws OrmException if an error occurs while reading from ReviewDb
+   * @throws IOException if an error occurs while reading from NoteDb
+   * @throws ConfigInvalidException if the group couldn't be retrieved from NoteDb
    */
   public List<AccountGroupMemberAudit> getMembersAudit(ReviewDb db, AccountGroup.UUID groupUuid)
-      throws OrmException {
+      throws OrmException, IOException, ConfigInvalidException {
     if (groupsMigration.readFromNoteDb()) {
-      // TODO(dborowitz): Implement.
+      return auditLogReader.getMembersAudit(groupUuid);
-      throw new OrmException("Audit logs not yet implemented in NoteDb");
     }
     Optional<AccountGroup> group = getGroupFromReviewDb(db, groupUuid);
     if (!group.isPresent()) {
       return ImmutableList.of();
     }
-    return db.accountGroupMembersAudit().byGroup(group.get().getId()).toList();
+
+    List<AccountGroupMemberAudit> audits =
+        db.accountGroupMembersAudit().byGroup(group.get().getId()).toList();
+    Collections.sort(audits, comparing((AccountGroupMemberAudit a) -> a.getAddedOn()));
+    return audits;
   }
 
   /**
@@ -332,17 +342,22 @@ public class Groups {
    * @param groupUuid the UUID of the group
    * @return the audit records, in arbitrary order; empty if the group does not exist
    * @throws OrmException if an error occurs while reading from ReviewDb
+   * @throws IOException if an error occurs while reading from NoteDb
+   * @throws ConfigInvalidException if the group couldn't be retrieved from NoteDb
    */
   public List<AccountGroupByIdAud> getSubgroupsAudit(ReviewDb db, AccountGroup.UUID groupUuid)
-      throws OrmException {
+      throws OrmException, IOException, ConfigInvalidException {
     if (groupsMigration.readFromNoteDb()) {
-      // TODO(dborowitz): Implement.
+      return auditLogReader.getSubgroupsAudit(groupUuid);
-      throw new OrmException("Audit logs not yet implemented in NoteDb");
     }
     Optional<AccountGroup> group = getGroupFromReviewDb(db, groupUuid);
     if (!group.isPresent()) {
       return ImmutableList.of();
     }
-    return db.accountGroupByIdAud().byGroup(group.get().getId()).toList();
+
+    List<AccountGroupByIdAud> audits =
+        db.accountGroupByIdAud().byGroup(group.get().getId()).toList();
+    Collections.sort(audits, comparing((AccountGroupByIdAud a) -> a.getAddedOn()));
+    return audits;
   }
 }

java/com/google/gerrit/server/group/db/testing/GroupTestUtil.java

@@ -24,6 +24,7 @@ import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.git.CommitUtil;
+import java.io.IOException;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.notes.Note;
@@ -55,7 +56,17 @@ public class GroupTestUtil {
       if (ref != null) {
         rw.sort(RevSort.REVERSE);
         rw.markStart(rw.parseCommit(ref.getObjectId()));
-        return Streams.stream(rw).map(CommitUtil::toCommitInfo).collect(toImmutableList());
+        return Streams.stream(rw)
+            .map(
+                c -> {
+                  try {
+                    return CommitUtil.toCommitInfo(c);
+                  } catch (IOException e) {
+                    throw new IllegalStateException(
+                        "unexpected state when converting commit " + c.getName(), e);
+                  }
+                })
+            .collect(toImmutableList());
       }
     }
     return ImmutableList.of();

java/com/google/gerrit/server/notedb/ChangeBundle.java

@@ -17,8 +17,8 @@ package com.google.gerrit.server.notedb;
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.common.TimeUtil.roundToSecond;
+import static com.google.gerrit.reviewdb.server.ReviewDbUtil.checkColumns;
 import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering;
 import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB;
 import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB;
@@ -71,7 +71,6 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
-import java.util.TreeSet;
 
 /**
  * A bundle of all entities rooted at a single {@link Change} entity.
@@ -212,23 +211,6 @@ public class ChangeBundle {
         .compare(a.get(), b.get());
   }
 
-  private static void checkColumns(Class<?> clazz, Integer... expected) {
-    Set<Integer> ids = new TreeSet<>();
-    for (Field f : clazz.getDeclaredFields()) {
-      Column col = f.getAnnotation(Column.class);
-      if (col != null) {
-        ids.add(col.id());
-      }
-    }
-    Set<Integer> expectedIds = Sets.newTreeSet(Arrays.asList(expected));
-    checkState(
-        ids.equals(expectedIds),
-        "Unexpected column set for %s: %s != %s",
-        clazz.getSimpleName(),
-        ids,
-        expectedIds);
-  }
-
   static {
     // Initialization-time checks that the column set hasn't changed since the
     // last time this file was updated.

java/com/google/gerrit/server/project/GetCommit.java

@@ -18,12 +18,13 @@ import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.gerrit.server.git.CommitUtil;
 import com.google.inject.Singleton;
+import java.io.IOException;
 
 @Singleton
 public class GetCommit implements RestReadView<CommitResource> {
 
   @Override
-  public CommitInfo apply(CommitResource rsrc) {
+  public CommitInfo apply(CommitResource rsrc) throws IOException {
     return CommitUtil.toCommitInfo(rsrc.getCommit());
   }
 }

javatests/com/google/gerrit/acceptance/api/group/GroupRebuilderIT.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.acceptance.api.group;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.common.truth.Truth8.assertThat;
 import static com.google.gerrit.extensions.common.testing.CommitInfoSubject.assertThat;
 import static com.google.gerrit.server.notedb.NoteDbTable.GROUPS;
 import static com.google.gerrit.server.notedb.NotesMigration.READ;
@@ -25,19 +24,15 @@ import com.google.common.collect.ImmutableList;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.TestAccount;
-import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.extensions.common.GroupInfo;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.config.GerritServerId;
 import com.google.gerrit.server.git.CommitUtil;
-import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.ServerInitiated;
 import com.google.gerrit.server.group.db.GroupBundle;
-import com.google.gerrit.server.group.db.GroupConfig;
 import com.google.gerrit.server.group.db.GroupRebuilder;
-import com.google.gerrit.server.group.db.Groups;
 import com.google.gerrit.server.group.db.GroupsUpdate;
 import com.google.gerrit.testing.ConfigSuite;
 import com.google.gerrit.testing.TestTimeUtil;
@@ -47,7 +42,6 @@ import com.google.inject.Provider;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
-import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
@@ -74,8 +68,8 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
 
   @Inject @GerritServerId private String serverId;
   @Inject @ServerInitiated private Provider<GroupsUpdate> groupsUpdate;
+  @Inject private GroupBundle.Factory bundleFactory;
   @Inject private GroupRebuilder rebuilder;
-  @Inject private Groups groups;
 
   @Before
   public void setTimeForTesting() {
@@ -91,10 +85,11 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
   public void basicGroupProperties() throws Exception {
     GroupInfo createdGroup = gApi.groups().create(name("group")).get();
     try (BlockReviewDbUpdatesForGroups ctx = new BlockReviewDbUpdatesForGroups()) {
-      InternalGroup reviewDbGroup =
+      GroupBundle reviewDbBundle =
-          groups.getGroup(db, new AccountGroup.UUID(createdGroup.id)).get();
+          bundleFactory.fromReviewDb(db, new AccountGroup.Id(createdGroup.groupId));
-      deleteGroupRefs(reviewDbGroup);
+      deleteGroupRefs(reviewDbBundle);
-      assertThat(removeRefState(rebuild(reviewDbGroup))).isEqualTo(roundToSecond(reviewDbGroup));
+
+      assertThat(rebuild(reviewDbBundle)).isEqualTo(reviewDbBundle.roundToSecond());
     }
   }
 
@@ -111,11 +106,12 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     gApi.groups().id(group1.id).addGroups(group2.id);
 
     try (BlockReviewDbUpdatesForGroups ctx = new BlockReviewDbUpdatesForGroups()) {
-      InternalGroup reviewDbGroup = groups.getGroup(db, new AccountGroup.UUID(group1.id)).get();
+      GroupBundle reviewDbBundle =
-      deleteGroupRefs(reviewDbGroup);
+          bundleFactory.fromReviewDb(db, new AccountGroup.Id(group1.groupId));
+      deleteGroupRefs(reviewDbBundle);
 
-      InternalGroup noteDbGroup = rebuild(reviewDbGroup);
+      GroupBundle noteDbBundle = rebuild(reviewDbBundle);
-      assertThat(removeRefState(noteDbGroup)).isEqualTo(roundToSecond(reviewDbGroup));
+      assertThat(noteDbBundle).isEqualTo(reviewDbBundle.roundToSecond());
 
       ImmutableList<CommitInfo> log = log(group1);
       assertThat(log).hasSize(4);
@@ -123,7 +119,7 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
       assertThat(log.get(0)).message().isEqualTo("Create group");
       assertThat(log.get(0)).author().name().isEqualTo(serverIdent.get().getName());
       assertThat(log.get(0)).author().email().isEqualTo(serverIdent.get().getEmailAddress());
-      assertThat(log.get(0)).author().date().isEqualTo(noteDbGroup.getCreatedOn());
+      assertThat(log.get(0)).author().date().isEqualTo(noteDbBundle.group().getCreatedOn());
       assertThat(log.get(0)).author().tz().isEqualTo(serverIdent.get().getTimeZoneOffset());
       assertThat(log.get(0)).committer().isEqualTo(log.get(0).author);
 
@@ -154,13 +150,9 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     }
   }
 
-  private static InternalGroup removeRefState(InternalGroup group) throws Exception {
+  private void deleteGroupRefs(GroupBundle bundle) throws Exception {
-    return group.toBuilder().setRefState(null).build();
-  }
-
-  private void deleteGroupRefs(InternalGroup group) throws Exception {
     try (Repository repo = repoManager.openRepository(allUsers)) {
-      String refName = RefNames.refsGroups(group.getGroupUUID());
+      String refName = RefNames.refsGroups(bundle.uuid());
       RefUpdate ru = repo.updateRef(refName);
       ru.setForceUpdate(true);
       Ref oldRef = repo.exactRef(refName);
@@ -173,30 +165,13 @@ public class GroupRebuilderIT extends AbstractDaemonTest {
     }
   }
 
-  private InternalGroup rebuild(InternalGroup group) throws Exception {
+  private GroupBundle rebuild(GroupBundle reviewDbBundle) throws Exception {
     try (Repository repo = repoManager.openRepository(allUsers)) {
-      rebuilder.rebuild(repo, GroupBundle.fromReviewDb(db, group.getId()), null);
+      rebuilder.rebuild(repo, reviewDbBundle, null);
-      GroupConfig groupConfig = GroupConfig.loadForGroup(repo, group.getGroupUUID());
+      return bundleFactory.fromNoteDb(repo, reviewDbBundle.uuid());
-      Optional<InternalGroup> result = groupConfig.getLoadedGroup();
-      assertThat(result).isPresent();
-      return result.get();
     }
   }
 
-  private InternalGroup roundToSecond(InternalGroup g) {
-    return InternalGroup.builder()
-        .setId(g.getId())
-        .setNameKey(g.getNameKey())
-        .setDescription(g.getDescription())
-        .setOwnerGroupUUID(g.getOwnerGroupUUID())
-        .setVisibleToAll(g.isVisibleToAll())
-        .setGroupUUID(g.getGroupUUID())
-        .setCreatedOn(TimeUtil.roundToSecond(g.getCreatedOn()))
-        .setMembers(g.getMembers())
-        .setSubgroups(g.getSubgroups())
-        .build();
-  }
-
   private ImmutableList<CommitInfo> log(GroupInfo g) throws Exception {
     ImmutableList.Builder<CommitInfo> result = ImmutableList.builder();
     List<Date> commitDates = new ArrayList<>();

javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java

@@ -778,7 +778,6 @@ public class GroupsIT extends AbstractDaemonTest {
 
   @Test
   public void getAuditLog() throws Exception {
-    assume().that(cfg.getBoolean(SECTION_NOTE_DB, GROUPS.key(), READ, false)).isFalse();
     GroupApi g = gApi.groups().create(name("group"));
     List<? extends GroupAuditEventInfo> auditEvents = g.auditLog();
     assertThat(auditEvents).hasSize(1);
@@ -806,10 +805,30 @@ public class GroupsIT extends AbstractDaemonTest {
     assertThat(auditEvents).hasSize(5);
     assertAuditEvent(auditEvents.get(0), Type.REMOVE_GROUP, admin.id, otherGroup);
 
+    /**
+     * Make sure the new commit is created in a different second. This is added for NoteDb since the
+     * resolution of Timestamp is 1s there. Adding here is enough because the sort used in {@code
+     * GetAuditLog} is stable and we process {@code AccountGroupMemberAudit} before {@code
+     * AccountGroupByIdAud}.
+     */
+    Thread.sleep(1000);
+
+    // Add a removed member back again.
+    g.addMembers(user.username);
+    auditEvents = g.auditLog();
+    assertThat(auditEvents).hasSize(6);
+    assertAuditEvent(auditEvents.get(0), Type.ADD_USER, admin.id, user.id);
+
+    // Add a removed group back again.
+    g.addGroups(otherGroup);
+    auditEvents = g.auditLog();
+    assertThat(auditEvents).hasSize(7);
+    assertAuditEvent(auditEvents.get(0), Type.ADD_GROUP, admin.id, otherGroup);
+
     Timestamp lastDate = null;
     for (GroupAuditEventInfo auditEvent : auditEvents) {
       if (lastDate != null) {
-        assertThat(lastDate).isGreaterThan(auditEvent.date);
+        assertThat(lastDate).isAtLeast(auditEvent.date);
       }
       lastDate = auditEvent.date;
     }

javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java

@@ -0,0 +1,143 @@
+// Copyright (C) 2017 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.group.db;
+
+import static com.google.gerrit.extensions.common.testing.CommitInfoSubject.assertThat;
+
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.extensions.common.CommitInfo;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.AllUsersNameProvider;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.CommitUtil;
+import com.google.gerrit.server.git.MetaDataUpdate;
+import com.google.gerrit.testing.GerritBaseTests;
+import com.google.gerrit.testing.InMemoryRepositoryManager;
+import java.sql.Timestamp;
+import java.util.TimeZone;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Ignore;
+
+@Ignore
+public class AbstractGroupTest extends GerritBaseTests {
+  protected static final TimeZone TZ = TimeZone.getTimeZone("America/Los_Angeles");
+  protected static final String SERVER_ID = "server-id";
+  protected static final String SERVER_NAME = "Gerrit Server";
+  protected static final String SERVER_EMAIL = "noreply@gerritcodereview.com";
+  protected static final int SERVER_ACCOUNT_NUMBER = 100000;
+  protected static final int USER_ACCOUNT_NUMBER = 100001;
+
+  protected AllUsersName allUsersName;
+  protected InMemoryRepositoryManager repoManager;
+  protected Repository allUsersRepo;
+  protected Account.Id serverAccountId;
+  protected PersonIdent serverIdent;
+  protected Account.Id userId;
+  protected PersonIdent userIdent;
+
+  @Before
+  public void abstractGroupTestSetUp() throws Exception {
+    allUsersName = new AllUsersName(AllUsersNameProvider.DEFAULT);
+    repoManager = new InMemoryRepositoryManager();
+    allUsersRepo = repoManager.createRepository(allUsersName);
+    serverAccountId = new Account.Id(SERVER_ACCOUNT_NUMBER);
+    serverIdent = new PersonIdent(SERVER_NAME, SERVER_EMAIL, TimeUtil.nowTs(), TZ);
+    userId = new Account.Id(USER_ACCOUNT_NUMBER);
+    userIdent = newPersonIdent(userId, serverIdent);
+  }
+
+  @After
+  public void abstractGroupTestTearDown() throws Exception {
+    allUsersRepo.close();
+  }
+
+  protected Timestamp getTipTimestamp(AccountGroup.UUID uuid) throws Exception {
+    try (RevWalk rw = new RevWalk(allUsersRepo)) {
+      Ref ref = allUsersRepo.exactRef(RefNames.refsGroups(uuid));
+      return ref == null
+          ? null
+          : new Timestamp(rw.parseCommit(ref.getObjectId()).getAuthorIdent().getWhen().getTime());
+    }
+  }
+
+  protected void assertTipCommit(AccountGroup.UUID uuid, String expectedMessage) throws Exception {
+    try (RevWalk rw = new RevWalk(allUsersRepo)) {
+      Ref ref = allUsersRepo.exactRef(RefNames.refsGroups(uuid));
+      assertCommit(
+          CommitUtil.toCommitInfo(rw.parseCommit(ref.getObjectId()), rw),
+          expectedMessage,
+          getAccountName(userId),
+          getAccountEmail(userId));
+    }
+  }
+
+  protected static void assertServerCommit(CommitInfo commitInfo, String expectedMessage) {
+    assertCommit(commitInfo, expectedMessage, SERVER_NAME, SERVER_EMAIL);
+  }
+
+  protected static void assertCommit(
+      CommitInfo commitInfo, String expectedMessage, String expectedName, String expectedEmail) {
+    assertThat(commitInfo).message().isEqualTo(expectedMessage);
+    assertThat(commitInfo).author().name().isEqualTo(expectedName);
+    assertThat(commitInfo).author().email().isEqualTo(expectedEmail);
+
+    // Committer should always be the server, regardless of author.
+    assertThat(commitInfo).committer().name().isEqualTo(SERVER_NAME);
+    assertThat(commitInfo).committer().email().isEqualTo(SERVER_EMAIL);
+    assertThat(commitInfo).committer().date().isEqualTo(commitInfo.author.date);
+    assertThat(commitInfo).committer().tz().isEqualTo(commitInfo.author.tz);
+  }
+
+  protected MetaDataUpdate createMetaDataUpdate(PersonIdent authorIdent) {
+    MetaDataUpdate md =
+        new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, allUsersRepo);
+    md.getCommitBuilder().setAuthor(authorIdent);
+    md.getCommitBuilder().setCommitter(serverIdent); // Committer is always the server identity.
+    return md;
+  }
+
+  protected static PersonIdent newPersonIdent() {
+    return new PersonIdent(SERVER_NAME, SERVER_EMAIL, TimeUtil.nowTs(), TZ);
+  }
+
+  protected static PersonIdent newPersonIdent(Account.Id id, PersonIdent ident) {
+    return new PersonIdent(
+        getAccountName(id), getAccountEmail(id), ident.getWhen(), ident.getTimeZone());
+  }
+
+  protected static String getAccountNameEmail(Account.Id id) {
+    return String.format("%s <%s>", getAccountName(id), getAccountEmail(id));
+  }
+
+  protected static String getGroupName(AccountGroup.UUID uuid) {
+    return String.format("Group <%s>", uuid);
+  }
+
+  protected static String getAccountName(Account.Id id) {
+    return "Account " + id;
+  }
+
+  protected static String getAccountEmail(Account.Id id) {
+    return String.format("%s@%s", id, SERVER_ID);
+  }
+}

javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java

@@ -0,0 +1,381 @@
+// Copyright (C) 2017 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.group.db;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.AccountGroupByIdAud;
+import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
+import com.google.gerrit.server.account.GroupUUID;
+import com.google.gerrit.server.git.CommitUtil;
+import com.google.gerrit.server.group.InternalGroup;
+import java.sql.Timestamp;
+import java.util.Set;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Unit tests for {@link AuditLogReader}. */
+public final class AuditLogReaderTest extends AbstractGroupTest {
+
+  private AuditLogReader auditLogReader;
+
+  @Before
+  public void setUp() throws Exception {
+    auditLogReader = new AuditLogReader(SERVER_ID, repoManager, allUsersName);
+  }
+
+  @Test
+  public void createGroupAsUserIdent() throws Exception {
+    InternalGroup group = createGroupAsUser(1, "test-group");
+    AccountGroup.UUID uuid = group.getGroupUUID();
+
+    AccountGroupMemberAudit expAudit =
+        createExpMemberAudit(group.getId(), userId, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getMembersAudit(uuid)).containsExactly(expAudit);
+  }
+
+  @Test
+  public void createGroupAsServerIdent() throws Exception {
+    InternalGroup group = createGroup(1, "test-group", serverIdent, null);
+    assertThat(auditLogReader.getMembersAudit(group.getGroupUUID())).hasSize(0);
+  }
+
+  @Test
+  public void addAndRemoveMember() throws Exception {
+    InternalGroup group = createGroupAsUser(1, "test-group");
+    AccountGroup.UUID uuid = group.getGroupUUID();
+
+    AccountGroupMemberAudit expAudit1 =
+        createExpMemberAudit(group.getId(), userId, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getMembersAudit(uuid)).containsExactly(expAudit1);
+
+    // User adds account 100002 to the group.
+    Account.Id id = new Account.Id(100002);
+    addMembers(uuid, ImmutableSet.of(id));
+
+    AccountGroupMemberAudit expAudit2 =
+        createExpMemberAudit(group.getId(), id, userId, getTipTimestamp(uuid));
+    assertTipCommit(uuid, "Update group\n\nAdd: Account 100002 <100002@server-id>");
+    assertThat(auditLogReader.getMembersAudit(uuid))
+        .containsExactly(expAudit1, expAudit2)
+        .inOrder();
+
+    // User removes account 100002 from the group.
+    removeMembers(uuid, ImmutableSet.of(id));
+    assertTipCommit(uuid, "Update group\n\nRemove: Account 100002 <100002@server-id>");
+
+    expAudit2.removed(userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getMembersAudit(uuid))
+        .containsExactly(expAudit1, expAudit2)
+        .inOrder();
+  }
+
+  @Test
+  public void addMultiMembers() throws Exception {
+    InternalGroup group = createGroupAsUser(1, "test-group");
+    AccountGroup.Id groupId = group.getId();
+    AccountGroup.UUID uuid = group.getGroupUUID();
+
+    AccountGroupMemberAudit expAudit1 =
+        createExpMemberAudit(groupId, userId, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getMembersAudit(uuid)).containsExactly(expAudit1);
+
+    Account.Id id1 = new Account.Id(100002);
+    Account.Id id2 = new Account.Id(100003);
+    addMembers(uuid, ImmutableSet.of(id1, id2));
+
+    AccountGroupMemberAudit expAudit2 =
+        createExpMemberAudit(groupId, id1, userId, getTipTimestamp(uuid));
+    AccountGroupMemberAudit expAudit3 =
+        createExpMemberAudit(groupId, id2, userId, getTipTimestamp(uuid));
+
+    assertTipCommit(
+        uuid,
+        "Update group\n"
+            + "\n"
+            + "Add: Account 100002 <100002@server-id>\n"
+            + "Add: Account 100003 <100003@server-id>");
+    assertThat(auditLogReader.getMembersAudit(uuid))
+        .containsExactly(expAudit1, expAudit2, expAudit3)
+        .inOrder();
+  }
+
+  @Test
+  public void addAndRemoveSubgroups() throws Exception {
+    InternalGroup group = createGroupAsUser(1, "test-group");
+    AccountGroup.UUID uuid = group.getGroupUUID();
+
+    InternalGroup subgroup = createGroupAsUser(2, "test-group-2");
+    AccountGroup.UUID subgroupUuid = subgroup.getGroupUUID();
+
+    addSubgroups(uuid, ImmutableSet.of(subgroupUuid));
+    assertTipCommit(uuid, String.format("Update group\n\nAdd-group: Group <%s>", subgroupUuid));
+
+    AccountGroupByIdAud expAudit =
+        createExpGroupAudit(group.getId(), subgroupUuid, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getSubgroupsAudit(uuid)).containsExactly(expAudit);
+
+    removeSubgroups(uuid, ImmutableSet.of(subgroupUuid));
+    assertTipCommit(uuid, String.format("Update group\n\nRemove-group: Group <%s>", subgroupUuid));
+
+    expAudit.removed(userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getSubgroupsAudit(uuid)).containsExactly(expAudit);
+  }
+
+  @Test
+  public void addMultiSubgroups() throws Exception {
+    InternalGroup group = createGroupAsUser(1, "test-group");
+    AccountGroup.UUID uuid = group.getGroupUUID();
+
+    InternalGroup subgroup1 = createGroupAsUser(2, "test-group-2");
+    InternalGroup subgroup2 = createGroupAsUser(3, "test-group-3");
+    AccountGroup.UUID subgroupUuid1 = subgroup1.getGroupUUID();
+    AccountGroup.UUID subgroupUuid2 = subgroup2.getGroupUUID();
+
+    addSubgroups(uuid, ImmutableSet.of(subgroupUuid1, subgroupUuid2));
+
+    assertTipCommit(
+        uuid,
+        "Update group\n"
+            + "\n"
+            + String.format("Add-group: Group <%s>\n", subgroupUuid1)
+            + String.format("Add-group: Group <%s>", subgroupUuid2));
+
+    AccountGroupByIdAud expAudit1 =
+        createExpGroupAudit(group.getId(), subgroupUuid1, userId, getTipTimestamp(uuid));
+    AccountGroupByIdAud expAudit2 =
+        createExpGroupAudit(group.getId(), subgroupUuid2, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getSubgroupsAudit(uuid))
+        .containsExactly(expAudit1, expAudit2)
+        .inOrder();
+  }
+
+  @Test
+  public void addAndRemoveMembersAndSubgroups() throws Exception {
+    InternalGroup group = createGroupAsUser(1, "test-group");
+    AccountGroup.Id groupId = group.getId();
+    AccountGroup.UUID uuid = group.getGroupUUID();
+    AccountGroupMemberAudit expMemberAudit =
+        createExpMemberAudit(groupId, userId, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getMembersAudit(uuid)).containsExactly(expMemberAudit);
+
+    Account.Id id1 = new Account.Id(100002);
+    Account.Id id2 = new Account.Id(100003);
+    Account.Id id3 = new Account.Id(100004);
+    InternalGroup subgroup1 = createGroupAsUser(2, "test-group-2");
+    InternalGroup subgroup2 = createGroupAsUser(3, "test-group-3");
+    InternalGroup subgroup3 = createGroupAsUser(4, "test-group-4");
+    AccountGroup.UUID subgroupUuid1 = subgroup1.getGroupUUID();
+    AccountGroup.UUID subgroupUuid2 = subgroup2.getGroupUUID();
+    AccountGroup.UUID subgroupUuid3 = subgroup3.getGroupUUID();
+
+    // Add two accounts.
+    addMembers(uuid, ImmutableSet.of(id1, id2));
+    assertTipCommit(
+        uuid,
+        "Update group\n"
+            + "\n"
+            + String.format("Add: Account %s <%s@server-id>\n", id1, id1)
+            + String.format("Add: Account %s <%s@server-id>", id2, id2));
+    AccountGroupMemberAudit expMemberAudit1 =
+        createExpMemberAudit(groupId, id1, userId, getTipTimestamp(uuid));
+    AccountGroupMemberAudit expMemberAudit2 =
+        createExpMemberAudit(groupId, id2, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getMembersAudit(uuid))
+        .containsExactly(expMemberAudit, expMemberAudit1, expMemberAudit2)
+        .inOrder();
+
+    // Add one subgroup.
+    addSubgroups(uuid, ImmutableSet.of(subgroupUuid1));
+    assertTipCommit(uuid, String.format("Update group\n\nAdd-group: Group <%s>", subgroupUuid1));
+    AccountGroupByIdAud expGroupAudit1 =
+        createExpGroupAudit(group.getId(), subgroupUuid1, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getSubgroupsAudit(uuid)).containsExactly(expGroupAudit1);
+
+    // Remove one account.
+    removeMembers(uuid, ImmutableSet.of(id2));
+    assertTipCommit(
+        uuid, String.format("Update group\n\nRemove: Account %s <%s@server-id>", id2, id2));
+    expMemberAudit2.removed(userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getMembersAudit(uuid))
+        .containsExactly(expMemberAudit, expMemberAudit1, expMemberAudit2)
+        .inOrder();
+
+    // Add two subgroups.
+    addSubgroups(uuid, ImmutableSet.of(subgroupUuid2, subgroupUuid3));
+    assertTipCommit(
+        uuid,
+        "Update group\n"
+            + "\n"
+            + String.format("Add-group: Group <%s>\n", subgroupUuid2)
+            + String.format("Add-group: Group <%s>", subgroupUuid3));
+    AccountGroupByIdAud expGroupAudit2 =
+        createExpGroupAudit(group.getId(), subgroupUuid2, userId, getTipTimestamp(uuid));
+    AccountGroupByIdAud expGroupAudit3 =
+        createExpGroupAudit(group.getId(), subgroupUuid3, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getSubgroupsAudit(uuid))
+        .containsExactly(expGroupAudit1, expGroupAudit2, expGroupAudit3)
+        .inOrder();
+
+    // Add two account, including a removed account.
+    addMembers(uuid, ImmutableSet.of(id2, id3));
+    assertTipCommit(
+        uuid,
+        "Update group\n"
+            + "\n"
+            + String.format("Add: Account %s <%s@server-id>\n", id2, id2)
+            + String.format("Add: Account %s <%s@server-id>", id3, id3));
+    AccountGroupMemberAudit expMemberAudit4 =
+        createExpMemberAudit(groupId, id2, userId, getTipTimestamp(uuid));
+    AccountGroupMemberAudit expMemberAudit3 =
+        createExpMemberAudit(groupId, id3, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getMembersAudit(uuid))
+        .containsExactly(
+            expMemberAudit, expMemberAudit1, expMemberAudit2, expMemberAudit4, expMemberAudit3)
+        .inOrder();
+
+    // Remove two subgroups.
+    removeSubgroups(uuid, ImmutableSet.of(subgroupUuid1, subgroupUuid3));
+    assertTipCommit(
+        uuid,
+        "Update group\n"
+            + "\n"
+            + String.format("Remove-group: Group <%s>\n", subgroupUuid1)
+            + String.format("Remove-group: Group <%s>", subgroupUuid3));
+    expGroupAudit1.removed(userId, getTipTimestamp(uuid));
+    expGroupAudit3.removed(userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getSubgroupsAudit(uuid))
+        .containsExactly(expGroupAudit1, expGroupAudit2, expGroupAudit3)
+        .inOrder();
+
+    // Add back one removed subgroup.
+    addSubgroups(uuid, ImmutableSet.of(subgroupUuid1));
+    AccountGroupByIdAud expGroupAudit4 =
+        createExpGroupAudit(group.getId(), subgroupUuid1, userId, getTipTimestamp(uuid));
+    assertThat(auditLogReader.getSubgroupsAudit(uuid))
+        .containsExactly(expGroupAudit1, expGroupAudit2, expGroupAudit3, expGroupAudit4)
+        .inOrder();
+  }
+
+  private InternalGroup createGroupAsUser(int next, String groupName) throws Exception {
+    return createGroup(next, groupName, userIdent, userId);
+  }
+
+  private InternalGroup createGroup(
+      int next, String groupName, PersonIdent authorIdent, Account.Id authorId) throws Exception {
+    InternalGroupCreation groupCreation =
+        InternalGroupCreation.builder()
+            .setGroupUUID(GroupUUID.make(groupName, serverIdent))
+            .setNameKey(new AccountGroup.NameKey(groupName))
+            .setId(new AccountGroup.Id(next))
+            .setCreatedOn(TimeUtil.nowTs())
+            .build();
+    InternalGroupUpdate groupUpdate =
+        authorIdent.equals(serverIdent)
+            ? InternalGroupUpdate.builder().setDescription("Groups").build()
+            : InternalGroupUpdate.builder()
+                .setDescription("Groups")
+                .setMemberModification(members -> ImmutableSet.of(authorId))
+                .build();
+
+    GroupConfig groupConfig = GroupConfig.createForNewGroup(allUsersRepo, groupCreation);
+    groupConfig.setGroupUpdate(
+        groupUpdate, AbstractGroupTest::getAccountNameEmail, AbstractGroupTest::getGroupName);
+
+    RevCommit commit = groupConfig.commit(createMetaDataUpdate(authorIdent));
+    assertCreateGroup(authorIdent, commit);
+    return groupConfig
+        .getLoadedGroup()
+        .orElseThrow(() -> new IllegalStateException("create group failed"));
+  }
+
+  private void assertCreateGroup(PersonIdent authorIdent, RevCommit commit) throws Exception {
+    if (authorIdent.equals(serverIdent)) {
+      assertServerCommit(CommitUtil.toCommitInfo(commit), "Create group");
+    } else {
+      assertCommit(
+          CommitUtil.toCommitInfo(commit),
+          String.format("Create group\n\nAdd: Account %s <%s@%s>", userId, userId, SERVER_ID),
+          getAccountName(userId),
+          getAccountEmail(userId));
+    }
+  }
+
+  private InternalGroup updateGroup(AccountGroup.UUID uuid, InternalGroupUpdate groupUpdate)
+      throws Exception {
+    GroupConfig groupConfig = GroupConfig.loadForGroup(allUsersRepo, uuid);
+    groupConfig.setGroupUpdate(
+        groupUpdate, AbstractGroupTest::getAccountNameEmail, AbstractGroupTest::getGroupName);
+
+    groupConfig.commit(createMetaDataUpdate(userIdent));
+    return groupConfig
+        .getLoadedGroup()
+        .orElseThrow(() -> new IllegalStateException("updated group failed"));
+  }
+
+  private InternalGroup addMembers(AccountGroup.UUID groupUuid, Set<Account.Id> ids)
+      throws Exception {
+    InternalGroupUpdate update =
+        InternalGroupUpdate.builder()
+            .setMemberModification(memberIds -> Sets.union(memberIds, ids))
+            .build();
+    return updateGroup(groupUuid, update);
+  }
+
+  private InternalGroup removeMembers(AccountGroup.UUID groupUuid, Set<Account.Id> ids)
+      throws Exception {
+    InternalGroupUpdate update =
+        InternalGroupUpdate.builder()
+            .setMemberModification(memberIds -> Sets.difference(memberIds, ids))
+            .build();
+    return updateGroup(groupUuid, update);
+  }
+
+  private InternalGroup addSubgroups(AccountGroup.UUID groupUuid, Set<AccountGroup.UUID> uuids)
+      throws Exception {
+    InternalGroupUpdate update =
+        InternalGroupUpdate.builder()
+            .setSubgroupModification(memberIds -> Sets.union(memberIds, uuids))
+            .build();
+    return updateGroup(groupUuid, update);
+  }
+
+  private InternalGroup removeSubgroups(AccountGroup.UUID groupUuid, Set<AccountGroup.UUID> uuids)
+      throws Exception {
+    InternalGroupUpdate update =
+        InternalGroupUpdate.builder()
+            .setSubgroupModification(memberIds -> Sets.difference(memberIds, uuids))
+            .build();
+    return updateGroup(groupUuid, update);
+  }
+
+  private AccountGroupMemberAudit createExpMemberAudit(
+      AccountGroup.Id groupId, Account.Id id, Account.Id addedBy, Timestamp addedOn) {
+    return new AccountGroupMemberAudit(
+        new AccountGroupMemberAudit.Key(id, groupId, addedOn), addedBy);
+  }
+
+  private AccountGroupByIdAud createExpGroupAudit(
+      AccountGroup.Id groupId, AccountGroup.UUID uuid, Account.Id addedBy, Timestamp addedOn) {
+    return new AccountGroupByIdAud(new AccountGroupByIdAud.Key(groupId, uuid, addedOn), addedBy);
+  }
+}

javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.group.db;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.extensions.common.testing.CommitInfoSubject.assertThat;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_GROUPNAMES;
 import static com.google.gerrit.server.group.db.GroupBundle.builder;
 
@@ -31,58 +30,48 @@ import com.google.gerrit.reviewdb.client.AccountGroupByIdAud;
 import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
 import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.config.AllUsersNameProvider;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.db.testing.GroupTestUtil;
 import com.google.gerrit.server.update.RefUpdateUtil;
-import com.google.gerrit.testing.GerritBaseTests;
 import com.google.gerrit.testing.TestTimeUtil;
 import java.sql.Timestamp;
-import java.util.TimeZone;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.IntStream;
-import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
-import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
-public class GroupRebuilderTest extends GerritBaseTests {
+public class GroupRebuilderTest extends AbstractGroupTest {
-  private static final String SERVER_NAME = "Gerrit Server";
-  private static final String SERVER_EMAIL = "noreply@gerritcodereview.com";
-  private static final TimeZone TZ = TimeZone.getTimeZone("America/Los_Angeles");
-
   private AtomicInteger idCounter;
   private Repository repo;
   private GroupRebuilder rebuilder;
+  private GroupBundle.Factory bundleFactory;
 
   @Before
-  public void setUp() {
+  public void setUp() throws Exception {
     TestTimeUtil.resetWithClockStep(1, TimeUnit.SECONDS);
     idCounter = new AtomicInteger();
-    repo = new InMemoryRepository(new DfsRepositoryDescription(AllUsersNameProvider.DEFAULT));
+    repo = repoManager.createRepository(allUsersName);
     rebuilder =
         new GroupRebuilder(
             GroupRebuilderTest::newPersonIdent,
-            new AllUsersName(AllUsersNameProvider.DEFAULT),
+            allUsersName,
             (project, repo, batch) ->
                 new MetaDataUpdate(GitReferenceUpdated.DISABLED, project, repo, batch),
             // Note that the expected name/email values in tests are not necessarily realistic,
             // since they use these trivial name/email functions. GroupRebuilderIT checks the actual
             // values.
-            (id, ident) ->
+            AbstractGroupTest::newPersonIdent,
-                new PersonIdent(
+            AbstractGroupTest::getAccountNameEmail,
-                    "Account " + id, id + "@server-id", ident.getWhen(), ident.getTimeZone()),
+            AbstractGroupTest::getGroupName);
-            id -> String.format("Account %s <%s@server-id>", id, id),
+    bundleFactory =
-            uuid -> "Group " + uuid);
+        new GroupBundle.Factory(new AuditLogReader(SERVER_ID, repoManager, allUsersName));
   }
 
   @After
@@ -97,7 +86,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(1);
     assertCommit(log.get(0), "Create group", SERVER_NAME, SERVER_EMAIL);
@@ -114,7 +103,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(1);
     assertServerCommit(log.get(0), "Create group");
@@ -132,7 +121,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(2);
     assertServerCommit(log.get(0), "Create group");
@@ -142,8 +131,8 @@ public class GroupRebuilderTest extends GerritBaseTests {
             + "\n"
             + "Add: Account 1 <1@server-id>\n"
             + "Add: Account 2 <2@server-id>\n"
-            + "Add-group: Group x\n"
+            + "Add-group: Group <x>\n"
-            + "Add-group: Group y");
+            + "Add-group: Group <y>");
   }
 
   @Test
@@ -161,7 +150,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(4);
     assertServerCommit(log.get(0), "Create group");
@@ -187,7 +176,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(4);
     assertServerCommit(log.get(0), "Create group");
@@ -211,7 +200,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(3);
     assertServerCommit(log.get(0), "Create group");
@@ -236,13 +225,13 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(4);
     assertServerCommit(log.get(0), "Create group");
-    assertCommit(log.get(1), "Update group\n\nAdd-group: Group y", "Account 8", "8@server-id");
+    assertCommit(log.get(1), "Update group\n\nAdd-group: Group <y>", "Account 8", "8@server-id");
-    assertCommit(log.get(2), "Update group\n\nAdd-group: Group x", "Account 8", "8@server-id");
+    assertCommit(log.get(2), "Update group\n\nAdd-group: Group <x>", "Account 8", "8@server-id");
-    assertCommit(log.get(3), "Update group\n\nRemove-group: Group y", "Account 9", "9@server-id");
+    assertCommit(log.get(3), "Update group\n\nRemove-group: Group <y>", "Account 9", "9@server-id");
   }
 
   @Test
@@ -257,12 +246,12 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(3);
     assertServerCommit(log.get(0), "Create group");
-    assertCommit(log.get(1), "Update group\n\nAdd-group: Group x", "Account 8", "8@server-id");
+    assertCommit(log.get(1), "Update group\n\nAdd-group: Group <x>", "Account 8", "8@server-id");
-    assertServerCommit(log.get(2), "Update group\n\nAdd-group: Group y\nAdd-group: Group z");
+    assertServerCommit(log.get(2), "Update group\n\nAdd-group: Group <y>\nAdd-group: Group <z>");
   }
 
   @Test
@@ -287,7 +276,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(5);
     assertServerCommit(log.get(0), "Create group");
@@ -306,12 +295,12 @@ public class GroupRebuilderTest extends GerritBaseTests {
         log.get(3),
         "Update group\n"
             + "\n"
-            + "Add-group: Group x\n"
+            + "Add-group: Group <x>\n"
-            + "Add-group: Group y\n"
+            + "Add-group: Group <y>\n"
-            + "Add-group: Group z",
+            + "Add-group: Group <z>",
         "Account 8",
         "8@server-id");
-    assertCommit(log.get(4), "Update group\n\nRemove-group: Group z", "Account 8", "8@server-id");
+    assertCommit(log.get(4), "Update group\n\nRemove-group: Group <z>", "Account 8", "8@server-id");
   }
 
   @Test
@@ -334,7 +323,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(5);
     assertServerCommit(log.get(0), "Create group");
@@ -345,12 +334,12 @@ public class GroupRebuilderTest extends GerritBaseTests {
         "8@server-id");
     assertCommit(
         log.get(2),
-        "Update group\n\nAdd-group: Group x\nAdd-group: Group z",
+        "Update group\n\nAdd-group: Group <x>\nAdd-group: Group <z>",
         "Account 8",
         "8@server-id");
     assertCommit(
         log.get(3), "Update group\n\nAdd: Account 2 <2@server-id>", "Account 9", "9@server-id");
-    assertCommit(log.get(4), "Update group\n\nAdd-group: Group y", "Account 9", "9@server-id");
+    assertCommit(log.get(4), "Update group\n\nAdd-group: Group <y>", "Account 9", "9@server-id");
   }
 
   @Test
@@ -369,12 +358,12 @@ public class GroupRebuilderTest extends GerritBaseTests {
 
     rebuilder.rebuild(repo, b, null);
 
-    assertThat(reload(g)).isEqualTo(b.toInternalGroup());
+    assertThat(reload(g)).isEqualTo(b);
     ImmutableList<CommitInfo> log = log(g);
     assertThat(log).hasSize(3);
     assertServerCommit(log.get(0), "Create group");
-    assertCommit(log.get(1), "Update group\n\nAdd-group: Group x", "Account 8", "8@server-id");
+    assertCommit(log.get(1), "Update group\n\nAdd-group: Group <x>", "Account 8", "8@server-id");
-    assertServerCommit(log.get(2), "Update group\n\nAdd-group: Group y\nAdd-group: Group z");
+    assertServerCommit(log.get(2), "Update group\n\nAdd-group: Group <y>\nAdd-group: Group <z>");
 
     assertThat(log.stream().map(c -> c.committer.date).collect(toImmutableList()))
         .named("%s", log)
@@ -410,14 +399,14 @@ public class GroupRebuilderTest extends GerritBaseTests {
     assertThat(log(g1)).hasSize(1);
     assertThat(log(g2)).hasSize(1);
     assertThat(logGroupNames()).hasSize(1);
-    assertThat(reload(g1)).isEqualTo(b1.toInternalGroup());
+    assertThat(reload(g1)).isEqualTo(b1);
-    assertThat(reload(g2)).isEqualTo(b2.toInternalGroup());
+    assertThat(reload(g2)).isEqualTo(b2);
 
     assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2");
   }
 
-  private InternalGroup reload(AccountGroup g) throws Exception {
+  private GroupBundle reload(AccountGroup g) throws Exception {
-    return removeRefState(GroupConfig.loadForGroup(repo, g.getGroupUUID()).getLoadedGroup().get());
+    return bundleFactory.fromNoteDb(repo, g.getGroupUUID());
   }
 
   private AccountGroup newGroup(String name) {
@@ -487,28 +476,7 @@ public class GroupRebuilderTest extends GerritBaseTests {
     return GroupTestUtil.log(repo, REFS_GROUPNAMES);
   }
 
-  private static void assertServerCommit(CommitInfo commitInfo, String expectedMessage) {
-    assertCommit(commitInfo, expectedMessage, SERVER_NAME, SERVER_EMAIL);
-  }
-
-  private static void assertCommit(
-      CommitInfo commitInfo, String expectedMessage, String expectedName, String expectedEmail) {
-    assertThat(commitInfo).message().isEqualTo(expectedMessage);
-    assertThat(commitInfo).author().name().isEqualTo(expectedName);
-    assertThat(commitInfo).author().email().isEqualTo(expectedEmail);
-
-    // Committer should always be the server, regardless of author.
-    assertThat(commitInfo).committer().name().isEqualTo(SERVER_NAME);
-    assertThat(commitInfo).committer().email().isEqualTo(SERVER_EMAIL);
-    assertThat(commitInfo).committer().date().isEqualTo(commitInfo.author.date);
-    assertThat(commitInfo).committer().tz().isEqualTo(commitInfo.author.tz);
-  }
-
   private static InternalGroup removeRefState(InternalGroup group) throws Exception {
     return group.toBuilder().setRefState(null).build();
   }
-
-  private static PersonIdent newPersonIdent() {
-    return new PersonIdent(SERVER_NAME, SERVER_EMAIL, TimeUtil.nowTs(), TZ);
-  }
 }