Add notify section in project.config

The notify section allows project owners to include emails to users
directly from project.config. This removes the need to create fake
user accounts to always BCC a group mailing list. For example:

  [notify "dev-group"]
    email = dev-team <dev-team@example.com>
    filter = branch:master visibleto:dev

Internal groups may also be used as a mailing list, automatically
BCCing any user that is a member of the group if the group can see
the change:

  [access "refs/heads/*"]
    read = group Developers
  [notify "reviewers"]
    email = group Developers

Change-Id: I02bd05b562e420a4742ff27ffacad8f20648e4f0

Documentation/index.txt

@@ -19,6 +19,7 @@ User Guide
 * link:access-control.html[Access Controls]
 * link:error-messages.html[Error Messages]
 * link:rest-api.html[REST API]
+* link:user-notify.html[Subscribing to Email Notifications]
 * link:user-submodules.html[Subscribing to Git Submodules]
 
 Installation

Documentation/user-notify.txt

@@ -0,0 +1,127 @@
+Gerrit Code Review - Email Notifications
+========================================
+
+Description
+-----------
+
+Gerrit can automatically notify users by email when new changes are
+uploaded for review, after comments have been posted on a change,
+or after the change has been submitted to a branch.
+
+User Level Settings
+-------------------
+
+Individual users can configure email subscriptions by editing
+watched projects through Settings > Watched Projects with the web UI.
+
+Specific projects may be watched, or the special project
+`All-Projects` can be watched to watch all projects that
+are visible to the user.
+
+Change search expressions can be used to filter change notifications
+to specific subsets, for example `branch:master` to only see changes
+proposed for the master branch.
+
+Project Level Settings
+----------------------
+
+Project owners and site administrators can configure project level
+notifications, enabling Gerrit Code Review to automatically send
+emails to team mailing lists, or groups of users. Project settings
+are stored inside of the `refs/meta/config` branch of each Git
+repository, and are placed inside of the `project.config` file.
+
+To edit the project level notify settings, ensure the project owner
+has Push permission already granted for the `refs/meta/config`
+branch. Consult link:access-control.html[access controls] for
+details on how access permissions work.
+
+Initialize a temporary Git repository to edit the configuration:
+====
+  mkdir cfg_dir
+  cd cfg_dir
+  git init
+====
+
+Download the existing configuration from Gerrit:
+====
+  git fetch ssh://localhost:29418/project refs/meta/config
+  git checkout FETCH_HEAD
+====
+
+Enable notifications to an email address by adding to
+`project.config`, this can be done using the `git config` command:
+====
+  git config -f project.config --add notify.team.email team-address@example.com
+  git config -f project.config --add notify.team.email paranoid-manager@example.com
+====
+
+Examining the project.config file with any text editor should show
+a new notify section describing the email addresses to deliver to:
+----
+  [notify "team"]
+  	email = team-address@example.com
+  	email = paranoid-manager@example.com
+----
+
+Each notify section within a single project.config file must have a
+unique name. The section name itself does not matter and may later
+appear in the web UI. Naming a section after the email address or
+group it delivers to is typical. Multiple sections can be specified
+if different filters are needed.
+
+Commit the configuration change, and push it back:
+====
+  git commit -a -m "Notify team-address@example.com of changes"
+  git push ssh://localhost:29418/project HEAD:refs/meta/config
+====
+
+[[notify.name.email]]notify.<name>.email::
++
+List of email addresses to send matching notifications to. Each
+email address should be placed on its own line.
++
+Internal groups within Gerrit Code Review can also be named using
+`group NAME` syntax. If this format is used the group's UUID must
+also appear in the corresponding `groups` file. Gerrit will expand
+the group membership and BCC all current users.
+
+[[notify.name.type]]notify.<name>.type::
++
+Types of notifications to send. If not specified, all notifications
+are sent.
++
+* `new_changes`: Only newly created changes.
+* `all_comments`: Only comments on existing changes.
+* `submitted_changes`: Only changes that have been submitted.
+* `all`: All notifications.
+
++
+Like email, this variable may be a list of options.
+
+[[notify.name.filter]]notify.<name>.filter::
++
+link:user-search.html[Change search expression] to match changes that
+should be sent to the emails named in this section. Within a Git-style
+configuration file double quotes around complex operator values may
+need to be escaped, e.g. `filter = branch:\"^(maint|stable)-.*\"`.
+
+When sending email to a bare email address in a notify block, Gerrit
+Code Review ignores read access controls and assumes the administrator
+has set the filtering options correctly. Project owners can implement
+security filtering by adding the `visibleto:groupname` predicate to
+the filter expression, for example:
+
+====
+  [notify "Developers"]
+  	email = team-address@example.com
+  	filter = visibleto:Developers
+====
+
+When sending email to an internal group, the internal group's read
+access is automatically checked by Gerrit and therefore does not
+need to use the `visibleto:` operator in the filter.
+
+GERRIT
+------
+Part of link:index.html[Gerrit Code Review]

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/AccountProjectWatch.java

