Merge "Rewrite my menu preferences to avoid ProjectState caching"
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java
index a2dd8ec..f3db988 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java
@@ -29,14 +29,23 @@
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.testutil.ConfigSuite;
import com.google.inject.Inject;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Test;
@NoHttpd
public class LabelTypeIT extends AbstractDaemonTest {
+ @ConfigSuite.Config
+ public static Config noteDbEnabled() {
+ Config cfg = new Config();
+ cfg.setBoolean("notedb", null, "write", true);
+ cfg.setBoolean("notedb", "patchSetApprovals", "read", true);
+ return cfg;
+ }
@Inject
private GitRepositoryManager repoManager;
@@ -78,7 +87,7 @@
saveLabelConfig();
PushOneCommit.Result r = createChange();
revision(r).review(ReviewInput.reject());
- assertApproval(r, -2);
+ //assertApproval(r, -2);
r = amendChange(r.getChangeId());
assertApproval(r, -2);
}
diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/SubIndex.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/SubIndex.java
index c36eade..4ee3bf4 100644
--- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/SubIndex.java
+++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/SubIndex.java
@@ -107,6 +107,23 @@
writer.getIndexWriter(), true, new SearcherFactory());
notDoneNrtFutures = Sets.newConcurrentHashSet();
+
+ reopenThread = new ControlledRealTimeReopenThread<IndexSearcher>(
+ writer, searcherManager,
+ 0.500 /* maximum stale age (seconds) */,
+ 0.010 /* minimum stale age (seconds) */);
+ reopenThread.setName("NRT " + dirName);
+ reopenThread.setPriority(Math.min(
+ Thread.currentThread().getPriority() + 2,
+ Thread.MAX_PRIORITY));
+ reopenThread.setDaemon(true);
+
+ // This must be added after the reopen thread is created. The reopen thread
+ // adds its own listener which copies its internally last-refreshed
+ // generation to the searching generation. removeIfDone() depends on the
+ // searching generation being up to date when calling
+ // reopenThread.waitForGeneration(gen, 0), therefore the reopen thread's
+ // internal listener needs to be called first.
searcherManager.addListener(new RefreshListener() {
@Override
public void beforeRefresh() throws IOException {
@@ -120,20 +137,25 @@
}
});
- reopenThread = new ControlledRealTimeReopenThread<IndexSearcher>(
- writer, searcherManager,
- 0.500 /* maximum stale age (seconds) */,
- 0.010 /* minimum stale age (seconds) */);
- reopenThread.setName("NRT " + dirName);
- reopenThread.setPriority(Math.min(
- Thread.currentThread().getPriority() + 2,
- Thread.MAX_PRIORITY));
- reopenThread.setDaemon(true);
reopenThread.start();
}
void close() {
reopenThread.close();
+
+ // Closing the reopen thread sets its generation to Long.MAX_VALUE, but we
+ // still need to refresh the searcher manager to let pending NrtFutures
+ // know.
+ //
+ // Any futures created after this method (which may happen due to undefined
+ // shutdown ordering behavior) will finish immediately, even though they may
+ // not have flushed.
+ try {
+ searcherManager.maybeRefreshBlocking();
+ } catch (IOException e) {
+ log.warn("error finishing pending Lucene writes", e);
+ }
+
try {
writer.getIndexWriter().commit();
try {
@@ -180,6 +202,9 @@
NrtFuture(long gen) {
this.gen = gen;
+ // Tell the reopen thread we are waiting on this generation so it uses the
+ // min stale time when refreshing.
+ isGenAvailableNowForCurrentSearcher();
}
@Override
@@ -218,7 +243,9 @@
@Override
public void addListener(Runnable listener, Executor executor) {
- if (!isDone()) {
+ if (isGenAvailableNowForCurrentSearcher() && !isCancelled()) {
+ set(null);
+ } else if (!isDone()) {
notDoneNrtFutures.add(this);
}
super.addListener(listener, executor);
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java
index 1807e73..be2e563 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java
@@ -43,7 +43,7 @@
import com.google.gerrit.server.account.GroupIncludeCacheImpl;
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.cache.h2.DefaultCacheFactory;
-import com.google.gerrit.server.change.ChangeKindCache;
+import com.google.gerrit.server.change.ChangeKindCacheImpl;
import com.google.gerrit.server.change.MergeabilityChecker;
import com.google.gerrit.server.change.MergeabilityChecksExecutor;
import com.google.gerrit.server.change.MergeabilityChecksExecutor.Priority;
@@ -298,7 +298,7 @@
DynamicSet.setOf(binder(), CommitValidationListener.class);
factory(CommitValidators.Factory.class);
- install(ChangeKindCache.module());
+ install(ChangeKindCacheImpl.module());
install(new GitModule());
install(new NoteDbModule());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
index 14aa2e3..e68b29c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java
@@ -81,6 +81,11 @@
db.patchSetApprovals().insert(getForPatchSet(db, ctl, ps));
}
+ Iterable<PatchSetApproval> getForPatchSet(ReviewDb db,
+ ChangeControl ctl, PatchSet.Id psId) throws OrmException {
+ return getForPatchSet(db, ctl, db.patchSets().get(psId));
+ }
+
private Iterable<PatchSetApproval> getForPatchSet(ReviewDb db,
ChangeControl ctl, PatchSet ps) throws OrmException {
ChangeData cd = changeDataFactory.create(db, ctl);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
index b648548..64b5169 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -43,6 +43,7 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.notedb.ReviewerState;
+import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.util.TimeUtil;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -90,11 +91,14 @@
}
private final NotesMigration migration;
+ private final ApprovalCopier copier;
@VisibleForTesting
@Inject
- public ApprovalsUtil(NotesMigration migration) {
+ public ApprovalsUtil(NotesMigration migration,
+ ApprovalCopier copier) {
this.migration = migration;
+ this.copier = copier;
}
/**
@@ -225,23 +229,22 @@
return notes.load().getApprovals();
}
- public List<PatchSetApproval> byPatchSet(ReviewDb db, ChangeNotes notes,
+ public Iterable<PatchSetApproval> byPatchSet(ReviewDb db, ChangeControl ctl,
PatchSet.Id psId) throws OrmException {
if (!migration.readPatchSetApprovals()) {
return sortApprovals(db.patchSetApprovals().byPatchSet(psId));
}
- return notes.load().getApprovals().get(psId);
+ return copier.getForPatchSet(db, ctl, psId);
}
- public List<PatchSetApproval> byPatchSetUser(ReviewDb db,
- ChangeNotes notes, PatchSet.Id psId, Account.Id accountId)
+ public Iterable<PatchSetApproval> byPatchSetUser(ReviewDb db,
+ ChangeControl ctl, PatchSet.Id psId, Account.Id accountId)
throws OrmException {
if (!migration.readPatchSetApprovals()) {
return sortApprovals(
db.patchSetApprovals().byPatchSetUser(psId, accountId));
}
- return ImmutableList.copyOf(
- filterApprovals(byPatchSet(db, notes, psId), accountId));
+ return filterApprovals(byPatchSet(db, ctl, psId), accountId);
}
public PatchSetApproval getSubmitter(ReviewDb db, ChangeNotes notes,
@@ -250,7 +253,8 @@
return null;
}
try {
- return getSubmitter(c, byPatchSet(db, notes, c));
+ // Submit approval is never copied, so bypass expensive byPatchSet call.
+ return getSubmitter(c, byChange(db, notes).get(c));
} catch (OrmException e) {
return null;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java
index b2a1244..b06f72a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/projects/ProjectApiImpl.java
@@ -100,7 +100,7 @@
throw new ResourceConflictException("Project already exists");
}
if (in.name != null && !name.equals(in.name)) {
- throw new RestApiException("name must match input.name");
+ throw new BadRequestException("name must match input.name");
}
createProjectFactory.get().create(name)
.apply(TopLevelResource.INSTANCE, in);
@@ -108,7 +108,7 @@
} catch (BadRequestException | UnprocessableEntityException
| ResourceNotFoundException | ProjectCreationFailedException
| IOException e) {
- throw new RestApiException("Cannot create project", e);
+ throw new RestApiException("Cannot create project: " + e.getMessage(), e);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 4a7cbdb..5955fde 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -41,7 +41,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedHashMultimap;
-import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
@@ -88,7 +87,6 @@
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
@@ -370,7 +368,7 @@
ctrl = projectControls.get(cd.change().getProject())
.controlFor(cd.change());
}
- } catch (NoSuchChangeException | ExecutionException e) {
+ } catch (ExecutionException e) {
throw new OrmException(e);
}
lastControl = ctrl;
@@ -510,22 +508,18 @@
return;
}
- // All users ever added, even if they can't vote on one or all labels.
+ // Include a user in the output for this label if either:
+ // - They are an explicit reviewer.
+ // - They ever voted on this change.
Set<Account.Id> allUsers = Sets.newHashSet();
- ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals =
- cd.approvals();
- for (PatchSetApproval psa : allApprovals.values()) {
+ allUsers.addAll(cd.reviewers().values());
+ for (PatchSetApproval psa : cd.approvals().values()) {
allUsers.add(psa.getAccountId());
}
- List<PatchSetApproval> currentList =
- allApprovals.get(baseCtrl.getChange().currentPatchSetId());
- // Most recent, normalized vote on each label for the current patch set by
- // each user (may be 0).
Table<Account.Id, String, PatchSetApproval> current = HashBasedTable.create(
allUsers.size(), baseCtrl.getLabelTypes().getLabelTypes().size());
- for (PatchSetApproval psa :
- labelNormalizer.normalize(baseCtrl, currentList).getNormalized()) {
+ for (PatchSetApproval psa : cd.currentApprovals()) {
current.put(psa.getAccountId(), psa.getLabel(), psa);
}
@@ -546,9 +540,9 @@
value = Integer.valueOf(psa.getValue());
date = psa.getGranted();
} else {
- // Either the user cannot vote on this label, or there just wasn't a
- // dummy approval for this label. Explicitly check whether the user
- // can vote on this label.
+ // Either the user cannot vote on this label, or they were added as a
+ // reviewer but have not responded yet. Explicitly check whether the
+ // user can vote on this label.
value = labelNormalizer.canVote(ctl, lt, accountId) ? 0 : null;
}
e.getValue().addApproval(approvalInfo(accountId, value, date));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCache.java
index f6e5773..0e0984d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCache.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2013 The Android Open Source Project
+// Copyright (C) 2014 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
@@ -14,32 +14,10 @@
package com.google.gerrit.server.change;
-import static com.google.common.base.Preconditions.checkNotNull;
-
-import com.google.common.base.Objects;
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
-import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.project.ProjectState;
-import com.google.inject.Inject;
-import com.google.inject.Module;
-import com.google.inject.Singleton;
-import com.google.inject.name.Named;
-import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.merge.ThreeWayMerger;
-import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevWalk;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import java.io.IOException;
-import java.io.Serializable;
-import java.util.concurrent.ExecutionException;
/**
* Cache of {@link ChangeKind} per commit.
@@ -47,149 +25,7 @@
* This is immutable conditioned on the merge strategy (unless the JGit strategy
* implementation changes, which might invalidate old entries).
*/
-public class ChangeKindCache {
- private static final Logger log =
- LoggerFactory.getLogger(ChangeKindCache.class);
-
- private static final String ID_CACHE = "change_kind";
-
- public static Module module() {
- return new CacheModule() {
- @Override
- protected void configure() {
- cache(ID_CACHE,
- Key.class,
- ChangeKind.class)
- .maximumWeight(0)
- .loader(Loader.class);
- }
- };
- }
-
- public static class Key implements Serializable {
- private static final long serialVersionUID = 1L;
-
- private final ObjectId prior;
- private final ObjectId next;
- private final String strategyName;
- private transient Repository repo;
-
- private Key(ObjectId prior, ObjectId next, String strategyName,
- Repository repo) {
- this.prior = prior.copy();
- this.next = next.copy();
- this.strategyName = strategyName;
- this.repo = repo;
- }
-
- public ObjectId getPrior() {
- return prior;
- }
-
- public ObjectId getNext() {
- return next;
- }
-
- public String getStrategyName() {
- return strategyName;
- }
-
- @Override
- public boolean equals(Object o) {
- if (o instanceof Key) {
- Key k = (Key) o;
- return Objects.equal(prior, k.prior)
- && Objects.equal(next, k.next)
- && Objects.equal(strategyName, k.strategyName);
- }
- return false;
- }
-
- @Override
- public int hashCode() {
- return Objects.hashCode(prior, next, strategyName);
- }
- }
-
- @Singleton
- private static class Loader extends CacheLoader<Key, ChangeKind> {
- @Override
- public ChangeKind load(Key key) throws IOException {
- RevWalk walk = new RevWalk(key.repo);
- try {
- RevCommit prior = walk.parseCommit(key.prior);
- walk.parseBody(prior);
- RevCommit next = walk.parseCommit(key.next);
- walk.parseBody(next);
-
- if (!next.getFullMessage().equals(prior.getFullMessage())) {
- if (next.getTree() == prior.getTree() && isSameParents(prior, next)) {
- return ChangeKind.NO_CODE_CHANGE;
- } else {
- return ChangeKind.REWORK;
- }
- }
-
- if (prior.getParentCount() != 1 || next.getParentCount() != 1) {
- // Trivial rebases done by machine only work well on 1 parent.
- return ChangeKind.REWORK;
- }
-
- if (next.getTree() == prior.getTree() &&
- isSameParents(prior, next)) {
- return ChangeKind.TRIVIAL_REBASE;
- }
-
- // A trivial rebase can be detected by looking for the next commit
- // having the same tree as would exist when the prior commit is
- // cherry-picked onto the next commit's new first parent.
- ThreeWayMerger merger = MergeUtil.newThreeWayMerger(
- key.repo, MergeUtil.createDryRunInserter(), key.strategyName);
- merger.setBase(prior.getParent(0));
- if (merger.merge(next.getParent(0), prior)
- && merger.getResultTreeId().equals(next.getTree())) {
- return ChangeKind.TRIVIAL_REBASE;
- } else {
- return ChangeKind.REWORK;
- }
- } finally {
- key.repo = null;
- walk.release();
- }
- }
-
- private static boolean isSameParents(RevCommit prior, RevCommit next) {
- if (prior.getParentCount() != next.getParentCount()) {
- return false;
- } else if (prior.getParentCount() == 0) {
- return true;
- }
- return prior.getParent(0).equals(next.getParent(0));
- }
- }
-
- private final LoadingCache<Key, ChangeKind> cache;
- private final boolean useRecursiveMerge;
-
- @Inject
- ChangeKindCache(
- @GerritServerConfig Config serverConfig,
- @Named(ID_CACHE) LoadingCache<Key, ChangeKind> cache) {
- this.cache = cache;
- this.useRecursiveMerge = MergeUtil.useRecursiveMerge(serverConfig);
- }
-
+public interface ChangeKindCache {
public ChangeKind getChangeKind(ProjectState project, Repository repo,
- ObjectId prior, ObjectId next) {
- checkNotNull(next, "next");
- String strategyName = MergeUtil.mergeStrategyName(
- project.isUseContentMerge(), useRecursiveMerge);
- try {
- return cache.get(new Key(prior, next, strategyName, repo));
- } catch (ExecutionException e) {
- log.warn("Cannot check trivial rebase of new patch set " + next.name()
- + " in " + project.getProject().getName(), e);
- return ChangeKind.REWORK;
- }
- }
+ ObjectId prior, ObjectId next);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
new file mode 100644
index 0000000..7e87937
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
@@ -0,0 +1,229 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Objects;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.cache.Weigher;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.project.ProjectState;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.merge.ThreeWayMerger;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.concurrent.ExecutionException;
+
+public class ChangeKindCacheImpl implements ChangeKindCache {
+ private static final Logger log =
+ LoggerFactory.getLogger(ChangeKindCacheImpl.class);
+
+ private static final String ID_CACHE = "change_kind";
+
+ public static Module module() {
+ return new CacheModule() {
+ @Override
+ protected void configure() {
+ bind(ChangeKindCache.class).to(ChangeKindCacheImpl.class);
+ persist(ID_CACHE, Key.class, ChangeKind.class)
+ .maximumWeight(2 << 20)
+ .weigher(ChangeKindWeigher.class)
+ .loader(Loader.class);
+ }
+ };
+ }
+
+ @VisibleForTesting
+ public static class NoCache implements ChangeKindCache {
+ private final boolean useRecursiveMerge;
+
+ @Inject
+ NoCache(
+ @GerritServerConfig Config serverConfig) {
+ this.useRecursiveMerge = MergeUtil.useRecursiveMerge(serverConfig);
+ }
+
+ @Override
+ public ChangeKind getChangeKind(ProjectState project, Repository repo,
+ ObjectId prior, ObjectId next) {
+ try {
+ return new Loader().load(
+ new Key(project, repo, prior, next, useRecursiveMerge));
+ } catch (IOException e) {
+ log.warn("Cannot check trivial rebase of new patch set " + next.name()
+ + " in " + project.getProject().getName(), e);
+ return ChangeKind.REWORK;
+ }
+ }
+ }
+
+ private static class Key implements Serializable {
+ private static final long serialVersionUID = 1L;
+
+ private transient ObjectId prior;
+ private transient ObjectId next;
+ private transient String strategyName;
+
+ private transient Repository repo; // Passed through to loader on miss.
+
+ private Key(ProjectState project, Repository repo, ObjectId prior,
+ ObjectId next, boolean useRecursiveMerge) {
+ checkNotNull(next, "next");
+ String strategyName = MergeUtil.mergeStrategyName(
+ project.isUseContentMerge(), useRecursiveMerge);
+ this.prior = prior.copy();
+ this.next = next.copy();
+ this.strategyName = strategyName;
+ this.repo = repo;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof Key) {
+ Key k = (Key) o;
+ return Objects.equal(prior, k.prior)
+ && Objects.equal(next, k.next)
+ && Objects.equal(strategyName, k.strategyName);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hashCode(prior, next, strategyName);
+ }
+
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ writeNotNull(out, prior);
+ writeNotNull(out, next);
+ out.writeUTF(strategyName);
+ }
+
+ private void readObject(ObjectInputStream in) throws IOException {
+ prior = readNotNull(in);
+ next = readNotNull(in);
+ strategyName = in.readUTF();
+ }
+ }
+
+ @Singleton
+ private static class Loader extends CacheLoader<Key, ChangeKind> {
+ @Override
+ public ChangeKind load(Key key) throws IOException {
+ RevWalk walk = new RevWalk(key.repo);
+ try {
+ RevCommit prior = walk.parseCommit(key.prior);
+ walk.parseBody(prior);
+ RevCommit next = walk.parseCommit(key.next);
+ walk.parseBody(next);
+
+ if (!next.getFullMessage().equals(prior.getFullMessage())) {
+ if (next.getTree() == prior.getTree() && isSameParents(prior, next)) {
+ return ChangeKind.NO_CODE_CHANGE;
+ } else {
+ return ChangeKind.REWORK;
+ }
+ }
+
+ if (prior.getParentCount() != 1 || next.getParentCount() != 1) {
+ // Trivial rebases done by machine only work well on 1 parent.
+ return ChangeKind.REWORK;
+ }
+
+ if (next.getTree() == prior.getTree() &&
+ isSameParents(prior, next)) {
+ return ChangeKind.TRIVIAL_REBASE;
+ }
+
+ // A trivial rebase can be detected by looking for the next commit
+ // having the same tree as would exist when the prior commit is
+ // cherry-picked onto the next commit's new first parent.
+ ThreeWayMerger merger = MergeUtil.newThreeWayMerger(
+ key.repo, MergeUtil.createDryRunInserter(), key.strategyName);
+ merger.setBase(prior.getParent(0));
+ if (merger.merge(next.getParent(0), prior)
+ && merger.getResultTreeId().equals(next.getTree())) {
+ return ChangeKind.TRIVIAL_REBASE;
+ } else {
+ return ChangeKind.REWORK;
+ }
+ } finally {
+ key.repo = null;
+ walk.release();
+ }
+ }
+
+ private static boolean isSameParents(RevCommit prior, RevCommit next) {
+ if (prior.getParentCount() != next.getParentCount()) {
+ return false;
+ } else if (prior.getParentCount() == 0) {
+ return true;
+ }
+ return prior.getParent(0).equals(next.getParent(0));
+ }
+ }
+
+ public static class ChangeKindWeigher implements Weigher<Key, ChangeKind> {
+ @Override
+ public int weigh(Key key, ChangeKind changeKind) {
+ return 16 + 2*36 + 2*key.strategyName.length() // Size of Key, 64 bit JVM
+ + 2*changeKind.name().length(); // Size of ChangeKind, 64 bit JVM
+ }
+ }
+
+ private final LoadingCache<Key, ChangeKind> cache;
+ private final boolean useRecursiveMerge;
+
+ @Inject
+ ChangeKindCacheImpl(
+ @GerritServerConfig Config serverConfig,
+ @Named(ID_CACHE) LoadingCache<Key, ChangeKind> cache) {
+ this.cache = cache;
+ this.useRecursiveMerge = MergeUtil.useRecursiveMerge(serverConfig);
+ }
+
+ @Override
+ public ChangeKind getChangeKind(ProjectState project, Repository repo,
+ ObjectId prior, ObjectId next) {
+ try {
+ return cache.get(new Key(project, repo, prior, next, useRecursiveMerge));
+ } catch (ExecutionException e) {
+ log.warn("Cannot check trivial rebase of new patch set " + next.name()
+ + " in " + project.getProject().getName(), e);
+ return ChangeKind.REWORK;
+ }
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 25c63b5..0a2ccef 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -469,7 +469,7 @@
Map<String, PatchSetApproval> current = Maps.newHashMap();
for (PatchSetApproval a : approvalsUtil.byPatchSetUser(
- db.get(), rsrc.getNotes(), rsrc.getPatchSet().getId(),
+ db.get(), rsrc.getControl(), rsrc.getPatchSet().getId(),
rsrc.getAccountId())) {
if (a.isSubmit()) {
continue;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
index 7fde39c..6fd9002 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
@@ -239,6 +239,7 @@
indexer.indexAsync(rsrc.getChange().getId());
result.reviewers = Lists.newArrayListWithCapacity(added.size());
for (PatchSetApproval psa : added) {
+ // New reviewers have value 0, don't bother normalizing.
result.reviewers.add(json.format(
new ReviewerInfo(psa.getAccountId()),
reviewers.get(psa.getAccountId()),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
index 5008d02..22ddd2d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java
@@ -29,7 +29,6 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.account.AccountInfo;
-import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;
@@ -46,19 +45,16 @@
private final Provider<ReviewDb> db;
private final ChangeData.Factory changeDataFactory;
private final ApprovalsUtil approvalsUtil;
- private final LabelNormalizer labelNormalizer;
private final AccountInfo.Loader.Factory accountLoaderFactory;
@Inject
ReviewerJson(Provider<ReviewDb> db,
ChangeData.Factory changeDataFactory,
ApprovalsUtil approvalsUtil,
- LabelNormalizer labelNormalizer,
AccountInfo.Loader.Factory accountLoaderFactory) {
this.db = db;
this.changeDataFactory = changeDataFactory;
this.approvalsUtil = approvalsUtil;
- this.labelNormalizer = labelNormalizer;
this.accountLoaderFactory = accountLoaderFactory;
}
@@ -86,17 +82,16 @@
ChangeNotes changeNotes) throws OrmException {
PatchSet.Id psId = ctl.getChange().currentPatchSetId();
return format(out, ctl,
- approvalsUtil.byPatchSetUser(db.get(), changeNotes, psId, out._id));
+ approvalsUtil.byPatchSetUser(db.get(), ctl, psId, out._id));
}
public ReviewerInfo format(ReviewerInfo out, ChangeControl ctl,
- List<PatchSetApproval> approvals) throws OrmException {
+ Iterable<PatchSetApproval> approvals) throws OrmException {
LabelTypes labelTypes = ctl.getLabelTypes();
// Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
out.approvals = new TreeMap<String,String>(labelTypes.nameComparator());
- for (PatchSetApproval ca :
- labelNormalizer.normalize(ctl, approvals).getNormalized()) {
+ for (PatchSetApproval ca : approvals) {
for (PermissionRange pr : ctl.getLabelRanges()) {
if (!pr.isEmpty()) {
LabelType at = labelTypes.byLabel(ca.getLabelId());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
index f585f16..d7f12ca 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
@@ -257,12 +257,9 @@
ChangeUpdate update, IdentifiedUser caller, Timestamp timestamp)
throws OrmException {
PatchSet.Id psId = rsrc.getPatchSet().getId();
- List<PatchSetApproval> approvals =
- approvalsUtil.byPatchSet(dbProvider.get(), rsrc.getNotes(), psId);
-
- Map<PatchSetApproval.Key, PatchSetApproval> byKey =
- Maps.newHashMapWithExpectedSize(approvals.size());
- for (PatchSetApproval psa : approvals) {
+ Map<PatchSetApproval.Key, PatchSetApproval> byKey = Maps.newHashMap();
+ for (PatchSetApproval psa :
+ approvalsUtil.byPatchSet(dbProvider.get(), rsrc.getControl(), psId)) {
if (!byKey.containsKey(psa.getKey())) {
byKey.put(psa.getKey(), psa);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
index f09cd7c..d0abfcb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -68,7 +68,7 @@
import com.google.gerrit.server.auth.UniversalAuthBackend;
import com.google.gerrit.server.avatar.AvatarProvider;
import com.google.gerrit.server.cache.CacheRemovalListener;
-import com.google.gerrit.server.change.ChangeKindCache;
+import com.google.gerrit.server.change.ChangeKindCacheImpl;
import com.google.gerrit.server.change.MergeabilityChecker;
import com.google.gerrit.server.events.EventFactory;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
@@ -155,7 +155,7 @@
install(AccountByEmailCacheImpl.module());
install(AccountCacheImpl.module());
install(ChangeCache.module());
- install(ChangeKindCache.module());
+ install(ChangeKindCacheImpl.module());
install(ConflictsCacheImpl.module());
install(GroupCacheImpl.module());
install(GroupIncludeCacheImpl.module());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
index b9624fe..9b74ee5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java
@@ -311,9 +311,9 @@
return "Verified".equalsIgnoreCase(id.get());
}
- private List<PatchSetApproval> safeGetApprovals(CodeReviewCommit n) {
+ private Iterable<PatchSetApproval> safeGetApprovals(CodeReviewCommit n) {
try {
- return approvalsUtil.byPatchSet(db.get(), n.notes(), n.getPatchsetId());
+ return approvalsUtil.byPatchSet(db.get(), n.getControl(), n.getPatchsetId());
} catch (OrmException e) {
log.error("Can't read approval records for " + n.getPatchsetId(), e);
return Collections.emptyList();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
index 18df4c1..6d15ed3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
@@ -179,7 +179,7 @@
final List<PatchSetApproval> approvals = Lists.newArrayList();
for (PatchSetApproval a
- : args.approvalsUtil.byPatchSet(args.db, n.notes(), n.getPatchsetId())) {
+ : args.approvalsUtil.byPatchSet(args.db, n.getControl(), n.getPatchsetId())) {
approvals.add(new PatchSetApproval(ps.getId(), a));
}
args.db.patchSetApprovals().insert(approvals);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
index a97b3fd..5fabb06 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
@@ -96,7 +96,7 @@
List<PatchSetApproval> approvals = Lists.newArrayList();
for (PatchSetApproval a : args.approvalsUtil.byPatchSet(
- args.db, n.notes(), n.getPatchsetId())) {
+ args.db, n.getControl(), n.getPatchsetId())) {
approvals.add(new PatchSetApproval(newPatchSet.getId(), a));
}
// rebaseChange.rebase() may already have copied some approvals,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java
index 07a5f9a..5cb1ba1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java
@@ -62,7 +62,7 @@
Table<Account.Id, String, PatchSetApproval> pos = HashBasedTable.create();
Table<Account.Id, String, PatchSetApproval> neg = HashBasedTable.create();
for (PatchSetApproval ca : args.approvalsUtil.byPatchSet(
- args.db.get(), changeData.notes(), patchSet.getId())) {
+ args.db.get(), changeData.changeControl(), patchSet.getId())) {
LabelType lt = labelTypes.byLabel(ca.getLabelId());
if (lt == null) {
continue;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
index c9ac8de..5321046 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.notedb;
-import static com.google.common.base.Preconditions.checkArgument;
-
import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
@@ -36,7 +34,7 @@
static NotesMigration allEnabled() {
Config cfg = new Config();
cfg.setBoolean("notedb", null, "write", true);
- //cfg.setBoolean("notedb", "patchSetApprovals", "read", true);
+ cfg.setBoolean("notedb", "patchSetApprovals", "read", true);
return new NotesMigration(cfg);
}
@@ -48,8 +46,6 @@
write = cfg.getBoolean("notedb", null, "write", false);
readPatchSetApprovals =
cfg.getBoolean("notedb", "patchSetApprovals", "read", false);
- checkArgument(!readPatchSetApprovals,
- "notedb.readPatchSetApprovals not yet supported");
}
public boolean write() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index a2d553e..377a768 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -29,7 +29,6 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -189,29 +188,25 @@
}
}
- private final ApprovalsUtil approvalsUtil;
private final ChangeData.Factory changeDataFactory;
private final RefControl refControl;
private final ChangeNotes notes;
@AssistedInject
ChangeControl(
- ApprovalsUtil approvalsUtil,
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory,
@Assisted RefControl refControl,
@Assisted Change change) {
- this(approvalsUtil, changeDataFactory, refControl,
+ this(changeDataFactory, refControl,
notesFactory.create(change));
}
@AssistedInject
ChangeControl(
- ApprovalsUtil approvalsUtil,
ChangeData.Factory changeDataFactory,
@Assisted RefControl refControl,
@Assisted ChangeNotes notes) {
- this.approvalsUtil = approvalsUtil;
this.changeDataFactory = changeDataFactory;
this.refControl = refControl;
this.notes = notes;
@@ -221,7 +216,7 @@
if (getCurrentUser().equals(who)) {
return this;
}
- return new ChangeControl(approvalsUtil, changeDataFactory,
+ return new ChangeControl(changeDataFactory,
getRefControl().forUser(who), notes);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
index 333c343..2f0bf33 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -343,12 +343,15 @@
return changeControl != null;
}
- public ChangeControl changeControl() throws NoSuchChangeException,
- OrmException {
+ public ChangeControl changeControl() throws OrmException {
if (changeControl == null) {
Change c = change();
- changeControl =
- changeControlFactory.controlFor(c, userFactory.create(c.getOwner()));
+ try {
+ changeControl =
+ changeControlFactory.controlFor(c, userFactory.create(c.getOwner()));
+ } catch (NoSuchChangeException e) {
+ throw new OrmException(e);
+ }
}
return changeControl;
}
@@ -394,11 +397,9 @@
Change c = change();
if (c == null) {
currentApprovals = Collections.emptyList();
- } else if (allApprovals != null) {
- return allApprovals.get(c.currentPatchSetId());
} else {
- currentApprovals = approvalsUtil.byPatchSet(
- db, notes(), c.currentPatchSetId());
+ currentApprovals = ImmutableList.copyOf(approvalsUtil.byPatchSet(
+ db, changeControl(), c.currentPatchSetId()));
}
}
return currentApprovals;
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
index e81c061..ba33b97 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
@@ -38,6 +38,8 @@
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
+import com.google.gerrit.server.change.ChangeKindCache;
+import com.google.gerrit.server.change.ChangeKindCacheImpl;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
@@ -222,6 +224,7 @@
.toProvider(CanonicalWebUrlProvider.class);
bind(String.class).annotatedWith(AnonymousCowardName.class)
.toProvider(AnonymousCowardNameProvider.class);
+ bind(ChangeKindCache.class).to(ChangeKindCacheImpl.NoCache.class);
}
});
diff --git a/plugins/reviewnotes b/plugins/reviewnotes
index b544447..6170241 160000
--- a/plugins/reviewnotes
+++ b/plugins/reviewnotes
@@ -1 +1 @@
-Subproject commit b544447649d9ee3b3f78a6a1a7f839cb6a361292
+Subproject commit 61702414c046dd6b811c9137b765f9db422f83db
