Revert marking merged or abandoned changes in related changes
This incurs database lookups on every commit returned from the RevWalk
that does not have an open change associated with it. Typically this
is not very many, because the walk is limited to the current branch
tip.
However, there is very bad worst-case behavior in the case of
reviewing a merge that pulls in a long side branch whose merge base
with tip is far back in history:
M
|\
T \
| B1000
... ...
| B1
|/
O
In this case, we incur a thousand distinct database lookups to see
whether B1..B1000 have changes associated with them. In practice this
is a serious performance regression on some large repositories.
The original changes were a very useful bugfix, but it needs a more
efficient implementation.
This reverts the following commits:
d3b6e1d531f0bc6253c6d5ad6205ca1e72ef87f2
72d9193f626b89f379a5564aabcc1d416d933bdb
Change-Id: Ia3d4b200e677bca24fcd8fa9a4629bb406e90998
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 49ebb49..640ed87 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -3918,7 +3918,6 @@
|===========================
|Field Name ||Description
|`change_id` |optional|The Change-Id of the change.
-|`status` |optional|The status of the change.
|`commit` ||The commit as a
link:#commit-info[CommitInfo] entity.
|`_change_number` |optional|The change number.
diff --git a/Documentation/user-review-ui.txt b/Documentation/user-review-ui.txt
index 594c058..4dfb8ae 100644
--- a/Documentation/user-review-ui.txt
+++ b/Documentation/user-review-ui.txt
@@ -533,7 +533,7 @@
note that following the link to an indirect descendant change may
result in a completely different related changes listing.
-** [[merged-ancestor]]Black Dot:
+** [[closed-ancestor]]Black Dot:
+
Indicates a merged ancestor, e.g. the commit was directly pushed into
the repository bypassing code review, or the ancestor change was
@@ -542,10 +542,6 @@
the commit was done on `branch-a`, but was then pushed to
`refs/for/branch-b`.
-** [[abandoned-change]]Dark Red Dot:
-+
-Indicates an abandoned change.
-
+
image::images/user-review-ui-change-screen-related-changes-indicators.png[width=800, link="images/user-review-ui-change-screen-related-changes-indicators.png"]
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChanges.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChanges.java
index 0c91abc..0c78a67 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChanges.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChanges.java
@@ -55,8 +55,6 @@
String current();
String gitweb();
String indirect();
- String abandoned();
- String merged();
String notCurrent();
String pointer();
String row();
@@ -349,13 +347,6 @@
}
public final native String id() /*-{ return this.change_id }-*/;
-
- public final Change.Status status() {
- String s = statusRaw();
- return s != null ? Change.Status.valueOf(s) : null;
- }
- private final native String statusRaw() /*-{ return this.status; }-*/;
-
public final native CommitInfo commit() /*-{ return this.commit }-*/;
final native String branch() /*-{ return this.branch }-*/;
final native String project() /*-{ return this.project }-*/;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChangesTab.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChangesTab.java
index 492f30a..96d0d10 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChangesTab.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/RelatedChangesTab.java
@@ -20,7 +20,6 @@
import com.google.gerrit.client.changes.ChangeInfo.CommitInfo;
import com.google.gerrit.client.changes.Util;
import com.google.gerrit.common.PageLinks;
-import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gwt.core.client.GWT;
import com.google.gwt.core.client.JsArray;
@@ -310,14 +309,6 @@
sb.setStyleName(RelatedChanges.R.css().notCurrent());
sb.setAttribute("title", Util.C.notCurrent());
sb.append('\u25CF');
- } else if (Change.Status.MERGED == info.status()) {
- sb.setStyleName(RelatedChanges.R.css().merged());
- sb.setAttribute("title", Resources.C.merged());
- sb.append('\u25CF');
- } else if (Change.Status.ABANDONED == info.status()) {
- sb.setStyleName(RelatedChanges.R.css().abandoned());
- sb.setAttribute("title", Resources.C.abandoned());
- sb.append('\u25CF');
} else {
sb.setStyleName(RelatedChanges.R.css().current());
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/related_changes.css b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/related_changes.css
index 4d103ae..2e62b98 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/related_changes.css
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/related_changes.css
@@ -68,8 +68,6 @@
.current,
.gitweb,
.indirect,
-.abandoned,
-.merged,
.notCurrent {
display: inline-block;
text-align: center;
@@ -77,7 +75,6 @@
width: 12px;
}
-.merged,
.gitweb {
color: #000;
}
@@ -87,10 +84,6 @@
font-weight: bold;
}
-.abandoned {
- color: #900; /* dark red */
-}
-
.notCurrent {
color: #FFA62F; /* orange */
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java
index 8c01403..6cdae44 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java
@@ -20,13 +20,11 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
-import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetAncestor;
-import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CommonConverters;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -120,18 +118,6 @@
if (p != null) {
g = changes.get(p.getId().getParentKey()).change();
added.add(p.getId().getParentKey());
- } else {
- // check if there is a merged or abandoned change for this commit
- ReviewDb db = dbProvider.get();
- for (PatchSet ps : db.patchSets().byRevision(new RevId(c.name())).toList()) {
- Change change = db.changes().get(ps.getId().getParentKey());
- if (change != null && change.getDest().equals(rsrc.getChange().getDest())) {
- p = ps;
- g = change;
- added.add(g.getId());
- break;
- }
- }
}
parents.add(new ChangeAndCommit(g, p, c));
}
@@ -286,7 +272,6 @@
public static class ChangeAndCommit {
public String changeId;
- public ChangeStatus status;
public CommitInfo commit;
public Integer _changeNumber;
public Integer _revisionNumber;
@@ -295,7 +280,6 @@
ChangeAndCommit(@Nullable Change change, @Nullable PatchSet ps, RevCommit c) {
if (change != null) {
changeId = change.getKey().get();
- status = change.getStatus().asChangeStatus();
_changeNumber = change.getChangeId();
_revisionNumber = ps != null ? ps.getPatchSetId() : null;
PatchSet.Id curr = change.currentPatchSetId();
