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 71231e4d69..93881d2e76 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,7 +65,6 @@ class Actions extends Composite { private String message; private String branch; private String key; - private boolean canSubmit; Actions() { initWidget(uiBinder.createAndBindUi(this)); @@ -87,7 +86,12 @@ class Actions extends Composite { changeInfo = info; initChangeActions(info, hasUser); - initRevisionActions(info, revInfo, hasUser); + + NativeMap actionMap = revInfo.has_actions() + ? revInfo.actions() + : NativeMap. create(); + actionMap.copyKeysIntoChildren("id"); + reloadRevisionActions(actionMap); } private void initChangeActions(ChangeInfo info, boolean hasUser) { @@ -107,30 +111,29 @@ class Actions extends Composite { } } - private void initRevisionActions(ChangeInfo info, RevisionInfo revInfo, - boolean hasUser) { - NativeMap actions = revInfo.has_actions() - ? revInfo.actions() - : NativeMap. create(); - actions.copyKeysIntoChildren("id"); + void reloadRevisionActions(NativeMap 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); - 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))); - } + 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))); } } @@ -146,10 +149,6 @@ class Actions extends Composite { 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 8a27fd049a..98220dabb4 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 @@ -240,7 +240,10 @@ public class ChangeScreen extends Screen { @Override public void onSuccess(ChangeInfo info) { info.init(); + // Revision loading may be slower than the rest, so do it + // asynchronous to have the rest fast. loadConfigInfo(info, base); + loadRevisionInfo(); } })); } @@ -248,7 +251,7 @@ public class ChangeScreen extends Screen { void loadChangeInfo(boolean fg, AsyncCallback cb) { RestApi call = ChangeApi.detail(changeId.get()); ChangeList.addOptions(call, EnumSet.of( - ListChangesOption.CURRENT_ACTIONS, + ListChangesOption.CHANGE_ACTIONS, ListChangesOption.ALL_REVISIONS)); if (!fg) { call.background(); @@ -256,6 +259,18 @@ public class ChangeScreen extends Screen { call.get(cb); } + void loadRevisionInfo() { + RestApi call = ChangeApi.actions(changeId.get(), revision); + call.background(); + call.get(new GerritCallback>() { + @Override + public void onSuccess(NativeMap actionMap) { + actionMap.copyKeysIntoChildren("id"); + renderRevisionInfo(changeInfo, actionMap); + } + }); + } + @Override protected void onUnload() { if (replyAction != null) { @@ -403,7 +418,8 @@ public class ChangeScreen extends Screen { } } - private void initRevisionsAction(ChangeInfo info, String revision) { + private void initRevisionsAction(ChangeInfo info, String revision, + NativeMap actions) { int currentPatchSet; if (info.current_revision() != null && info.revisions().containsKey(info.current_revision())) { @@ -431,11 +447,6 @@ public class ChangeScreen extends Screen { RevisionInfo revInfo = info.revision(revision); if (revInfo.draft()) { - NativeMap actions = revInfo.has_actions() - ? revInfo.actions() - : NativeMap. create(); - actions.copyKeysIntoChildren("id"); - if (actions.containsKey("publish")) { publish.setVisible(true); publish.setTitle(actions.get("publish").title()); @@ -933,7 +944,6 @@ 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()); } @@ -1043,19 +1053,7 @@ 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); @@ -1064,7 +1062,6 @@ public class ChangeScreen extends Screen { initReplyButton(info, revision); initIncludedInAction(info); initChangeAction(info); - initRevisionsAction(info, revision); initDownloadAction(info, revision); initProjectLinks(info); initBranchLink(info); @@ -1084,6 +1081,37 @@ 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. create()); + } + + private void renderRevisionInfo(ChangeInfo info, + NativeMap 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); @@ -1105,14 +1133,7 @@ public class ChangeScreen extends Screen { quickApprove.setVisible(false); setVisible(strategy, 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()); + actions.reloadRevisionActions(actionMap); } 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 2036942b91..0a29a0081f 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,6 +95,13 @@ 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 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 3232f7729d..81694a36a5 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,17 +57,15 @@ public class ChangeResource implements RestResource, HasETag { return getControl().getNotes(); } - @Override - public String getETag() { - CurrentUser user = control.getCurrentUser(); - Hasher h = Hashing.md5().newHasher() - .putLong(getChange().getLastUpdatedOn().getTime()) + + // This includes all information relevant for ETag computation + // unrelated to the UI. + public void prepareETag(Hasher h, CurrentUser user) { + h.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 { @@ -82,6 +80,14 @@ 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(); } 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 d58c8d2a5b..b21bee227b 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,22 +14,59 @@ 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.extensions.restapi.RestReadView; +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.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; -@Singleton -public class GetRevisionActions implements RestReadView { - private final ActionJson delegate; +import org.eclipse.jgit.lib.Config; +@Singleton +public class GetRevisionActions implements ETagView { + private final ActionJson delegate; + private final Provider queryProvider; + private final Config config; @Inject - GetRevisionActions(ActionJson delegate) { + GetRevisionActions( + ActionJson delegate, + Provider queryProvider, + @GerritServerConfig Config config) { 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 41151ae8da..03bc7f1c72 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 @@ public class Submit implements RestModifyView, this.titlePattern = new ParameterizedString(MoreObjects.firstNonNull( cfg.getString("change", null, "submitTooltip"), DEFAULT_TOOLTIP)); - submitWholeTopic = cfg.getBoolean("change", null, "submitWholeTopic" , false); + submitWholeTopic = wholeTopicEnabled(cfg); this.submitTopicLabel = MoreObjects.firstNonNull( Strings.emptyToNull(cfg.getString("change", null, "submitTopicLabel")), "Submit whole topic"); @@ -206,17 +206,19 @@ public class Submit implements RestModifyView, rsrc.getPatchSet().getRevision().get())); } - change = submit(rsrc, caller, false); - if (change == null) { - throw new ResourceConflictException("change is " - + status(dbProvider.get().changes().get(rsrc.getChange().getId()))); - } + List submittedChanges = submit(rsrc, caller, false); if (input.waitForMerge) { - mergeQueue.merge(change.getDest()); + 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()); + } change = dbProvider.get().changes().get(change.getId()); } else { - mergeQueue.schedule(change.getDest()); + for (Change c : submittedChanges) { + mergeQueue.schedule(c.getDest()); + } } if (change == null) { @@ -345,9 +347,10 @@ public class Submit implements RestModifyView, .orNull(); } - private Change submitToDatabase(ReviewDb db, Change.Id changeId, - final Timestamp timestamp) throws OrmException { - return db.changes().atomicUpdate(changeId, + private Change submitToDatabase(final ReviewDb db, final Change.Id changeId, + final Timestamp timestamp) throws OrmException, + ResourceConflictException { + Change ret = db.changes().atomicUpdate(changeId, new AtomicUpdate() { @Override public Change update(Change change) { @@ -359,6 +362,12 @@ public class Submit implements RestModifyView, 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, @@ -380,9 +389,6 @@ public class Submit implements RestModifyView, // 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(); @@ -391,7 +397,7 @@ public class Submit implements RestModifyView, return change; } - private Change submitWholeTopic(RevisionResource rsrc, IdentifiedUser caller, + private List submitWholeTopic(RevisionResource rsrc, IdentifiedUser caller, boolean force, String topic) throws ResourceConflictException, OrmException, IOException { Preconditions.checkNotNull(topic); @@ -420,30 +426,31 @@ public class Submit implements RestModifyView, batch.write(update, new CommitBuilder()); for (ChangeData c : changesByTopic) { - if (submitToDatabase(db, c.getId(), timestamp) == null) { - return null; - } + submitToDatabase(db, c.getId(), timestamp); } db.commit(); } finally { db.rollback(); } List ids = new ArrayList<>(changesByTopic.size()); + List ret = new ArrayList<>(changesByTopic.size()); for (ChangeData c : changesByTopic) { ids.add(c.getId()); + ret.add(c.change()); } indexer.indexAsync(ids).checkedGet(); - return change; + + return ret; } - public Change submit(RevisionResource rsrc, IdentifiedUser caller, + public List 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 submitThisChange(rsrc, caller, force); + return Arrays.asList(submitThisChange(rsrc, caller, force)); } } @@ -654,6 +661,10 @@ public class Submit implements RestModifyView, 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 { private final Provider 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 cc2919d58b..cf63e78bb5 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,18 +1726,15 @@ public class ReceiveCommits { throws OrmException, IOException { Submit submit = submitProvider.get(); RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps); - Change c; + List changes; try { // Force submit even if submit rule evaluation fails. - c = submit.submit(rsrc, currentUser, true); + changes = submit.submit(rsrc, currentUser, true); } catch (ResourceConflictException e) { throw new IOException(e); } - if (c == null) { - addError("Submitting change " + changeCtl.getChange().getChangeId() - + " failed."); - } else { - addMessage(""); + addMessage(""); + for (Change c : changes) { mergeQueue.merge(c.getDest()); c = db.changes().get(c.getId()); switch (c.getStatus()) {