@@ -22,7 +22,7 @@ import com.google.gwtorm.client.StringKey;
 public final class AccountProjectWatch {
 
   public enum NotifyType {
-    NEW_CHANGES, ALL_COMMENTS, SUBMITTED_CHANGES
+    NEW_CHANGES, ALL_COMMENTS, SUBMITTED_CHANGES, ALL
   }
 
   public static final String FILTER_ALL = "*";
@@ -159,6 +159,12 @@ public final class AccountProjectWatch {
       case SUBMITTED_CHANGES:
         notifySubmittedChanges = v;
         break;
+
+      case ALL:
+        notifyNewChanges = v;
+        notifyAllComments = v;
+        notifySubmittedChanges = v;
+        break;
     }
   }
 }

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

@@ -0,0 +1,104 @@
+// Copyright (C) 2012 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.git;
+
+import com.google.common.base.Strings;
+import com.google.common.collect.Sets;
+import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
+import com.google.gerrit.server.mail.Address;
+
+import java.util.EnumSet;
+import java.util.Set;
+
+public class NotifyConfig implements Comparable<NotifyConfig> {
+  private String name;
+  private EnumSet<NotifyType> types = EnumSet.of(NotifyType.ALL);
+  private String filter;
+
+  private Set<GroupReference> groups = Sets.newHashSet();
+  private Set<Address> addresses = Sets.newHashSet();
+
+  public String getName() {
+    return name;
+  }
+
+  public void setName(String name) {
+    this.name = name;
+  }
+
+  public boolean isNotify(NotifyType type) {
+    return types.contains(type) || types.contains(NotifyType.ALL);
+  }
+
+  public EnumSet<NotifyType> getNotify() {
+    return types;
+  }
+
+  public void setTypes(EnumSet<NotifyType> newTypes) {
+    types = EnumSet.copyOf(newTypes);
+  }
+
+  public String getFilter() {
+    return filter;
+  }
+
+  public void setFilter(String filter) {
+    if ("*".equals(filter)) {
+      this.filter = null;
+    } else {
+      this.filter = Strings.emptyToNull(filter);
+    }
+  }
+
+  public Set<GroupReference> getGroups() {
+    return groups;
+  }
+
+  public Set<Address> getAddresses() {
+    return addresses;
+  }
+
+  public void addEmail(GroupReference group) {
+    groups.add(group);
+  }
+
+  public void addEmail(Address address) {
+    addresses.add(address);
+  }
+
+  @Override
+  public int compareTo(NotifyConfig o) {
+    return name.compareTo(o.name);
+  }
+
+  @Override
+  public int hashCode() {
+    return name.hashCode();
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (obj instanceof NotifyConfig) {
+      return compareTo((NotifyConfig) obj) == 0;
+    }
+    return false;
+  }
+
+  @Override
+  public String toString() {
+    return "NotifyConfig[" + name + " = " + addresses + " + " + groups + "]";
+  }
+}

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

@@ -16,6 +16,8 @@ package com.google.gerrit.server.git;
 
 import static com.google.gerrit.common.data.Permission.isPermission;
 
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
 import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.ContributorAgreement;
 import com.google.gerrit.common.data.GlobalCapability;
@@ -23,17 +25,21 @@ import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.PermissionRule;
 import com.google.gerrit.common.data.PermissionRule.Action;
+import com.google.gerrit.common.data.RefConfigSection;
 import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.Project.State;
 import com.google.gerrit.reviewdb.client.Project.SubmitType;
 import com.google.gerrit.server.account.GroupCache;
