Allow to propose changes to access rights through code review

Users that are able to upload changes for code review for the
refs/meta/config branch can now propose changes to the project access
rights through code review directly from the ProjectAccessScreen.

When editing the project access rights there will be a new button
'Save for Review' which will create a new change for the access
rights modifications. Project owners are automatically added as
reviewer to this change. If a project owner agrees to the access rights
modifications he can simply approve and submit the change.

Change-Id: Ica40ce3f57726bdb897e01783cc6da10f1d392cc
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
Signed-off-by: Sasa Zivkov <sasa.zivkov@sap.com>
Signed-off-by: Gustaf Lundh <gustaf.lundh@sonymobile.com>
Signed-off-by: Ulrik Sjölin <ulrik.sjolin@sonyericsson.com>

gerrit-common/src/main/java/com/google/gerrit/common/data/ProjectAccess.java

@@ -26,6 +26,7 @@ public class ProjectAccess {
   protected List<AccessSection> local;
   protected Set<String> ownerOf;
   protected boolean isConfigVisible;
+  protected boolean canUpload;
 
   public ProjectAccess() {
   }
@@ -94,4 +95,12 @@ public class ProjectAccess {
   public void setConfigVisible(boolean isConfigVisible) {
     this.isConfigVisible = isConfigVisible;
   }
+
+  public boolean canUpload() {
+    return canUpload;
+  }
+
+  public void setCanUpload(boolean canUpload) {
+    this.canUpload = canUpload;
+  }
 }

gerrit-common/src/main/java/com/google/gerrit/common/data/ProjectAdminService.java

@@ -16,6 +16,7 @@ package com.google.gerrit.common.data;
 
 import com.google.gerrit.common.auth.SignInRequired;
 import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gwtjsonrpc.common.AsyncCallback;
 import com.google.gwtjsonrpc.common.RemoteJsonService;
@@ -50,6 +51,11 @@ public interface ProjectAdminService extends RemoteJsonService {
       String message, List<AccessSection> sections,
       AsyncCallback<ProjectAccess> callback);
 
+  @SignInRequired
+  void reviewProjectAccess(Project.NameKey projectName, String baseRevision,
+      String message, List<AccessSection> sections,
+      AsyncCallback<Change.Id> callback);
+
   void listBranches(Project.NameKey projectName,
       AsyncCallback<ListBranchesResult> callback);
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.java

@@ -101,4 +101,5 @@ public interface GerritConstants extends Constants {
   String jumpMineDraftComments();
 
   String projectAccessError();
+  String projectAccessProposeForReviewHint();
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/GerritConstants.properties

@@ -84,3 +84,4 @@ jumpMineStarred = Go to starred changes
 jumpMineDraftComments = Go to draft comments
 
 projectAccessError = You don't have permissions to modify the access rights for the following refs:
+projectAccessProposeForReviewHint = You may propose these modifications to the project owners by clicking on 'Save for Review'.

gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java

@@ -182,7 +182,7 @@ public class AccessSectionEditor extends Composite implements
     Collections.sort(value.getPermissions());
 
     this.value = value;
-    this.readOnly = !editing || !projectAccess.isOwnerOf(value);
+    this.readOnly = !editing || !(projectAccess.isOwnerOf(value) || projectAccess.canUpload());
 
     name.setEnabled(!readOnly);
     deleteSection.setVisible(!readOnly);

gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessEditor.java

@@ -120,7 +120,7 @@ public class ProjectAccessEditor extends Composite implements
       history.getStyle().setDisplay(Display.NONE);
     }
 
-    addSection.setVisible(value != null && editing && !value.getOwnerOf().isEmpty());
+    addSection.setVisible(value != null && editing && (!value.getOwnerOf().isEmpty() || value.canUpload()));
   }
 
   @Override

gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.java

@@ -20,6 +20,7 @@ import com.google.gerrit.client.rpc.ScreenLoadCallback;
 import com.google.gerrit.common.PageLinks;
 import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.ProjectAccess;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gwt.core.client.GWT;
 import com.google.gwt.dom.client.DivElement;
