Merge changes Ice62183c,I41c3c444

* changes: MergeOp: Report change id in case of submit rule problems Submit: Include ancestors into problem description
2015-07-28 22:00:54 +00:00
parent f495ae4a0d c287787915
commit d6ab35e92a
5 changed files with 185 additions and 96 deletions
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -871,6 +871,17 @@ abbreviated commit SHA-1 (`c9c0edb`).
 +
 Default is "Submit patch set ${patchSet} into ${branch}".

+[[change.submitTooltipAncestors]]change.submitTooltipAncestors::
+
+Tooltip for the submit button if there are ancestors which would
+also be submitted by submitting the change. Additionally to the variables
+as in link:#change.submitTooltip[change.submitTooltip], there is the
+variable `${submitSize}` indicating the number of changes which are
+submitted.
+
+Default is "Submit all ${topicSize} changes of the same topic (${submitSize}
+changes including ancestors and other changes related by topic)".
+
 [[change.submitWholeTopic]]change.submitWholeTopic::
 +
 Determines if the submit button submits the whole topic instead of
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -52,15 +52,14 @@ public class ActionsIT extends AbstractDaemonTest {
    commonActionsAssertions(actions);
    // We want to treat a single change in a topic not as a whole topic,
    // so regardless of how submitWholeTopic is configured:
-    noSubmitWholeTopicAssertions(actions);
+    noSubmitWholeTopicAssertions(actions, 1);
  }

  @Test
