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
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index adba1aa..8f04d7c 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -844,30 +844,6 @@
+
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 (…)
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Actions.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Actions.java
index 37b90c4..525684d 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Actions.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Actions.java
@@ -65,6 +65,7 @@
private String message;
private String branch;
private String key;
+ private boolean canSubmit;
Actions() {
initWidget(uiBinder.createAndBindUi(this));
@@ -86,12 +87,7 @@
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 @@
}
}
- 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 @@
return ids;
}
+ void setSubmitEnabled() {
+ submit.setVisible(canSubmit);
+ }
+
@UiHandler("followUp")
void onFollowUp(@SuppressWarnings("unused") ClickEvent e) {
if (followUpAction == null) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
index bae181c..8a27fd0 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java
@@ -248,7 +248,7 @@
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 @@
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 @@
}
}
- 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 @@
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 @@
commentLinkProcessor = result.getCommentLinkProcessor();
setTheme(result.getTheme());
renderChangeInfo(info);
- loadRevisionInfo();
}
}));
}
@@ -942,6 +933,7 @@
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 @@
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 @@
initReplyButton(info, revision);
initIncludedInAction(info);
initChangeAction(info);
+ initRevisionsAction(info, revision);
initDownloadAction(info, revision);
initProjectLinks(info);
initBranchLink(info);
@@ -1079,37 +1084,6 @@
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 @@
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) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java
index 1135491..98595e7 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeApi.java
@@ -95,13 +95,6 @@
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);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
index 1555cdd..16472e3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
@@ -57,15 +57,17 @@
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 @@
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();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java
index b21bee2..d58c8d2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java
+++ b/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();
- }
}
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 03bc7f1..41151ae 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
@@ -169,7 +169,7 @@
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 @@
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 @@
.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 @@
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 @@
// 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 @@
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 @@
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 @@
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;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index cf63e78..cc2919d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -1726,15 +1726,18 @@
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()) {
