Don't expose RepoView#getRepository()
For the purposes of fusing BatchUpdateOp#updateChange and updateRepo
into a single batch update, implementations of updateChange need to be
able to observe the ref updates from updateRepo before they are
executed. If we expose the repository directly to updateRepo, it's too
easy for them use it in a wrong way that won't see the results of
updateRepo. That is why we limited the operations that are available
through RepoView.
Finish this refactoring by not exposing the Repository to any op phases,
including updateRepo.
The repo still needs to be directly accessed by the BatchUpdate
internals so it can actually do the writes, which is fine as it doesn't
escape this package.
Change-Id: I0959aa2d2bce346a9a6d493c23a38301b43d1e5d
diff --git a/gerrit-server/BUILD b/gerrit-server/BUILD
index 8182237..e5ca05e 100644
--- a/gerrit-server/BUILD
+++ b/gerrit-server/BUILD
@@ -246,6 +246,7 @@
"//lib:guava",
"//lib:guava-retrying",
"//lib:protobuf",
+ "//lib:truth-java8-extension",
"//lib/bouncycastle:bcprov",
"//lib/bouncycastle:bcpkix",
"//lib/dropwizard:dropwizard-core",
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
index 9e7e345..08473a0 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -529,7 +529,7 @@
commitId,
ctx.getIdentifiedUser())) {
commitValidatorsFactory
- .forGerritCommits(refControl, new NoSshInfo(), ctx.getRepository())
+ .forGerritCommits(refControl, new NoSshInfo(), ctx.getRevWalk())
.validate(event);
}
} catch (CommitValidationException e) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java
index aee9afc..c7cfef9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java
@@ -38,10 +38,10 @@
import com.google.inject.Inject;
import java.io.IOException;
import java.util.Collection;
+import java.util.Map;
+import java.util.Optional;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
@@ -137,16 +137,14 @@
}
private boolean isPatchSetMerged(ChangeContext ctx, PatchSet patchSet) throws IOException {
- Repository repository = ctx.getRepository();
- Ref destinationRef = repository.exactRef(ctx.getChange().getDest().get());
- if (destinationRef == null) {
+ Optional<ObjectId> destId = ctx.getRepoView().getRef(ctx.getChange().getDest().get());
+ if (!destId.isPresent()) {
return false;
}
RevWalk revWalk = ctx.getRevWalk();
ObjectId objectId = ObjectId.fromString(patchSet.getRevision().get());
- return revWalk.isMergedInto(
- revWalk.parseCommit(objectId), revWalk.parseCommit(destinationRef.getObjectId()));
+ return revWalk.isMergedInto(revWalk.parseCommit(objectId), revWalk.parseCommit(destId.get()));
}
private void deleteChangeElementsFromDb(ChangeContext ctx, Change.Id id) throws OrmException {
@@ -174,8 +172,8 @@
public void updateRepo(RepoContext ctx) throws IOException {
String prefix = new PatchSet.Id(id, 1).toRefName();
prefix = prefix.substring(0, prefix.length() - 1);
- for (Ref ref : ctx.getRepository().getRefDatabase().getRefs(prefix).values()) {
- ctx.addRefUpdate(new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), ref.getName()));
+ for (Map.Entry<String, ObjectId> e : ctx.getRepoView().getRefs(prefix).entrySet()) {
+ ctx.addRefUpdate(new ReceiveCommand(e.getValue(), ObjectId.zeroId(), prefix + e.getKey()));
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index 365b645..1512976 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -323,7 +323,7 @@
commitId,
ctx.getIdentifiedUser())) {
commitValidatorsFactory
- .forGerritCommits(origCtl.getRefControl(), new NoSshInfo(), ctx.getRepository())
+ .forGerritCommits(origCtl.getRefControl(), new NoSshInfo(), ctx.getRevWalk())
.validate(event);
} catch (CommitValidationException e) {
throw new ResourceConflictException(e.getFullMessage());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
index 4b5f24d..16e3a08 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -160,7 +160,9 @@
baseCommitId.name());
rebasedPatchSetId =
- ChangeUtil.nextPatchSetId(ctx.getRepository(), ctl.getChange().currentPatchSetId());
+ ChangeUtil.nextPatchSetIdFromChangeRefsMap(
+ ctx.getRepoView().getRefs(originalPatchSet.getId().getParentKey().toRefPrefix()),
+ ctl.getChange().currentPatchSetId());
patchSetInserter =
patchSetInserterFactory
.create(ctl, rebasedPatchSetId, rebasedCommit)
@@ -241,7 +243,7 @@
}
ThreeWayMerger merger =
- newMergeUtil().newThreeWayMerger(ctx.getInserter(), ctx.getRepository().getConfig());
+ newMergeUtil().newThreeWayMerger(ctx.getInserter(), ctx.getRepoView().getConfig());
merger.setBase(parentCommit);
merger.merge(original, base);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
index 8982fa9..92333c6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
@@ -59,6 +59,7 @@
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
/**
* Utility functions to manipulate change edits.
@@ -222,7 +223,9 @@
new BatchUpdateOp() {
@Override
public void updateRepo(RepoContext ctx) throws Exception {
- deleteRef(ctx.getRepository(), edit);
+ ctx.addRefUpdate(
+ new ReceiveCommand(
+ edit.getEditCommit().copy(), ObjectId.zeroId(), edit.getRefName()));
}
});
bu.execute();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 472f525..b10cc71 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -2778,7 +2778,7 @@
CommitValidators validators =
isMerged
? commitValidatorsFactory.forMergedCommits(ctl)
- : commitValidatorsFactory.forReceiveCommits(ctl, sshInfo, repo);
+ : commitValidatorsFactory.forReceiveCommits(ctl, sshInfo, repo, rw);
messages.addAll(validators.validate(receiveEvent));
} catch (CommitValidationException e) {
logDebug("Commit validation failed on {}", c.name());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
index bca1943..7bb8271 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReplaceOp.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
+import static org.eclipse.jgit.lib.Constants.R_HEADS;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
@@ -62,13 +63,11 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
-import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.PushCertificate;
@@ -199,15 +198,15 @@
changeKindCache.getChangeKind(
projectControl.getProject().getNameKey(),
ctx.getRevWalk(),
- ctx.getRepository().getConfig(),
+ ctx.getRepoView().getConfig(),
priorCommitId,
commitId);
if (checkMergedInto) {
- Ref mergedInto = findMergedInto(ctx, dest.get(), commit);
+ String mergedInto = findMergedInto(ctx, dest.get(), commit);
if (mergedInto != null) {
mergedByPushOp =
- mergedByPushOpFactory.create(requestScopePropagator, patchSetId, mergedInto.getName());
+ mergedByPushOpFactory.create(requestScopePropagator, patchSetId, mergedInto);
}
}
@@ -512,18 +511,17 @@
return this;
}
- private static Ref findMergedInto(Context ctx, String first, RevCommit commit) {
+ private static String findMergedInto(Context ctx, String first, RevCommit commit) {
try {
- RefDatabase refDatabase = ctx.getRepository().getRefDatabase();
-
- Ref firstRef = refDatabase.exactRef(first);
- if (firstRef != null && isMergedInto(ctx.getRevWalk(), commit, firstRef)) {
- return firstRef;
+ RevWalk rw = ctx.getRevWalk();
+ Optional<ObjectId> firstId = ctx.getRepoView().getRef(first);
+ if (firstId.isPresent() && rw.isMergedInto(commit, rw.parseCommit(firstId.get()))) {
+ return first;
}
- for (Ref ref : refDatabase.getRefs(Constants.R_HEADS).values()) {
- if (isMergedInto(ctx.getRevWalk(), commit, ref)) {
- return ref;
+ for (Map.Entry<String, ObjectId> e : ctx.getRepoView().getRefs(R_HEADS).entrySet()) {
+ if (rw.isMergedInto(commit, rw.parseCommit(e.getValue()))) {
+ return R_HEADS + e.getKey();
}
}
return null;
@@ -532,8 +530,4 @@
return null;
}
}
-
- private static boolean isMergedInto(RevWalk rw, RevCommit commit, Ref ref) throws IOException {
- return rw.isMergedInto(commit, rw.parseCommit(ref.getObjectId()));
- }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/RepoRefCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/RepoRefCache.java
index e7a86f1..6b2493a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/RepoRefCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/RepoRefCache.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.git;
import java.io.IOException;
+import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
@@ -44,4 +45,9 @@
ids.put(refName, id);
return id;
}
+
+ /** @return an unmodifiable view of the refs that have been cached by this instance. */
+ public Map<String, Optional<ObjectId>> getCachedRefs() {
+ return Collections.unmodifiableMap(ids);
+ }
}
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 183a8ca..879ca0e 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
@@ -95,7 +95,10 @@
// delta relative to that one parent and redoing that on the current merge
// tip.
args.rw.parseBody(toMerge);
- psId = ChangeUtil.nextPatchSetId(ctx.getRepository(), toMerge.change().currentPatchSetId());
+ psId =
+ ChangeUtil.nextPatchSetIdFromChangeRefsMap(
+ ctx.getRepoView().getRefs(getId().toRefPrefix()),
+ toMerge.change().currentPatchSetId());
RevCommit mergeTip = args.mergeTip.getCurrentTip();
args.rw.parseBody(mergeTip);
String cherryPickCmtMsg = args.mergeUtil.createCommitMessageOnSubmit(toMerge, mergeTip);
@@ -106,7 +109,7 @@
newCommit =
args.mergeUtil.createCherryPickFromCommit(
ctx.getInserter(),
- ctx.getRepository().getConfig(),
+ ctx.getRepoView().getConfig(),
args.mergeTip.getCurrentTip(),
toMerge,
committer,
@@ -197,7 +200,7 @@
myIdent,
args.rw,
ctx.getInserter(),
- ctx.getRepository().getConfig(),
+ ctx.getRepoView().getConfig(),
args.destBranch,
mergeTip.getCurrentTip(),
toMerge);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeOneOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeOneOp.java
index 9c02fcc..9fb75a8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeOneOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeOneOp.java
@@ -42,7 +42,7 @@
args.serverIdent,
args.rw,
ctx.getInserter(),
- ctx.getRepository().getConfig(),
+ ctx.getRepoView().getConfig(),
args.destBranch,
args.mergeTip.getCurrentTip(),
toMerge);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
index 7fb5728..18b4173 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java
@@ -127,7 +127,9 @@
// RebaseAlways means we modify commit message.
args.rw.parseBody(toMerge);
newPatchSetId =
- ChangeUtil.nextPatchSetId(ctx.getRepository(), toMerge.change().currentPatchSetId());
+ ChangeUtil.nextPatchSetIdFromChangeRefsMap(
+ ctx.getRepoView().getRefs(getId().toRefPrefix()),
+ toMerge.change().currentPatchSetId());
RevCommit mergeTip = args.mergeTip.getCurrentTip();
args.rw.parseBody(mergeTip);
String cherryPickCmtMsg = args.mergeUtil.createCommitMessageOnSubmit(toMerge, mergeTip);
@@ -137,7 +139,7 @@
newCommit =
args.mergeUtil.createCherryPickFromCommit(
ctx.getInserter(),
- ctx.getRepository().getConfig(),
+ ctx.getRepoView().getConfig(),
args.mergeTip.getCurrentTip(),
toMerge,
committer,
@@ -269,7 +271,7 @@
caller,
args.rw,
ctx.getInserter(),
- ctx.getRepository().getConfig(),
+ ctx.getRepoView().getConfig(),
args.destBranch,
mergeTip.getCurrentTip(),
toMerge);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
index e2feb80..6bf8b2e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
@@ -53,7 +53,6 @@
import com.google.gwtorm.server.OrmException;
import java.io.IOException;
import java.util.ArrayList;
-import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
@@ -62,7 +61,6 @@
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.slf4j.Logger;
@@ -168,19 +166,20 @@
}
CodeReviewRevWalk rw = (CodeReviewRevWalk) ctx.getRevWalk();
Change.Id id = getId();
+ String refPrefix = id.toRefPrefix();
- Collection<Ref> refs = ctx.getRepository().getRefDatabase().getRefs(id.toRefPrefix()).values();
+ Map<String, ObjectId> refs = ctx.getRepoView().getRefs(refPrefix);
List<CodeReviewCommit> commits = new ArrayList<>(refs.size());
- for (Ref ref : refs) {
- PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName());
+ for (Map.Entry<String, ObjectId> e : refs.entrySet()) {
+ PatchSet.Id psId = PatchSet.Id.fromRef(refPrefix + e.getKey());
if (psId == null) {
continue;
}
try {
- CodeReviewCommit c = rw.parseCommit(ref.getObjectId());
+ CodeReviewCommit c = rw.parseCommit(e.getValue());
c.setPatchsetId(psId);
commits.add(c);
- } catch (MissingObjectException | IncorrectObjectTypeException e) {
+ } catch (MissingObjectException | IncorrectObjectTypeException ex) {
continue; // Bogus ref, can't be merged into tip so we don't care.
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
index 85ae80c..d8a19bf 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -90,27 +90,24 @@
}
public CommitValidators forReceiveCommits(
- RefControl refControl, SshInfo sshInfo, Repository repo) throws IOException {
- try (RevWalk rw = new RevWalk(repo)) {
- NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw);
- return new CommitValidators(
- ImmutableList.of(
- new UploadMergesPermissionValidator(refControl),
- new AmendedGerritMergeCommitValidationListener(refControl, gerritIdent),
- new AuthorUploaderValidator(refControl, canonicalWebUrl),
- new CommitterUploaderValidator(refControl, canonicalWebUrl),
- new SignedOffByValidator(refControl),
- new ChangeIdValidator(
- refControl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
- new ConfigValidator(refControl, repo, allUsers),
- new BannedCommitsValidator(rejectCommits),
- new PluginCommitValidationListener(pluginValidators),
- new BlockExternalIdUpdateListener(allUsers)));
- }
+ RefControl refControl, SshInfo sshInfo, Repository repo, RevWalk rw) throws IOException {
+ NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw);
+ return new CommitValidators(
+ ImmutableList.of(
+ new UploadMergesPermissionValidator(refControl),
+ new AmendedGerritMergeCommitValidationListener(refControl, gerritIdent),
+ new AuthorUploaderValidator(refControl, canonicalWebUrl),
+ new CommitterUploaderValidator(refControl, canonicalWebUrl),
+ new SignedOffByValidator(refControl),
+ new ChangeIdValidator(
+ refControl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
+ new ConfigValidator(refControl, rw, allUsers),
+ new BannedCommitsValidator(rejectCommits),
+ new PluginCommitValidationListener(pluginValidators),
+ new BlockExternalIdUpdateListener(allUsers)));
}
- public CommitValidators forGerritCommits(
- RefControl refControl, SshInfo sshInfo, Repository repo) {
+ public CommitValidators forGerritCommits(RefControl refControl, SshInfo sshInfo, RevWalk rw) {
return new CommitValidators(
ImmutableList.of(
new UploadMergesPermissionValidator(refControl),
@@ -119,7 +116,7 @@
new SignedOffByValidator(refControl),
new ChangeIdValidator(
refControl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
- new ConfigValidator(refControl, repo, allUsers),
+ new ConfigValidator(refControl, rw, allUsers),
new PluginCommitValidationListener(pluginValidators),
new BlockExternalIdUpdateListener(allUsers)));
}
@@ -320,12 +317,12 @@
/** If this is the special project configuration branch, validate the config. */
public static class ConfigValidator implements CommitValidationListener {
private final RefControl refControl;
- private final Repository repo;
+ private final RevWalk rw;
private final AllUsersName allUsers;
- public ConfigValidator(RefControl refControl, Repository repo, AllUsersName allUsers) {
+ public ConfigValidator(RefControl refControl, RevWalk rw, AllUsersName allUsers) {
this.refControl = refControl;
- this.repo = repo;
+ this.rw = rw;
this.allUsers = allUsers;
}
@@ -339,7 +336,7 @@
try {
ProjectConfig cfg = new ProjectConfig(receiveEvent.project.getNameKey());
- cfg.load(repo, receiveEvent.command.getNewId());
+ cfg.load(rw, receiveEvent.command.getNewId());
if (!cfg.getValidationErrors().isEmpty()) {
addError("Invalid project configuration:", messages);
for (ValidationError err : cfg.getValidationErrors()) {
@@ -367,7 +364,7 @@
if (accountId != null) {
try {
WatchConfig wc = new WatchConfig(accountId);
- wc.load(repo, receiveEvent.command.getNewId());
+ wc.load(rw, receiveEvent.command.getNewId());
if (!wc.getValidationErrors().isEmpty()) {
addError("Invalid project configuration:", messages);
for (ValidationError err : wc.getValidationErrors()) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
index abe1d4f..4da2e5e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -322,11 +322,6 @@
: Optional.empty();
}
- protected Repository getRepository() throws IOException {
- initRepository();
- return repoView.getRepository();
- }
-
protected RevWalk getRevWalk() throws IOException {
initRepository();
return repoView.getRevWalk();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java
index db9239f..f33536d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java
@@ -24,7 +24,6 @@
import java.io.IOException;
import java.sql.Timestamp;
import java.util.TimeZone;
-import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
/**
@@ -41,15 +40,14 @@
Project.NameKey getProject();
/**
- * Get an open repository instance for this project.
+ * Get a read-only view of the open repository for this project.
*
- * <p>Will be opened lazily if necessary; callers should not close the repo. In some phases of the
- * update, the repository might be read-only; see {@link BatchUpdateOp} for details.
+ * <p>Will be opened lazily if necessary.
*
* @return repository instance.
* @throws IOException if an error occurred opening the repo.
*/
- Repository getRepository() throws IOException;
+ RepoView getRepoView() throws IOException;
/**
* Get a walk for this project.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
index f8c1de6..1c98440 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
@@ -140,19 +140,15 @@
}
class ContextImpl implements Context {
- private Repository repoWrapper;
-
@Override
- public Repository getRepository() throws IOException {
- if (repoWrapper == null) {
- repoWrapper = new ReadOnlyRepository(NoteDbBatchUpdate.this.getRepository());
- }
- return repoWrapper;
+ public RepoView getRepoView() throws IOException {
+ initRepository();
+ return NoteDbBatchUpdate.this.repoView;
}
@Override
public RevWalk getRevWalk() throws IOException {
- return NoteDbBatchUpdate.this.getRevWalk();
+ return getRepoView().getRevWalk();
}
@Override
@@ -188,13 +184,8 @@
private class RepoContextImpl extends ContextImpl implements RepoContext {
@Override
- public Repository getRepository() throws IOException {
- return NoteDbBatchUpdate.this.getRepository();
- }
-
- @Override
public ObjectInserter getInserter() throws IOException {
- return NoteDbBatchUpdate.this.getObjectInserter();
+ return getRepoView().getInserter();
}
@Override
@@ -370,7 +361,8 @@
logDebug("Executing change ops");
Map<Change.Id, ChangeResult> result =
Maps.newLinkedHashMapWithExpectedSize(ops.keySet().size());
- Repository repo = getRepository();
+ initRepository();
+ Repository repo = repoView.getRepository();
// TODO(dborowitz): Teach NoteDbUpdateManager to allow reusing the same inserter and batch ref
// update as in executeUpdateRepo.
try (ObjectInserter ins = repo.newObjectInserter();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/RepoContext.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/RepoContext.java
index 5009c50..3c1e896 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/RepoContext.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/RepoContext.java
@@ -31,8 +31,7 @@
/**
* Add a command to the pending list of commands.
*
- * <p>Callers should use this method instead of writing directly to the repository returned by
- * {@link #getRepository()}.
+ * <p>This method is the only way of updating refs in the repository from a {@link BatchUpdateOp}.
*
* @param cmd ref update command.
* @throws IOException if an error occurred opening the repo.
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/RepoView.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/RepoView.java
index d1a6f1b..cb1abdb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/RepoView.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/RepoView.java
@@ -17,15 +17,33 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
+import com.google.common.collect.Maps;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.git.GitRepositoryManager;
import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
import java.util.Optional;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
+/**
+ * Restricted view of a {@link Repository} for use by {@link BatchUpdateOp} implementations.
+ *
+ * <p>This class serves two purposes in the context of {@link BatchUpdate}. First, the subset of
+ * normal Repository functionality is purely read-only, which prevents implementors from modifying
+ * the repository outside of {@link BatchUpdateOp#updateRepo}. Write operations can only be
+ * performed by calling methods on {@link RepoContext}.
+ *
+ * <p>Second, the read methods take into account any pending operations on the repository that
+ * implementations have staged using the write methods on {@link RepoContext}. Callers do not have
+ * to worry about whether operations have been performed yet, and the implementation details may
+ * differ between ReviewDb and NoteDb, but callers just don't need to care.
+ */
public class RepoView {
private final Repository repo;
private final RevWalk rw;
@@ -54,25 +72,112 @@
closeRepo = false;
}
+ /**
+ * Get this repo's configuration.
+ *
+ * <p>This is the storage-level config you would get with {@link Repository#getConfig()}, not, for
+ * example, the Gerrit-level project config.
+ *
+ * @return a defensive copy of the config; modifications have no effect on the underlying config.
+ */
+ public Config getConfig() {
+ return new Config(repo.getConfig());
+ }
+
+ /**
+ * Get an open revwalk on the repo.
+ *
+ * <p>Guaranteed to be able to read back any objects inserted in the repository via {@link
+ * RepoContext#getInserter()}, even if objects have not been flushed to the underlying repo. In
+ * particular this includes any object returned by {@link #getRef(String)}, even taking into
+ * account not-yet-executed commands.
+ *
+ * @return revwalk.
+ */
public RevWalk getRevWalk() {
return rw;
}
- public ObjectInserter getInserter() {
- return inserter;
- }
-
- public ChainedReceiveCommands getCommands() {
- return commands;
- }
-
+ /**
+ * Read a single ref from the repo.
+ *
+ * <p>Takes into account any ref update commands added during the course of the update using
+ * {@link RepoContext#addRefUpdate}, even if they have not yet been executed on the underlying
+ * repo.
+ *
+ * <p>The results of individual ref lookups are cached: calling this method multiple times with
+ * the same ref name will return the same result (unless a command was added in the meantime). The
+ * repo is not reread.
+ *
+ * @param name exact ref name.
+ * @return the value of the ref, if present.
+ * @throws IOException if an error occurred.
+ */
public Optional<ObjectId> getRef(String name) throws IOException {
return getCommands().get(name);
}
- // TODO(dborowitz): Remove this so callers can't do arbitrary stuff.
- Repository getRepository() {
- return repo;
+ /**
+ * Look up refs by prefix.
+ *
+ * <p>Takes into account any ref update commands added during the course of the update using
+ * {@link RepoContext#addRefUpdate}, even if they have not yet been executed on the underlying
+ * repo.
+ *
+ * <p>For any ref that has previously been accessed with {@link #getRef(String)}, the value in the
+ * result map will be that same cached value. Any refs that have <em>not</em> been previously
+ * accessed are re-scanned from the repo on each call.
+ *
+ * @param prefix ref prefix; must end in '/' or else be empty.
+ * @return a map of ref suffixes to SHA-1s. The refs are all under {@code prefix} and have the
+ * prefix stripped; this matches the behavior of {@link
+ * org.eclipse.jgit.lib.RefDatabase#getRefs(String)}.
+ * @throws IOException if an error occurred.
+ */
+ public Map<String, ObjectId> getRefs(String prefix) throws IOException {
+ Map<String, ObjectId> result =
+ new HashMap<>(
+ Maps.transformValues(repo.getRefDatabase().getRefs(prefix), Ref::getObjectId));
+
+ // First, overwrite any cached reads from the underlying RepoRefCache. If any of these differ,
+ // it's because a ref was updated after the RepoRefCache read it. It feels a little odd to
+ // prefer the *old* value in this case, but it would be weirder to be inconsistent with getRef.
+ //
+ // Mostly this doesn't matter. If the caller was intending to write to the ref, they lost a
+ // race, and they will get a lock failure. If they just want to read, well, the JGit interface
+ // doesn't currently guarantee that any snapshot of multiple refs is consistent, so they were
+ // probably out of luck anyway.
+ commands
+ .getRepoRefCache()
+ .getCachedRefs()
+ .forEach((k, v) -> updateRefIfPrefixMatches(result, prefix, k, v));
+
+ // Second, overwrite with any pending commands.
+ commands
+ .getCommands()
+ .values()
+ .forEach(
+ c ->
+ updateRefIfPrefixMatches(result, prefix, c.getRefName(), toOptional(c.getNewId())));
+
+ return result;
+ }
+
+ private static Optional<ObjectId> toOptional(ObjectId id) {
+ return id.equals(ObjectId.zeroId()) ? Optional.empty() : Optional.of(id);
+ }
+
+ private static void updateRefIfPrefixMatches(
+ Map<String, ObjectId> map, String prefix, String fullRefName, Optional<ObjectId> maybeId) {
+ if (!fullRefName.startsWith(prefix)) {
+ return;
+ }
+ String suffix = fullRefName.substring(prefix.length());
+ if (maybeId.isPresent()) {
+ map.put(suffix, maybeId.get());
+ } else {
+ map.remove(suffix);
+ }
}
// Not AutoCloseable so callers can't improperly close it. Plus it's never managed with a try
@@ -84,4 +189,16 @@
repo.close();
}
}
+
+ Repository getRepository() {
+ return repo;
+ }
+
+ ObjectInserter getInserter() {
+ return inserter;
+ }
+
+ ChainedReceiveCommands getCommands() {
+ return commands;
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java
index 8c924c3..b59da8a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java
@@ -111,19 +111,15 @@
}
class ContextImpl implements Context {
- private Repository repoWrapper;
-
@Override
- public Repository getRepository() throws IOException {
- if (repoWrapper == null) {
- repoWrapper = new ReadOnlyRepository(ReviewDbBatchUpdate.this.getRepository());
- }
- return repoWrapper;
+ public RepoView getRepoView() throws IOException {
+ initRepository();
+ return ReviewDbBatchUpdate.this.repoView;
}
@Override
public RevWalk getRevWalk() throws IOException {
- return ReviewDbBatchUpdate.this.getRevWalk();
+ return getRepoView().getRevWalk();
}
@Override
@@ -159,13 +155,8 @@
private class RepoContextImpl extends ContextImpl implements RepoContext {
@Override
- public Repository getRepository() throws IOException {
- return ReviewDbBatchUpdate.this.getRepository();
- }
-
- @Override
public ObjectInserter getInserter() throws IOException {
- return ReviewDbBatchUpdate.this.getObjectInserter();
+ return getRepoView().getInserter();
}
@Override
@@ -201,11 +192,6 @@
}
@Override
- public Repository getRepository() {
- return threadLocalRepo;
- }
-
- @Override
public RevWalk getRevWalk() {
return threadLocalRevWalk;
}
@@ -534,9 +520,10 @@
// updates on the change repo first.
logDebug("Executing NoteDb updates for {} changes", tasks.size());
try {
- BatchRefUpdate changeRefUpdate = getRepository().getRefDatabase().newBatchUpdate();
+ initRepository();
+ BatchRefUpdate changeRefUpdate = repoView.getRepository().getRefDatabase().newBatchUpdate();
boolean hasAllUsersCommands = false;
- try (ObjectInserter ins = getRepository().newObjectInserter()) {
+ try (ObjectInserter ins = repoView.getRepository().newObjectInserter()) {
int objs = 0;
for (ChangeTask task : tasks) {
if (task.noteDbResult == null) {
@@ -643,7 +630,8 @@
public Void call() throws Exception {
taskId = id.toString() + "-" + Thread.currentThread().getId();
if (Thread.currentThread() == mainThread) {
- Repository repo = getRepository();
+ initRepository();
+ Repository repo = repoView.getRepository();
try (RevWalk rw = new RevWalk(repo)) {
call(ReviewDbBatchUpdate.this.db, repo, rw);
}
@@ -802,10 +790,10 @@
updateManagerFactory
.create(ctx.getProject())
.setChangeRepo(
- ctx.getRepository(),
- ctx.getRevWalk(),
+ ctx.threadLocalRepo,
+ ctx.threadLocalRevWalk,
null,
- new ChainedReceiveCommands(ctx.getRepository()));
+ new ChainedReceiveCommands(ctx.threadLocalRepo));
if (ctx.getUser().isIdentifiedUser()) {
updateManager.setRefLogIdent(
ctx.getUser().asIdentifiedUser().newRefLogIdent(ctx.getWhen(), tz));
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/update/RepoViewTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/update/RepoViewTest.java
new file mode 100644
index 0000000..0ea9f83
--- /dev/null
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/update/RepoViewTest.java
@@ -0,0 +1,154 @@
+// Copyright (C) 2017 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.update;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static org.eclipse.jgit.lib.Constants.R_HEADS;
+
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.testutil.InMemoryRepositoryManager;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.StoredConfig;
+import org.eclipse.jgit.transport.ReceiveCommand;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+public class RepoViewTest {
+ private static final String MASTER = "refs/heads/master";
+ private static final String BRANCH = "refs/heads/branch";
+
+ private Repository repo;
+ private TestRepository<?> tr;
+ private RepoView view;
+
+ @Before
+ public void setUp() throws Exception {
+ InMemoryRepositoryManager repoManager = new InMemoryRepositoryManager();
+ Project.NameKey project = new Project.NameKey("project");
+ repo = repoManager.createRepository(project);
+ tr = new TestRepository<>(repo);
+ tr.branch(MASTER).commit().create();
+ view = new RepoView(repoManager, project);
+ }
+
+ @After
+ public void tearDown() {
+ view.close();
+ repo.close();
+ }
+
+ @Test
+ public void getConfigIsDefensiveCopy() throws Exception {
+ StoredConfig orig = repo.getConfig();
+ orig.setString("a", "config", "option", "yes");
+ orig.save();
+
+ Config copy = view.getConfig();
+ copy.setString("a", "config", "option", "no");
+
+ assertThat(orig.getString("a", "config", "option")).isEqualTo("yes");
+ assertThat(repo.getConfig().getString("a", "config", "option")).isEqualTo("yes");
+ }
+
+ @Test
+ public void getRef() throws Exception {
+ ObjectId oldMaster = repo.exactRef(MASTER).getObjectId();
+ assertThat(repo.exactRef(MASTER).getObjectId()).isEqualTo(oldMaster);
+ assertThat(repo.exactRef(BRANCH)).isNull();
+ assertThat(view.getRef(MASTER)).hasValue(oldMaster);
+ assertThat(view.getRef(BRANCH)).isEmpty();
+
+ tr.branch(MASTER).commit().create();
+ tr.branch(BRANCH).commit().create();
+ assertThat(repo.exactRef(MASTER).getObjectId()).isNotEqualTo(oldMaster);
+ assertThat(repo.exactRef(BRANCH)).isNotNull();
+ assertThat(view.getRef(MASTER)).hasValue(oldMaster);
+ assertThat(view.getRef(BRANCH)).isEmpty();
+ }
+
+ @Test
+ public void getRefsRescansWhenNotCaching() throws Exception {
+ ObjectId oldMaster = repo.exactRef(MASTER).getObjectId();
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", oldMaster);
+
+ ObjectId newBranch = tr.branch(BRANCH).commit().create();
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", oldMaster, "branch", newBranch);
+ }
+
+ @Test
+ public void getRefsUsesCachedValueMatchingGetRef() throws Exception {
+ ObjectId master1 = repo.exactRef(MASTER).getObjectId();
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", master1);
+ assertThat(view.getRef(MASTER)).hasValue(master1);
+
+ // Doesn't reflect new value for master.
+ ObjectId master2 = tr.branch(MASTER).commit().create();
+ assertThat(repo.exactRef(MASTER).getObjectId()).isEqualTo(master2);
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", master1);
+
+ // Branch wasn't previously cached, so does reflect new value.
+ ObjectId branch1 = tr.branch(BRANCH).commit().create();
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", master1, "branch", branch1);
+
+ // Looking up branch causes it to be cached.
+ assertThat(view.getRef(BRANCH)).hasValue(branch1);
+ ObjectId branch2 = tr.branch(BRANCH).commit().create();
+ assertThat(repo.exactRef(BRANCH).getObjectId()).isEqualTo(branch2);
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", master1, "branch", branch1);
+ }
+
+ @Test
+ public void getRefsReflectsCommands() throws Exception {
+ ObjectId master1 = repo.exactRef(MASTER).getObjectId();
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", master1);
+
+ ObjectId master2 = tr.commit().create();
+ view.getCommands().add(new ReceiveCommand(master1, master2, MASTER));
+
+ assertThat(repo.exactRef(MASTER).getObjectId()).isEqualTo(master1);
+ assertThat(view.getRef(MASTER)).hasValue(master2);
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", master2);
+
+ view.getCommands().add(new ReceiveCommand(master2, ObjectId.zeroId(), MASTER));
+
+ assertThat(repo.exactRef(MASTER).getObjectId()).isEqualTo(master1);
+ assertThat(view.getRef(MASTER)).isEmpty();
+ assertThat(view.getRefs(R_HEADS)).isEmpty();
+ }
+
+ @Test
+ public void getRefsOverwritesCachedValueWithCommand() throws Exception {
+ ObjectId master1 = repo.exactRef(MASTER).getObjectId();
+ assertThat(view.getRef(MASTER)).hasValue(master1);
+
+ ObjectId master2 = tr.commit().create();
+ view.getCommands().add(new ReceiveCommand(master1, master2, MASTER));
+
+ assertThat(repo.exactRef(MASTER).getObjectId()).isEqualTo(master1);
+ assertThat(view.getRef(MASTER)).hasValue(master2);
+ assertThat(view.getRefs(R_HEADS)).containsExactly("master", master2);
+
+ view.getCommands().add(new ReceiveCommand(master2, ObjectId.zeroId(), MASTER));
+
+ assertThat(repo.exactRef(MASTER).getObjectId()).isEqualTo(master1);
+ assertThat(view.getRef(MASTER)).isEmpty();
+ assertThat(view.getRefs(R_HEADS)).isEmpty();
+ }
+}
