Defer object flushing when merging
Mergers in the most recent JGit know how to read previously-inserted
objects back, so a series of merges does not require aggressively
flushing the objects. Instead, flush objects in MergeOp just before
updating the relevant refs.
Change-Id: Ia85c5114e86739b4a480bea65f3413fe47cf572d
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
index 374cde3..262e7bd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
@@ -113,87 +113,89 @@
Project.NameKey project = change.getProject();
IdentifiedUser identifiedUser = (IdentifiedUser) currentUser.get();
- final Repository git;
+ Repository git = null;
+ ObjectInserter oi = null;
+ RevWalk revWalk = null;
+
try {
git = gitManager.openRepository(project);
+ oi = git.newObjectInserter();
+ revWalk = new RevWalk(oi.newReader());
+
+ Ref destRef = git.getRef(destinationBranch);
+ if (destRef == null) {
+ throw new InvalidChangeOperationException("Branch "
+ + destinationBranch + " does not exist.");
+ }
+
+ final RevCommit mergeTip = revWalk.parseCommit(destRef.getObjectId());
+
+ RevCommit commitToCherryPick =
+ revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get()));
+
+ PersonIdent committerIdent =
+ identifiedUser.newCommitterIdent(TimeUtil.nowTs(),
+ serverTimeZone);
+
+ final ObjectId computedChangeId =
+ ChangeIdUtil
+ .computeChangeId(commitToCherryPick.getTree(), mergeTip,
+ commitToCherryPick.getAuthorIdent(), committerIdent, message);
+ String commitMessage =
+ ChangeIdUtil.insertId(message, computedChangeId).trim() + '\n';
+
+ RevCommit cherryPickCommit;
+ ProjectState projectState = refControl.getProjectControl().getProjectState();
+ cherryPickCommit =
+ mergeUtilFactory.create(projectState).createCherryPickFromCommit(git, oi, mergeTip,
+ commitToCherryPick, committerIdent, commitMessage, revWalk);
+ oi.flush();
+
+ Change.Key changeKey;
+ final List<String> idList = cherryPickCommit.getFooterLines(
+ FooterConstants.CHANGE_ID);
+ if (!idList.isEmpty()) {
+ final String idStr = idList.get(idList.size() - 1).trim();
+ changeKey = new Change.Key(idStr);
+ } else {
+ changeKey = new Change.Key("I" + computedChangeId.name());
+ }
+
+ Branch.NameKey newDest =
+ new Branch.NameKey(change.getProject(), destRef.getName());
+ List<ChangeData> destChanges = queryProvider.get()
+ .setLimit(2)
+ .byBranchKey(newDest, changeKey);
+ if (destChanges.size() > 1) {
+ throw new InvalidChangeOperationException("Several changes with key "
+ + changeKey + " reside on the same branch. "
+ + "Cannot create a new patch set.");
+ } else if (destChanges.size() == 1) {
+ // The change key exists on the destination branch. The cherry pick
+ // will be added as a new patch set.
+ return insertPatchSet(git, revWalk, destChanges.get(0).change(),
+ cherryPickCommit, refControl, identifiedUser);
+ } else {
+ // Change key not found on destination branch. We can create a new
+ // change.
+ return createNewChange(git, revWalk, changeKey, project,
+ patch.getId(), destRef, cherryPickCommit, refControl,
+ identifiedUser, change.getTopic());
+ }
+ } catch (MergeIdenticalTreeException | MergeConflictException e) {
+ throw new MergeException("Cherry pick failed: " + e.getMessage());
} catch (RepositoryNotFoundException e) {
throw new NoSuchChangeException(change.getId(), e);
- }
-
- try {
- RevWalk revWalk = new RevWalk(git);
- try {
- Ref destRef = git.getRef(destinationBranch);
- if (destRef == null) {
- throw new InvalidChangeOperationException("Branch "
- + destinationBranch + " does not exist.");
- }
-
- final RevCommit mergeTip = revWalk.parseCommit(destRef.getObjectId());
-
- RevCommit commitToCherryPick =
- revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get()));
-
- PersonIdent committerIdent =
- identifiedUser.newCommitterIdent(TimeUtil.nowTs(),
- serverTimeZone);
-
- final ObjectId computedChangeId =
- ChangeIdUtil
- .computeChangeId(commitToCherryPick.getTree(), mergeTip,
- commitToCherryPick.getAuthorIdent(), committerIdent, message);
- String commitMessage =
- ChangeIdUtil.insertId(message, computedChangeId).trim() + '\n';
-
- RevCommit cherryPickCommit;
- ObjectInserter oi = git.newObjectInserter();
- try {
- ProjectState projectState = refControl.getProjectControl().getProjectState();
- cherryPickCommit =
- mergeUtilFactory.create(projectState).createCherryPickFromCommit(git, oi, mergeTip,
- commitToCherryPick, committerIdent, commitMessage, revWalk);
- } catch (MergeIdenticalTreeException | MergeConflictException e) {
- throw new MergeException("Cherry pick failed: " + e.getMessage());
- } finally {
- oi.release();
- }
-
- Change.Key changeKey;
- final List<String> idList = cherryPickCommit.getFooterLines(
- FooterConstants.CHANGE_ID);
- if (!idList.isEmpty()) {
- final String idStr = idList.get(idList.size() - 1).trim();
- changeKey = new Change.Key(idStr);
- } else {
- changeKey = new Change.Key("I" + computedChangeId.name());
- }
-
- Branch.NameKey newDest =
- new Branch.NameKey(change.getProject(), destRef.getName());
- List<ChangeData> destChanges = queryProvider.get()
- .setLimit(2)
- .byBranchKey(newDest, changeKey);
- if (destChanges.size() > 1) {
- throw new InvalidChangeOperationException("Several changes with key "
- + changeKey + " reside on the same branch. "
- + "Cannot create a new patch set.");
- } else if (destChanges.size() == 1) {
- // The change key exists on the destination branch. The cherry pick
- // will be added as a new patch set.
- return insertPatchSet(git, revWalk, destChanges.get(0).change(),
- cherryPickCommit, refControl, identifiedUser);
- } else {
- // Change key not found on destination branch. We can create a new
- // change.
- return createNewChange(git, revWalk, changeKey, project,
- patch.getId(), destRef, cherryPickCommit, refControl,
- identifiedUser, change.getTopic());
- }
- } finally {
+ } finally {
+ if (revWalk != null) {
revWalk.release();
}
- } finally {
- git.close();
+ if (oi != null) {
+ oi.release();
+ }
+ if (git != null) {
+ git.close();
+ }
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/CodeReviewCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/CodeReviewCommit.java
index fac0f7f..1ff37b0 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/CodeReviewCommit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/CodeReviewCommit.java
@@ -23,6 +23,7 @@
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -50,12 +51,11 @@
}).nullsFirst();
public static RevWalk newRevWalk(Repository repo) {
- return new RevWalk(repo) {
- @Override
- protected RevCommit createCommit(AnyObjectId id) {
- return new CodeReviewCommit(id);
- }
- };
+ return new CodeReviewRevWalk(repo);
+ }
+
+ public static RevWalk newRevWalk(ObjectReader reader) {
+ return new CodeReviewRevWalk(reader);
}
static CodeReviewCommit revisionGone(ChangeControl ctl) {
@@ -85,6 +85,21 @@
return r;
}
+ private static class CodeReviewRevWalk extends RevWalk {
+ private CodeReviewRevWalk(Repository repo) {
+ super(repo);
+ }
+
+ private CodeReviewRevWalk(ObjectReader reader) {
+ super(reader);
+ }
+
+ @Override
+ protected RevCommit createCommit(AnyObjectId id) {
+ return new CodeReviewCommit(id);
+ }
+ }
+
/**
* Unique key of the PatchSet entity from the code review system.
* <p>
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index 106c1e3..8ae3d2b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -430,13 +430,11 @@
String m = "Error opening repository \"" + name.get() + '"';
throw new MergeException(m, err);
}
-
- rw = CodeReviewCommit.newRevWalk(repo);
+ inserter = repo.newObjectInserter();
+ rw = CodeReviewCommit.newRevWalk(inserter.newReader());
rw.sort(RevSort.TOPO);
rw.sort(RevSort.COMMIT_TIME_DESC, true);
canMergeFlag = rw.newFlag("CAN_MERGE");
-
- inserter = repo.newObjectInserter();
}
private RefUpdate openBranch()
@@ -674,6 +672,11 @@
+ destProject.getProject().getName(), e);
}
}
+ try {
+ inserter.flush();
+ } catch (IOException e) {
+ throw new MergeException("Cannot flush merge results", e);
+ }
branchUpdate.setRefLogIdent(refLogIdent);
branchUpdate.setForceUpdate(false);
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 295ad52..c798f4d 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
@@ -43,7 +43,6 @@
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.NoMergeBaseException;
import org.eclipse.jgit.errors.NoMergeBaseException.MergeBaseFailureReason;
-import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Constants;
@@ -67,7 +66,6 @@
import java.io.IOException;
import java.io.InputStream;
-import java.io.UnsupportedEncodingException;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collection;
@@ -79,6 +77,12 @@
import java.util.Set;
import java.util.TimeZone;
+/**
+ * Utilities for various kinds of merges and cherry-picks.
+ * <p>
+ * <b>Note:</b> Unless otherwise noted, the methods in this class do not flush
+ * the {@link ObjectInserter}s passed in after performing a merge.
+ */
public class MergeUtil {
private static final Logger log = LoggerFactory.getLogger(MergeUtil.class);
private static final String R_HEADS_MASTER =
@@ -177,7 +181,7 @@
final ThreeWayMerger m = newThreeWayMerger(repo, inserter);
m.setBase(originalCommit.getParent(0));
- if (m.merge(mergeTip, originalCommit)) {
+ if (m.merge(false, mergeTip, originalCommit)) {
ObjectId tree = m.getResultTreeId();
if (tree.equals(mergeTip.getTree())) {
throw new MergeIdenticalTreeException("identical tree");
@@ -189,7 +193,7 @@
mergeCommit.setAuthor(originalCommit.getAuthorIdent());
mergeCommit.setCommitter(cherryPickCommitterIdent);
mergeCommit.setMessage(commitMsg);
- return rw.parseCommit(commit(inserter, mergeCommit));
+ return rw.parseCommit(inserter.insert(mergeCommit));
} else {
throw new MergeConflictException("merge conflict");
}
@@ -393,7 +397,7 @@
ThreeWayMerger m = newThreeWayMerger(repo, createDryRunInserter(repo));
try {
- return m.merge(new AnyObjectId[] {mergeTip, toMerge});
+ return m.merge(false, mergeTip, toMerge);
} catch (LargeObjectException e) {
log.warn("Cannot merge due to LargeObjectException: " + toMerge.name());
return false;
@@ -442,7 +446,7 @@
try {
ThreeWayMerger m = newThreeWayMerger(repo, createDryRunInserter(repo));
m.setBase(toMerge.getParent(0));
- return m.merge(mergeTip, toMerge);
+ return m.merge(false, mergeTip, toMerge);
} catch (IOException e) {
throw new MergeException("Cannot merge " + toMerge.name(), e);
}
@@ -494,7 +498,7 @@
throws MergeException {
final ThreeWayMerger m = newThreeWayMerger(repo, inserter);
try {
- if (m.merge(new AnyObjectId[] {mergeTip, n})) {
+ if (m.merge(false, mergeTip, n)) {
return writeMergeCommit(myIdent, rw, inserter, canMergeFlag, destBranch,
mergeTip, m.getResultTreeId(), n);
} else {
@@ -582,7 +586,7 @@
mergeCommit.setMessage(msgbuf.toString());
CodeReviewCommit mergeResult =
- (CodeReviewCommit) rw.parseCommit(commit(inserter, mergeCommit));
+ (CodeReviewCommit) rw.parseCommit(inserter.insert(mergeCommit));
mergeResult.setControl(n.getControl());
return mergeResult;
}
@@ -656,31 +660,10 @@
Merger m = strategy.newMerger(repo, true);
checkArgument(m instanceof ThreeWayMerger,
"merge strategy %s does not support three-way merging", strategyName);
- m.setObjectInserter(new ObjectInserter.Filter() {
- @Override
- protected ObjectInserter delegate() {
- return inserter;
- }
-
- @Override
- public void flush() {
- }
-
- @Override
- public void release() {
- }
- });
+ m.setObjectInserter(inserter);
return (ThreeWayMerger) m;
}
- public ObjectId commit(final ObjectInserter inserter,
- final CommitBuilder mergeCommit) throws IOException,
- UnsupportedEncodingException {
- ObjectId id = inserter.insert(mergeCommit);
- inserter.flush();
- return id;
- }
-
public PatchSetApproval markCleanMerges(final RevWalk rw,
final RevFlag canMergeFlag, final CodeReviewCommit mergeTip,
final Set<RevCommit> alreadyAccepted) throws MergeException {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
index 0c8c5ec..50a8117 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
@@ -275,24 +275,11 @@
try {
DirCache dc = DirCache.newInCore();
m.setDirCache(dc);
- m.setObjectInserter(new ObjectInserter.Filter() {
- @Override
- protected ObjectInserter delegate() {
- return ins;
- }
-
- @Override
- public void flush() {
- }
-
- @Override
- public void release() {
- }
- });
+ m.setObjectInserter(ins);
boolean couldMerge;
try {
- couldMerge = m.merge(b.getParents());
+ couldMerge = m.merge(false, b.getParents());
} catch (IOException e) {
// It is not safe to continue further down in this method as throwing
// an exception most likely means that the merge tree was not created