-  public void revisionActionsTwoChangeChangesInTopic() throws Exception {
+  public void revisionActionsTwoChangesInTopic() throws Exception {
    String changeId = createChangeWithTopic().getChangeId();
    approve(changeId);
-    // create another change with the same topic
-    createChangeWithTopic().getChangeId();
+    String changeId2 = createChangeWithTopic().getChangeId();
    Map<String, ActionInfo> actions = getActions(changeId);
    commonActionsAssertions(actions);
    if (isSubmitWholeTopicEnabled()) {
@@ -68,14 +67,19 @@ public class ActionsIT extends AbstractDaemonTest {
      assertThat(info.enabled).isNull();
      assertThat(info.label).isEqualTo("Submit whole topic");
      assertThat(info.method).isEqualTo("POST");
-      assertThat(info.title).isEqualTo("Other changes in this topic are not ready");
+      assertThat(info.title).isEqualTo("This change depends on other " +
+          "changes which are not ready");
    } else {
-      noSubmitWholeTopicAssertions(actions);
+      noSubmitWholeTopicAssertions(actions, 1);
+
+      assertThat(getActions(changeId2).get("submit")).isNull();
+      approve(changeId2);
+      noSubmitWholeTopicAssertions(getActions(changeId2), 2);
    }
  }

  @Test
-  public void revisionActionsTwoChangeChangesInTopic_conflicting() throws Exception {
+  public void revisionActionsTwoChangesInTopic_conflicting() throws Exception {
    String changeId = createChangeWithTopic().getChangeId();
    approve(changeId);

@@ -99,38 +103,66 @@ public class ActionsIT extends AbstractDaemonTest {
      assertThat(info.label).isEqualTo("Submit whole topic");
      assertThat(info.method).isEqualTo("POST");
      assertThat(info.title).isEqualTo(
-          "Clicking the button would fail for other changes in the topic");
+          "Clicking the button would fail for other changes");
    } else {
-      noSubmitWholeTopicAssertions(actions);
+      noSubmitWholeTopicAssertions(actions, 1);
    }
  }

  @Test
-  public void revisionActionsTwoChangeChangesInTopicReady() throws Exception {
-    String changeId = createChangeWithTopic().getChangeId();
+  public void revisionActionsTwoChangesInTopicWithAncestorReady()
+      throws Exception {
+    String changeId = createChange().getChangeId();
    approve(changeId);
+    approve(changeId);
+    String changeId1 = createChangeWithTopic().getChangeId();
+    approve(changeId1);
    // create another change with the same topic
    String changeId2 = createChangeWithTopic().getChangeId();
    approve(changeId2);
-    Map<String, ActionInfo> actions = getActions(changeId);
+    Map<String, ActionInfo> actions = getActions(changeId1);
    commonActionsAssertions(actions);
    if (isSubmitWholeTopicEnabled()) {
      ActionInfo info = actions.get("submit");
      assertThat(info.enabled).isTrue();
      assertThat(info.label).isEqualTo("Submit whole topic");
      assertThat(info.method).isEqualTo("POST");
-      assertThat(info.title).isEqualTo("Submit all 2 changes of the same topic");
+      assertThat(info.title).isEqualTo("Submit all 2 changes of the same " +
+          "topic (3 changes including ancestors " +
+          "and other changes related by topic)");
    } else {
-      noSubmitWholeTopicAssertions(actions);
+      noSubmitWholeTopicAssertions(actions, 2);
    }
  }

-  private void noSubmitWholeTopicAssertions(Map<String, ActionInfo> actions) {
+  @Test
+  public void revisionActionsReadyWithAncestors() throws Exception {
+    String changeId = createChange().getChangeId();
+    approve(changeId);
+    approve(changeId);
+    String changeId1 = createChange().getChangeId();
+    approve(changeId1);
+    String changeId2 = createChangeWithTopic().getChangeId();
+    approve(changeId2);
+    Map<String, ActionInfo> actions = getActions(changeId2);
+    commonActionsAssertions(actions);
+    // The topic contains only one change, so standard text applies
+    noSubmitWholeTopicAssertions(actions, 3);
+  }
+
+  private void noSubmitWholeTopicAssertions(Map<String, ActionInfo> actions,
+      int nrChanges) {
    ActionInfo info = actions.get("submit");
    assertThat(info.enabled).isTrue();
    assertThat(info.label).isEqualTo("Submit");
    assertThat(info.method).isEqualTo("POST");
+    if (nrChanges == 1) {
      assertThat(info.title).isEqualTo("Submit patch set 1 into master");
+    } else {
+      assertThat(info.title).isEqualTo(String.format(
+          "Submit patch set 1 and ancestors (%d changes " +
+          "altogether) into master", nrChanges));
+    }
  }

  private void commonActionsAssertions(Map<String, ActionInfo> actions) {
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
@@ -46,6 +46,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.ChangeSet;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeOp;
+import com.google.gerrit.server.git.MergeSuperSet;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.SubmitRuleEvaluator;
@@ -64,8 +65,6 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
@@ -77,16 +76,21 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,

  private static final String DEFAULT_TOOLTIP =
      "Submit patch set ${patchSet} into ${branch}";
+  private static final String DEFAULT_TOOLTIP_ANCESTORS =
+      "Submit patch set ${patchSet} and ancestors (${submitSize} changes " +
+      "altogether) into ${branch}";
  private static final String DEFAULT_TOPIC_TOOLTIP =
-      "Submit all ${topicSize} changes of the same topic";
-  private static final String BLOCKED_TOPIC_TOOLTIP =
-      "Other changes in this topic are not ready";
-  private static final String BLOCKED_HIDDEN_TOPIC_TOOLTIP =
-      "Other hidden changes in this topic are not ready";
-  private static final String CLICK_FAILURE_OTHER_TOOLTIP =
-      "Clicking the button would fail for other changes in the topic";
+      "Submit all ${topicSize} changes of the same topic " +
+      "(${submitSize} changes including ancestors and other " +
+      "changes related by topic)";
+  private static final String BLOCKED_SUBMIT_TOOLTIP =
+      "This change depends on other changes which are not ready";
+  private static final String BLOCKED_HIDDEN_SUBMIT_TOOLTIP =
+      "This change depends on other hidden changes which are not ready";
  private static final String CLICK_FAILURE_TOOLTIP =
-      "Clicking the button would fail.";
+      "Clicking the button would fail";
+  private static final String CLICK_FAILURE_OTHER_TOOLTIP =
+      "Clicking the button would fail for other changes";

  public static class Output {
    transient Change change;
@@ -100,11 +104,14 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
  private final GitRepositoryManager repoManager;
  private final ChangeData.Factory changeDataFactory;
  private final ChangeMessagesUtil cmUtil;
+  private final ChangeControl.GenericFactory changeControlFactory;
  private final Provider<MergeOp> mergeOpProvider;
+  private final MergeSuperSet mergeSuperSet;
  private final AccountsCollection accounts;
  private final ChangesCollection changes;
  private final String label;
  private final ParameterizedString titlePattern;
+  private final ParameterizedString titlePatternWithAncestors;
  private final String submitTopicLabel;
  private final ParameterizedString submitTopicTooltip;
  private final boolean submitWholeTopic;
@@ -115,7 +122,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
      GitRepositoryManager repoManager,
      ChangeData.Factory changeDataFactory,
      ChangeMessagesUtil cmUtil,
+      ChangeControl.GenericFactory changeControlFactory,
      Provider<MergeOp> mergeOpProvider,
+      MergeSuperSet mergeSuperSet,
      AccountsCollection accounts,
      ChangesCollection changes,
      @GerritServerConfig Config cfg,
@@ -124,7 +133,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
    this.repoManager = repoManager;
    this.changeDataFactory = changeDataFactory;
    this.cmUtil = cmUtil;
+    this.changeControlFactory = changeControlFactory;
    this.mergeOpProvider = mergeOpProvider;
+    this.mergeSuperSet = mergeSuperSet;
    this.accounts = accounts;
    this.changes = changes;
    this.label = MoreObjects.firstNonNull(
@@ -133,6 +144,10 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
    this.titlePattern = new ParameterizedString(MoreObjects.firstNonNull(
        cfg.getString("change", null, "submitTooltip"),
        DEFAULT_TOOLTIP));
+    this.titlePatternWithAncestors = new ParameterizedString(
+        MoreObjects.firstNonNull(
+            cfg.getString("change", null, "submitTooltipAncestors"),
+            DEFAULT_TOOLTIP_ANCESTORS));
    submitWholeTopic = wholeTopicEnabled(cfg);
    this.submitTopicLabel = MoreObjects.firstNonNull(
        Strings.emptyToNull(cfg.getString("change", null, "submitTopicLabel")),
@@ -170,16 +185,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
          rsrc.getPatchSet().getRevision().get()));
    }

-    List<Change> changes;
-    if (submitWholeTopic && !Strings.isNullOrEmpty(change.getTopic())) {
-      changes = new ArrayList<>();
-      for (ChangeData cd : getChangesByTopic(change.getTopic())) {
-        changes.add(cd.change());
-      }
-    } else {
-      changes = Arrays.asList(change);
-    }
-    ChangeSet submittedChanges = ChangeSet.create(changes);
+    ChangeSet submittedChanges = ChangeSet.create(change);

    try {
      ReviewDb db = dbProvider.get();
@@ -207,22 +213,24 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
  }

  /**
-   * @param changeList list of changes to be submitted at once
+   * @param cs set of changes to be submitted at once
   * @param identifiedUser the user who is checking to submit
   * @return a reason why any of the changes is not submittable or null
   */
-  private String problemsForSubmittingChanges(
-      List<ChangeData> changeList,
-      IdentifiedUser identifiedUser) {
+  private String problemsForSubmittingChangeset(
+      ChangeSet cs, IdentifiedUser identifiedUser) {
    try {
-      for (ChangeData c : changeList) {
-        ChangeControl changeControl = c.changeControl().forUser(
-            identifiedUser);
+      for (PatchSet.Id psId : cs.patchIds()) {
+        ReviewDb db = dbProvider.get();
+        ChangeControl changeControl = changeControlFactory
+            .controlFor(psId.getParentKey(), identifiedUser);
+        ChangeData c = changeDataFactory.create(db, changeControl);
+
        if (!changeControl.isVisible(dbProvider.get())) {
-          return BLOCKED_HIDDEN_TOPIC_TOOLTIP;
+          return BLOCKED_HIDDEN_SUBMIT_TOOLTIP;
        }
        if (!changeControl.canSubmit()) {
-          return BLOCKED_TOPIC_TOOLTIP;
+          return BLOCKED_SUBMIT_TOOLTIP;
        }
        // Recheck mergeability rather than using value stored in the index,
        // which may be stale.
@@ -240,8 +248,8 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
        checkSubmitRule(c, c.currentPatchSet(), false);
      }
    } catch (ResourceConflictException e) {
-      return BLOCKED_TOPIC_TOOLTIP;
-    } catch (OrmException e) {
+      return BLOCKED_SUBMIT_TOOLTIP;
+    } catch (NoSuchChangeException | OrmException e) {
      log.error("Error checking if change is submittable", e);
      throw new OrmRuntimeException("Could not determine problems for the change", e);
    }
@@ -282,40 +290,55 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
      throw new OrmRuntimeException("Could not determine mergeability", e);
    }

-    List<ChangeData> changesByTopic = null;
-    if (submitWholeTopic && !Strings.isNullOrEmpty(topic)) {
-      changesByTopic = getChangesByTopic(topic);
+    ChangeSet cs;
+    try {
+      cs = mergeSuperSet.completeChangeSet(db,
+          ChangeSet.create(cd.change()));
+    } catch (OrmException | IOException e) {
+      throw new OrmRuntimeException("Could not determine complete set of " +
+          "changes to be submitted", e);
    }
-    if (submitWholeTopic
+
+    int topicSize = 0;
+    if (!Strings.isNullOrEmpty(topic)) {
+      topicSize = getChangesByTopic(topic).size();
+    }
+    boolean treatWithTopic = submitWholeTopic
        && !Strings.isNullOrEmpty(topic)
-        && changesByTopic.size() > 1) {
-      Map<String, String> params = ImmutableMap.of(
-          "topicSize", String.valueOf(changesByTopic.size()));
-      String topicProblems = problemsForSubmittingChanges(changesByTopic,
+        && topicSize > 1;
+
+    String submitProblems = problemsForSubmittingChangeset(cs,
        resource.getUser());
-      if (topicProblems != null) {
+    if (submitProblems != null) {
      return new UiAction.Description()
-          .setLabel(submitTopicLabel)
-          .setTitle(topicProblems)
+        .setLabel(treatWithTopic ? submitTopicLabel : label)
+        .setTitle(submitProblems)
        .setVisible(true)
        .setEnabled(false);
-      } else {
+    }
+
+    if (treatWithTopic) {
+      Map<String, String> params = ImmutableMap.of(
+          "topicSize", String.valueOf(topicSize),
+          "submitSize", String.valueOf(cs.size()));
      return new UiAction.Description()
          .setLabel(submitTopicLabel)
          .setTitle(Strings.emptyToNull(
              submitTopicTooltip.replace(params)))
          .setVisible(true)
          .setEnabled(Boolean.TRUE.equals(enabled));
-      }
    } else {
      RevId revId = resource.getPatchSet().getRevision();
      Map<String, String> params = ImmutableMap.of(
          "patchSet", String.valueOf(resource.getPatchSet().getPatchSetId()),
          "branch", resource.getChange().getDest().getShortName(),
-          "commit", ObjectId.fromString(revId.get()).abbreviate(7).name());
+          "commit", ObjectId.fromString(revId.get()).abbreviate(7).name(),
+          "submitSize", String.valueOf(cs.size()));
+      ParameterizedString tp = cs.size() > 1 ? titlePatternWithAncestors :
+          titlePattern;
      return new UiAction.Description()
        .setLabel(label)
-        .setTitle(Strings.emptyToNull(titlePattern.replace(params)))
+        .setTitle(Strings.emptyToNull(tp.replace(params)))
        .setVisible(true)
        .setEnabled(Boolean.TRUE.equals(enabled));
    }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 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.Project;

 /** A set of changes grouped together to be submitted atomically.*/
@@ -29,6 +30,7 @@ public abstract class ChangeSet {
    ImmutableSet.Builder<Project.NameKey> pb = ImmutableSet.builder();
    ImmutableSet.Builder<Branch.NameKey> bb = ImmutableSet.builder();
    ImmutableSet.Builder<Change.Id> ib = ImmutableSet.builder();
+    ImmutableSet.Builder<PatchSet.Id> psb = ImmutableSet.builder();
    ImmutableSetMultimap.Builder<Project.NameKey, Branch.NameKey> pbb =
        ImmutableSetMultimap.builder();
    ImmutableSetMultimap.Builder<Project.NameKey, Change.Id> pcb =
@@ -42,13 +44,14 @@ public abstract class ChangeSet {
      pb.add(project);
      bb.add(branch);
      ib.add(c.getId());
+      psb.add(c.currentPatchSetId());
      pbb.put(project, branch);
      pcb.put(project, c.getId());
      cbb.put(branch, c.getId());
    }

-    return new AutoValue_ChangeSet(pb.build(), bb.build(),
-        ib.build(), pbb.build(), pcb.build(), cbb.build());
+    return new AutoValue_ChangeSet(pb.build(), bb.build(), ib.build(),
+        psb.build(), pbb.build(), pcb.build(), cbb.build());
  }

  public static ChangeSet create(Change change) {
@@ -58,6 +61,7 @@ public abstract class ChangeSet {
  public abstract ImmutableSet<Project.NameKey> projects();
  public abstract ImmutableSet<Branch.NameKey> branches();
  public abstract ImmutableSet<Change.Id> ids();
+  public abstract ImmutableSet<PatchSet.Id> patchIds();
  public abstract ImmutableSetMultimap<Project.NameKey, Branch.NameKey>
      branchesByProject();
  public abstract ImmutableSetMultimap<Project.NameKey, Change.Id>
@@ -69,4 +73,8 @@ public abstract class ChangeSet {
  public int hashCode() {
    return ids().hashCode();
  }
+
+  public int size() {
+    return ids().size();
+  }
 }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -237,7 +237,9 @@ public class MergeOp {
      return ImmutableList.of(ok.get());
    } else if (results.isEmpty()) {
      throw new IllegalStateException(String.format(
-          "SubmitRuleEvaluator.evaluate returned empty list for %s in %s",
+          "SubmitRuleEvaluator.evaluate for change %s " +
+          "returned empty list for %s in %s",
+          cd.getId(),
          patchSet.getId(),
          cd.change().getProject().get()));
    }
@@ -245,15 +247,17 @@ public class MergeOp {
    for (SubmitRecord record : results) {
      switch (record.status) {
        case CLOSED:
-          throw new ResourceConflictException("change is closed");
+          throw new ResourceConflictException(String.format(
+              "change %s is closed", cd.getId()));

        case RULE_ERROR:
          throw new ResourceConflictException(String.format(
-              "rule error: %s",
-              record.errorMessage));
+              "rule error for change %s: %s",
+              cd.getId(), record.errorMessage));

        case NOT_READY:
          StringBuilder msg = new StringBuilder();
+          msg.append(cd.getId() + ":");
          for (SubmitRecord.Label lbl : record.labels) {
            switch (lbl.status) {
              case OK:
@@ -261,32 +265,27 @@ public class MergeOp {
                continue;

              case REJECT:
-                if (msg.length() > 0) {
-                  msg.append("; ");
-                }
-                msg.append("blocked by ").append(lbl.label);
+                msg.append(" blocked by ").append(lbl.label);
+                msg.append(";");
                continue;

              case NEED:
-                if (msg.length() > 0) {
-                  msg.append("; ");
-                }
-                msg.append("needs ").append(lbl.label);
+                msg.append(" needs ").append(lbl.label);
+                msg.append(";");
                continue;

              case IMPOSSIBLE:
-                if (msg.length() > 0) {
-                  msg.append("; ");
-                }
-                msg.append("needs ").append(lbl.label)
+                msg.append(" needs ").append(lbl.label)
                .append(" (check project access)");
+                msg.append(";");
                continue;

              default:
                throw new IllegalStateException(String.format(
-                    "Unsupported SubmitRecord.Label %s for %s in %s",
+                    "Unsupported SubmitRecord.Label %s for %s in %s in %s",
                    lbl.toString(),
                    patchSet.getId(),
+                    cd.getId(),
                    cd.change().getProject().get()));
            }
          }
@@ -303,21 +302,37 @@ public class MergeOp {
    throw new IllegalStateException();
  }

-  private void checkPermissions(ChangeSet cs)
+  private void checkSubmitRulesAndState(ChangeSet cs)
      throws ResourceConflictException, OrmException {
+
+    StringBuilder msgbuf = new StringBuilder();
+    List<Change.Id> problemChanges = new ArrayList<>();
    for (Change.Id id : cs.ids()) {
+      try {
        ChangeData cd = changeDataFactory.create(db, id);
        if (cd.change().getStatus() != Change.Status.NEW){
-        throw new OrmException("Change " + cd.change().getChangeId()
-            + " is in state " + cd.change().getStatus());
+          throw new ResourceConflictException("Change " +
+              cd.change().getChangeId() + " is in state " +
+              cd.change().getStatus());
        } else {
          records.put(cd.change().getId(), checkSubmitRule(cd));
        }
+      } catch (ResourceConflictException e) {
+        msgbuf.append(e.getMessage() + "\n");
+        problemChanges.add(id);
+      }
+    }
+    String reason = msgbuf.toString();
+    if (!reason.isEmpty()) {
+        throw new ResourceConflictException("The change could not be " +
+            "submitted because it depends on change(s) " +
+            problemChanges.toString() + ", which could not be submitted " +
+            "because:\n" + reason);
    }
  }

  public void merge(ReviewDb db, ChangeSet changes, IdentifiedUser caller,
-      boolean checkPermissions) throws NoSuchChangeException,
+      boolean checkSubmitRules) throws NoSuchChangeException,
      OrmException, ResourceConflictException {
    logPrefix = String.format("[%s]: ", String.valueOf(changes.hashCode()));
    this.db = db;
@@ -325,9 +340,9 @@ public class MergeOp {
    try {
      ChangeSet cs = mergeSuperSet.completeChangeSet(db, changes);
      logDebug("Calculated to merge {}", cs);
-      if (checkPermissions) {
-        logDebug("Checking permissions");
-        checkPermissions(cs);
+      if (checkSubmitRules) {
+        logDebug("Checking submit rules and state");
+        checkSubmitRulesAndState(cs);
      }
      try {
        integrateIntoHistory(cs, caller);