Merge "Track generated suggestion edits only when suggestions remain"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 091d229..ec02cdd 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5795,7 +5795,7 @@
Content-Disposition: attachment
Content-Type: text/plain; charset=UTF-8
- The existing change edit could not be merged with another tree.
+ Rebasing change edit onto another patchset results in merge conflicts. Download the edit patchset and rebase manually to preserve changes.
----
[[apply-provided-fix]]
diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java
index 56fb748..fad3aa8 100644
--- a/java/com/google/gerrit/entities/Change.java
+++ b/java/com/google/gerrit/entities/Change.java
@@ -433,6 +433,9 @@
/** Locally assigned unique identifier of the change */
private Id changeId;
+ /** ServerId of the Gerrit instance that has created the change */
+ private String serverId;
+
/** Globally assigned unique identifier of the change */
private Key changeKey;
@@ -530,6 +533,22 @@
return changeId;
}
+ /**
+ * Set the serverId of the Gerrit instance that created the change. It can be set to null for
+ * testing purposes in the protobuf converter tests.
+ */
+ public void setServerId(@Nullable String serverId) {
+ this.serverId = serverId;
+ }
+
+ /**
+ * ServerId of the Gerrit instance that created the change. It could be null when the change is
+ * not fetched from NoteDb but obtained through protobuf deserialisation.
+ */
+ public @Nullable String getServerId() {
+ return serverId;
+ }
+
/** 32 bit integer identity for a change. */
public int getChangeId() {
return changeId.get();
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index 33a2f34..4a3b423 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -102,6 +102,7 @@
public Boolean containsGitConflicts;
public Integer _number;
+ public Integer _virtualIdNumber;
public AccountInfo owner;
diff --git a/java/com/google/gerrit/index/QueryOptions.java b/java/com/google/gerrit/index/QueryOptions.java
index bee8fa1..632e469 100644
--- a/java/com/google/gerrit/index/QueryOptions.java
+++ b/java/com/google/gerrit/index/QueryOptions.java
@@ -204,4 +204,8 @@
allowIncompleteResults(),
filter.apply(this));
}
+
+ public int getLimitBasedOnPaginationType() {
+ return PaginationType.NONE == config().paginationType() ? limit() : pageSize();
+ }
}
diff --git a/java/com/google/gerrit/index/query/FilteredSource.java b/java/com/google/gerrit/index/query/FilteredSource.java
index 9746850..95e6435 100644
--- a/java/com/google/gerrit/index/query/FilteredSource.java
+++ b/java/com/google/gerrit/index/query/FilteredSource.java
@@ -21,6 +21,7 @@
import com.google.common.collect.Iterables;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.index.QueryOptions;
import java.util.ArrayList;
import java.util.List;
@@ -51,11 +52,36 @@
return new LazyResultSet<>(
() -> {
List<T> r = new ArrayList<>();
+ T last = null;
+ int pageResultSize = 0;
for (T data : buffer(resultSet)) {
if (!isMatchable() || match(data)) {
r.add(data);
}
+ last = data;
+ pageResultSize++;
}
+
+ if (last != null && source instanceof Paginated) {
+ // Restart source and continue if we have not filled the
+ // full limit the caller wants.
+ Paginated<T> p = (Paginated<T>) source;
+ QueryOptions opts = p.getOptions();
+ final int limit = opts.limit();
+ int nextStart = pageResultSize;
+ while (pageResultSize == limit && r.size() < limit) {
+ ResultSet<T> next = p.restart(nextStart);
+ pageResultSize = 0;
+ for (T data : buffer(next)) {
+ if (match(data)) {
+ r.add(data);
+ }
+ pageResultSize++;
+ }
+ nextStart += pageResultSize;
+ }
+ }
+
if (start >= r.size()) {
return ImmutableList.of();
} else if (start > 0) {
diff --git a/java/com/google/gerrit/index/query/IndexedQuery.java b/java/com/google/gerrit/index/query/IndexedQuery.java
index ee25ef9..cb98c06 100644
--- a/java/com/google/gerrit/index/query/IndexedQuery.java
+++ b/java/com/google/gerrit/index/query/IndexedQuery.java
@@ -87,6 +87,12 @@
}
@Override
+ public ResultSet<T> restart(int start) {
+ opts = opts.withStart(start);
+ return search();
+ }
+
+ @Override
public ResultSet<T> restart(int start, int pageSize) {
opts = opts.withStart(start).withPageSize(pageSize);
return search();
diff --git a/java/com/google/gerrit/index/query/Paginated.java b/java/com/google/gerrit/index/query/Paginated.java
index 5521990..1065019 100644
--- a/java/com/google/gerrit/index/query/Paginated.java
+++ b/java/com/google/gerrit/index/query/Paginated.java
@@ -19,6 +19,8 @@
public interface Paginated<T> {
QueryOptions getOptions();
+ ResultSet<T> restart(int start);
+
ResultSet<T> restart(int start, int pageSize);
ResultSet<T> restart(Object searchAfter, int pageSize);
diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java
index be789ee..19251ca 100644
--- a/java/com/google/gerrit/index/query/PaginatingSource.java
+++ b/java/com/google/gerrit/index/query/PaginatingSource.java
@@ -54,7 +54,6 @@
if (last != null && source instanceof Paginated) {
// Restart source and continue if we have not filled the
// full limit the caller wants.
- //
@SuppressWarnings("unchecked")
Paginated<T> p = (Paginated<T>) source;
QueryOptions opts = p.getOptions();
diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
index 65c02d9..570290e 100644
--- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
+++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
@@ -150,10 +150,14 @@
.findFirst()
.orElse(-1)
+ 1;
- int toIndex = Math.min(fromIndex + opts.pageSize(), valueList.size());
+ int toIndex = Math.min(fromIndex + opts.getLimitBasedOnPaginationType(), valueList.size());
results = valueList.subList(fromIndex, toIndex);
} else {
- results = valueStream.skip(opts.start()).limit(opts.pageSize()).collect(toImmutableList());
+ results =
+ valueStream
+ .skip(opts.start())
+ .limit(opts.getLimitBasedOnPaginationType())
+ .collect(toImmutableList());
}
queryCount++;
resultsSizes.add(results.size());
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 9fcf5ae..ffd25ba 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -17,6 +17,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.lucene.AbstractLuceneIndex.sortFieldName;
import static com.google.gerrit.server.git.QueueProvider.QueueType.INTERACTIVE;
+import static com.google.gerrit.server.index.change.ChangeField.CHANGENUM_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.NUMERIC_ID_STR_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.PROJECT_SPEC;
import static com.google.gerrit.server.index.change.ChangeIndexRewriter.CLOSED_STATUSES;
@@ -113,7 +114,7 @@
private static final String CHANGE_FIELD = ChangeField.CHANGE_SPEC.getName();
static Term idTerm(ChangeData cd) {
- return idTerm(cd.getVirtualId());
+ return idTerm(cd.virtualId());
}
static Term idTerm(Change.Id id) {
@@ -405,11 +406,11 @@
IndexSearcher[] searchers = new IndexSearcher[indexes.size()];
Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex = new HashMap<>();
try {
- int realPageSize = opts.start() + opts.pageSize();
- if (Integer.MAX_VALUE - opts.pageSize() < opts.start()) {
- realPageSize = Integer.MAX_VALUE;
+ int pageLimit = AbstractLuceneIndex.getLimitBasedOnPaginationType(opts, opts.pageSize());
+ int queryLimit = opts.start() + pageLimit;
+ if (Integer.MAX_VALUE - pageLimit < opts.start()) {
+ queryLimit = Integer.MAX_VALUE;
}
- int queryLimit = AbstractLuceneIndex.getLimitBasedOnPaginationType(opts, realPageSize);
List<TopFieldDocs> hits = new ArrayList<>();
int searchAfterHitsCount = 0;
for (int i = 0; i < indexes.size(); i++) {
@@ -417,7 +418,7 @@
searchers[i] = subIndex.acquire();
if (isSearchAfterPagination) {
ScoreDoc searchAfter = getSearchAfter(subIndex);
- int maxRemainingHits = realPageSize - searchAfterHitsCount;
+ int maxRemainingHits = queryLimit - searchAfterHitsCount;
if (maxRemainingHits > 0) {
TopFieldDocs subIndexHits =
searchers[i].searchAfter(
@@ -528,7 +529,11 @@
ImmutableList.Builder<ChangeData> result =
ImmutableList.builderWithExpectedSize(docs.size());
for (Document doc : docs) {
- result.add(toChangeData(fields(doc, fields), fields, NUMERIC_ID_STR_SPEC.getName()));
+ String fieldName =
+ doc.getField(CHANGENUM_SPEC.getName()) != null
+ ? CHANGENUM_SPEC.getName()
+ : NUMERIC_ID_STR_SPEC.getName();
+ result.add(toChangeData(fields(doc, fields), fields, fieldName));
}
return result.build();
} catch (InterruptedException e) {
@@ -571,7 +576,13 @@
IndexableField cb = Iterables.getFirst(doc.get(CHANGE_FIELD), null);
if (cb != null) {
BytesRef proto = cb.binaryValue();
- cd = changeDataFactory.create(parseProtoFrom(proto, ChangeProtoConverter.INSTANCE));
+ // pass the id field value (which is the change virtual id for the imported changes) when
+ // available
+ IndexableField f = Iterables.getFirst(doc.get(idFieldName), null);
+ cd =
+ changeDataFactory.create(
+ parseProtoFrom(proto, ChangeProtoConverter.INSTANCE),
+ f != null ? Change.id(Integer.valueOf(f.stringValue())) : null);
} else {
IndexableField f = Iterables.getFirst(doc.get(idFieldName), null);
diff --git a/java/com/google/gerrit/server/ChangeDraftUpdate.java b/java/com/google/gerrit/server/ChangeDraftUpdate.java
index d9cea31..a3b13e5 100644
--- a/java/com/google/gerrit/server/ChangeDraftUpdate.java
+++ b/java/com/google/gerrit/server/ChangeDraftUpdate.java
@@ -80,7 +80,9 @@
default <UpdateT extends ChangeDraftUpdate> Optional<UpdateT> toOptionalChangeDraftUpdateSubtype(
Class<UpdateT> subtype) {
if (this.getClass().isAssignableFrom(subtype)) {
- return Optional.of((UpdateT) this);
+ @SuppressWarnings("unchecked")
+ UpdateT update = (UpdateT) this;
+ return Optional.of(update);
}
return Optional.empty();
}
diff --git a/java/com/google/gerrit/server/account/AccountResource.java b/java/com/google/gerrit/server/account/AccountResource.java
index 9629809..14b363b 100644
--- a/java/com/google/gerrit/server/account/AccountResource.java
+++ b/java/com/google/gerrit/server/account/AccountResource.java
@@ -99,6 +99,10 @@
public Change getChange() {
return change.getChange();
}
+
+ public Change.Id getVirtualId() {
+ return change.getVirtualId();
+ }
}
public static class Star implements RestResource {
diff --git a/java/com/google/gerrit/server/change/ChangeFinder.java b/java/com/google/gerrit/server/change/ChangeFinder.java
index 9f253de..5668c27 100644
--- a/java/com/google/gerrit/server/change/ChangeFinder.java
+++ b/java/com/google/gerrit/server/change/ChangeFinder.java
@@ -208,6 +208,12 @@
return Optional.of(notes.get(0));
}
+ /**
+ * @deprecated this method is not reliable in Gerrit instances with imported changes, since
+ * multiple changes can have the same change number and make the `changeIdProjectCache` cache
+ * pointless.
+ */
+ @Deprecated(since = "3.10", forRemoval = true)
public List<ChangeNotes> find(Change.Id id) {
String project = changeIdProjectCache.getIfPresent(id);
if (project != null) {
@@ -245,7 +251,7 @@
// this case.)
Set<Change.Id> seen = Sets.newHashSetWithExpectedSize(cds.size());
for (ChangeData cd : cds) {
- if (seen.add(cd.getId())) {
+ if (seen.add(cd.virtualId())) {
try {
notes.add(cd.notes());
} catch (NoSuchChangeException e) {
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index d0f5d6b..0abe9cf 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -63,7 +63,6 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.entities.SubmitRecord.Status;
import com.google.gerrit.entities.SubmitRequirementResult;
@@ -86,7 +85,6 @@
import com.google.gerrit.extensions.common.SubmitRequirementResultInfo;
import com.google.gerrit.extensions.common.TrackingIdInfo;
import com.google.gerrit.extensions.restapi.Url;
-import com.google.gerrit.index.RefState;
import com.google.gerrit.index.query.QueryResult;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
@@ -513,6 +511,9 @@
if (has(REVIEWED) && userProvider.get().isIdentifiedUser()) {
ChangeData.ensureReviewedByLoadedForOpenChanges(all);
}
+ if (has(STAR) && userProvider.get().isIdentifiedUser()) {
+ ChangeData.ensureChangeServerId(all);
+ }
ChangeData.ensureCurrentApprovalsLoaded(all);
}
} else {
@@ -549,7 +550,8 @@
}
continue;
}
- ChangeInfo info = cache.get(cd.getId());
+ Change.Id cdUniqueId = cd.virtualId();
+ ChangeInfo info = cache.get(cdUniqueId);
if (info != null && isCacheable) {
changeInfos.add(info);
continue;
@@ -561,7 +563,7 @@
info = format(cd, Optional.empty(), false, pluginInfosByChange.get(cd.getId()));
changeInfos.add(info);
if (isCacheable) {
- cache.put(Change.id(info._number), info);
+ cache.put(cdUniqueId, info);
}
} catch (RuntimeException e) {
Optional<RequestCancelledException> requestCancelledException =
@@ -802,6 +804,8 @@
}
}
+ out._virtualIdNumber = cd.virtualId().get();
+
return out;
}
@@ -868,14 +872,8 @@
TraceContext.newTimer(
"Get change meta ref",
Metadata.builder().changeId(cd.change().getId().get()).build())) {
- if (cd.getRefStates() != null) {
- String metaName = RefNames.changeMetaRef(cd.getId());
- Optional<RefState> metaState =
- cd.getRefStates().values().stream().filter(r -> r.ref().equals(metaName)).findAny();
- return metaState.map(RefState::id);
- }
+ return cd.metaRevision();
}
- return Optional.empty();
}
private Boolean isReviewedByCurrentUser(ChangeData cd, CurrentUser user) {
@@ -1071,14 +1069,15 @@
// repository only once
try (Repository allUsersRepo = repoManager.openRepository(allUsers)) {
List<Change.Id> changeIds =
- changeInfos.stream().map(c -> Change.id(c._number)).collect(Collectors.toList());
+ changeInfos.stream().map(c -> Change.id(c._virtualIdNumber)).collect(Collectors.toList());
Set<Change.Id> starredChanges =
starredChangesreader.areStarred(
allUsersRepo, changeIds, userProvider.get().asIdentifiedUser().getAccountId());
if (starredChanges.isEmpty()) {
return;
}
- changeInfos.stream().forEach(c -> c.starred = starredChanges.contains(Change.id(c._number)));
+ changeInfos.stream()
+ .forEach(c -> c.starred = starredChanges.contains(Change.id(c._virtualIdNumber)));
} catch (IOException e) {
logger.atWarning().withCause(e).log("Failed to open All-Users repo.");
}
@@ -1091,7 +1090,11 @@
private ImmutableListMultimap<Change.Id, PluginDefinedInfo> getPluginInfos(
Collection<ChangeData> cds) {
if (pluginDefinedInfosFactory.isPresent()) {
- return pluginDefinedInfosFactory.get().createPluginDefinedInfos(cds);
+ try (TraceTimer timer =
+ TraceContext.newTimer(
+ "Get plugin infos", Metadata.builder().resourceCount(cds.size()).build())) {
+ return pluginDefinedInfosFactory.get().createPluginDefinedInfos(cds);
+ }
}
return ImmutableListMultimap.of();
}
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index dc09bf4..8300541 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -161,6 +161,10 @@
return changeData;
}
+ public Change.Id getVirtualId() {
+ return getChangeData().virtualId();
+ }
+
// This includes all information relevant for ETag computation
// unrelated to the UI.
public void prepareETag(Hasher h, CurrentUser user) {
@@ -237,7 +241,7 @@
.build())) {
Hasher h = Hashing.murmur3_128().newHasher();
if (user.isIdentifiedUser()) {
- h.putBoolean(starredChangesReader.isStarred(user.getAccountId(), getId()));
+ h.putBoolean(starredChangesReader.isStarred(user.getAccountId(), getVirtualId()));
}
prepareETag(h, user);
return h.hash().toString();
diff --git a/java/com/google/gerrit/server/change/DeleteChangeOp.java b/java/com/google/gerrit/server/change/DeleteChangeOp.java
index ba7bd90a..bcfa48a 100644
--- a/java/com/google/gerrit/server/change/DeleteChangeOp.java
+++ b/java/com/google/gerrit/server/change/DeleteChangeOp.java
@@ -81,7 +81,8 @@
ensureDeletable(ctx, id, patchSets);
// Cleaning up is only possible as long as the change and its elements are
// still part of the database.
- cleanUpReferences(id);
+ ChangeData cd = changeDataFactory.create(ctx.getChange());
+ cleanUpReferences(cd);
logger.atFine().log(
"Deleting change %s, current patch set %d is commit %s",
@@ -95,7 +96,7 @@
.map(p -> p.commitId().name())
.orElse("n/a")));
ctx.deleteChange();
- changeDeleted.fire(changeDataFactory.create(ctx.getChange()), ctx.getAccount(), ctx.getWhen());
+ changeDeleted.fire(cd, ctx.getAccount(), ctx.getWhen());
return true;
}
@@ -124,11 +125,11 @@
revWalk.parseCommit(patchSet.commitId()), revWalk.parseCommit(destId.get()));
}
- private void cleanUpReferences(Change.Id id) throws IOException {
- accountPatchReviewStore.run(s -> s.clearReviewed(id));
+ private void cleanUpReferences(ChangeData cd) throws IOException {
+ accountPatchReviewStore.run(s -> s.clearReviewed(cd.virtualId()));
// Non-atomic operation on All-Users refs; not much we can do to make it atomic.
- starredChangesWriter.unstarAllForChangeDeletion(id);
+ starredChangesWriter.unstarAllForChangeDeletion(cd.virtualId());
}
@Override
diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
index d9a118d..565d471 100644
--- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -566,7 +566,8 @@
if (!successful) {
throw new MergeConflictException(
- "The existing change edit could not be merged with another tree.");
+ "Rebasing change edit onto another patchset results in merge conflicts. Download the edit"
+ + " patchset and rebase manually to preserve changes.");
}
return threeWayMerger.getResultTreeId();
}
diff --git a/java/com/google/gerrit/server/edit/tree/TreeCreator.java b/java/com/google/gerrit/server/edit/tree/TreeCreator.java
index dfc1ffb..572001d 100644
--- a/java/com/google/gerrit/server/edit/tree/TreeCreator.java
+++ b/java/com/google/gerrit/server/edit/tree/TreeCreator.java
@@ -18,9 +18,11 @@
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.UsedAt;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import java.util.Optional;
import org.eclipse.jgit.dircache.DirCache;
import org.eclipse.jgit.dircache.DirCacheBuilder;
import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -39,26 +41,41 @@
private final ObjectId baseTreeId;
private final ImmutableList<? extends ObjectId> baseParents;
+ private final Optional<ObjectInserter> objectInserter;
private final List<TreeModification> treeModifications = new ArrayList<>();
public static TreeCreator basedOn(RevCommit baseCommit) {
requireNonNull(baseCommit, "baseCommit is required");
- return new TreeCreator(baseCommit.getTree(), ImmutableList.copyOf(baseCommit.getParents()));
+ return new TreeCreator(
+ baseCommit.getTree(), ImmutableList.copyOf(baseCommit.getParents()), Optional.empty());
+ }
+
+ @UsedAt(UsedAt.Project.GOOGLE)
+ public static TreeCreator basedOn(RevCommit baseCommit, ObjectInserter objectInserter) {
+ requireNonNull(baseCommit, "baseCommit is required");
+ return new TreeCreator(
+ baseCommit.getTree(),
+ ImmutableList.copyOf(baseCommit.getParents()),
+ Optional.of(objectInserter));
}
public static TreeCreator basedOnTree(
ObjectId baseTreeId, ImmutableList<? extends ObjectId> baseParents) {
requireNonNull(baseTreeId, "baseTreeId is required");
- return new TreeCreator(baseTreeId, baseParents);
+ return new TreeCreator(baseTreeId, baseParents, Optional.empty());
}
public static TreeCreator basedOnEmptyTree() {
- return new TreeCreator(ObjectId.zeroId(), ImmutableList.of());
+ return new TreeCreator(ObjectId.zeroId(), ImmutableList.of(), Optional.empty());
}
- private TreeCreator(ObjectId baseTreeId, ImmutableList<? extends ObjectId> baseParents) {
+ private TreeCreator(
+ ObjectId baseTreeId,
+ ImmutableList<? extends ObjectId> baseParents,
+ Optional<ObjectInserter> objectInserter) {
this.baseTreeId = requireNonNull(baseTreeId, "baseTree is required");
this.baseParents = baseParents;
+ this.objectInserter = objectInserter;
}
/**
@@ -141,17 +158,22 @@
return pathEdits;
}
+ private ObjectId writeAndGetId(Repository repository, DirCache tree) throws IOException {
+ ObjectInserter oi = objectInserter.orElse(repository.newObjectInserter());
+ try {
+ ObjectId treeId = tree.writeTree(oi);
+ oi.flush();
+ return treeId;
+ } finally {
+ if (objectInserter.isEmpty()) {
+ oi.close();
+ }
+ }
+ }
+
private static void applyPathEdits(DirCache tree, List<DirCacheEditor.PathEdit> pathEdits) {
DirCacheEditor dirCacheEditor = tree.editor();
pathEdits.forEach(dirCacheEditor::add);
dirCacheEditor.finish();
}
-
- private static ObjectId writeAndGetId(Repository repository, DirCache tree) throws IOException {
- try (ObjectInserter objectInserter = repository.newObjectInserter()) {
- ObjectId treeId = tree.writeTree(objectInserter);
- objectInserter.flush();
- return treeId;
- }
- }
}
diff --git a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
index 83024e3..d8afbbb 100644
--- a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
+++ b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
@@ -155,13 +155,14 @@
.byProject(key);
Map<Change.Id, CachedChange> result = new HashMap<>(cds.size());
for (ChangeData cd : cds) {
- if (result.containsKey(cd.getId())) {
+ final Change.Id cdUniqueId = cd.virtualId();
+ if (result.containsKey(cdUniqueId)) {
logger.atWarning().log(
"Duplicate changes returned from change query by project %s: %s, %s",
- key, cd.change(), result.get(cd.getId()).change());
+ key, cd.change(), result.get(cdUniqueId).change());
}
result.put(
- cd.getId(),
+ cdUniqueId,
new AutoValue_SearchingChangeCacheImpl_CachedChange(cd.change(), cd.reviewers()));
}
return List.copyOf(result.values());
diff --git a/java/com/google/gerrit/server/index/IndexUtils.java b/java/com/google/gerrit/server/index/IndexUtils.java
index f2a2904..f81a9ce 100644
--- a/java/com/google/gerrit/server/index/IndexUtils.java
+++ b/java/com/google/gerrit/server/index/IndexUtils.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.index;
+import static com.google.gerrit.server.index.change.ChangeField.CHANGENUM_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.CHANGE_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.NUMERIC_ID_STR_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.PROJECT_SPEC;
@@ -81,10 +82,18 @@
// A Change is always sufficient.
return fs;
}
- if (fs.contains(PROJECT_SPEC.getName()) && fs.contains(NUMERIC_ID_STR_SPEC.getName())) {
+
+ Set<String> requiredFields =
+ CHANGENUM_SPEC.getName() != null
+ ? ImmutableSet.of(
+ NUMERIC_ID_STR_SPEC.getName(), PROJECT_SPEC.getName(), CHANGENUM_SPEC.getName())
+ : ImmutableSet.of(NUMERIC_ID_STR_SPEC.getName(), PROJECT_SPEC.getName());
+
+ if (fs.containsAll(requiredFields)) {
return fs;
}
- return Sets.union(fs, ImmutableSet.of(NUMERIC_ID_STR_SPEC.getName(), PROJECT_SPEC.getName()));
+
+ return Sets.union(fs, ImmutableSet.copyOf(requiredFields));
}
/**
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index c5afba4..045482a 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -17,6 +17,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.server.query.change.ChangeQueryBuilder.FIELD_CHANGE_NUMBER;
import static com.google.gerrit.server.util.AttentionSetUtil.additionsOnly;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.joining;
@@ -128,11 +129,20 @@
.required()
// The numeric change id is integer in string form
.size(10)
- .build(cd -> String.valueOf(cd.getVirtualId().get()));
+ .build(cd -> String.valueOf(cd.virtualId().get()));
public static final IndexedField<ChangeData, String>.SearchSpec NUMERIC_ID_STR_SPEC =
NUMERIC_ID_STR_FIELD.exact("legacy_id_str");
+ public static final IndexedField<ChangeData, Integer> CHANGENUM_FIELD =
+ IndexedField.<ChangeData>integerBuilder("ChangeNumber")
+ .stored()
+ .required()
+ .build(cd -> cd.getId().get());
+
+ public static final IndexedField<ChangeData, Integer>.SearchSpec CHANGENUM_SPEC =
+ CHANGENUM_FIELD.integer(FIELD_CHANGE_NUMBER);
+
/** Newer style Change-Id key. */
public static final IndexedField<ChangeData, String> CHANGE_ID_FIELD =
IndexedField.<ChangeData>stringBuilder("ChangeId")
diff --git a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
index 3d48907..4921b3f 100644
--- a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -259,8 +259,15 @@
.build();
/** Upgrade Lucene to 9.x requires reindexing. */
- @SuppressWarnings("deprecation")
- static final Schema<ChangeData> V85 = schema(V84);
+ @Deprecated static final Schema<ChangeData> V85 = schema(V84);
+
+ /** Add ChangeNumber field */
+ static final Schema<ChangeData> V86 =
+ new Schema.Builder<ChangeData>()
+ .add(V85)
+ .addIndexedFields(ChangeField.CHANGENUM_FIELD)
+ .addSearchSpecs(ChangeField.CHANGENUM_SPEC)
+ .build();
/**
* Name of the change index to be used when contacting index backends or loading configurations.
diff --git a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
index f7ff13c..00642a9 100644
--- a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
+++ b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
@@ -21,7 +21,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.gerrit.entities.Change;
import com.google.gerrit.index.IndexConfig;
@@ -52,10 +51,6 @@
*/
public class IndexedChangeQuery extends IndexedQuery<Change.Id, ChangeData>
implements ChangeDataSource, Matchable<ChangeData> {
- public static QueryOptions oneResult() {
- IndexConfig config = IndexConfig.createDefault();
- return createOptions(config, 0, 1, config.pageSizeMultiplier(), 1, ImmutableSet.of());
- }
public static QueryOptions createOptions(
IndexConfig config, int start, int limit, Set<String> fields) {
diff --git a/java/com/google/gerrit/server/logging/PerformanceLogRecord.java b/java/com/google/gerrit/server/logging/PerformanceLogRecord.java
index 046eeb3..07d9b90 100644
--- a/java/com/google/gerrit/server/logging/PerformanceLogRecord.java
+++ b/java/com/google/gerrit/server/logging/PerformanceLogRecord.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.logging;
import com.google.auto.value.AutoValue;
+import java.time.Instant;
import java.util.Optional;
/**
@@ -33,7 +34,8 @@
* @return the performance log record
*/
public static PerformanceLogRecord create(String operation, long durationMs) {
- return new AutoValue_PerformanceLogRecord(operation, durationMs, Optional.empty());
+ return new AutoValue_PerformanceLogRecord(
+ operation, durationMs, Instant.now(), Optional.empty());
}
/**
@@ -45,20 +47,23 @@
* @return the performance log record
*/
public static PerformanceLogRecord create(String operation, long durationMs, Metadata metadata) {
- return new AutoValue_PerformanceLogRecord(operation, durationMs, Optional.of(metadata));
+ return new AutoValue_PerformanceLogRecord(
+ operation, durationMs, Instant.now(), Optional.of(metadata));
}
public abstract String operation();
public abstract long durationMs();
+ public abstract Instant endTime();
+
public abstract Optional<Metadata> metadata();
void writeTo(PerformanceLogger performanceLogger) {
if (metadata().isPresent()) {
- performanceLogger.log(operation(), durationMs(), metadata().get());
+ performanceLogger.log(operation(), durationMs(), endTime(), metadata().get());
} else {
- performanceLogger.log(operation(), durationMs());
+ performanceLogger.log(operation(), durationMs(), endTime());
}
}
}
diff --git a/java/com/google/gerrit/server/logging/PerformanceLogger.java b/java/com/google/gerrit/server/logging/PerformanceLogger.java
index 74a1684..bed53ba 100644
--- a/java/com/google/gerrit/server/logging/PerformanceLogger.java
+++ b/java/com/google/gerrit/server/logging/PerformanceLogger.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.logging;
import com.google.gerrit.extensions.annotations.ExtensionPoint;
+import java.time.Instant;
/**
* Extension point for logging performance records.
@@ -35,8 +36,8 @@
* @param operation operation that was performed
* @param durationMs time that the execution of the operation took (in milliseconds)
*/
- default void log(String operation, long durationMs) {
- log(operation, durationMs, Metadata.empty());
+ default void log(String operation, long durationMs, Instant endTime) {
+ log(operation, durationMs, endTime, Metadata.empty());
}
/**
@@ -46,5 +47,5 @@
* @param durationMs time that the execution of the operation took (in milliseconds)
* @param metadata metadata
*/
- void log(String operation, long durationMs, Metadata metadata);
+ void log(String operation, long durationMs, Instant endTime, Metadata metadata);
}
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index df30573..6c38210 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -252,7 +252,7 @@
queryProvider
.get()
.enforceVisibility(true)
- .byLegacyChangeId(Change.id(metadata.changeNumber));
+ .byChangeNumber(Change.id(metadata.changeNumber));
if (changeDataList.isEmpty()) {
sendRejectionEmail(message, InboundEmailError.CHANGE_NOT_FOUND);
return;
diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
index 1edc7bd..4bb347a 100644
--- a/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
@@ -40,6 +40,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.query.change.ChangeNumberVirtualIdAlgorithm;
import com.google.gerrit.server.update.BatchUpdateListener;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -73,6 +74,8 @@
* <p>This class is not thread safe.
*/
public class ChangeDraftNotesUpdate extends AbstractChangeUpdate implements ChangeDraftUpdate {
+ private final ChangeNumberVirtualIdAlgorithm virtualIdFunc;
+
public interface Factory extends ChangeDraftUpdateFactory {
@Override
ChangeDraftNotesUpdate create(
@@ -234,6 +237,7 @@
AllUsersName allUsers,
ChangeNoteUtil noteUtil,
ExperimentFeatures experimentFeatures,
+ @Nullable ChangeNumberVirtualIdAlgorithm virtualIdFunc,
@Assisted ChangeNotes notes,
@Assisted("effective") Account.Id accountId,
@Assisted("real") Account.Id realAccountId,
@@ -242,6 +246,7 @@
super(noteUtil, serverIdent, notes, null, accountId, realAccountId, authorIdent, when);
this.draftsProject = allUsers;
this.experimentFeatures = experimentFeatures;
+ this.virtualIdFunc = virtualIdFunc;
}
@AssistedInject
@@ -250,6 +255,7 @@
AllUsersName allUsers,
ChangeNoteUtil noteUtil,
ExperimentFeatures experimentFeatures,
+ @Nullable ChangeNumberVirtualIdAlgorithm virtualIdFunc,
@Assisted Change change,
@Assisted("effective") Account.Id accountId,
@Assisted("real") Account.Id realAccountId,
@@ -258,6 +264,7 @@
super(noteUtil, serverIdent, null, change, accountId, realAccountId, authorIdent, when);
this.draftsProject = allUsers;
this.experimentFeatures = experimentFeatures;
+ this.virtualIdFunc = virtualIdFunc;
}
@Override
@@ -320,6 +327,7 @@
draftsProject,
noteUtil,
experimentFeatures,
+ virtualIdFunc,
new Change(getChange()),
accountId,
realAccountId,
@@ -430,7 +438,7 @@
@Override
protected String getRefName() {
- return RefNames.refsDraftComments(getId(), accountId);
+ return RefNames.refsDraftComments(getVirtualId(), accountId);
}
@Override
@@ -447,4 +455,11 @@
public boolean isEmpty() {
return delete.isEmpty() && put.isEmpty();
}
+
+ private Change.Id getVirtualId() {
+ Change change = getChange();
+ return virtualIdFunc == null
+ ? change.getId()
+ : virtualIdFunc.apply(change.getServerId(), change.getId());
+ }
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 7f0b068..97d65be 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -214,23 +214,6 @@
return changes.get(0).notes();
}
- /**
- * Create change notes based on a list of {@link com.google.gerrit.entities.Change.Id}s. This
- * requires using the Change index and should only be used when {@link
- * com.google.gerrit.entities.Project.NameKey} and the numeric change ID are not available.
- */
- public List<ChangeNotes> createUsingIndexLookup(Collection<Change.Id> changeIds) {
- List<ChangeNotes> notes = new ArrayList<>();
- for (Change.Id changeId : changeIds) {
- try {
- notes.add(createCheckedUsingIndexLookup(changeId));
- } catch (NoSuchChangeException e) {
- // Ignore missing changes to match Access#get(Iterable) behavior.
- }
- }
- return notes;
- }
-
public List<ChangeNotes> create(
Repository repo,
Project.NameKey project,
@@ -550,11 +533,21 @@
}
public ImmutableList<HumanComment> getDraftComments(Account.Id author) {
- return getDraftComments(author, null);
+ return getDraftComments(author, null, null);
}
- ImmutableList<HumanComment> getDraftComments(Account.Id author, @Nullable Ref ref) {
- loadDraftComments(author, ref);
+ public ImmutableList<HumanComment> getDraftComments(Account.Id author, @Nullable Ref ref) {
+ return getDraftComments(author, null, ref);
+ }
+
+ public ImmutableList<HumanComment> getDraftComments(
+ Account.Id author, @Nullable Change.Id virtualId) {
+ return getDraftComments(author, virtualId, null);
+ }
+
+ ImmutableList<HumanComment> getDraftComments(
+ Account.Id author, @Nullable Change.Id virtualId, @Nullable Ref ref) {
+ loadDraftComments(author, virtualId, ref);
// Filter out any zombie draft comments. These are drafts that are also in
// the published map, and arise when the update to All-Users to delete them
// during the publish operation failed.
@@ -573,9 +566,10 @@
* However, this method will load the comments if no draft comments have been loaded or if the
* caller would like the drafts for another author.
*/
- private void loadDraftComments(Account.Id author, @Nullable Ref ref) {
+ private void loadDraftComments(
+ Account.Id author, @Nullable Change.Id virtualId, @Nullable Ref ref) {
if (draftCommentNotes == null || !author.equals(draftCommentNotes.getAuthor()) || ref != null) {
- draftCommentNotes = new DraftCommentNotes(args, getChangeId(), author, ref);
+ draftCommentNotes = new DraftCommentNotes(args, getChangeId(), virtualId, author, ref);
draftCommentNotes.load();
}
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
index e012942..eb6c15a 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -364,6 +364,7 @@
change.setOwner(c.owner());
change.setDest(BranchNameKey.create(change.getProject(), c.branch()));
change.setCreatedOn(c.createdOn());
+ change.setServerId(serverId());
copyNonConstructorColumnsTo(change);
}
diff --git a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
index 186b49a..639633e 100644
--- a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
+++ b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
@@ -42,8 +42,15 @@
public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final Change.Id virtualId;
+
public interface Factory {
DraftCommentNotes create(Change.Id changeId, Account.Id accountId);
+
+ DraftCommentNotes create(
+ @Assisted("changeId") Change.Id changeId,
+ @Assisted("virtualId") Change.Id virtualId,
+ Account.Id accountId);
}
private final Account.Id author;
@@ -54,11 +61,26 @@
@AssistedInject
DraftCommentNotes(Args args, @Assisted Change.Id changeId, @Assisted Account.Id author) {
- this(args, changeId, author, null);
+ this(args, changeId, null, author, null);
}
- DraftCommentNotes(Args args, Change.Id changeId, Account.Id author, @Nullable Ref ref) {
+ @AssistedInject
+ DraftCommentNotes(
+ Args args,
+ @Assisted("changeId") Change.Id changeId,
+ @Assisted("virtualId") Change.Id virtualId,
+ @Assisted Account.Id author) {
+ this(args, changeId, virtualId, author, null);
+ }
+
+ DraftCommentNotes(
+ Args args,
+ Change.Id changeId,
+ @Nullable Change.Id virtualId,
+ Account.Id author,
+ @Nullable Ref ref) {
super(args, changeId, null);
+ this.virtualId = virtualId;
this.author = requireNonNull(author);
this.ref = ref;
if (ref != null) {
@@ -94,7 +116,7 @@
@Override
protected String getRefName() {
- return refsDraftComments(getChangeId(), author);
+ return refsDraftComments(virtualId != null ? virtualId : getChangeId(), author);
}
@Override
diff --git a/java/com/google/gerrit/server/notedb/DraftCommentsNotesReader.java b/java/com/google/gerrit/server/notedb/DraftCommentsNotesReader.java
index 27c59f9..ea3dd0a 100644
--- a/java/com/google/gerrit/server/notedb/DraftCommentsNotesReader.java
+++ b/java/com/google/gerrit/server/notedb/DraftCommentsNotesReader.java
@@ -26,6 +26,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.query.change.ChangeNumberVirtualIdAlgorithm;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
@@ -43,15 +44,18 @@
private final DraftCommentNotes.Factory draftCommentNotesFactory;
private final GitRepositoryManager repoManager;
private final AllUsersName allUsers;
+ private final ChangeNumberVirtualIdAlgorithm virtualIdAlgorithm;
@Inject
DraftCommentsNotesReader(
DraftCommentNotes.Factory draftCommentNotesFactory,
GitRepositoryManager repoManager,
- AllUsersName allUsers) {
+ AllUsersName allUsers,
+ ChangeNumberVirtualIdAlgorithm virtualIdAlgorithm) {
this.draftCommentNotesFactory = draftCommentNotesFactory;
this.repoManager = repoManager;
this.allUsers = allUsers;
+ this.virtualIdAlgorithm = virtualIdAlgorithm;
}
@Override
@@ -64,7 +68,7 @@
@Override
public List<HumanComment> getDraftsByChangeAndDraftAuthor(ChangeNotes notes, Account.Id author) {
- return sort(new ArrayList<>(notes.getDraftComments(author)));
+ return sort(new ArrayList<>(notes.getDraftComments(author, getVirtualId(notes))));
}
@Override
@@ -77,7 +81,7 @@
public List<HumanComment> getDraftsByPatchSetAndDraftAuthor(
ChangeNotes notes, PatchSet.Id psId, Account.Id author) {
return sort(
- notes.load().getDraftComments(author).stream()
+ notes.load().getDraftComments(author, getVirtualId(notes)).stream()
.filter(c -> c.key.patchSetId == psId.get())
.collect(Collectors.toList()));
}
@@ -136,7 +140,7 @@
private List<Ref> getDraftRefs(ChangeNotes notes) {
try (Repository repo = repoManager.openRepository(allUsers)) {
return repo.getRefDatabase()
- .getRefsByPrefix(RefNames.refsDraftCommentsPrefix(notes.getChangeId()));
+ .getRefsByPrefix(RefNames.refsDraftCommentsPrefix(getVirtualId(notes)));
} catch (IOException e) {
throw new StorageException(e);
}
@@ -145,4 +149,10 @@
private List<HumanComment> sort(List<HumanComment> comments) {
return CommentsUtil.sort(comments);
}
+
+ private Change.Id getVirtualId(ChangeNotes notes) {
+ return virtualIdAlgorithm == null
+ ? notes.getChangeId()
+ : virtualIdAlgorithm.apply(notes.getServerId(), notes.getChangeId());
+ }
}
diff --git a/java/com/google/gerrit/server/notedb/StarredChangesUtilNoteDbImpl.java b/java/com/google/gerrit/server/notedb/StarredChangesUtilNoteDbImpl.java
index c52f082..f13b832 100644
--- a/java/com/google/gerrit/server/notedb/StarredChangesUtilNoteDbImpl.java
+++ b/java/com/google/gerrit/server/notedb/StarredChangesUtilNoteDbImpl.java
@@ -85,31 +85,31 @@
}
@Override
- public boolean isStarred(Account.Id accountId, Change.Id changeId) {
+ public boolean isStarred(Account.Id accountId, Change.Id virtualId) {
try (Repository repo = repoManager.openRepository(allUsers)) {
- return getStarRef(repo, RefNames.refsStarredChanges(changeId, accountId)).isPresent();
+ return getStarRef(repo, RefNames.refsStarredChanges(virtualId, accountId)).isPresent();
} catch (IOException e) {
throw new StorageException(
String.format(
"Reading stars from change %d for account %d failed",
- changeId.get(), accountId.get()),
+ virtualId.get(), accountId.get()),
e);
}
}
@Override
- public void star(Account.Id accountId, Change.Id changeId) {
- updateStar(accountId, changeId, true);
+ public void star(Account.Id accountId, Change.Id virtualId) {
+ updateStar(accountId, virtualId, true);
}
@Override
- public void unstar(Account.Id accountId, Change.Id changeId) {
- updateStar(accountId, changeId, false);
+ public void unstar(Account.Id accountId, Change.Id virtualId) {
+ updateStar(accountId, virtualId, false);
}
- private void updateStar(Account.Id accountId, Change.Id changeId, boolean shouldAdd) {
+ private void updateStar(Account.Id accountId, Change.Id virtualId, boolean shouldAdd) {
try (Repository repo = repoManager.openRepository(allUsers)) {
- String refName = RefNames.refsStarredChanges(changeId, accountId);
+ String refName = RefNames.refsStarredChanges(virtualId, accountId);
if (shouldAdd) {
addRef(repo, refName, null);
} else {
@@ -120,16 +120,16 @@
}
} catch (IOException e) {
throw new StorageException(
- String.format("Star change %d for account %d failed", changeId.get(), accountId.get()),
+ String.format("Star change %d for account %d failed", virtualId.get(), accountId.get()),
e);
}
}
@Override
public Set<Change.Id> areStarred(
- Repository allUsersRepo, List<Change.Id> changeIds, Account.Id caller) {
+ Repository allUsersRepo, List<Change.Id> virtualIds, Account.Id caller) {
List<String> starRefs =
- changeIds.stream()
+ virtualIds.stream()
.map(c -> RefNames.refsStarredChanges(c, caller))
.collect(Collectors.toList());
try {
@@ -140,21 +140,21 @@
} catch (IOException e) {
logger.atWarning().withCause(e).log(
"Failed getting starred changes for account %d within changes: %s",
- caller.get(), Joiner.on(", ").join(changeIds));
+ caller.get(), Joiner.on(", ").join(virtualIds));
return ImmutableSet.of();
}
}
@Override
- public void unstarAllForChangeDeletion(Change.Id changeId) throws IOException {
+ public void unstarAllForChangeDeletion(Change.Id virtualId) throws IOException {
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {
BatchRefUpdate batchUpdate = repo.getRefDatabase().newBatchUpdate();
batchUpdate.setAllowNonFastForwards(true);
batchUpdate.setRefLogIdent(serverIdent.get());
- batchUpdate.setRefLogMessage("Unstar change " + changeId.get(), true);
- for (Account.Id accountId : getStars(repo, changeId)) {
- String refName = RefNames.refsStarredChanges(changeId, accountId);
+ batchUpdate.setRefLogMessage("Unstar change " + virtualId.get(), true);
+ for (Account.Id accountId : getStars(repo, virtualId)) {
+ String refName = RefNames.refsStarredChanges(virtualId, accountId);
Ref ref = repo.getRefDatabase().exactRef(refName);
if (ref != null) {
batchUpdate.addCommand(new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), refName));
@@ -166,7 +166,7 @@
String message =
String.format(
"Unstar change %d failed, ref %s could not be deleted: %s",
- changeId.get(), command.getRefName(), command.getResult());
+ virtualId.get(), command.getRefName(), command.getResult());
if (command.getResult() == ReceiveCommand.Result.LOCK_FAILURE) {
throw new LockFailureException(message, batchUpdate);
}
@@ -177,11 +177,11 @@
}
@Override
- public ImmutableList<Account.Id> byChange(Change.Id changeId) {
+ public ImmutableList<Account.Id> byChange(Change.Id virtualId) {
try (Repository repo = repoManager.openRepository(allUsers)) {
ImmutableList.Builder<Account.Id> builder = ImmutableList.builder();
- for (Account.Id accountId : getStars(repo, changeId)) {
- Optional<Ref> starRef = getStarRef(repo, RefNames.refsStarredChanges(changeId, accountId));
+ for (Account.Id accountId : getStars(repo, virtualId)) {
+ Optional<Ref> starRef = getStarRef(repo, RefNames.refsStarredChanges(virtualId, accountId));
if (starRef.isPresent()) {
builder.add(accountId);
}
@@ -189,7 +189,7 @@
return builder.build();
} catch (IOException e) {
throw new StorageException(
- String.format("Get accounts that starred change %d failed", changeId.get()), e);
+ String.format("Get accounts that starred change %d failed", virtualId.get()), e);
}
}
@@ -223,9 +223,9 @@
}
}
- private static Set<Account.Id> getStars(Repository allUsers, Change.Id changeId)
+ private static Set<Account.Id> getStars(Repository allUsers, Change.Id virtualId)
throws IOException {
- String prefix = RefNames.refsStarredChangesPrefix(changeId);
+ String prefix = RefNames.refsStarredChangesPrefix(virtualId);
RefDatabase refDb = allUsers.getRefDatabase();
return refDb.getRefsByPrefix(prefix).stream()
.map(r -> r.getName().substring(prefix.length()))
diff --git a/java/com/google/gerrit/server/plugins/DisablePlugin.java b/java/com/google/gerrit/server/plugins/DisablePlugin.java
index 9e238f8..d845ec17 100644
--- a/java/com/google/gerrit/server/plugins/DisablePlugin.java
+++ b/java/com/google/gerrit/server/plugins/DisablePlugin.java
@@ -53,7 +53,12 @@
if (mandatoryPluginsCollection.contains(name)) {
throw new MethodNotAllowedException("Plugin " + name + " is mandatory");
}
- loader.disablePlugins(ImmutableSet.of(name));
+ try {
+ loader.disablePlugins(ImmutableSet.of(name));
+ } catch (PluginInstallException e) {
+ throw new MethodNotAllowedException("Plugin " + name + " cannot be disabled", e);
+ }
+
return Response.ok(ListPlugins.toPluginInfo(loader.get(name)));
}
}
diff --git a/java/com/google/gerrit/server/plugins/PluginLoader.java b/java/com/google/gerrit/server/plugins/PluginLoader.java
index 9e23c0b..e122d8d 100644
--- a/java/com/google/gerrit/server/plugins/PluginLoader.java
+++ b/java/com/google/gerrit/server/plugins/PluginLoader.java
@@ -221,7 +221,7 @@
toCleanup.add(plugin);
}
- public void disablePlugins(Set<String> names) {
+ public void disablePlugins(Set<String> names) throws PluginInstallException {
if (!isRemoteAdminEnabled()) {
logger.atWarning().log(
"Remote plugin administration is disabled, ignoring disablePlugins(%s)", names);
@@ -235,6 +235,12 @@
continue;
}
+ if (active.getApiModule().isPresent()) {
+ throw new PluginInstallException(
+ String.format(
+ "Plugin %s has registered an ApiModule therefore it cannot be disabled", name));
+ }
+
if (mandatoryPlugins.contains(name)) {
logger.atWarning().log("Mandatory plugin %s cannot be disabled", name);
continue;
@@ -381,11 +387,14 @@
try {
logger.atInfo().log("Reloading plugin %s", name);
Plugin newPlugin = runPlugin(name, active.getSrcFile(), active);
- logger.atInfo().log(
- "Reloaded plugin %s%s, version %s",
- newPlugin.getName(),
- newPlugin.getApiModule().isPresent() ? " (w/ ApiModule)" : "",
- newPlugin.getVersion());
+
+ if (newPlugin != active) {
+ logger.atInfo().log(
+ "Reloaded plugin %s%s, version %s",
+ newPlugin.getName(),
+ newPlugin.getApiModule().isPresent() ? " (w/ ApiModule)" : "",
+ newPlugin.getVersion());
+ }
} catch (PluginInstallException e) {
logger.atWarning().withCause(e.getCause()).log("Cannot reload plugin %s", name);
throw e;
@@ -522,6 +531,13 @@
return oldPlugin;
}
+ if (oldPlugin != null && oldPlugin.getApiModule().isPresent()) {
+ throw new PluginInstallException(
+ String.format(
+ "Plugin %s has registered an ApiModule therefore its restart/reload is not allowed",
+ name));
+ }
+
Plugin newPlugin = loadPlugin(name, plugin, snapshot);
if (newPlugin.getCleanupHandle() != null) {
cleanupHandles.put(newPlugin, newPlugin.getCleanupHandle());
@@ -573,7 +589,13 @@
}
}
for (String name : unload) {
- unloadPlugin(running.get(name));
+ Plugin runningPlugin = running.get(name);
+
+ if (runningPlugin.getApiModule().isPresent()) {
+ logger.atWarning().log("Cannot remove plugin %s as it has registered an ApiModule", name);
+ } else {
+ unloadPlugin(running.get(name));
+ }
}
}
diff --git a/java/com/google/gerrit/server/plugins/ServerPlugin.java b/java/com/google/gerrit/server/plugins/ServerPlugin.java
index c275ff9..036285e 100644
--- a/java/com/google/gerrit/server/plugins/ServerPlugin.java
+++ b/java/com/google/gerrit/server/plugins/ServerPlugin.java
@@ -172,6 +172,11 @@
@Override
protected boolean canReload() {
Attributes main = manifest.getMainAttributes();
+ String apiModule = main.getValue("Gerrit-ApiModule");
+ if (apiModule != null) {
+ return false;
+ }
+
String v = main.getValue("Gerrit-ReloadMode");
if (Strings.isNullOrEmpty(v) || "reload".equalsIgnoreCase(v)) {
return true;
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 5676ab4..d153bc9 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -50,7 +50,6 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.RobotComment;
import com.google.gerrit.entities.SubmitRecord;
@@ -78,7 +77,6 @@
import com.google.gerrit.server.change.PureRevert;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.config.SkipCurrentRulesEvaluationOnClosedChanges;
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -110,6 +108,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
@@ -248,11 +247,11 @@
}
public ChangeData create(Project.NameKey project, Change.Id id) {
- return assistedFactory.create(project, id, null, null);
+ return assistedFactory.create(project, id, null, null, null);
}
public ChangeData create(Project.NameKey project, Change.Id id, ObjectId metaRevision) {
- ChangeData cd = assistedFactory.create(project, id, null, null);
+ ChangeData cd = assistedFactory.create(project, id, null, null, null);
cd.setMetaRevision(metaRevision);
return cd;
}
@@ -265,19 +264,29 @@
}
public ChangeData create(Change change) {
- return assistedFactory.create(change.getProject(), change.getId(), change, null);
+ return create(change, null);
+ }
+
+ public ChangeData create(Change change, Change.Id virtualId) {
+ return assistedFactory.create(
+ change.getProject(),
+ change.getId(),
+ !Objects.equals(virtualId, change.getId()) ? virtualId : null,
+ change,
+ null);
}
public ChangeData create(ChangeNotes notes) {
return assistedFactory.create(
- notes.getChange().getProject(), notes.getChangeId(), notes.getChange(), notes);
+ notes.getChange().getProject(), notes.getChangeId(), null, notes.getChange(), notes);
}
}
public interface AssistedFactory {
ChangeData create(
Project.NameKey project,
- Change.Id id,
+ @Assisted("changeId") Change.Id id,
+ @Assisted("virtualId") @Nullable Change.Id virtualId,
@Nullable Change change,
@Nullable ChangeNotes notes);
}
@@ -296,7 +305,7 @@
*/
public static ChangeData createForTest(
Project.NameKey project, Change.Id id, int currentPatchSetId, ObjectId commitId) {
- return createForTest(project, id, currentPatchSetId, commitId, null, null, null);
+ return createForTest(project, id, currentPatchSetId, commitId, null, null);
}
/**
@@ -309,7 +318,6 @@
* @param id change ID
* @param currentPatchSetId current patchset number
* @param commitId commit SHA1 of the current patchset
- * @param serverId Gerrit server id
* @param virtualIdAlgo algorithm for virtualising the Change number
* @param changeNotes notes associated with the Change
* @return instance for testing.
@@ -319,7 +327,6 @@
Change.Id id,
int currentPatchSetId,
ObjectId commitId,
- @Nullable String serverId,
@Nullable ChangeNumberVirtualIdAlgorithm virtualIdAlgo,
@Nullable ChangeNotes changeNotes) {
ChangeData cd =
@@ -343,12 +350,12 @@
null,
null,
null,
- serverId,
virtualIdAlgo,
false,
project,
id,
null,
+ null,
changeNotes);
cd.currentPatchSet =
PatchSet.builder()
@@ -450,12 +457,12 @@
private Integer totalCommentCount;
private LabelTypes labelTypes;
private Optional<Instant> mergedOn;
- private ImmutableSetMultimap<NameKey, RefState> refStates;
+ private ImmutableSetMultimap<Project.NameKey, RefState> refStates;
private ImmutableList<byte[]> refStatePatterns;
- private String gerritServerId;
private String changeServerId;
private ChangeNumberVirtualIdAlgorithm virtualIdFunc;
private Boolean failedParsingFromIndex = false;
+ private Change.Id virtualId;
@Inject
private ChangeData(
@@ -478,11 +485,11 @@
SubmitRequirementsEvaluator submitRequirementsEvaluator,
SubmitRequirementsUtil submitRequirementsUtil,
SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory,
- @GerritServerId String gerritServerId,
ChangeNumberVirtualIdAlgorithm virtualIdFunc,
@SkipCurrentRulesEvaluationOnClosedChanges Boolean skipCurrentRulesEvaluationOnClosedChange,
@Assisted Project.NameKey project,
- @Assisted Change.Id id,
+ @Assisted("changeId") Change.Id id,
+ @Assisted("virtualId") @Nullable Change.Id virtualId,
@Assisted @Nullable Change change,
@Assisted @Nullable ChangeNotes notes) {
this.approvalsUtil = approvalsUtil;
@@ -516,8 +523,8 @@
this.notes = notes;
this.changeServerId = notes == null ? null : notes.getServerId();
- this.gerritServerId = gerritServerId;
this.virtualIdFunc = virtualIdFunc;
+ this.virtualId = virtualId;
}
/**
@@ -648,12 +655,33 @@
return legacyId;
}
- public Change.Id getVirtualId() {
- if (virtualIdFunc == null || changeServerId == null || changeServerId.equals(gerritServerId)) {
- return legacyId;
+ public static void ensureChangeServerId(Iterable<ChangeData> changes) {
+ ChangeData first = Iterables.getFirst(changes, null);
+ if (first == null) {
+ return;
}
- return Change.id(virtualIdFunc.apply(changeServerId, legacyId.get()));
+ for (ChangeData cd : changes) {
+ var unused = cd.changeServerId();
+ }
+ }
+
+ @Nullable
+ public String changeServerId() {
+ if (changeServerId == null) {
+ if (!lazyload()) {
+ return null;
+ }
+ changeServerId = notes().getServerId();
+ }
+ return changeServerId;
+ }
+
+ public Change.Id virtualId() {
+ if (virtualId == null) {
+ return virtualIdFunc == null ? legacyId : virtualIdFunc.apply(changeServerId, legacyId);
+ }
+ return virtualId;
}
public Project.NameKey project() {
@@ -692,10 +720,10 @@
return this;
}
- public ObjectId metaRevisionOrThrow() {
+ public Optional<ObjectId> metaRevision() {
if (notes == null) {
if (metaRevision != null) {
- return metaRevision;
+ return Optional.of(metaRevision);
}
if (refStates != null) {
ImmutableSet<RefState> refs = refStates.get(project);
@@ -703,17 +731,25 @@
String metaRef = RefNames.changeMetaRef(getId());
for (RefState r : refs) {
if (r.ref().equals(metaRef)) {
- return r.id();
+ return Optional.of(r.id());
}
}
}
}
- throwIfNotLazyLoad("metaRevision");
+ if (!lazyload()) {
+ return Optional.empty();
+ }
@SuppressWarnings("unused")
var unused = notes();
}
- return notes.getRevision();
+ metaRevision = notes.getRevision();
+ return Optional.of(metaRevision);
+ }
+
+ public ObjectId metaRevisionOrThrow() {
+ return metaRevision()
+ .orElseThrow(() -> new IllegalStateException("'metaRevision' field not populated"));
}
boolean fastIsVisibleTo(CurrentUser user) {
@@ -1436,7 +1472,7 @@
if (!lazyload()) {
return ImmutableList.of();
}
- starAccounts = requireNonNull(starredChangesReader).byChange(legacyId);
+ starAccounts = requireNonNull(starredChangesReader).byChange(virtualId());
}
return starAccounts;
}
@@ -1499,13 +1535,14 @@
}
}
- public SetMultimap<NameKey, RefState> getRefStates() {
+ public SetMultimap<Project.NameKey, RefState> getRefStates() {
if (refStates == null) {
if (!lazyload()) {
return ImmutableSetMultimap.of();
}
- ImmutableSetMultimap.Builder<NameKey, RefState> result = ImmutableSetMultimap.builder();
+ ImmutableSetMultimap.Builder<Project.NameKey, RefState> result =
+ ImmutableSetMultimap.builder();
for (Table.Cell<Account.Id, PatchSet.Id, Ref> edit : editRefs().cellSet()) {
result.put(
project,
diff --git a/java/com/google/gerrit/server/query/change/ChangeNumberBitmapMaskAlgorithm.java b/java/com/google/gerrit/server/query/change/ChangeNumberBitmapMaskAlgorithm.java
index 726a376..95c287a 100644
--- a/java/com/google/gerrit/server/query/change/ChangeNumberBitmapMaskAlgorithm.java
+++ b/java/com/google/gerrit/server/query/change/ChangeNumberBitmapMaskAlgorithm.java
@@ -16,7 +16,9 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.server.config.GerritImportedServerIds;
+import com.google.gerrit.server.config.GerritServerId;
import com.google.inject.Inject;
import com.google.inject.ProvisionException;
import com.google.inject.Singleton;
@@ -38,9 +40,11 @@
Integer.BYTES * 8 - CHANGE_NUM_BIT_LEN; // Allows up to 64 ServerIds
private final ImmutableMap<String, Integer> serverIdCodes;
+ private final String localServerId;
@Inject
public ChangeNumberBitmapMaskAlgorithm(
+ @GerritServerId String localServerId,
@GerritImportedServerIds ImmutableList<String> importedServerIds) {
if (importedServerIds.size() >= 1 << SERVER_ID_BIT_LEN) {
throw new ProvisionException(
@@ -54,10 +58,17 @@
}
serverIdCodes = serverIdCodesBuilder.build();
+ this.localServerId = localServerId;
}
@Override
- public int apply(String changeServerId, int changeNum) {
+ public Change.Id apply(String changeServerId, Change.Id changeNumId) {
+ if (changeServerId == null || localServerId.equals(changeServerId)) {
+ return changeNumId;
+ }
+
+ int changeNum = changeNumId.get();
+
if ((changeNum & LEGACY_ID_BIT_MASK) != changeNum) {
throw new IllegalArgumentException(
String.format(
@@ -71,6 +82,6 @@
}
int virtualId = (changeNum & LEGACY_ID_BIT_MASK) | (encodedServerId << CHANGE_NUM_BIT_LEN);
- return virtualId;
+ return Change.id(virtualId);
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeNumberVirtualIdAlgorithm.java b/java/com/google/gerrit/server/query/change/ChangeNumberVirtualIdAlgorithm.java
index ab21705..6daf16f 100644
--- a/java/com/google/gerrit/server/query/change/ChangeNumberVirtualIdAlgorithm.java
+++ b/java/com/google/gerrit/server/query/change/ChangeNumberVirtualIdAlgorithm.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.query.change;
+import com.google.gerrit.entities.Change;
import com.google.inject.ImplementedBy;
/**
@@ -31,5 +32,5 @@
* @param legacyChangeNum legacy change number
* @return virtual id which combines serverId and legacyChangeNum together
*/
- int apply(String serverId, int legacyChangeNum);
+ Change.Id apply(String serverId, Change.Id legacyChangeNum);
}
diff --git a/java/com/google/gerrit/server/query/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index 587f2de..d5f9c5b 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -18,6 +18,7 @@
import com.google.common.base.CharMatcher;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
@@ -87,7 +88,10 @@
/**
* Returns a predicate that matches changes where the provided {@link
* com.google.gerrit.entities.Account.Id} has a pending draft comment.
+ *
+ * <p>The predicates filter by "legacy_id_str" field.
*/
+ @UsedAt(UsedAt.Project.GOOGLE)
public static Predicate<ChangeData> draftBy(
DraftCommentsReader draftCommentsReader, Account.Id id) {
ImmutableSet<Predicate<ChangeData>> changeIdPredicates =
@@ -102,7 +106,10 @@
/**
* Returns a predicate that matches changes where the provided {@link
* com.google.gerrit.entities.Account.Id} has starred changes with {@code label}.
+ *
+ * <p>The predicates filter by "legacy_id_str" field.
*/
+ @UsedAt(UsedAt.Project.GOOGLE)
public static Predicate<ChangeData> starBy(
StarredChangesReader starredChangesReader, Account.Id id) {
ImmutableSet<Predicate<ChangeData>> starredChanges =
@@ -144,6 +151,19 @@
}
/**
+ * Returns a predicate that matches the change number with the provided {@link
+ * com.google.gerrit.entities.Change.Id}.
+ */
+ public static Predicate<ChangeData> changeNumber(
+ Change.Id id, ChangeQueryBuilder.Arguments args) {
+ if (args.getSchema().hasField(ChangeField.CHANGENUM_SPEC)) {
+ return new ChangeIndexCardinalPredicate(
+ ChangeField.CHANGENUM_SPEC, ChangeQueryBuilder.FIELD_CHANGE, id.toString(), 1);
+ }
+ return idStr(id);
+ }
+
+ /**
* Returns a predicate that matches changes owned by the provided {@link
* com.google.gerrit.entities.Account.Id}.
*/
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index a64b68d..d598739 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -144,7 +144,7 @@
public interface ChangeIsOperandFactory extends ChangeOperandFactory {}
- private static final Pattern PAT_LEGACY_ID = Pattern.compile("^[1-9][0-9]*$");
+ private static final Pattern PAT_CHANGE_NUMBER = Pattern.compile("^[1-9][0-9]*$");
private static final Pattern PAT_PROJECT_CHANGE_NUM = Pattern.compile("^([^~]+)~([1-9][0-9]*)$");
private static final Pattern PAT_CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN);
private static final Pattern DEF_CHANGE =
@@ -166,6 +166,7 @@
public static final String FIELD_CHANGE = "change";
public static final String FIELD_CHANGE_ID = "change_id";
+ public static final String FIELD_CHANGE_NUMBER = "changenumber";
public static final String FIELD_COMMENT = "comment";
public static final String FIELD_COMMENTBY = "commentby";
public static final String FIELD_COMMIT = "commit";
@@ -588,6 +589,18 @@
@Operator
public Predicate<ChangeData> change(String query) throws QueryParseException {
+ return getPredicateChangeData(query, changeId -> ChangePredicates.changeNumber(changeId, args));
+ }
+
+ // Keep using the index legacy document-id (legacy_id_str) for URLs queries like: "/q/123456",
+ // "/q/Iasdw2312321", "/q/project~123456" that are expecting to always find a single element.
+ private Predicate<ChangeData> defaultSearch(String query) throws QueryParseException {
+ return getPredicateChangeData(query, ChangePredicates::idStr);
+ }
+
+ private Predicate<ChangeData> getPredicateChangeData(
+ String query, Function<Change.Id, Predicate<ChangeData>> changePredicateGetter)
+ throws QueryParseException {
Optional<ChangeTriplet> triplet = ChangeTriplet.parse(query);
if (triplet.isPresent()) {
return Predicate.and(
@@ -601,11 +614,10 @@
return Predicate.and(
project(projectChangeNumber.group(1)),
ChangePredicates.idStr(projectChangeNumber.group(2)));
-
- } else if (PAT_LEGACY_ID.matcher(query).matches()) {
+ } else if (PAT_CHANGE_NUMBER.matcher(query).matches()) {
Integer id = Ints.tryParse(query);
if (id != null) {
- return ChangePredicates.idStr(Change.id(id));
+ return changePredicateGetter.apply(Change.id(id));
}
} else if (PAT_CHANGE_ID.matcher(query).matches()) {
return ChangePredicates.idPrefix(parseChangeId(query));
@@ -1690,13 +1702,13 @@
} else if (DEF_CHANGE.matcher(query).matches()) {
List<Predicate<ChangeData>> predicates = Lists.newArrayListWithCapacity(2);
try {
- predicates.add(change(query));
+ predicates.add(defaultSearch(query));
} catch (QueryParseException e) {
// Skip.
}
- // For PAT_LEGACY_ID, it may also be the prefix of some commits.
- if (query.length() >= 6 && PAT_LEGACY_ID.matcher(query).matches()) {
+ // For PAT_CHANGE_NUMBER, it may also be the prefix of some commits.
+ if (query.length() >= 6 && PAT_CHANGE_NUMBER.matcher(query).matches()) {
predicates.add(commit(query));
}
@@ -1860,12 +1872,12 @@
}
private List<ChangeData> parseChangeData(String value) throws QueryParseException {
- if (PAT_LEGACY_ID.matcher(value).matches()) {
+ if (PAT_CHANGE_NUMBER.matcher(value).matches()) {
Optional<Change.Id> id = Change.Id.tryParse(value);
if (!id.isPresent()) {
throw error("Invalid change id " + value);
}
- return args.queryProvider.get().byLegacyChangeId(id.get());
+ return args.queryProvider.get().byChangeNumber(id.get());
} else if (PAT_CHANGE_ID.matcher(value).matches()) {
List<ChangeData> changes = args.queryProvider.get().byKeyPrefix(parseChangeId(value));
if (changes.isEmpty()) {
diff --git a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
index fc4c1d0..87b8915 100644
--- a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
@@ -89,7 +89,7 @@
List<Predicate<ChangeData>> and = new ArrayList<>(5);
and.add(ChangePredicates.project(c.getProject()));
and.add(ChangePredicates.ref(c.getDest().branch()));
- and.add(Predicate.not(ChangePredicates.idStr(c.getId())));
+ and.add(Predicate.not(ChangePredicates.changeNumber(c.getId(), args)));
and.add(Predicate.or(filePredicates));
ChangeDataCache changeDataCache = new ChangeDataCache(cd, args.projectCache);
diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index f3eda15..a8ee4bc 100644
--- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -88,6 +88,7 @@
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory notesFactory;
private final EditByPredicateProvider editByPredicateProvider;
+ private final Provider<ChangeQueryBuilder.Arguments> queryBuilderArgsProvider;
@Inject
InternalChangeQuery(
@@ -96,11 +97,13 @@
IndexConfig indexConfig,
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory,
- EditByPredicateProvider editByPredicateProvider) {
+ EditByPredicateProvider editByPredicateProvider,
+ Provider<ChangeQueryBuilder.Arguments> queryBuilderArgsProvider) {
super(queryProcessor, indexes, indexConfig);
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
this.editByPredicateProvider = editByPredicateProvider;
+ this.queryBuilderArgsProvider = queryBuilderArgsProvider;
}
public List<ChangeData> byKey(Change.Key key) {
@@ -115,6 +118,10 @@
return query(ChangePredicates.idStr(id));
}
+ public List<ChangeData> byChangeNumber(Change.Id id) {
+ return query(ChangePredicates.changeNumber(id, queryBuilderArgsProvider.get()));
+ }
+
@UsedAt(UsedAt.Project.GOOGLE)
public List<ChangeData> byLegacyChangeIds(Collection<Change.Id> ids) {
List<Predicate<ChangeData>> preds = new ArrayList<>(ids.size());
@@ -319,7 +326,7 @@
for (List<String> part : Iterables.partition(groups, batchSize)) {
for (ChangeData cd :
queryExhaustively(querySupplier, byProjectGroupsPredicate(indexConfig, project, part))) {
- if (!seen.add(cd.getId())) {
+ if (!seen.add(cd.virtualId())) {
result.add(cd);
}
}
diff --git a/java/com/google/gerrit/server/query/change/OrSource.java b/java/com/google/gerrit/server/query/change/OrSource.java
index 9633a18..8894287 100644
--- a/java/com/google/gerrit/server/query/change/OrSource.java
+++ b/java/com/google/gerrit/server/query/change/OrSource.java
@@ -52,7 +52,7 @@
Set<Change.Id> have = new HashSet<>();
for (ResultSet<ChangeData> resultSet : results) {
for (ChangeData result : resultSet) {
- if (have.add(result.getId())) {
+ if (have.add(result.virtualId())) {
r.add(result);
}
}
diff --git a/java/com/google/gerrit/server/query/change/PredicateArgs.java b/java/com/google/gerrit/server/query/change/PredicateArgs.java
index ebe4390..02d2ca6 100644
--- a/java/com/google/gerrit/server/query/change/PredicateArgs.java
+++ b/java/com/google/gerrit/server/query/change/PredicateArgs.java
@@ -71,7 +71,7 @@
*
* @param args arguments to be parsed
*/
- PredicateArgs(String args) throws QueryParseException {
+ public PredicateArgs(String args) throws QueryParseException {
positional = new ArrayList<>();
keyValue = new HashMap<>();
diff --git a/java/com/google/gerrit/server/restapi/account/StarredChanges.java b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
index 1565aba..95aa07c 100644
--- a/java/com/google/gerrit/server/restapi/account/StarredChanges.java
+++ b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
@@ -69,7 +69,7 @@
throws RestApiException, PermissionBackendException, IOException {
IdentifiedUser user = parent.getUser();
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
- if (starredChangesReader.isStarred(user.getAccountId(), change.getId())) {
+ if (starredChangesReader.isStarred(user.getAccountId(), change.getVirtualId())) {
return new AccountResource.StarredChange(user, change);
}
throw new ResourceNotFoundException(id);
@@ -125,7 +125,7 @@
}
try {
- starredChangesWriter.star(self.get().getAccountId(), change.getId());
+ starredChangesWriter.star(self.get().getAccountId(), change.getVirtualId());
} catch (DuplicateKeyException e) {
return Response.none();
}
@@ -168,7 +168,7 @@
if (!self.get().hasSameAccountId(rsrc.getUser())) {
throw new AuthException("not allowed remove starred change");
}
- starredChangesWriter.unstar(self.get().getAccountId(), rsrc.getChange().getId());
+ starredChangesWriter.unstar(self.get().getAccountId(), rsrc.getVirtualId());
return Response.none();
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java
index 341a9d9..e254bfc 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java
@@ -26,15 +26,18 @@
import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.converter.ChangeInputProtoConverter;
import com.google.gerrit.exceptions.InvalidMergeStrategyException;
import com.google.gerrit.exceptions.MergeWithConflictsNotSupportedException;
import com.google.gerrit.extensions.api.accounts.AccountInput;
+import com.google.gerrit.extensions.api.changes.ApplyPatchInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.ListChangesOption;
@@ -52,6 +55,7 @@
import com.google.gerrit.extensions.restapi.RestCollectionModifyView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.proto.Entities;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
@@ -96,6 +100,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Optional;
+import org.eclipse.jgit.annotations.NonNull;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.errors.MissingObjectException;
@@ -118,6 +123,8 @@
public class CreateChange
implements RestCollectionModifyView<TopLevelResource, ChangeResource, ChangeInput> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final ChangeInputProtoConverter CHANGE_INPUT_PROTO_CONVERTER =
+ ChangeInputProtoConverter.INSTANCE;
private final BatchUpdate.Factory updateFactory;
private final String anonymousCowardName;
@@ -191,11 +198,51 @@
return execute(updateFactory, input, projectsCollection.parse(input.project));
}
- /** Creates the changes in the given project. This is public for reuse in the project API. */
+ @UsedAt(UsedAt.Project.GOOGLE)
+ @FunctionalInterface
+ public interface CommitTreeSupplier {
+ @NonNull
+ ObjectId get(Repository repo, ObjectInserter oi, ChangeInput input, RevCommit mergeTip)
+ throws IOException, RestApiException;
+ }
+
+ /**
+ * Creates the changes in the given project, using the proto representation of ChangeInput -
+ * {@link com.google.gerrit.proto.Entities.ChangeInput}.
+ */
+ @UsedAt(UsedAt.Project.GOOGLE)
+ public Response<ChangeInfo> execute(
+ BatchUpdate.Factory updateFactory,
+ Entities.ChangeInput input,
+ CommitTreeSupplier commitTreeSupplier)
+ throws IOException, RestApiException, UpdateException, PermissionBackendException,
+ ConfigInvalidException {
+ return execute(
+ updateFactory,
+ CHANGE_INPUT_PROTO_CONVERTER.fromProto(input),
+ projectsCollection.parse(input.getProject()),
+ Optional.of(commitTreeSupplier));
+ }
+
+ /**
+ * Creates the changes in the given project, using the java-class representation of ChangeInput -
+ * {@link com.google.gerrit.extensions.common.ChangeInput}. This is public for reuse in the
+ * project API.
+ */
public Response<ChangeInfo> execute(
BatchUpdate.Factory updateFactory, ChangeInput input, ProjectResource projectResource)
throws IOException, RestApiException, UpdateException, PermissionBackendException,
ConfigInvalidException {
+ return execute(updateFactory, input, projectResource, Optional.empty());
+ }
+
+ private Response<ChangeInfo> execute(
+ BatchUpdate.Factory updateFactory,
+ ChangeInput input,
+ ProjectResource projectResource,
+ Optional<CommitTreeSupplier> commitTreeSupplier)
+ throws IOException, RestApiException, UpdateException, PermissionBackendException,
+ ConfigInvalidException {
if (!user.get().isIdentifiedUser()) {
throw new AuthException("Authentication required");
}
@@ -204,14 +251,15 @@
projectState.checkStatePermitsWrite();
IdentifiedUser me = user.get().asIdentifiedUser();
- checkAndSanitizeChangeInput(input, me);
+ checkAndSanitizeChangeInput(input, me, commitTreeSupplier);
Project.NameKey project = projectResource.getNameKey();
contributorAgreements.check(project, user.get());
checkRequiredPermissions(project, input.branch, input.author);
- ChangeInfo newChange = createNewChange(input, me, projectState, updateFactory);
+ ChangeInfo newChange =
+ createNewChange(input, me, projectState, updateFactory, commitTreeSupplier);
return Response.created(newChange);
}
@@ -225,7 +273,8 @@
* @param me the user who sent the current request to create a change.
* @throws BadRequestException if the input is not legal.
*/
- private void checkAndSanitizeChangeInput(ChangeInput input, IdentifiedUser me)
+ private void checkAndSanitizeChangeInput(
+ ChangeInput input, IdentifiedUser me, Optional<CommitTreeSupplier> commitTreeSupplier)
throws RestApiException, PermissionBackendException, IOException {
if (Strings.isNullOrEmpty(input.branch)) {
throw new BadRequestException("branch must be non-empty");
@@ -303,6 +352,11 @@
throw new BadRequestException("Only one of `merge` and `patch` arguments can be set.");
}
+ if ((input.merge != null || input.patch != null) && commitTreeSupplier.isPresent()) {
+ throw new BadRequestException(
+ "`CommitTreeSupplier` cannot be provided along with `merge` or `patch` arguments");
+ }
+
if (input.author != null
&& (Strings.isNullOrEmpty(input.author.email)
|| Strings.isNullOrEmpty(input.author.name))) {
@@ -332,7 +386,8 @@
ChangeInput input,
IdentifiedUser me,
ProjectState projectState,
- BatchUpdate.Factory updateFactory)
+ BatchUpdate.Factory updateFactory,
+ Optional<CommitTreeSupplier> commitTreeSupplier)
throws RestApiException, PermissionBackendException, IOException, ConfigInvalidException,
UpdateException {
try (RefUpdateContext ctx = RefUpdateContext.open(CHANGE_MODIFICATION)) {
@@ -404,31 +459,22 @@
}
} else if (input.patch != null) {
// create a commit with the given patch.
- if (mergeTip == null) {
- throw new BadRequestException("Cannot apply patch on top of an empty tree.");
- }
- PatchApplier.Result applyResult =
- ApplyPatchUtil.applyPatch(git, oi, input.patch, mergeTip);
- ObjectId treeId = applyResult.getTreeId();
- logger.atFine().log("tree ID after applying patch: %s", treeId.name());
- String appliedPatchCommitMessage =
- getCommitMessage(
- ApplyPatchUtil.buildCommitMessage(
- input.subject,
- ImmutableList.of(),
- input.patch.patch,
- ApplyPatchUtil.getResultPatch(git, reader, mergeTip, rw.lookupTree(treeId)),
- applyResult.getErrors()),
- me);
c =
- rw.parseCommit(
- CommitUtil.createCommitWithTree(
- oi,
- author,
- committer,
- ImmutableList.of(mergeTip),
- appliedPatchCommitMessage,
- treeId));
+ createCommitWithPatch(
+ git, reader, oi, rw, mergeTip, input.patch, input.subject, author, committer, me);
+ } else if (commitTreeSupplier.isPresent()) {
+ c =
+ createCommitWithSuppliedTree(
+ git,
+ oi,
+ rw,
+ mergeTip,
+ input,
+ commitTreeSupplier.get(),
+ author,
+ committer,
+ commitMessage);
+
} else {
// create an empty commit.
c = createEmptyCommit(oi, rw, author, committer, mergeTip, commitMessage);
@@ -615,6 +661,63 @@
oi, authorIdent, committerIdent, parents, commitMessage, treeId));
}
+ private CodeReviewCommit createCommitWithPatch(
+ Repository repo,
+ ObjectReader reader,
+ ObjectInserter oi,
+ CodeReviewRevWalk rw,
+ RevCommit mergeTip,
+ ApplyPatchInput patch,
+ String subject,
+ PersonIdent authorIdent,
+ PersonIdent committerIdent,
+ IdentifiedUser me)
+ throws IOException, RestApiException {
+ if (mergeTip == null) {
+ throw new BadRequestException("Cannot apply patch on top of an empty tree.");
+ }
+ PatchApplier.Result applyResult = ApplyPatchUtil.applyPatch(repo, oi, patch, mergeTip);
+ ObjectId treeId = applyResult.getTreeId();
+ logger.atFine().log("tree ID after applying patch: %s", treeId.name());
+ String appliedPatchCommitMessage =
+ getCommitMessage(
+ ApplyPatchUtil.buildCommitMessage(
+ subject,
+ ImmutableList.of(),
+ patch.patch,
+ ApplyPatchUtil.getResultPatch(repo, reader, mergeTip, rw.lookupTree(treeId)),
+ applyResult.getErrors()),
+ me);
+ return rw.parseCommit(
+ CommitUtil.createCommitWithTree(
+ oi,
+ authorIdent,
+ committerIdent,
+ ImmutableList.of(mergeTip),
+ appliedPatchCommitMessage,
+ treeId));
+ }
+
+ private static CodeReviewCommit createCommitWithSuppliedTree(
+ Repository repo,
+ ObjectInserter oi,
+ CodeReviewRevWalk rw,
+ RevCommit mergeTip,
+ ChangeInput input,
+ CommitTreeSupplier commitTreeSupplier,
+ PersonIdent authorIdent,
+ PersonIdent committerIdent,
+ String commitMessage)
+ throws IOException, RestApiException {
+ if (mergeTip == null) {
+ throw new BadRequestException("`CommitTreeSupplier` cannot be used on top of an empty tree.");
+ }
+ ObjectId treeId = commitTreeSupplier.get(repo, oi, input, mergeTip);
+ return rw.parseCommit(
+ CommitUtil.createCommitWithTree(
+ oi, authorIdent, committerIdent, ImmutableList.of(mergeTip), commitMessage, treeId));
+ }
+
private static ObjectId emptyTreeId(ObjectInserter inserter) throws IOException {
return inserter.insert(new TreeFormatter());
}
diff --git a/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java b/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
index 0119349b..d478a9d 100644
--- a/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
+++ b/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
@@ -17,6 +17,7 @@
import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE;
import com.google.common.collect.Sets;
+import com.google.gerrit.server.plugins.PluginInstallException;
import com.google.gerrit.sshd.CommandMetaData;
import java.util.List;
import org.kohsuke.args4j.Argument;
@@ -29,7 +30,11 @@
@Override
protected void doRun() throws UnloggedFailure {
if (names != null && !names.isEmpty()) {
- loader.disablePlugins(Sets.newHashSet(names));
+ try {
+ loader.disablePlugins(Sets.newHashSet(names));
+ } catch (PluginInstallException e) {
+ throw die(e);
+ }
}
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index dcdbce3..698eac8 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -51,6 +51,7 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.converter.ChangeInputProtoConverter;
import com.google.gerrit.extensions.api.accounts.AccountInput;
import com.google.gerrit.extensions.api.changes.ApplyPatchInput;
import com.google.gerrit.extensions.api.changes.ChangeApi;
@@ -82,7 +83,11 @@
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
+import com.google.gerrit.server.restapi.change.ApplyPatchUtil;
+import com.google.gerrit.server.restapi.change.CreateChange;
+import com.google.gerrit.server.restapi.change.CreateChange.CommitTreeSupplier;
import com.google.gerrit.server.submit.ChangeAlreadyMergedException;
+import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.gson.stream.JsonReader;
import com.google.inject.Inject;
@@ -110,9 +115,13 @@
@UseClockStep
public class CreateChangeIT extends AbstractDaemonTest {
+ private static final ChangeInputProtoConverter CHANGE_INPUT_PROTO_CONVERTER =
+ ChangeInputProtoConverter.INSTANCE;
@Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ExtensionRegistry extensionRegistry;
+ @Inject private CreateChange createChangeImpl;
+ @Inject private BatchUpdate.Factory updateFactory;;
@Before
public void addNonCommitHead() throws Exception {
@@ -1157,6 +1166,26 @@
}
@Test
+ public void createChangeWithCommitTreeSupplier_success() throws Exception {
+ createBranch(BranchNameKey.create(project, "other"));
+ ChangeInput input = newChangeInput(ChangeStatus.NEW);
+ input.branch = "other";
+ input.subject = "custom commit message";
+ ApplyPatchInput applyPatchInput = new ApplyPatchInput();
+ applyPatchInput.patch = PATCH_INPUT;
+ CommitTreeSupplier commitTreeSupplier =
+ (repo, oi, in, mergeTip) ->
+ ApplyPatchUtil.applyPatch(repo, oi, applyPatchInput, mergeTip).getTreeId();
+
+ ChangeInfo info = assertCreateWithCommitTreeSupplierSucceeds(input, commitTreeSupplier);
+
+ DiffInfo diff = gApi.changes().id(info.id).current().file(PATCH_FILE_NAME).diff();
+ assertDiffForNewFile(diff, info.currentRevision, PATCH_FILE_NAME, PATCH_NEW_FILE_CONTENT);
+ assertThat(info.revisions.get(info.currentRevision).commit.message)
+ .isEqualTo("custom commit message\n\nChange-Id: " + info.changeId + "\n");
+ }
+
+ @Test
@UseSystemTime
public void sha1sOfTwoNewChangesDiffer() throws Exception {
ChangeInput changeInput = newChangeInput(ChangeStatus.NEW);
@@ -1351,6 +1380,18 @@
return out;
}
+ private ChangeInfo assertCreateWithCommitTreeSupplierSucceeds(
+ ChangeInput input, CommitTreeSupplier commitTreeSupplier) throws Exception {
+ ChangeInfo res =
+ createChangeImpl
+ .execute(updateFactory, CHANGE_INPUT_PROTO_CONVERTER.toProto(input), commitTreeSupplier)
+ .value();
+ // The original result doesn't contain any revision data.
+ ChangeInfo out = gApi.changes().id(res.changeId).get(ALL_REVISIONS, CURRENT_COMMIT);
+ validateCreateSucceeds(input, out);
+ return out;
+ }
+
private static <T> T readContentFromJson(RestResponse r, Class<T> clazz) throws Exception {
try (JsonReader jsonReader = new JsonReader(r.getReader())) {
return newGson().fromJson(jsonReader, clazz);
diff --git a/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java b/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java
index 84c3936..c4497dc 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/SshTraceIT.java
@@ -32,6 +32,7 @@
import com.google.gerrit.server.validators.ProjectCreationValidationListener;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
+import java.time.Instant;
import org.junit.Test;
@UseSsh
@@ -123,8 +124,8 @@
private ImmutableList.Builder<PerformanceLogEntry> logEntries = ImmutableList.builder();
@Override
- public void log(String operation, long durationMs, Metadata metadata) {
- logEntries.add(PerformanceLogEntry.create(operation, metadata));
+ public void log(String operation, long durationMs, Instant endTime, Metadata metadata) {
+ logEntries.add(PerformanceLogEntry.create(operation, endTime, metadata));
}
ImmutableList<PerformanceLogEntry> logEntries() {
@@ -134,12 +135,14 @@
@AutoValue
abstract static class PerformanceLogEntry {
- static PerformanceLogEntry create(String operation, Metadata metadata) {
- return new AutoValue_SshTraceIT_PerformanceLogEntry(operation, metadata);
+ static PerformanceLogEntry create(String operation, Instant endTime, Metadata metadata) {
+ return new AutoValue_SshTraceIT_PerformanceLogEntry(operation, endTime, metadata);
}
abstract String operation();
+ abstract Instant endTime();
+
abstract Metadata metadata();
}
}
diff --git a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
index bbf10bd..812a0df 100644
--- a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
+++ b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
@@ -32,6 +32,7 @@
public class ChangeProtoConverterTest {
private final ChangeProtoConverter changeProtoConverter = ChangeProtoConverter.INSTANCE;
+ private static final String TEST_SERVER_ID = "test-server-id";
@Test
public void allValuesConvertedToProto() {
@@ -42,6 +43,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch 74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
change.setLastUpdatedOn(Instant.ofEpochMilli(1234567L));
change.setStatus(Change.Status.MERGED);
change.setCurrentPatchSet(
@@ -89,6 +91,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch-74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
Entities.Change proto = changeProtoConverter.toProto(change);
@@ -124,6 +127,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch-74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
// O as ID actually means that no current patch set is present.
change.setCurrentPatchSet(PatchSet.id(Change.id(14), 0), null, null);
@@ -161,6 +165,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch-74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
change.setCurrentPatchSet(PatchSet.id(Change.id(14), 23), "subject ABC", null);
Entities.Change proto = changeProtoConverter.toProto(change);
@@ -189,7 +194,7 @@
}
@Test
- public void allValuesConvertedToProtoAndBackAgain() {
+ public void allValuesConvertedToProtoAndBackAgainExceptServerId() {
Change change =
new Change(
Change.key("change 1"),
@@ -197,6 +202,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch-74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
change.setLastUpdatedOn(Instant.ofEpochMilli(1234567L));
change.setStatus(Change.Status.MERGED);
change.setCurrentPatchSet(
@@ -209,6 +215,11 @@
change.setRevertOf(Change.id(180));
Change convertedChange = changeProtoConverter.fromProto(changeProtoConverter.toProto(change));
+
+ // Change serverId is not one of the protobuf definitions, hence is not supposed to be converted
+ // from proto
+ assertThat(convertedChange.getServerId()).isNull();
+ change.setServerId(null);
assertEqualChange(convertedChange, change);
}
@@ -275,6 +286,7 @@
.hasFields(
ImmutableMap.<String, Type>builder()
.put("changeId", Change.Id.class)
+ .put("serverId", String.class)
.put("changeKey", Change.Key.class)
.put("createdOn", Instant.class)
.put("lastUpdatedOn", Instant.class)
@@ -298,6 +310,7 @@
// an AutoValue.
private static void assertEqualChange(Change change, Change expectedChange) {
assertThat(change.getChangeId()).isEqualTo(expectedChange.getChangeId());
+ assertThat(change.getServerId()).isEqualTo(expectedChange.getServerId());
assertThat(change.getKey()).isEqualTo(expectedChange.getKey());
assertThat(change.getCreatedOn()).isEqualTo(expectedChange.getCreatedOn());
assertThat(change.getLastUpdatedOn()).isEqualTo(expectedChange.getLastUpdatedOn());
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
index 8cb4974..53431d1 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
+++ b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
@@ -158,7 +158,7 @@
public void tolerateNullValuesForInsertion() {
Project.NameKey project = Project.nameKey("project");
ChangeData cd =
- ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId(), null, null, null);
+ ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId(), null, null);
assertThat(ChangeField.ADDED_LINES_SPEC.setIfPossible(cd, new FakeStoredValue(null))).isTrue();
}
@@ -166,7 +166,7 @@
public void tolerateNullValuesForDeletion() {
Project.NameKey project = Project.nameKey("project");
ChangeData cd =
- ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId(), null, null, null);
+ ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId(), null, null);
assertThat(ChangeField.DELETED_LINES_SPEC.setIfPossible(cd, new FakeStoredValue(null)))
.isTrue();
}
diff --git a/javatests/com/google/gerrit/server/logging/LoggingContextAwareExecutorServiceTest.java b/javatests/com/google/gerrit/server/logging/LoggingContextAwareExecutorServiceTest.java
index 1f0da16..c1b9f13 100644
--- a/javatests/com/google/gerrit/server/logging/LoggingContextAwareExecutorServiceTest.java
+++ b/javatests/com/google/gerrit/server/logging/LoggingContextAwareExecutorServiceTest.java
@@ -24,6 +24,7 @@
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
+import java.time.Instant;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutorService;
@@ -51,7 +52,7 @@
testPerformanceLogger =
new PerformanceLogger() {
@Override
- public void log(String operation, long durationMs, Metadata metadata) {
+ public void log(String operation, long durationMs, Instant endTime, Metadata metadata) {
// do nothing
}
};
diff --git a/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java b/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
index 512a1b1..c93061d 100644
--- a/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
+++ b/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
@@ -32,6 +32,7 @@
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
+import java.time.Instant;
import java.util.concurrent.CopyOnWriteArrayList;
import org.eclipse.jgit.lib.Config;
import org.junit.After;
@@ -364,7 +365,7 @@
private ImmutableList.Builder<PerformanceLogEntry> logEntries = ImmutableList.builder();
@Override
- public void log(String operation, long durationMs, Metadata metadata) {
+ public void log(String operation, long durationMs, Instant endTime, Metadata metadata) {
logEntries.add(PerformanceLogEntry.create(operation, metadata));
}
diff --git a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
index 0ce00eb..f954a57 100644
--- a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
+++ b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
@@ -34,8 +34,6 @@
@RunWith(MockitoJUnitRunner.class)
public class ChangeDataTest {
- private static final String GERRIT_SERVER_ID = UUID.randomUUID().toString();
-
@Mock private ChangeNotes changeNotesMock;
@Test
@@ -55,7 +53,7 @@
@Test
public void getChangeVirtualIdUsingAlgorithm() throws Exception {
Project.NameKey project = Project.nameKey("project");
- final int encodedChangeNum = 12345678;
+ final Change.Id encodedChangeNum = Change.id(12345678);
when(changeNotesMock.getServerId()).thenReturn(UUID.randomUUID().toString());
@@ -65,11 +63,10 @@
Change.id(1),
1,
ObjectId.zeroId(),
- GERRIT_SERVER_ID,
(s, c) -> encodedChangeNum,
changeNotesMock);
- assertThat(cd.getVirtualId().get()).isEqualTo(encodedChangeNum);
+ assertThat(cd.virtualId().get()).isEqualTo(encodedChangeNum.get());
}
private static PatchSet newPatchSet(Change.Id changeId, int num) {
diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
index c7f4f64..d00cc45 100644
--- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
@@ -26,6 +26,8 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.UseClockStep;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -90,11 +92,49 @@
@SuppressWarnings("unused")
var unused = newQuery("status:new").withLimit(5).get();
+ // Since the limit of the query (i.e. 5) is more than the total number of changes (i.e. 4),
+ // only 1 index search is expected.
assertThatSearchQueryWasNotPaginated(idx.getQueryCount());
}
@Test
@UseClockStep
+ public void queryRightNumberOfTimes() throws Exception {
+ Project.NameKey project = Project.nameKey("repo");
+ TestRepository<Repository> repo = createAndOpenProject(project);
+ Account.Id user2 =
+ accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+
+ // create 1 visible change
+ Change visibleChange1 = insert(project, newChangeWithStatus(repo, Change.Status.NEW));
+
+ // create 4 private changes
+ Change invisibleChange2 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ Change invisibleChange3 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ Change invisibleChange4 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ Change invisibleChange5 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ gApi.changes().id(invisibleChange2.getKey().get()).setPrivate(true, null);
+ gApi.changes().id(invisibleChange3.getKey().get()).setPrivate(true, null);
+ gApi.changes().id(invisibleChange4.getKey().get()).setPrivate(true, null);
+ gApi.changes().id(invisibleChange5.getKey().get()).setPrivate(true, null);
+
+ AbstractFakeIndex<?, ?, ?> idx =
+ (AbstractFakeIndex<?, ?, ?>) changeIndexCollection.getSearchIndex();
+ idx.resetQueryCount();
+ List<ChangeInfo> queryResult = newQuery("status:new").withLimit(2).get();
+ assertThat(queryResult).hasSize(1);
+ assertThat(queryResult.get(0).changeId).isEqualTo(visibleChange1.getKey().get());
+
+ // Since the limit of the query (i.e. 2), 2 index searches are expected in fact:
+ // 1: The first query will return invisibleChange5, invisibleChange4 and invisibleChange3,
+ // 2: Another query is needed to back-fill the limit requested by the user.
+ // even if one result in the second query is skipped because it is not visible,
+ // there are no more results to query.
+ assertThatSearchQueryWasPaginated(idx.getQueryCount(), 2);
+ }
+
+ @Test
+ @UseClockStep
public void noLimitQueryPaginates() throws Exception {
assumeFalse(PaginationType.NONE == getCurrentPaginationType());
@@ -139,39 +179,7 @@
@Test
@UseClockStep
- public void invisibleChangesNotPaginatedWithNonePaginationType() throws Exception {
- assumeTrue(PaginationType.NONE == getCurrentPaginationType());
- AbstractFakeIndex<?, ?, ?> idx = setupRepoWithFourChanges();
- final int LIMIT = 3;
-
- projectOperations
- .project(allProjectsName)
- .forUpdate()
- .removeAllAccessSections()
- .add(allow(Permission.READ).ref("refs/*").group(SystemGroupBackend.REGISTERED_USERS))
- .update();
-
- // Set queryLimit to 3
- projectOperations
- .project(allProjects)
- .forUpdate()
- .add(allowCapability(QUERY_LIMIT).group(ANONYMOUS_USERS).range(0, LIMIT))
- .update();
-
- @SuppressWarnings("unused")
- var unused = requestContext.setContext(anonymousUserProvider::get);
-
- List<ChangeInfo> result = newQuery("status:new").withLimit(LIMIT).get();
- assertThat(result.size()).isEqualTo(0);
- assertThatSearchQueryWasNotPaginated(idx.getQueryCount());
- assertThat(idx.getResultsSizes().get(0)).isEqualTo(LIMIT + 1);
- }
-
- @Test
- @UseClockStep
public void invisibleChangesPaginatedWithPagination() throws Exception {
- assumeFalse(PaginationType.NONE == getCurrentPaginationType());
-
AbstractFakeIndex<?, ?, ?> idx = setupRepoWithFourChanges();
final int LIMIT = 3;
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
index 987a87e..82c9065 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -109,4 +110,28 @@
Change[] expected = new Change[] {change6, change5, change4, change3, change2, change1};
assertQuery(newQuery("project:repo").withNoLimit(), expected);
}
+
+ @Test
+ public void skipChangesNotVisible() throws Exception {
+ // create 1 new change on a repo
+ Project.NameKey project = Project.nameKey("repo");
+ repo = createAndOpenProject(project);
+ Change visibleChange = insert(project, newChangeWithStatus(repo, Change.Status.NEW));
+ Change[] expected = new Change[] {visibleChange};
+
+ // pagination does not need to restart the datasource, the request is fulfilled
+ assertQuery(newQuery("status:new").withLimit(1), expected);
+
+ // create 2 new private changes
+ Account.Id user2 =
+ accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+
+ Change invisibleChange1 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ Change invisibleChange2 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ gApi.changes().id(invisibleChange1.getKey().get()).setPrivate(true, null);
+ gApi.changes().id(invisibleChange2.getKey().get()).setPrivate(true, null);
+
+ // pagination should back-fill when the results skipped because of the visibility
+ assertQuery(newQuery("status:new").withLimit(1), expected);
+ }
}
diff --git a/plugins/replication b/plugins/replication
index 012f042..aab6937 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit 012f04240eafe6dfa21fd94e012e97498881c621
+Subproject commit aab69373db6152ea98e7627f816fa3d777d5fe46
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 23c5746..50ec339 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -42,6 +42,7 @@
ChangeActionDialog,
ChangeInfo,
CherryPickInput,
+ CommentThread,
CommitId,
InheritedBooleanInfo,
isDetailedLabelInfo,
@@ -116,6 +117,8 @@
import {ParsedChangeInfo} from '../../../types/types';
import {configModelToken} from '../../../models/config/config-model';
import {readJSONResponsePayload} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
+import {commentsModelToken} from '../../../models/comments/comments-model';
+import {when} from 'lit/directives/when.js';
const ERR_BRANCH_EMPTY = 'The destination branch can’t be empty.';
const ERR_COMMIT_EMPTY = 'The commit message can’t be empty.';
@@ -366,6 +369,8 @@
@query('#confirmDeleteEditDialog') confirmDeleteEditDialog?: GrDialog;
+ @query('#confirmPublishEditDialog') confirmPublishEditDialog?: GrDialog;
+
@query('#moreActions') moreActions?: GrDropdown;
@query('#secondaryActions') secondaryActions?: HTMLElement;
@@ -480,6 +485,8 @@
@state() pluginsLoaded = false;
+ @state() threadsWithSuggestions?: CommentThread[];
+
private readonly restApiService = getAppContext().restApiService;
private readonly reporting = getAppContext().reportingService;
@@ -496,6 +503,8 @@
private readonly getNavigation = resolve(this, navigationToken);
+ private readonly getCommentsModel = resolve(this, commentsModelToken);
+
constructor() {
super();
subscribe(
@@ -568,6 +577,11 @@
() => this.getConfigModel().repoConfig$,
config => (this.privateByDefault = config?.private_by_default)
);
+ subscribe(
+ this,
+ () => this.getCommentsModel().threadsWithSuggestions$,
+ x => (this.threadsWithSuggestions = x)
+ );
}
override connectedCallback() {
@@ -620,6 +634,15 @@
.hidden {
display: none;
}
+ .info {
+ background-color: var(--info-background);
+ padding: var(--spacing-l) var(--spacing-xl);
+ margin-bottom: var(--spacing-l);
+ }
+ .info gr-icon {
+ color: var(--selected-foreground);
+ margin-right: var(--spacing-xl);
+ }
@media screen and (max-width: 50em) {
#mainContent {
flex-wrap: wrap;
@@ -786,6 +809,27 @@
Do you really want to delete the edit?
</div>
</gr-dialog>
+ <gr-dialog
+ id="confirmPublishEditDialog"
+ class="confirmDialog"
+ confirm-label="Publish"
+ confirm-on-enter=""
+ @cancel=${this.handleConfirmDialogCancel}
+ @confirm=${this.handlePublishEditConfirm}
+ >
+ <div class="header" slot="header">Publish Change Edit</div>
+ <div class="main" slot="main">
+ ${when(
+ this.numberOfThreadsWithSuggestions() > 0,
+ () => html`<p class="info">
+ <gr-icon id="icon" icon="info" small></gr-icon>
+ Heads Up! ${this.numberOfThreadsWithSuggestions()} comments have
+ suggestions you can apply before publishing
+ </p>`
+ )}
+ Do you really want to publish the edit?
+ </div>
+ </gr-dialog>
</dialog>
`;
}
@@ -1691,6 +1735,23 @@
);
}
+ private handlePublishEditConfirm() {
+ this.hideAllDialogs();
+
+ if (!this.actions.publishEdit) return;
+
+ // We need to make sure that all cached version of a change
+ // edit are deleted.
+ this.getStorage().eraseEditableContentItemsForChangeEdit(this.changeNum);
+
+ this.fireAction(
+ '/edit:publish',
+ assertUIActionInfo(this.actions.publishEdit),
+ false,
+ {notify: NotifyType.NONE}
+ );
+ }
+
// private but used in test
handleSubmitConfirm() {
if (!this.canSubmitChange()) {
@@ -2044,18 +2105,8 @@
}
private handlePublishEditTap() {
- if (!this.actions.publishEdit) return;
-
- // We need to make sure that all cached version of a change
- // edit are deleted.
- this.getStorage().eraseEditableContentItemsForChangeEdit(this.changeNum);
-
- this.fireAction(
- '/edit:publish',
- assertUIActionInfo(this.actions.publishEdit),
- false,
- {notify: NotifyType.NONE}
- );
+ assertIsDefined(this.confirmPublishEditDialog, 'confirmPublishEditDialog');
+ this.showActionDialog(this.confirmPublishEditDialog);
}
private handleRebaseEditTap() {
@@ -2212,6 +2263,11 @@
private handleStopEditTap() {
fireNoBubbleNoCompose(this, 'stop-edit-tap', {});
}
+
+ private numberOfThreadsWithSuggestions() {
+ if (!this.threadsWithSuggestions) return 0;
+ return this.threadsWithSuggestions.length;
+ }
}
declare global {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 8275c0b..a926f26 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -278,6 +278,18 @@
Do you really want to delete the edit?
</div>
</gr-dialog>
+ <gr-dialog
+ class="confirmDialog"
+ confirm-label="Publish"
+ confirm-on-enter=""
+ id="confirmPublishEditDialog"
+ role="dialog"
+ >
+ <div class="header" slot="header">Publish Change Edit</div>
+ <div class="main" slot="main">
+ Do you really want to publish the edit?
+ </div>
+ </gr-dialog>
</dialog>
`
);
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 0bf49db..1a18e9c 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -98,7 +98,10 @@
} from '../../../utils/attention-set-util';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
import {resolve} from '../../../models/dependency';
-import {changeModelToken} from '../../../models/change/change-model';
+import {
+ changeModelToken,
+ updateRevisionsWithCommitShas,
+} from '../../../models/change/change-model';
import {LabelNameToValuesMap, PatchSetNumber} from '../../../api/rest-api';
import {css, html, PropertyValues, LitElement, nothing} from 'lit';
import {sharedStyles} from '../../../styles/shared-styles';
@@ -1459,8 +1462,10 @@
return this.saveReview(reviewInput, errFn)
.then(result => {
this.getChangeModel().updateStateChange(
- GrReviewerUpdatesParser.parse(
- result?.change_info as ChangeViewChangeInfo
+ updateRevisionsWithCommitShas(
+ GrReviewerUpdatesParser.parse(
+ result?.change_info as ChangeViewChangeInfo
+ )
)
);
diff --git a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
index 3b679fd..0a31677 100644
--- a/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
+++ b/polygerrit-ui/app/elements/diff/gr-apply-fix-dialog/gr-apply-fix-dialog.ts
@@ -315,6 +315,8 @@
fixSuggestion.replacements
);
} else {
+ // TODO(b/227463363) Remove once Robot Comments are deprecated.
+ // We don't use this for user suggestions or comments.fix_suggestions.
res = await this.restApiService.getRobotCommentFixPreview(
this.changeNum,
this.patchNum,
diff --git a/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts b/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
index 4d5889f..9e78e2a 100644
--- a/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
+++ b/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
@@ -20,6 +20,7 @@
import {OpenFixPreviewEventDetail} from '../../../types/events';
import {pluginLoaderToken} from '../gr-js-api-interface/gr-plugin-loader';
import {SuggestionsProvider} from '../../../api/suggestions';
+import {PROVIDED_FIX_ID} from '../../../utils/comment-util';
/**
* gr-fix-suggestions is UI for comment.fix_suggestions.
@@ -149,7 +150,11 @@
fixSuggestions: this.comment.fix_suggestions.map(s => {
return {
...s,
- description: 'Suggested Edit from comment',
+ fix_id: PROVIDED_FIX_ID,
+ description:
+ this.suggestionsProvider?.getFixSuggestionTitle?.(
+ this.comment?.fix_suggestions
+ ) || 'Suggested edit',
};
}),
patchNum: this.comment.patch_set,
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index 41b47ef..f9a8401 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -25,9 +25,12 @@
convertToCommentInput,
createNew,
createNewPatchsetLevel,
+ getFirstComment,
+ hasSuggestion,
id,
isDraftThread,
isNewThread,
+ isUnresolved,
reportingDetails,
} from '../../utils/comment-util';
import {deepEqual} from '../../utils/deep-util';
@@ -429,6 +432,17 @@
threads.filter(t => !isNewThread(t) && isDraftThread(t))
);
+ public readonly threadsWithSuggestions$ = select(
+ combineLatest([this.threads$, this.changeModel.latestPatchNum$]),
+ ([threads, latestPs]) =>
+ threads.filter(
+ t =>
+ isUnresolved(t) &&
+ hasSuggestion(t) &&
+ getFirstComment(t)?.patch_set === latestPs
+ )
+ );
+
public readonly commentedPaths$ = select(
combineLatest([
this.changeComments$,
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 364f372..e82b328 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -262,6 +262,15 @@
return isRobot(getFirstComment(thread));
}
+export function hasSuggestion(thread: CommentThread): boolean {
+ const firstComment = getFirstComment(thread);
+ if (!firstComment) return false;
+ return (
+ hasUserSuggestion(firstComment) ||
+ firstComment.fix_suggestions?.[0] !== undefined
+ );
+}
+
export function hasHumanReply(thread: CommentThread): boolean {
return countComments(thread) > 1 && !isRobot(getLastComment(thread));
}
