diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 610b65f060..322a0040a2 100644 --- 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 diff --git 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 index 66c99f1f79..24bb72b308 100644 --- 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 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 actions = getActions(changeId); + Map 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 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 actions = getActions(changeId2); + commonActionsAssertions(actions); + // The topic contains only one change, so standard text applies + noSubmitWholeTopicAssertions(actions, 3); + } + + private void noSubmitWholeTopicAssertions(Map actions, + int nrChanges) { ActionInfo info = actions.get("submit"); assertThat(info.enabled).isTrue(); assertThat(info.label).isEqualTo("Submit"); assertThat(info.method).isEqualTo("POST"); - assertThat(info.title).isEqualTo("Submit patch set 1 into master"); + 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 actions) { diff --git 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 index 3d4dc26e7b..8e389e6160 100644 --- 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, 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, private final GitRepositoryManager repoManager; private final ChangeData.Factory changeDataFactory; private final ChangeMessagesUtil cmUtil; + private final ChangeControl.GenericFactory changeControlFactory; private final Provider 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, GitRepositoryManager repoManager, ChangeData.Factory changeDataFactory, ChangeMessagesUtil cmUtil, + ChangeControl.GenericFactory changeControlFactory, Provider mergeOpProvider, + MergeSuperSet mergeSuperSet, AccountsCollection accounts, ChangesCollection changes, @GerritServerConfig Config cfg, @@ -124,7 +133,9 @@ public class Submit implements RestModifyView, 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, 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, rsrc.getPatchSet().getRevision().get())); } - List 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, } /** - * @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 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, 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, throw new OrmRuntimeException("Could not determine mergeability", e); } - List 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) { + && topicSize > 1; + + String submitProblems = problemsForSubmittingChangeset(cs, + resource.getUser()); + if (submitProblems != null) { + return new UiAction.Description() + .setLabel(treatWithTopic ? submitTopicLabel : label) + .setTitle(submitProblems) + .setVisible(true) + .setEnabled(false); + } + + if (treatWithTopic) { Map params = ImmutableMap.of( - "topicSize", String.valueOf(changesByTopic.size())); - String topicProblems = problemsForSubmittingChanges(changesByTopic, - resource.getUser()); - if (topicProblems != null) { - return new UiAction.Description() - .setLabel(submitTopicLabel) - .setTitle(topicProblems) - .setVisible(true) - .setEnabled(false); - } else { - return new UiAction.Description() + "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 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)); } diff --git 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 index 6c3b49908f..a18a3a3bf9 100644 --- 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 pb = ImmutableSet.builder(); ImmutableSet.Builder bb = ImmutableSet.builder(); ImmutableSet.Builder ib = ImmutableSet.builder(); + ImmutableSet.Builder psb = ImmutableSet.builder(); ImmutableSetMultimap.Builder pbb = ImmutableSetMultimap.builder(); ImmutableSetMultimap.Builder 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 projects(); public abstract ImmutableSet branches(); public abstract ImmutableSet ids(); + public abstract ImmutableSet patchIds(); public abstract ImmutableSetMultimap branchesByProject(); public abstract ImmutableSetMultimap @@ -69,4 +73,8 @@ public abstract class ChangeSet { public int hashCode() { return ids().hashCode(); } + + public int size() { + return ids().size(); + } }