@@ -78,6 +79,9 @@ public class ProjectAccessScreen extends ProjectScreen {
   @UiField
   Button commit;
 
+  @UiField
+  Button review;
+
   private Driver driver;
 
   private ProjectAccess access;
@@ -111,8 +115,8 @@ public class ProjectAccessScreen extends ProjectScreen {
   private void displayReadOnly(ProjectAccess access) {
     this.access = access;
     accessEditor.setEditing(false);
-    UIObject.setVisible(editTools, !access.getOwnerOf().isEmpty());
+    UIObject.setVisible(editTools, !access.getOwnerOf().isEmpty() || access.canUpload());
-    edit.setEnabled(!access.getOwnerOf().isEmpty());
+    edit.setEnabled(!access.getOwnerOf().isEmpty() || access.canUpload());
     cancel1.setVisible(false);
     UIObject.setVisible(commitTools, false);
     driver.edit(access);
@@ -125,6 +129,8 @@ public class ProjectAccessScreen extends ProjectScreen {
     edit.setEnabled(false);
     cancel1.setVisible(true);
     UIObject.setVisible(commitTools, true);
+    commit.setVisible(!access.getOwnerOf().isEmpty());
+    review.setVisible(access.canUpload());
     accessEditor.setEditing(true);
     driver.edit(access);
   }
@@ -180,6 +186,9 @@ public class ProjectAccessScreen extends ProjectScreen {
               for (final String diff : diffs) {
                 error.add(new Label(diff));
               }
+              if (access.canUpload()) {
+                error.add(new Label(Gerrit.C.projectAccessProposeForReviewHint()));
+              }
             }
           }
 
@@ -211,9 +220,52 @@ public class ProjectAccessScreen extends ProjectScreen {
         });
   }
 
+  @UiHandler("review")
+  void onReview(ClickEvent event) {
+    final ProjectAccess access = driver.flush();
+
+    if (driver.hasErrors()) {
+      Window.alert(Util.C.errorsMustBeFixed());
+      return;
+    }
+
+    String message = commitMessage.getText().trim();
+    if ("".equals(message)) {
+      message = null;
+    }
+
+    enable(false);
+    Util.PROJECT_SVC.reviewProjectAccess( //
+        getProjectKey(), //
+        access.getRevision(), //
+        message, //
+        access.getLocal(), //
+        new GerritCallback<Change.Id>() {
+          @Override
+          public void onSuccess(Change.Id changeId) {
+            enable(true);
+            commitMessage.setText("");
+            error.clear();
+            if (changeId != null) {
+              Gerrit.display(PageLinks.toChange(changeId));
+            } else {
+              displayReadOnly(access);
+            }
+          }
+
+          @Override
+          public void onFailure(Throwable caught) {
+            error.clear();
+            enable(true);
+            super.onFailure(caught);
+          }
+        });
+  }
+
   private void enable(boolean enabled) {
     commitMessage.setEnabled(enabled);
-    commit.setEnabled(enabled);
+    commit.setEnabled(enabled ? !access.getOwnerOf().isEmpty() : false);
+    review.setEnabled(enabled ? access.canUpload() : false);
     cancel1.setEnabled(enabled);
     cancel2.setEnabled(enabled);
   }

gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectAccessScreen.ui.xml

@@ -72,6 +72,11 @@ limitations under the License.
         text='Save Changes'>
       <ui:attribute name='text'/>
     </g:Button>
+    <g:Button
+        ui:field='review'
+        text='Save for Review'>
+      <ui:attribute name='text'/>
+    </g:Button>
     <g:Button
         ui:field='cancel2'
         text='Cancel'>

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ChangeProjectAccess.java

@@ -15,41 +15,26 @@
 package com.google.gerrit.httpd.rpc.project;
 
 import com.google.gerrit.common.data.AccessSection;
-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.ProjectAccess;
-import com.google.gerrit.common.errors.InvalidNameException;
-import com.google.gerrit.common.errors.NoSuchGroupException;
-import com.google.gerrit.httpd.rpc.Handler;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.account.GroupBackend;
-import com.google.gerrit.server.account.GroupBackends;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectControl;
-import com.google.gerrit.server.project.RefControl;
-import com.google.gwtorm.server.OrmConcurrencyException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.ObjectId;
 
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.HashSet;
-import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.Map;
-import java.util.Set;
 
 import javax.annotation.Nullable;
 