-import com.google.gerrit.common.data.RefConfigSection;
+import com.google.gerrit.server.config.ConfigUtil;
+import com.google.gerrit.server.mail.Address;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.util.StringUtils;
 
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -41,6 +47,7 @@ import java.io.StringReader;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -67,6 +74,11 @@ public class ProjectConfig extends VersionedMetaData {
   private static final String KEY_AUTO_VERIFY = "autoVerify";
   private static final String KEY_AGREEMENT_URL = "agreementUrl";
 
+  private static final String NOTIFY = "notify";
+  private static final String KEY_EMAIL = "email";
+  private static final String KEY_FILTER = "filter";
+  private static final String KEY_TYPE = "type";
+
   private static final String CAPABILITY = "capability";
 
   private static final String RECEIVE = "receive";
@@ -91,6 +103,7 @@ public class ProjectConfig extends VersionedMetaData {
   private Map<AccountGroup.UUID, GroupReference> groupsByUUID;
   private Map<String, AccessSection> accessSections;
   private Map<String, ContributorAgreement> contributorAgreements;
+  private Map<String, NotifyConfig> notifySections;
   private List<ValidationError> validationErrors;
   private ObjectId rulesId;
 
@@ -185,6 +198,10 @@ public class ProjectConfig extends VersionedMetaData {
     contributorAgreements.put(section.getName(), section);
   }
 
+  public Collection<NotifyConfig> getNotifyConfigs() {
+    return notifySections.values();
+  }
+
   public GroupReference resolve(AccountGroup group) {
     return resolve(GroupReference.forGroup(group));
   }
@@ -275,6 +292,7 @@ public class ProjectConfig extends VersionedMetaData {
     loadAccountsSection(rc, groupsByName);
     loadContributorAgreements(rc, groupsByName);
     loadAccessSections(rc, groupsByName);
+    loadNotifySections(rc, groupsByName);
   }
 
   private void loadAccountsSection(
@@ -318,6 +336,67 @@ public class ProjectConfig extends VersionedMetaData {
     }
   }
 
+  /**
+   * Parses the [notify] sections out of the configuration file.
+   *
+   * <pre>
+   *   [notify "reviewers"]
+   *     email = group Reviewers
+   *     type = new_changes
+   *
+   *   [notify "dev-team"]
+   *     email = dev-team@example.com
+   *     filter = branch:master
+   *
+   *   [notify "qa"]
+   *     email = qa@example.com
+   *     filter = branch:\"^(maint|stable)-.*\"
+   *     type = submitted_changes
+   * </pre>
+   */
+  private void loadNotifySections(
+      Config rc, Map<String, GroupReference> groupsByName) {
+    notifySections = Maps.newHashMap();
+    for (String sectionName : rc.getSubsections(NOTIFY)) {
+      NotifyConfig n = new NotifyConfig();
+      n.setName(sectionName);
+      n.setFilter(rc.getString(NOTIFY, sectionName, KEY_FILTER));
+
+      EnumSet<NotifyType> types = EnumSet.noneOf(NotifyType.class);
+      types.addAll(ConfigUtil.getEnumList(rc,
+          NOTIFY, sectionName, KEY_TYPE,
+          NotifyType.ALL));
+      n.setTypes(types);
+
+      for (String dst : rc.getStringList(NOTIFY, sectionName, KEY_EMAIL)) {
+        if (dst.startsWith("group ")) {
+          String groupName = dst.substring(6).trim();
+          GroupReference ref = groupsByName.get(groupName);
+          if (ref == null) {
+            ref = new GroupReference(null, groupName);
+            groupsByName.put(ref.getName(), ref);
+          }
+          if (ref.getUUID() != null) {
+            n.addEmail(ref);
+          } else {
+            error(new ValidationError(PROJECT_CONFIG,
+                "group \"" + ref.getName() + "\" not in " + GROUP_LIST));
+          }
+        } else if (dst.startsWith("user ")) {
+          error(new ValidationError(PROJECT_CONFIG, dst + " not supported"));
+        } else {
+          try {
+            n.addEmail(Address.parse(dst));
+          } catch (IllegalArgumentException err) {
+            error(new ValidationError(PROJECT_CONFIG,
+                "notify section \"" + sectionName + "\" has invalid email \"" + dst + "\""));
+          }
+        }
+      }
+      notifySections.put(sectionName, n);
+    }
+  }
+
   private void loadAccessSections(
       Config rc, Map<String, GroupReference> groupsByName) {
     accessSections = new HashMap<String, AccessSection>();
@@ -458,6 +537,7 @@ public class ProjectConfig extends VersionedMetaData {
     saveAccountsSection(rc, keepGroups);
     saveContributorAgreements(rc, keepGroups);
     saveAccessSections(rc, keepGroups);
+    saveNotifySections(rc, keepGroups);
     groupsByUUID.keySet().retainAll(keepGroups);
 
     saveConfig(PROJECT_CONFIG, rc);
@@ -493,6 +573,47 @@ public class ProjectConfig extends VersionedMetaData {
     }
   }
 
+  private void saveNotifySections(
+      Config rc, Set<AccountGroup.UUID> keepGroups) {
+    for (NotifyConfig nc : sort(notifySections.values())) {
+      List<String> email = Lists.newArrayList();
+      for (GroupReference gr : nc.getGroups()) {
+        if (gr.getUUID() != null) {
+          keepGroups.add(gr.getUUID());
+        }
+        email.add(new PermissionRule(gr).asString(false));
+      }
+      Collections.sort(email);
+
+      List<String> addrs = Lists.newArrayList();
+      for (Address addr : nc.getAddresses()) {
+        addrs.add(addr.toString());
+      }
+      Collections.sort(addrs);
+      email.addAll(addrs);
+
+      if (email.isEmpty()) {
+        rc.unset(NOTIFY, nc.getName(), KEY_EMAIL);
+      } else {
+        rc.setStringList(NOTIFY, nc.getName(), KEY_EMAIL, email);
+      }
+
+      if (nc.getNotify().equals(EnumSet.of(NotifyType.ALL))) {
+        rc.unset(NOTIFY, nc.getName(), KEY_TYPE);
+      } else {
+        List<String> types = Lists.newArrayListWithCapacity(4);
+        for (NotifyType t : NotifyType.values()) {
+          if (nc.isNotify(t)) {
+            types.add(StringUtils.toLowerCase(t.name()));
+          }
+        }
+        rc.setStringList(NOTIFY, nc.getName(), KEY_TYPE, types);
+      }
+
+      set(rc, NOTIFY, nc.getName(), KEY_FILTER, nc.getFilter());
+    }
+  }
+
   private List<String> ruleToStringList(
       List<PermissionRule> list, Set<AccountGroup.UUID> keepGroups) {
     List<String> rules = new ArrayList<String>();

gerrit-server/src/main/java/com/google/gerrit/server/mail/AbandonedSender.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.mail;
 
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
@@ -38,7 +39,7 @@ public class AbandonedSender extends ReplyToChangeSender {
 
     ccAllApprovals();
     bccStarredBy();
-    bccWatchesNotifyAllComments();
+    bccWatches(NotifyType.ALL_COMMENTS);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/mail/Address.java

@@ -54,6 +54,19 @@ public class Address {
     return email;
   }
 
+  @Override
+  public int hashCode() {
+    return email.hashCode();
+  }
+
+  @Override
+  public boolean equals(Object other) {
+    if (other instanceof Address) {
+      return email.equals(((Address) other).email);
+    }
+    return false;
+  }
+
   @Override
   public String toString() {
     try {

gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java

@@ -14,18 +14,25 @@
 
 package com.google.gerrit.server.mail;
 
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.AccountGroupInclude;
+import com.google.gerrit.reviewdb.client.AccountGroupMember;
 import com.google.gerrit.reviewdb.client.AccountProjectWatch;
+import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
+import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.StarredChange;
-import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.git.NotifyConfig;
 import com.google.gerrit.server.patch.PatchList;
 import com.google.gerrit.server.patch.PatchListEntry;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
@@ -34,17 +41,17 @@ import com.google.gerrit.server.query.Predicate;
 import com.google.gerrit.server.query.QueryParseException;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeQueryBuilder;
+import com.google.gerrit.server.query.change.SingleGroupUser;
 import com.google.gwtorm.server.OrmException;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.text.MessageFormat;
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashSet;
-import java.util.List;
+import java.util.Queue;
 import java.util.Set;
 import java.util.TreeSet;
 
@@ -305,53 +312,148 @@ public abstract class ChangeEmail extends OutgoingEmail {
       // Just don't BCC everyone. Better to send a partial message to those
       // we already have queued up then to fail deliver entirely to people
       // who have a lower interest in the change.
+      log.warn("Cannot BCC users that starred updated change", err);
     }
   }
 
-  /** BCC any user who has set "notify all comments" on this project. */
-  protected void bccWatchesNotifyAllComments() {
+  /** BCC users and groups that want notification of events. */
+  protected void bccWatches(NotifyType type) {
     try {
-      // BCC anyone else who has interest in this project's changes
-      //
-      for (final AccountProjectWatch w : getWatches()) {
-        if (w.isNotify(NotifyType.ALL_COMMENTS)) {
-          add(RecipientType.BCC, w.getAccountId());
-        }
+      Watchers matching = getWatches(type);
+      for (Account.Id user : matching.accounts) {
+        add(RecipientType.BCC, user);
+      }
+      for (Address addr : matching.emails) {
+        add(RecipientType.BCC, addr);
       }
     } catch (OrmException err) {
       // Just don't CC everyone. Better to send a partial message to those
       // we already have queued up then to fail deliver entirely to people
       // who have a lower interest in the change.
+      log.warn("Cannot BCC watchers for " + type, err);
     }
   }
 
   /** Returns all watches that are relevant */
-  protected final List<AccountProjectWatch> getWatches() throws OrmException {
+  protected final Watchers getWatches(NotifyType type) throws OrmException {
+    Watchers matching = new Watchers();
     if (changeData == null) {
-      return Collections.emptyList();
+      return matching;
     }
 
-    List<AccountProjectWatch> matching = new ArrayList<AccountProjectWatch>();
     Set<Account.Id> projectWatchers = new HashSet<Account.Id>();
 
     for (AccountProjectWatch w : args.db.get().accountProjectWatches()
         .byProject(change.getProject())) {
       projectWatchers.add(w.getAccountId());
-      add(matching, w);
-    }
-
-    for (AccountProjectWatch w : args.db.get().accountProjectWatches()
-        .byProject(args.allProjectsName)) {
-      if (!projectWatchers.contains(w.getAccountId())) {
+      if (w.isNotify(type)) {
         add(matching, w);
       }
     }
 
-    return Collections.unmodifiableList(matching);
+    for (AccountProjectWatch w : args.db.get().accountProjectWatches()
+        .byProject(args.allProjectsName)) {
+      if (!projectWatchers.contains(w.getAccountId()) && w.isNotify(type)) {
+        add(matching, w);
+      }
+    }
+
+    ProjectState state = projectState;
+    while (state != null) {
+      for (NotifyConfig nc : state.getConfig().getNotifyConfigs()) {
+        if (nc.isNotify(type)) {
+          try {
+            add(matching, nc, state.getProject().getNameKey());
+          } catch (QueryParseException e) {
+            log.warn(String.format(
+                "Project %s has invalid notify %s filter \"%s\": %s",
+                state.getProject().getName(), nc.getName(),
+                nc.getFilter(), e.getMessage()));
+          }
+        }
+      }
+      state = state.getParentState();
+    }
+
+    return matching;
+  }
+
+  protected static class Watchers {
+    protected final Set<Account.Id> accounts = Sets.newHashSet();
+    protected final Set<Address> emails = Sets.newHashSet();
   }
 
   @SuppressWarnings("unchecked")
-  private void add(List<AccountProjectWatch> matching, AccountProjectWatch w)
+  private void add(Watchers matching, NotifyConfig nc, Project.NameKey project)
+      throws OrmException, QueryParseException {
+    for (GroupReference ref : nc.getGroups()) {
+      AccountGroup group = args.groupCache.get(ref.getUUID());
+      if (group == null) {
+        log.warn(String.format(
+            "Project %s has invalid group %s in notify section %s",
+            project.get(), ref.getName(), nc.getName()));
+        continue;
+      }
+
+      if (group.getType() != AccountGroup.Type.INTERNAL) {
+        log.warn(String.format(
+            "Project %s cannot use group %s of type %s in notify section %s",
+            project.get(), ref.getName(), group.getType(), nc.getName()));
+        continue;
+      }
+
+      ChangeQueryBuilder qb = args.queryBuilder.create(new SingleGroupUser(
+          args.capabilityControlFactory,
+          ref.getUUID()));
+      qb.setAllowFile(true);
+      Predicate<ChangeData> p = qb.is_visible();
+      if (nc.getFilter() != null) {
+        p = Predicate.and(qb.parse(nc.getFilter()), p);
+        p = args.queryRewriter.get().rewrite(p);
+      }
+      if (p.match(changeData)) {
+        recursivelyAddAllAccounts(matching, group);
+      }
+    }
+
+    if (!nc.getAddresses().isEmpty()) {
+      if (nc.getFilter() != null) {
+        ChangeQueryBuilder qb = args.queryBuilder.create(args.anonymousUser);
+        qb.setAllowFile(true);
+        Predicate<ChangeData> p = qb.parse(nc.getFilter());
+        p = args.queryRewriter.get().rewrite(p);
+        if (p.match(changeData)) {
+          matching.emails.addAll(nc.getAddresses());
+        }
+      } else {
+        matching.emails.addAll(nc.getAddresses());
+      }
+    }
+  }
+
+  private void recursivelyAddAllAccounts(Watchers matching, AccountGroup group)
+      throws OrmException {
+    Set<AccountGroup.Id> seen = Sets.newHashSet();
+    Queue<AccountGroup.Id> scan = Lists.newLinkedList();
+    scan.add(group.getId());
+    seen.add(group.getId());
+    while (!scan.isEmpty()) {
+      AccountGroup.Id next = scan.remove();
+      for (AccountGroupMember m : args.db.get().accountGroupMembers()
+          .byGroup(next)) {
+        matching.accounts.add(m.getAccountId());
+      }
+      for (AccountGroupInclude m : args.db.get().accountGroupIncludes()
+          .byGroup(next)) {
+        if (seen.add(m.getIncludeId())) {
+          scan.add(m.getIncludeId());
+        }
+      }
+    }
+  }
+
+  @SuppressWarnings("unchecked")
+  private void add(Watchers matching, AccountProjectWatch w)
       throws OrmException {
     IdentifiedUser user =
         args.identifiedUserFactory.create(args.db, w.getAccountId());
@@ -363,13 +465,13 @@ public abstract class ChangeEmail extends OutgoingEmail {
         p = Predicate.and(qb.parse(w.getFilter()), p);
         p = args.queryRewriter.get().rewrite(p);
         if (p.match(changeData)) {
-          matching.add(w);
+          matching.accounts.add(w.getAccountId());
         }
       } catch (QueryParseException e) {
         // Ignore broken filter expressions.
       }
     } else if (p.match(changeData)) {
-      matching.add(w);
+      matching.accounts.add(w.getAccountId());
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.mail;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.patch.PatchFile;
 import com.google.gerrit.server.patch.PatchList;
@@ -68,7 +69,7 @@ public class CommentSender extends ReplyToChangeSender {
 
     ccAllApprovals();
     bccStarredBy();
-    bccWatchesNotifyAllComments();
+    bccWatches(NotifyType.ALL_COMMENTS);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/mail/CreateChangeSender.java

@@ -15,74 +15,65 @@
 package com.google.gerrit.server.mail;
 
 import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.reviewdb.client.AccountGroupMember;
-import com.google.gerrit.reviewdb.client.AccountProjectWatch;
-import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
-import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.ssh.SshInfo;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 
-import java.util.HashSet;
-import java.util.Set;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /** Notify interested parties of a brand new change. */
 public class CreateChangeSender extends NewChangeSender {
+  private static final Logger log =
+      LoggerFactory.getLogger(CreateChangeSender.class);
+
   public static interface Factory {
     public CreateChangeSender create(Change change);
   }
 
-  private final GroupCache groupCache;
-
   @Inject
   public CreateChangeSender(EmailArguments ea,
       @AnonymousCowardName String anonymousCowardName, SshInfo sshInfo,
-      GroupCache groupCache, @Assisted Change c) {
+      @Assisted Change c) {
     super(ea, anonymousCowardName, sshInfo, c);
-    this.groupCache = groupCache;
   }
 
   @Override
   protected void init() throws EmailException {
     super.init();
 
-    bccWatchers();
-  }
-
-  private void bccWatchers() {
     try {
+      // BCC anyone who has interest in this project's changes
       // Try to mark interested owners with a TO and not a BCC line.
       //
-      final Set<Account.Id> owners = new HashSet<Account.Id>();
-      for (AccountGroup.UUID uuid : getProjectOwners()) {
-        AccountGroup group = groupCache.get(uuid);
-        if (group != null) {
-          for (AccountGroupMember m : args.db.get().accountGroupMembers()
-              .byGroup(group.getId())) {
-            owners.add(m.getAccountId());
-          }
+      Watchers matching = getWatches(NotifyType.NEW_CHANGES);
+      for (Account.Id user : matching.accounts) {
+        if (isOwnerOfProjectOrBranch(user)) {
+          add(RecipientType.TO, user);
+        } else {
+          add(RecipientType.BCC, user);
         }
       }
-
-      // BCC anyone who has interest in this project's changes
-      //
-      for (final AccountProjectWatch w : getWatches()) {
-        if (w.isNotify(NotifyType.NEW_CHANGES)) {
-          if (owners.contains(w.getAccountId())) {
-            add(RecipientType.TO, w.getAccountId());
-          } else {
-            add(RecipientType.BCC, w.getAccountId());
-          }
-        }
+      for (Address addr : matching.emails) {
+        add(RecipientType.BCC, addr);
       }
     } catch (OrmException err) {
       // Just don't CC everyone. Better to send a partial message to those
       // we already have queued up then to fail deliver entirely to people
       // who have a lower interest in the change.
+      log.warn("Cannot BCC watchers for new change", err);
     }
   }
+
+  private boolean isOwnerOfProjectOrBranch(Account.Id user) {
+    return projectState != null
+        && change != null
+        && projectState.controlFor(args.identifiedUserFactory.create(user))
+          .controlForRef(change.getDest())
+          .isOwner();
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java

@@ -15,9 +15,11 @@
 package com.google.gerrit.server.mail;
 
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.AnonymousUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.IdentifiedUser.GenericFactory;
 import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.CapabilityControl;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.config.AllProjectsName;
 import com.google.gerrit.server.config.CanonicalWebUrl;
@@ -44,6 +46,8 @@ class EmailArguments {
   final EmailSender emailSender;
   final PatchSetInfoFactory patchSetInfoFactory;
   final IdentifiedUser.GenericFactory identifiedUserFactory;
+  final CapabilityControl.Factory capabilityControlFactory;
+  final AnonymousUser anonymousUser;
   final Provider<String> urlProvider;
   final AllProjectsName allProjectsName;
 
@@ -58,6 +62,8 @@ class EmailArguments {
       PatchListCache patchListCache, FromAddressGenerator fromAddressGenerator,
       EmailSender emailSender, PatchSetInfoFactory patchSetInfoFactory,
       GenericFactory identifiedUserFactory,
+      CapabilityControl.Factory capabilityControlFactory,
+      AnonymousUser anonymousUser,
       @CanonicalWebUrl @Nullable Provider<String> urlProvider,
       AllProjectsName allProjectsName,
       ChangeQueryBuilder.Factory queryBuilder,
@@ -72,6 +78,8 @@ class EmailArguments {
     this.emailSender = emailSender;
     this.patchSetInfoFactory = patchSetInfoFactory;
     this.identifiedUserFactory = identifiedUserFactory;
+    this.capabilityControlFactory = capabilityControlFactory;
+    this.anonymousUser = anonymousUser;
     this.urlProvider = urlProvider;
     this.allProjectsName = allProjectsName;
     this.queryBuilder = queryBuilder;

gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java

@@ -17,7 +17,6 @@ package com.google.gerrit.server.mail;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
 import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountProjectWatch;
 import com.google.gerrit.reviewdb.client.ApprovalCategory;
 import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
 import com.google.gerrit.reviewdb.client.Change;
@@ -53,8 +52,8 @@ public class MergedSender extends ReplyToChangeSender {
 
     ccAllApprovals();
     bccStarredBy();
-    bccWatchesNotifyAllComments();
-    bccWatchesNotifySubmittedChanges();
+    bccWatches(NotifyType.ALL_COMMENTS);
+    bccWatches(NotifyType.SUBMITTED_CHANGES);
   }
 
   @Override
@@ -140,20 +139,4 @@ public class MergedSender extends ReplyToChangeSender {
     }
     m.put(ca.getCategoryId(), ca);
   }
-
-  private void bccWatchesNotifySubmittedChanges() {
-    try {
-      // BCC anyone else who has interest in this project's changes
-      //
-      for (final AccountProjectWatch w : getWatches()) {
-        if (w.isNotify(NotifyType.SUBMITTED_CHANGES)) {
-          add(RecipientType.BCC, w.getAccountId());
-        }
-      }
-    } catch (OrmException err) {
-      // Just don't CC everyone. Better to send a partial message to those
-      // we already have queued up then to fail deliver entirely to people
-      // who have a lower interest in the change.
-    }
-  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/mail/OutgoingEmail.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.mail;
 
+import com.google.common.collect.Sets;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.UserIdentity;
 import com.google.gerrit.server.account.AccountState;
@@ -34,14 +35,13 @@ import java.io.StringReader;
 import java.io.StringWriter;
 import java.net.MalformedURLException;
 import java.net.URL;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /** Sends an email to one or more interested parties. */
 public abstract class OutgoingEmail {
@@ -53,7 +53,7 @@ public abstract class OutgoingEmail {
   protected String messageClass;
   private final HashSet<Account.Id> rcptTo = new HashSet<Account.Id>();
   private final Map<String, EmailHeader> headers;
-  private final List<Address> smtpRcptTo = new ArrayList<Address>();
+  private final Set<Address> smtpRcptTo = Sets.newHashSet();
   private Address smtpFromAddress;
   private StringBuilder body;
   protected VelocityContext velocityContext;
@@ -282,7 +282,7 @@ public abstract class OutgoingEmail {
       return false;
     }
 
-    if (rcptTo.size() == 1 && rcptTo.contains(fromId)) {
+    if (smtpRcptTo.size() == 1 && rcptTo.size() == 1 && rcptTo.contains(fromId)) {
       // If the only recipient is also the sender, don't bother.
       //
       return false;
@@ -324,14 +324,15 @@ public abstract class OutgoingEmail {
   protected void add(final RecipientType rt, final Address addr) {
     if (addr != null && addr.email != null && addr.email.length() > 0) {
       if (args.emailSender.canEmail(addr.email)) {
-        smtpRcptTo.add(addr);
-        switch (rt) {
-          case TO:
-            ((EmailHeader.AddressList) headers.get(HDR_TO)).add(addr);
-            break;
-          case CC:
-            ((EmailHeader.AddressList) headers.get(HDR_CC)).add(addr);
-            break;
+        if (smtpRcptTo.add(addr)) {
+          switch (rt) {
+            case TO:
+              ((EmailHeader.AddressList) headers.get(HDR_TO)).add(addr);
+              break;
+            case CC:
+              ((EmailHeader.AddressList) headers.get(HDR_CC)).add(addr);
+              break;
+          }
         }
       } else {
         log.warn("Not emailing " + addr.email + " (prohibited by allowrcpt)");

gerrit-server/src/main/java/com/google/gerrit/server/mail/RestoredSender.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.mail;
 
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
@@ -38,7 +39,7 @@ public class RestoredSender extends ReplyToChangeSender {
 
     ccAllApprovals();
     bccStarredBy();
-    bccWatchesNotifyAllComments();
+    bccWatches(NotifyType.ALL_COMMENTS);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/mail/RevertedSender.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.mail;
 
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
@@ -37,7 +38,7 @@ public class RevertedSender extends ReplyToChangeSender {
 
     ccAllApprovals();
     bccStarredBy();
-    bccWatchesNotifyAllComments();
+    bccWatches(NotifyType.ALL_COMMENTS);
   }
 
   @Override

gerrit-server/src/main/java/com/google/gerrit/server/query/Predicate.java

@@ -43,6 +43,12 @@ import java.util.List;
  * @type <T> type of object the predicate can evaluate in memory.
  */
 public abstract class Predicate<T> {
+  /** A predicate that matches any input, always, with no cost. */
+  @SuppressWarnings("unchecked")
+  public static <T> Predicate<T> any() {
+    return (Predicate<T>) Any.INSTANCE;
+  }
+
   /** Combine the passed predicates into a single AND node. */
   public static <T> Predicate<T> and(final Predicate<T>... that) {
     if (that.length == 1) {
@@ -120,4 +126,36 @@ public abstract class Predicate<T> {
 
   @Override
   public abstract boolean equals(Object other);
+
+  private static class Any<T> extends Predicate<T> {
+    private static final Any<Object> INSTANCE = new Any<Object>();
+
+    private Any() {
+    }
+
+    @Override
+    public Predicate<T> copy(Collection<? extends Predicate<T>> children) {
+      return this;
+    }
+
+    @Override
+    public boolean match(T object) {
+      return true;
+    }
+
+    @Override
+    public int getCost() {
+      return 0;
+    }
+
+    @Override
+    public int hashCode() {
+      return System.identityHashCode(this);
+    }
+
+    @Override
+    public boolean equals(Object other) {
+      return other == this;
+    }
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java

@@ -27,15 +27,15 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.Set;
 
-final class SingleGroupUser extends CurrentUser {
+public final class SingleGroupUser extends CurrentUser {
   private final GroupMembership groups;
 
-  SingleGroupUser(CapabilityControl.Factory capabilityControlFactory,
+  public SingleGroupUser(CapabilityControl.Factory capabilityControlFactory,
       AccountGroup.UUID groupId) {
     this(capabilityControlFactory, Collections.singleton(groupId));
   }
 
-  SingleGroupUser(CapabilityControl.Factory capabilityControlFactory,
+  public SingleGroupUser(CapabilityControl.Factory capabilityControlFactory,
       Set<AccountGroup.UUID> groups) {
     super(capabilityControlFactory, AccessPath.UNKNOWN);
     this.groups = new ListGroupMembership(groups);