Revert changes from topic 'submitWholeTopic'

Recently merged changes related to the 'submitWholeTopic' feature have
introduced regressions in the change screen.

This feature is not targetted for the 2.11 release. Instead of waiting
for fixes on master, just revert the changes on the stable-2.11 branch.

* This reverts the following commits:
  dbefbe0: Merge changes from topic 'submitWholeTopic'
  3bb4767: ChangeScreen: Avoid race condition for revision changes
  6287784: Submit Whole Topic: trigger merge queue for all submitted changes
  b9db274: ChangeScreen: explicitly load revision info
  46c319e: Actions: reloadRevisionActions determines submit button visibility
  a6fa971: ChangeScreen: introduce renderRevisionInfo
  3b0b9c3: ChangeAPI: add call to new actions REST API call
  add7038: ETag computation needs to honor changes in other changes

Note that the whole 'submitWholeTopic' series is not reverted; only the
recent changes that caused regression. The remaining parts of the series
are working as expected and only when the setting is explicitly enabled.

Also remove the documentation of the 'submitWholeTopic' setting from the
configuration documentation.

Change-Id: I4b7538bec1820743c4a4ac0c4100f56eb02d5ef2

Documentation/config-gerrit.txt

@@ -844,30 +844,6 @@ abbreviated commit SHA-1 (`c9c0edb`).
 +
 Default is "Submit patch set ${patchSet} into ${branch}".
 
-[[change.submitWholeTopic]]change.submitWholeTopic::
-+
-Determines if the submit button submits the whole topic instead of
-just the current change.
-+
-Default is false.
-
-[[change.submitTopicLabel]]change.submitTopicLabel::
-+
-If `change.submitWholeTopic` is set and a change has a topic,
-the label name for the submit button is given here instead of
-the configuration `change.submitLabel`.
-+
-Defaults to "Submit whole topic"
-
-[[change.submitTopicTooltip]]change.submitTopicTooltip::
-+
-If `change.submitWholeTopic` is configuerd to true and a change has a
-topic, this configuration determines the tooltip for the submit button
-instead of `change.submitTooltip`. The variable `${topicSize}` is available
-for the number of changes to be submitted.
-+
-Defaults to "Submit all ${topicSize} changes within the topic".
-
 [[change.replyLabel]]change.replyLabel::
 +
 Label name for the reply button. In the user interface an ellipsis (…)

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Actions.java