-class ChangeProjectAccess extends Handler<ProjectAccess> {
+class ChangeProjectAccess extends ProjectAccessHandler<ProjectAccess> {
   interface Factory {
     ChangeProjectAccess create(@Assisted Project.NameKey projectName,
         @Nullable @Assisted ObjectId base,
@@ -58,15 +43,7 @@ class ChangeProjectAccess extends Handler<ProjectAccess> {
   }
 
   private final ProjectAccessFactory.Factory projectAccessFactory;
-  private final ProjectControl.Factory projectControlFactory;
   private final ProjectCache projectCache;
-  private final GroupBackend groupBackend;
-  private final MetaDataUpdate.User metaDataUpdateFactory;
-
-  private final Project.NameKey projectName;
-  private final ObjectId base;
-  private List<AccessSection> sectionList;
-  private String message;
 
   @Inject
   ChangeProjectAccess(final ProjectAccessFactory.Factory projectAccessFactory,
@@ -78,132 +55,17 @@ class ChangeProjectAccess extends Handler<ProjectAccess> {
       @Nullable @Assisted final ObjectId base,
       @Assisted List<AccessSection> sectionList,
       @Nullable @Assisted String message) {
+    super(projectControlFactory, groupBackend, metaDataUpdateFactory,
+        projectName, base, sectionList, message, true);
     this.projectAccessFactory = projectAccessFactory;
-    this.projectControlFactory = projectControlFactory;
     this.projectCache = projectCache;
-    this.groupBackend = groupBackend;
-    this.metaDataUpdateFactory = metaDataUpdateFactory;
-
-    this.projectName = projectName;
-    this.base = base;
-    this.sectionList = sectionList;
-    this.message = message;
   }
 
   @Override
-  public ProjectAccess call() throws NoSuchProjectException, IOException,
+  protected ProjectAccess updateProjectConfig(ProjectConfig config,
-      ConfigInvalidException, InvalidNameException, NoSuchGroupException,
+      MetaDataUpdate md) throws IOException, NoSuchProjectException, ConfigInvalidException {
-      OrmConcurrencyException {
+    config.commit(md);
-    final ProjectControl projectControl =
-        projectControlFactory.controlFor(projectName);
-
-    final MetaDataUpdate md;
-    try {
-      md = metaDataUpdateFactory.create(projectName);
-    } catch (RepositoryNotFoundException notFound) {
-      throw new NoSuchProjectException(projectName);
-    }
-    try {
-      ProjectConfig config = ProjectConfig.read(md, base);
-      Set<String> toDelete = scanSectionNames(config);
-
-      for (AccessSection section : mergeSections(sectionList)) {
-        String name = section.getName();
-
-        if (AccessSection.GLOBAL_CAPABILITIES.equals(name)) {
-          if (!projectControl.isOwner()) {
-            continue;
-          }
-          replace(config, toDelete, section);
-
-        } else if (AccessSection.isValid(name)) {
-          if (!projectControl.controlForRef(name).isOwner()) {
-            continue;
-          }
-
-          RefControl.validateRefPattern(name);
-
-          replace(config, toDelete, section);
-        }
-      }
-
-      for (String name : toDelete) {
-        if (AccessSection.GLOBAL_CAPABILITIES.equals(name)) {
-          if (projectControl.isOwner()) {
-            config.remove(config.getAccessSection(name));
-          }
-
-        } else if (projectControl.controlForRef(name).isOwner()) {
-          config.remove(config.getAccessSection(name));
-        }
-      }
-
-      if (message != null && !message.isEmpty()) {
-        if (!message.endsWith("\n")) {
-          message += "\n";
-        }
-        md.setMessage(message);
-      } else {
-        md.setMessage("Modify access rules\n");
-      }
-
-      if (config.commit(md)) {
     projectCache.evict(config.getProject());
     return projectAccessFactory.create(projectName).call();
-
-      } else {
-        throw new OrmConcurrencyException("Cannot update " + projectName);
-      }
-    } finally {
-      md.close();
-    }
-  }
-
-  private void replace(ProjectConfig config, Set<String> toDelete,
-      AccessSection section) throws NoSuchGroupException {
-    for (Permission permission : section.getPermissions()) {
-      for (PermissionRule rule : permission.getRules()) {
-        lookupGroup(rule);
-      }
-    }
-    config.replace(section);
-    toDelete.remove(section.getName());
-  }
-
-  private static List<AccessSection> mergeSections(List<AccessSection> src) {
-    Map<String, AccessSection> map = new LinkedHashMap<String, AccessSection>();
-    for (AccessSection section : src) {
-      if (section.getPermissions().isEmpty()) {
-        continue;
-      }
-
-      AccessSection prior = map.get(section.getName());
-      if (prior != null) {
-        prior.mergeFrom(section);
-      } else {
-        map.put(section.getName(), section);
-      }
-    }
-    return new ArrayList<AccessSection>(map.values());
-  }
-
-  private static Set<String> scanSectionNames(ProjectConfig config) {
-    Set<String> names = new HashSet<String>();
-    for (AccessSection section : config.getAccessSections()) {
-      names.add(section.getName());
-    }
-    return names;
-  }
-
-  private void lookupGroup(PermissionRule rule) throws NoSuchGroupException {
-    GroupReference ref = rule.getGroup();
-    if (ref.getUUID() == null) {
-      final GroupReference group =
-          GroupBackends.findBestSuggestion(groupBackend, ref.getName());
-      if (group == null) {
-        throw new NoSuchGroupException(ref.getName());
-      }
-      ref.setUUID(group.getUUID());
-    }
   }
 }

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ChangeProjectSettings.java

@@ -86,10 +86,11 @@ class ChangeProjectSettings extends Handler<ProjectDetail> {
       config.getProject().copySettingsFrom(update);
 
       md.setMessage("Modified project settings\n");
-      if (config.commit(md)) {
+      try {
+        config.commit(md);
         mgr.setProjectDescription(projectName, update.getDescription());
         userCache.get().evict(config.getProject());
-      } else {
+      } catch (IOException e) {
         throw new OrmConcurrencyException("Cannot update " + projectName);
       }
     } catch (ConfigInvalidException err) {

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java

@@ -96,10 +96,9 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
 
       if (config.updateGroupNames(groupBackend)) {
         md.setMessage("Update group names\n");
-        if (config.commit(md)) {
+        config.commit(md);
         projectCache.evict(config.getProject());
         pc = open();
-        }
       } else if (config.getRevision() != null
           && !config.getRevision().equals(
               pc.getProjectState().getConfig().getRevision())) {
@@ -196,6 +195,8 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
 
     detail.setLocal(local);
     detail.setOwnerOf(ownerOf);
+    detail.setCanUpload(pc.isOwner()
+        || (metaConfigControl.isVisible() && metaConfigControl.canUpload()));
     detail.setConfigVisible(pc.isOwner() || metaConfigControl.isVisible());
     return detail;
   }

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessHandler.java

@@ -0,0 +1,190 @@
+// 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.httpd.rpc.project;
+
+import com.google.gerrit.common.data.AccessSection;
+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.errors.InvalidNameException;
+import com.google.gerrit.common.errors.NoSuchGroupException;
+import com.google.gerrit.httpd.rpc.Handler;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.account.GroupBackends;
+import com.google.gerrit.server.git.MetaDataUpdate;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectControl;
+import com.google.gerrit.server.project.RefControl;
+import com.google.gwtorm.server.OrmException;
+
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.ObjectId;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+public abstract class ProjectAccessHandler<T> extends Handler<T> {
+
+  private final ProjectControl.Factory projectControlFactory;
+  protected final GroupBackend groupBackend;
+  private final MetaDataUpdate.User metaDataUpdateFactory;
+
+  protected final Project.NameKey projectName;
+  protected final ObjectId base;
+  private List<AccessSection> sectionList;
+  protected String message;
+  private boolean checkIfOwner;
+
+  protected ProjectAccessHandler(
+      final ProjectControl.Factory projectControlFactory,
+      final GroupBackend groupBackend,
+      final MetaDataUpdate.User metaDataUpdateFactory,
+      final Project.NameKey projectName, final ObjectId base,
+      final List<AccessSection> sectionList, final String message,
+      final boolean checkIfOwner) {
+    this.projectControlFactory = projectControlFactory;
+    this.groupBackend = groupBackend;
+    this.metaDataUpdateFactory = metaDataUpdateFactory;
+
+    this.projectName = projectName;
+    this.base = base;
+    this.sectionList = sectionList;
+    this.message = message;
+    this.checkIfOwner = checkIfOwner;
+  }
+
+  @Override
+  public final T call() throws NoSuchProjectException, IOException,
+      ConfigInvalidException, InvalidNameException, NoSuchGroupException,
+      OrmException {
+    final ProjectControl projectControl =
+        projectControlFactory.controlFor(projectName);
+
+    final MetaDataUpdate md;
+    try {
+      md = metaDataUpdateFactory.create(projectName);
+    } catch (RepositoryNotFoundException notFound) {
+      throw new NoSuchProjectException(projectName);
+    }
+    try {
+      ProjectConfig config = ProjectConfig.read(md, base);
+      Set<String> toDelete = scanSectionNames(config);
+
+      for (AccessSection section : mergeSections(sectionList)) {
+        String name = section.getName();
+
+        if (AccessSection.GLOBAL_CAPABILITIES.equals(name)) {
+          if (checkIfOwner && !projectControl.isOwner()) {
+            continue;
+          }
+          replace(config, toDelete, section);
+
+        } else if (AccessSection.isValid(name)) {
+          if (checkIfOwner && !projectControl.controlForRef(name).isOwner()) {
+            continue;
+          }
+
+          RefControl.validateRefPattern(name);
+
+          replace(config, toDelete, section);
+        }
+      }
+
+      for (String name : toDelete) {
+        if (AccessSection.GLOBAL_CAPABILITIES.equals(name)) {
+          if (!checkIfOwner || projectControl.isOwner()) {
+            config.remove(config.getAccessSection(name));
+          }
+
+        } else if (!checkIfOwner ||  projectControl.controlForRef(name).isOwner()) {
+          config.remove(config.getAccessSection(name));
+        }
+      }
+
+      if (message != null && !message.isEmpty()) {
+        if (!message.endsWith("\n")) {
+          message += "\n";
+        }
+        md.setMessage(message);
+      } else {
+        md.setMessage("Modify access rules\n");
+      }
+
+      return updateProjectConfig(config, md);
+    } finally {
+      md.close();
+    }
+  }
+
+  protected abstract T updateProjectConfig(ProjectConfig config,
+      MetaDataUpdate md) throws IOException, NoSuchProjectException,
+      ConfigInvalidException, OrmException;
+
+  private void replace(ProjectConfig config, Set<String> toDelete,
+      AccessSection section) throws NoSuchGroupException {
+    for (Permission permission : section.getPermissions()) {
+      for (PermissionRule rule : permission.getRules()) {
+        lookupGroup(rule);
+      }
+    }
+    config.replace(section);
+    toDelete.remove(section.getName());
+  }
+
+  private static List<AccessSection> mergeSections(List<AccessSection> src) {
+    Map<String, AccessSection> map = new LinkedHashMap<String, AccessSection>();
+    for (AccessSection section : src) {
+      if (section.getPermissions().isEmpty()) {
+        continue;
+      }
+
+      AccessSection prior = map.get(section.getName());
+      if (prior != null) {
+        prior.mergeFrom(section);
+      } else {
+        map.put(section.getName(), section);
+      }
+    }
+    return new ArrayList<AccessSection>(map.values());
+  }
+
+  private static Set<String> scanSectionNames(ProjectConfig config) {
+    Set<String> names = new HashSet<String>();
+    for (AccessSection section : config.getAccessSections()) {
+      names.add(section.getName());
+    }
+    return names;
+  }
+
+  private void lookupGroup(PermissionRule rule) throws NoSuchGroupException {
+    GroupReference ref = rule.getGroup();
+    if (ref.getUUID() == null) {
+      final GroupReference group =
+          GroupBackends.findBestSuggestion(groupBackend, ref.getName());
+      if (group == null) {
+        throw new NoSuchGroupException(ref.getName());
+      }
+      ref.setUUID(group.getUUID());
+    }
+  }
+}

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAdminServiceImpl.java

@@ -20,6 +20,7 @@ import com.google.gerrit.common.data.ProjectAccess;
 import com.google.gerrit.common.data.ProjectAdminService;
 import com.google.gerrit.common.data.ProjectDetail;
 import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gwtjsonrpc.common.AsyncCallback;
 import com.google.gwtjsonrpc.common.VoidResult;
@@ -33,6 +34,7 @@ import java.util.Set;
 class ProjectAdminServiceImpl implements ProjectAdminService {
   private final AddBranch.Factory addBranchFactory;
   private final ChangeProjectAccess.Factory changeProjectAccessFactory;
+  private final ReviewProjectAccess.Factory reviewProjectAccessFactory;
   private final ChangeProjectSettings.Factory changeProjectSettingsFactory;
   private final DeleteBranches.Factory deleteBranchesFactory;
   private final ListBranches.Factory listBranchesFactory;
@@ -44,6 +46,7 @@ class ProjectAdminServiceImpl implements ProjectAdminService {
   @Inject
   ProjectAdminServiceImpl(final AddBranch.Factory addBranchFactory,
       final ChangeProjectAccess.Factory changeProjectAccessFactory,
+      final ReviewProjectAccess.Factory reviewProjectAccessFactory,
       final ChangeProjectSettings.Factory changeProjectSettingsFactory,
       final DeleteBranches.Factory deleteBranchesFactory,
       final ListBranches.Factory listBranchesFactory,
@@ -53,6 +56,7 @@ class ProjectAdminServiceImpl implements ProjectAdminService {
       final CreateProjectHandler.Factory createNewProjectFactory) {
     this.addBranchFactory = addBranchFactory;
     this.changeProjectAccessFactory = changeProjectAccessFactory;
+    this.reviewProjectAccessFactory = reviewProjectAccessFactory;
     this.changeProjectSettingsFactory = changeProjectSettingsFactory;
     this.deleteBranchesFactory = deleteBranchesFactory;
     this.listBranchesFactory = listBranchesFactory;
@@ -98,6 +102,14 @@ class ProjectAdminServiceImpl implements ProjectAdminService {
     changeProjectAccessFactory.create(projectName, base, sections, msg).to(cb);
   }
 
+  @Override
+  public void reviewProjectAccess(Project.NameKey projectName,
+      String baseRevision, String msg, List<AccessSection> sections,
+      AsyncCallback<Change.Id> cb) {
+    ObjectId base = ObjectId.fromString(baseRevision);
+    reviewProjectAccessFactory.create(projectName, base, sections, msg).to(cb);
+  }
+
   @Override
   public void listBranches(final Project.NameKey projectName,
       final AsyncCallback<ListBranchesResult> callback) {

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectModule.java

@@ -30,6 +30,7 @@ public class ProjectModule extends RpcServletModule {
       protected void configure() {
         factory(AddBranch.Factory.class);
         factory(ChangeProjectAccess.Factory.class);
+        factory(ReviewProjectAccess.Factory.class);
         factory(CreateProjectHandler.Factory.class);
         factory(ChangeProjectSettings.Factory.class);
         factory(DeleteBranches.Factory.class);

gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java

@@ -0,0 +1,127 @@
+// 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.httpd.rpc.project;
+
+import com.google.gerrit.common.data.AccessSection;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.PatchSetInfo;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.MetaDataUpdate;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.patch.AddReviewer;
+import com.google.gerrit.server.patch.PatchSetInfoFactory;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectControl;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevCommit;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+
+import javax.annotation.Nullable;
+
+public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
+  interface Factory {
+    ReviewProjectAccess create(@Assisted Project.NameKey projectName,
+        @Nullable @Assisted ObjectId base,
+        @Assisted List<AccessSection> sectionList,
+        @Nullable @Assisted String message);
+  }
+
+  private final ReviewDb db;
+  private final IdentifiedUser user;
+  private final PatchSetInfoFactory patchSetInfoFactory;
+  private final AddReviewer.Factory addReviewerFactory;
+
+  @Inject
+  ReviewProjectAccess(final ProjectControl.Factory projectControlFactory,
+      final GroupBackend groupBackend,
+      final MetaDataUpdate.User metaDataUpdateFactory, final ReviewDb db,
+      final IdentifiedUser user, final PatchSetInfoFactory patchSetInfoFactory,
+      final AddReviewer.Factory addReviewerFactory,
+
+      @Assisted final Project.NameKey projectName,
+      @Nullable @Assisted final ObjectId base,
+      @Assisted List<AccessSection> sectionList,
+      @Nullable @Assisted String message) {
+    super(projectControlFactory, groupBackend, metaDataUpdateFactory,
+        projectName, base, sectionList, message, false);
+    this.db = db;
+    this.user = user;
+    this.patchSetInfoFactory = patchSetInfoFactory;
+    this.addReviewerFactory = addReviewerFactory;
+  }
+
+  @Override
+  protected Change.Id updateProjectConfig(ProjectConfig config, MetaDataUpdate md)
+      throws IOException, NoSuchProjectException, ConfigInvalidException, OrmException {
+    int nextChangeId = db.nextChangeId();
+    PatchSet.Id patchSetId = new PatchSet.Id(new Change.Id(nextChangeId), 1);
+    final PatchSet ps = new PatchSet(patchSetId);
+    RevCommit commit = config.commitToNewRef(md, ps.getRefName());
+    if (commit.getId().equals(base)) {
+      return null;
+    }
+    Change.Key changeKey = new Change.Key("I" + commit.name());
+    final Change change =
+        new Change(changeKey, new Change.Id(nextChangeId), user.getAccountId(),
+            new Branch.NameKey(config.getProject().getNameKey(),
+                GitRepositoryManager.REF_CONFIG));
+    change.nextPatchSetId();
+
+    ps.setCreatedOn(change.getCreatedOn());
+    ps.setUploader(user.getAccountId());
+    ps.setRevision(new RevId(commit.name()));
+
+    db.patchSets().insert(Collections.singleton(ps));
+
+    final PatchSetInfo info = patchSetInfoFactory.get(commit, ps.getId());
+    change.setCurrentPatchSet(info);
+    ChangeUtil.updated(change);
+
+    db.changes().insert(Collections.singleton(change));
+
+    addProjectOwnersAsReviewers(change.getId());
+
+    return change.getId();
+  }
+
+  private void addProjectOwnersAsReviewers(final Change.Id changeId) {
+    final String projectOwners =
+        groupBackend.get(AccountGroup.PROJECT_OWNERS).getName();
+    try {
+      addReviewerFactory.create(changeId, Collections.singleton(projectOwners),
+          false).call();
+    } catch (Exception e) {
+      // one of the owner groups is not visible to the user and this it why it
+      // can't be added as reviewer
+    }
+  }
+}

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

@@ -123,11 +123,13 @@ public class RenameGroupOp extends DefaultQueueOp {
       ref.setName(newName);
       md.getCommitBuilder().setAuthor(author);
       md.setMessage("Rename group " + oldName + " to " + newName + "\n");
-      if (config.commit(md)) {
+      try {
+        config.commit(md);
         projectCache.evict(config.getProject());
         success = true;
-
+      } catch (IOException e) {
-      } else {
+        log.error("Could not commit rename of group " + oldName + " to "
+            + newName + " in " + md.getProjectName().get(), e);
         try {
           Thread.sleep(25 /* milliseconds */);
         } catch (InterruptedException wakeUp) {

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

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.git;
 
 import com.google.common.base.Objects;
+
 import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.dircache.DirCacheBuilder;
 import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -145,12 +146,12 @@ public abstract class VersionedMetaData {
    * Update this metadata branch, recording a new commit on its reference.
    *
    * @param update helper information to define the update that will occur.
-   * @return true if the update was successful, false if it failed because of a
+   * @return the commit that was created
-   *         concurrent update to the same reference.
    * @throws IOException if there is a storage problem and the update cannot be
-   *         executed as requested.
+   *         executed as requested or if it failed because of a concurrent
+   *         update to the same reference
    */
-  public boolean commit(MetaDataUpdate update) throws IOException {
+  public RevCommit commit(MetaDataUpdate update) throws IOException {
     BatchMetaDataUpdate batch = openUpdate(update);
     try {
       batch.write(update.getCommitBuilder());
@@ -160,11 +161,32 @@ public abstract class VersionedMetaData {
     }
   }
 
+  /**
+   * Creates a new commit and a new ref based on this commit.
+   *
+   * @param update helper information to define the update that will occur.
+   * @param refName name of the ref that should be created
+   * @return the commit that was created
+   * @throws IOException if there is a storage problem and the update cannot be
+   *         executed as requested or if it failed because of a concurrent
+   *         update to the same reference
+   */
+  public RevCommit commitToNewRef(MetaDataUpdate update, String refName) throws IOException {
+    BatchMetaDataUpdate batch = openUpdate(update);
+    try {
+      batch.write(update.getCommitBuilder());
+      return batch.createRef(refName);
+    } finally {
+      batch.close();
+    }
+  }
+
   public interface BatchMetaDataUpdate {
     void write(CommitBuilder commit) throws IOException;
     void write(VersionedMetaData config, CommitBuilder commit) throws IOException;
-    boolean commit() throws IOException;
+    RevCommit createRef(String refName) throws IOException;
-    boolean commitAt(ObjectId revision) throws IOException;
+    RevCommit commit() throws IOException;
+    RevCommit commitAt(ObjectId revision) throws IOException;
     void close();
   }
 
@@ -224,14 +246,35 @@ public abstract class VersionedMetaData {
       }
 
       @Override
-      public boolean commit() throws IOException {
+      public RevCommit createRef(String refName) throws IOException {
+        if (Objects.equal(src, revision)) {
+          return revision;
+        }
+
+        RefUpdate ru = db.updateRef(refName);
+        ru.setExpectedOldObjectId(ObjectId.zeroId());
+        ru.setNewObjectId(src);
+        RefUpdate.Result result = ru.update();
+        switch (result) {
+          case NEW:
+            revision = rw.parseCommit(ru.getNewObjectId());
+            update.replicate(ru.getName());
+            return revision;
+          default:
+            throw new IOException("Cannot update " + ru.getName() + " in "
+                + db.getDirectory() + ": " + ru.getResult());
+        }
+      }
+
+      @Override
+      public RevCommit commit() throws IOException {
         return commitAt(revision);
       }
 
       @Override
-      public boolean commitAt(ObjectId expected) throws IOException {
+      public RevCommit commitAt(ObjectId expected) throws IOException {
         if (Objects.equal(src, expected)) {
-          return true;
+          return revision;
         }
 
         RefUpdate ru = db.updateRef(getRefName());
@@ -249,10 +292,7 @@ public abstract class VersionedMetaData {
           case FAST_FORWARD:
             revision = rw.parseCommit(ru.getNewObjectId());
             update.replicate(ru.getName());
-            return true;
+            return revision;
-
-          case LOCK_FAILURE:
-            return false;
 
           default:
             throw new IOException("Cannot update " + ru.getName() + " in "

gerrit-server/src/main/java/com/google/gerrit/server/project/CreateProject.java

@@ -198,10 +198,7 @@ public class CreateProject {
       }
 
       md.setMessage("Created project\n");
-      if (!config.commit(md)) {
+      config.commit(md);
-        throw new IOException("Cannot create "
-            + createProjectArgs.getProjectName());
-      }
     } finally {
       md.close();
     }

gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaCreator.java

@@ -257,9 +257,7 @@ public class SchemaCreator {
       metaReadPermission.add(rule(config, owners));
 
       md.setMessage("Initialized Gerrit Code Review " + Version.getVersion());
-      if (!config.commit(md)) {
+      config.commit(md);
-        throw new IOException("Cannot create " + allProjectsName.get());
-      }
     } finally {
       git.close();
     }

gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_53.java

@@ -184,9 +184,7 @@ class Schema_53 extends SchemaVersion {
         }
 
         md.setMessage("Import project configuration from SQL\n");
-        if (!config.commit(md)) {
+        config.commit(md);
-          throw new OrmException("Cannot export project " + name);
-        }
       } catch (ConfigInvalidException err) {
         throw new OrmException("Cannot read project " + name, err);
       } catch (IOException err) {

gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_57.java

@@ -135,9 +135,7 @@ public class Schema_57 extends SchemaVersion {
         }
 
         md.setMessage("Upgrade to Gerrit Code Review schema 57\n");
-        if (!config.commit(md)) {
+        config.commit(md);
-          throw new OrmException("Cannot update " + allProjects);
-        }
       } finally {
         git.close();
       }

gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_64.java

@@ -106,9 +106,7 @@ public class Schema_64 extends SchemaVersion {
       }
 
       md.setMessage("Upgrade to Gerrit Code Review schema 64\n");
-      if (!config.commit(md)) {
+      config.commit(md);
-        throw new OrmException("Cannot update " + allProjects);
-      }
     } catch (IOException e) {
       throw new OrmException(e);
     } catch (ConfigInvalidException e) {

gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_65.java

@@ -207,9 +207,7 @@ public class Schema_65 extends SchemaVersion {
         batch.write(config, commit);
 
         // Save the the final metadata.
-        if (!batch.commitAt(config.getRevision())) {
+        batch.commitAt(config.getRevision());
-          throw new OrmException("Cannot update " + allProjects);
-        }
       } finally {
         batch.close();
       }

gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_69.java

@@ -181,9 +181,7 @@ public class Schema_69 extends SchemaVersion {
         }
 
         md.setMessage("Switch LDAP group UUIDs to DNs\n");
-        if (!config.commit(md)) {
+        config.commit(md);
-          throw new OrmException("Cannot update " + name);
-        }
       } catch (IOException e) {
         throw new OrmException(e);
       } catch (ConfigInvalidException e) {

gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java

@@ -209,7 +209,7 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase {
     util.tick(5);
     util.setAuthorAndCommitter(md.getCommitBuilder());
     md.setMessage("Edit\n");
-    assertTrue("commit finished", cfg.commit(md));
+    cfg.commit(md);
 
     Ref ref = db.getRef(GitRepositoryManager.REF_CONFIG);
     return util.getRevWalk().parseCommit(ref.getObjectId());

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/AdminSetParent.java

@@ -137,9 +137,7 @@ final class AdminSetParent extends SshCommand {
           config.getProject().setParentName(newParentKey);
           md.setMessage("Inherit access from "
               + (newParentKey != null ? newParentKey.get() : allProjectsName.get()) + "\n");
-          if (!config.commit(md)) {
+          config.commit(md);
-            err.append("error: Could not update project " + name + "\n");
-          }
         } finally {
           md.close();
         }