@@ -65,6 +65,7 @@ class Actions extends Composite {
   private String message;
   private String branch;
   private String key;
+  private boolean canSubmit;
 
   Actions() {
     initWidget(uiBinder.createAndBindUi(this));
@@ -86,12 +87,7 @@ class Actions extends Composite {
     changeInfo = info;
 
     initChangeActions(info, hasUser);
-
-    NativeMap<ActionInfo> actionMap = revInfo.has_actions()
-        ? revInfo.actions()
-        : NativeMap.<ActionInfo> create();
-    actionMap.copyKeysIntoChildren("id");
-    reloadRevisionActions(actionMap);
+    initRevisionActions(info, revInfo, hasUser);
   }
 
   private void initChangeActions(ChangeInfo info, boolean hasUser) {
@@ -111,29 +107,30 @@ class Actions extends Composite {
     }
   }
 
-  void reloadRevisionActions(NativeMap<ActionInfo> actions) {
-    if (!Gerrit.isSignedIn()) {
-      return;
-    }
-    boolean canSubmit = actions.containsKey("submit");
-    if (canSubmit) {
-      ActionInfo action = actions.get("submit");
-      submit.setTitle(action.title());
-      submit.setEnabled(action.enabled());
-      submit.setHTML(new SafeHtmlBuilder()
-          .openDiv()
-          .append(action.label())
-          .closeDiv());
-      submit.setEnabled(action.enabled());
-    }
-    submit.setVisible(canSubmit);
+  private void initRevisionActions(ChangeInfo info, RevisionInfo revInfo,
+      boolean hasUser) {
+    NativeMap<ActionInfo> actions = revInfo.has_actions()
+        ? revInfo.actions()
+        : NativeMap.<ActionInfo> create();
+    actions.copyKeysIntoChildren("id");
 
-    a2b(actions, "cherrypick", cherrypick);
-    a2b(actions, "rebase", rebase);
-
-    RevisionInfo revInfo = changeInfo.revision(revision);
-    for (String id : filterNonCore(actions)) {
-      add(new ActionButton(changeInfo, revInfo, actions.get(id)));
+    canSubmit = false;
+    if (hasUser) {
+      canSubmit = actions.containsKey("submit");
+      if (canSubmit) {
+        ActionInfo action = actions.get("submit");
+        submit.setTitle(action.title());
+        submit.setEnabled(action.enabled());
+        submit.setHTML(new SafeHtmlBuilder()
+            .openDiv()
+            .append(action.label())
+            .closeDiv());
+      }
+      a2b(actions, "cherrypick", cherrypick);
+      a2b(actions, "rebase", rebase);
+      for (String id : filterNonCore(actions)) {
+        add(new ActionButton(info, revInfo, actions.get(id)));
+      }
     }
   }
 
@@ -149,6 +146,10 @@ class Actions extends Composite {
     return ids;
   }
 
+  void setSubmitEnabled() {
+    submit.setVisible(canSubmit);
+  }
+
   @UiHandler("followUp")
   void onFollowUp(@SuppressWarnings("unused") ClickEvent e) {
     if (followUpAction == null) {

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java

@@ -248,7 +248,7 @@ public class ChangeScreen extends Screen {
   void loadChangeInfo(boolean fg, AsyncCallback<ChangeInfo> cb) {
     RestApi call = ChangeApi.detail(changeId.get());
     ChangeList.addOptions(call, EnumSet.of(
-      ListChangesOption.CHANGE_ACTIONS,
+      ListChangesOption.CURRENT_ACTIONS,
       ListChangesOption.ALL_REVISIONS));
     if (!fg) {
       call.background();
@@ -256,18 +256,6 @@ public class ChangeScreen extends Screen {
     call.get(cb);
   }
 
-  void loadRevisionInfo() {
-    RestApi call = ChangeApi.actions(changeId.get(), revision);
-    call.background();
-    call.get(new GerritCallback<NativeMap<ActionInfo>>() {
-      @Override
-      public void onSuccess(NativeMap<ActionInfo> actionMap) {
-        actionMap.copyKeysIntoChildren("id");
-        renderRevisionInfo(changeInfo, actionMap);
-      }
-    });
-  }
-
   @Override
   protected void onUnload() {
     if (replyAction != null) {
@@ -415,8 +403,7 @@ public class ChangeScreen extends Screen {
     }
   }
 
-  private void initRevisionsAction(ChangeInfo info, String revision,
-      NativeMap<ActionInfo> actions) {
+  private void initRevisionsAction(ChangeInfo info, String revision) {
     int currentPatchSet;
     if (info.current_revision() != null
         && info.revisions().containsKey(info.current_revision())) {
@@ -444,6 +431,11 @@ public class ChangeScreen extends Screen {
 
     RevisionInfo revInfo = info.revision(revision);
     if (revInfo.draft()) {
+      NativeMap<ActionInfo> actions = revInfo.has_actions()
+          ? revInfo.actions()
+          : NativeMap.<ActionInfo> create();
+      actions.copyKeysIntoChildren("id");
+
       if (actions.containsKey("publish")) {
         publish.setVisible(true);
         publish.setTitle(actions.get("publish").title());
@@ -814,7 +806,6 @@ public class ChangeScreen extends Screen {
           commentLinkProcessor = result.getCommentLinkProcessor();
           setTheme(result.getTheme());
           renderChangeInfo(info);
-          loadRevisionInfo();
         }
       }));
   }
@@ -942,6 +933,7 @@ public class ChangeScreen extends Screen {
 
   private void loadSubmitType(final Change.Status status, final boolean canSubmit) {
     if (canSubmit) {
+      actions.setSubmitEnabled();
       if (status == Change.Status.NEW) {
         statusText.setInnerText(Util.C.readyToSubmit());
       }
@@ -1051,7 +1043,19 @@ public class ChangeScreen extends Screen {
   private void renderChangeInfo(ChangeInfo info) {
     changeInfo = info;
     lastDisplayedUpdate = info.updated();
+    RevisionInfo revisionInfo = info.revision(revision);
+    boolean current = info.status().isOpen()
+        && revision.equals(info.current_revision())
+        && !revisionInfo.is_edit();
 
+    if (revisionInfo.is_edit()) {
+      statusText.setInnerText(Util.C.changeEdit());
+    } else if (!current && info.status() == Change.Status.NEW) {
+      statusText.setInnerText(Util.C.notCurrent());
+      labels.setVisible(false);
+    } else {
+      statusText.setInnerText(Util.toLongString(info.status()));
+    }
     labels.set(info);
 
     renderOwner(info);
@@ -1060,6 +1064,7 @@ public class ChangeScreen extends Screen {
     initReplyButton(info, revision);
     initIncludedInAction(info);
     initChangeAction(info);
+    initRevisionsAction(info, revision);
     initDownloadAction(info, revision);
     initProjectLinks(info);
     initBranchLink(info);
@@ -1079,37 +1084,6 @@ public class ChangeScreen extends Screen {
       setVisible(hashtagTableRow, false);
     }
 
-    StringBuilder sb = new StringBuilder();
-    sb.append(Util.M.changeScreenTitleId(info.id_abbreviated()));
-    if (info.subject() != null) {
-      sb.append(": ");
-      sb.append(info.subject());
-    }
-    setWindowTitle(sb.toString());
-
-    // Properly render revision actions initially while waiting for
-    // the callback to populate them correctly.
-    renderRevisionInfo(changeInfo, NativeMap.<ActionInfo> create());
-  }
-
-  private void renderRevisionInfo(ChangeInfo info,
-      NativeMap<ActionInfo> actionMap) {
-    RevisionInfo revisionInfo = info.revision(revision);
-    boolean current = info.status().isOpen()
-        && revision.equals(info.current_revision())
-        && !revisionInfo.is_edit();
-
-    if (revisionInfo.is_edit()) {
-      statusText.setInnerText(Util.C.changeEdit());
-    } else if (!current && info.status() == Change.Status.NEW) {
-      statusText.setInnerText(Util.C.notCurrent());
-      labels.setVisible(false);
-    } else {
-      statusText.setInnerText(Util.toLongString(info.status()));
-    }
-
-    initRevisionsAction(info, revision, actionMap);
-
     if (Gerrit.isSignedIn()) {
       replyAction = new ReplyAction(info, revision,
           style, commentLinkProcessor, reply, quickApprove);
@@ -1131,7 +1105,14 @@ public class ChangeScreen extends Screen {
       quickApprove.setVisible(false);
       setVisible(strategy, false);
     }
-    actions.reloadRevisionActions(actionMap);
+
+    StringBuilder sb = new StringBuilder();
+    sb.append(Util.M.changeScreenTitleId(info.id_abbreviated()));
+    if (info.subject() != null) {
+      sb.append(": ");
+      sb.append(info.subject());
+    }
+    setWindowTitle(sb.toString());
   }
 
   private void renderOwner(ChangeInfo info) {

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java

@@ -95,13 +95,6 @@ public class ChangeApi {
     return call(id, "detail");
   }
 
-  public static RestApi actions(int id, String revision) {
-    if (revision == null || revision.equals("")) {
-      revision = "current";
-    }
-    return call(id, revision, "actions");
-  }
-
   public static void edit(int id, AsyncCallback<EditInfo> cb) {
     edit(id).get(cb);
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java

@@ -57,15 +57,17 @@ public class ChangeResource implements RestResource, HasETag {
     return getControl().getNotes();
   }
 
-
-  // This includes all information relevant for ETag computation
-  // unrelated to the UI.
-  public void prepareETag(Hasher h, CurrentUser user) {
-    h.putLong(getChange().getLastUpdatedOn().getTime())
+  @Override
+  public String getETag() {
+    CurrentUser user = control.getCurrentUser();
+    Hasher h = Hashing.md5().newHasher()
+      .putLong(getChange().getLastUpdatedOn().getTime())
       .putInt(getChange().getRowVersion())
+      .putBoolean(user.getStarredChanges().contains(getChange().getId()))
       .putInt(user.isIdentifiedUser()
           ? ((IdentifiedUser) user).getAccountId().get()
           : 0);
+
     byte[] buf = new byte[20];
     ObjectId noteId;
     try {
@@ -80,14 +82,6 @@ public class ChangeResource implements RestResource, HasETag {
     for (ProjectState p : control.getProjectControl().getProjectState().tree()) {
       hashObjectId(h, p.getConfig().getRevision(), buf);
     }
-  }
-
-  @Override
-  public String getETag() {
-    CurrentUser user = control.getCurrentUser();
-    Hasher h = Hashing.md5().newHasher()
-        .putBoolean(user.getStarredChanges().contains(getChange().getId()));
-    prepareETag(h, user);
     return h.hash().toString();
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java

@@ -14,59 +14,22 @@
 
 package com.google.gerrit.server.change;
 
-import com.google.common.base.Strings;
-import com.google.common.hash.Hasher;
-import com.google.common.hash.Hashing;
-import com.google.gerrit.extensions.restapi.ETagView;
 import com.google.gerrit.extensions.restapi.Response;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.InternalChangeQuery;
-import com.google.gwtorm.server.OrmException;
-import com.google.gwtorm.server.OrmRuntimeException;
+import com.google.gerrit.extensions.restapi.RestReadView;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.Singleton;
 
-import org.eclipse.jgit.lib.Config;
-
 @Singleton
-public class GetRevisionActions implements ETagView<RevisionResource> {
+public class GetRevisionActions implements RestReadView<RevisionResource> {
   private final ActionJson delegate;
-  private final Provider<InternalChangeQuery> queryProvider;
-  private final Config config;
+
   @Inject
-  GetRevisionActions(
-      ActionJson delegate,
-      Provider<InternalChangeQuery> queryProvider,
-      @GerritServerConfig Config config) {
+  GetRevisionActions(ActionJson delegate) {
     this.delegate = delegate;
-    this.queryProvider = queryProvider;
-    this.config = config;
   }
 
   @Override
   public Object apply(RevisionResource rsrc) {
     return Response.withMustRevalidate(delegate.format(rsrc));
   }
-
-  @Override
-  public String getETag(RevisionResource rsrc) {
-    String topic = rsrc.getChange().getTopic();
-    if (!Submit.wholeTopicEnabled(config)
-        || Strings.isNullOrEmpty(topic)) {
-      return rsrc.getETag();
-    }
-    Hasher h = Hashing.md5().newHasher();
-    CurrentUser user = rsrc.getControl().getCurrentUser();
-    try {
-      for (ChangeData c : queryProvider.get().byTopicOpen(topic)) {
-        new ChangeResource(c.changeControl()).prepareETag(h, user);
-      }
-    } catch (OrmException e){
-      throw new OrmRuntimeException(e);
-    }
-    return h.hash().toString();
-  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java

@@ -169,7 +169,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
     this.titlePattern = new ParameterizedString(MoreObjects.firstNonNull(
         cfg.getString("change", null, "submitTooltip"),
         DEFAULT_TOOLTIP));
-    submitWholeTopic = wholeTopicEnabled(cfg);
+    submitWholeTopic = cfg.getBoolean("change", null, "submitWholeTopic" , false);
     this.submitTopicLabel = MoreObjects.firstNonNull(
         Strings.emptyToNull(cfg.getString("change", null, "submitTopicLabel")),
         "Submit whole topic");
@@ -206,19 +206,17 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
           rsrc.getPatchSet().getRevision().get()));
     }
 
-    List<Change> submittedChanges = submit(rsrc, caller, false);
+    change = submit(rsrc, caller, false);
+    if (change == null) {
+      throw new ResourceConflictException("change is "
+          + status(dbProvider.get().changes().get(rsrc.getChange().getId())));
+    }
 
     if (input.waitForMerge) {
-      for (Change c : submittedChanges) {
-        // TODO(sbeller): We should make schedule return a Future, then we
-        // could do these all in parallel and still block until they're done.
-        mergeQueue.merge(c.getDest());
-      }
+      mergeQueue.merge(change.getDest());
       change = dbProvider.get().changes().get(change.getId());
     } else {
-      for (Change c : submittedChanges) {
-        mergeQueue.schedule(c.getDest());
-      }
+      mergeQueue.schedule(change.getDest());
     }
 
     if (change == null) {
@@ -347,10 +345,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
         .orNull();
   }
 
-  private Change submitToDatabase(final ReviewDb db, final Change.Id changeId,
-      final Timestamp timestamp) throws OrmException,
-      ResourceConflictException {
-    Change ret = db.changes().atomicUpdate(changeId,
+  private Change submitToDatabase(ReviewDb db, Change.Id changeId,
+      final Timestamp timestamp) throws OrmException {
+    return db.changes().atomicUpdate(changeId,
       new AtomicUpdate<Change>() {
         @Override
         public Change update(Change change) {
@@ -362,12 +359,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
           return null;
         }
       });
-    if (ret != null) {
-      return ret;
-    } else {
-      throw new ResourceConflictException("change " + changeId + " is "
-          + status(db.changes().get(changeId)));
-    }
   }
 
   private Change submitThisChange(RevisionResource rsrc, IdentifiedUser caller,
@@ -389,6 +380,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
       // Write update commit after all normalized label commits.
       batch.write(update, new CommitBuilder());
       change = submitToDatabase(db, change.getId(), timestamp);
+      if (change == null) {
+        return null;
+      }
       db.commit();
     } finally {
       db.rollback();
@@ -397,7 +391,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
     return change;
   }
 
-  private List<Change> submitWholeTopic(RevisionResource rsrc, IdentifiedUser caller,
+  private Change submitWholeTopic(RevisionResource rsrc, IdentifiedUser caller,
       boolean force, String topic) throws ResourceConflictException, OrmException,
       IOException {
     Preconditions.checkNotNull(topic);
@@ -426,31 +420,30 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
       batch.write(update, new CommitBuilder());
 
       for (ChangeData c : changesByTopic) {
-        submitToDatabase(db, c.getId(), timestamp);
+        if (submitToDatabase(db, c.getId(), timestamp) == null) {
+          return null;
+        }
       }
       db.commit();
     } finally {
       db.rollback();
     }
     List<Change.Id> ids = new ArrayList<>(changesByTopic.size());
-    List<Change> ret = new ArrayList<>(changesByTopic.size());
     for (ChangeData c : changesByTopic) {
       ids.add(c.getId());
-      ret.add(c.change());
     }
     indexer.indexAsync(ids).checkedGet();
-
-    return ret;
+    return change;
   }
 
-  public List<Change> submit(RevisionResource rsrc, IdentifiedUser caller,
+  public Change submit(RevisionResource rsrc, IdentifiedUser caller,
       boolean force) throws ResourceConflictException, OrmException,
       IOException {
     String topic = rsrc.getChange().getTopic();
     if (submitWholeTopic && !Strings.isNullOrEmpty(topic)) {
       return submitWholeTopic(rsrc, caller, force, topic);
     } else {
-      return Arrays.asList(submitThisChange(rsrc, caller, force));
+      return submitThisChange(rsrc, caller, force);
     }
   }
 
@@ -661,10 +654,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
     return new RevisionResource(changes.parse(target), rsrc.getPatchSet());
   }
 
-  static boolean wholeTopicEnabled(Config config) {
-    return config.getBoolean("change", null, "submitWholeTopic" , false);
-  }
-
   public static class CurrentRevision implements
       RestModifyView<ChangeResource, SubmitInput> {
     private final Provider<ReviewDb> dbProvider;

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

@@ -1726,15 +1726,18 @@ public class ReceiveCommits {
       throws OrmException, IOException {
     Submit submit = submitProvider.get();
     RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps);
-    List<Change> changes;
+    Change c;
     try {
       // Force submit even if submit rule evaluation fails.
-      changes = submit.submit(rsrc, currentUser, true);
+      c = submit.submit(rsrc, currentUser, true);
     } catch (ResourceConflictException e) {
       throw new IOException(e);
     }
-    addMessage("");
-    for (Change c : changes) {
+    if (c == null) {
+      addError("Submitting change " + changeCtl.getChange().getChangeId()
+          + " failed.");
+    } else {
+      addMessage("");
       mergeQueue.merge(c.getDest());
       c = db.changes().get(c.getId());
       switch (c.getStatus()) {