opendev/gerrit
Code Issues Proposed changes

MergeTip: Record ObjectIds instead of Strings

Browse Source 
The values in the map of original commits to merge results are really
object IDs. Use the actual ObjectId type instead of materializing them
to strings, which hurts readability and performance.

Change-Id: I2b59f19ff82def9119cf49e9ec162d73d9cd5ca7
This commit is contained in:
Dave Borowitz
2015-02-17 16:24:11 -08:00
parent d89a1cccae
commit e7f7f9530b
7 changed files with 27 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -612,7 +612,7 @@ public class MergeOp {
idstr, ps.getId());
commit.setStatusCode(CommitMergeStatus.ALREADY_MERGED);
try {
setMerged(chg, null, commit.getName());
setMerged(chg, null, commit);
} catch (OrmException e) {
logError("Cannot mark change " + chg.getId() + " merged", e);
}
@@ -776,10 +776,9 @@ public class MergeOp {
String txt = s.getMessage();
logDebug("Status of change {} ({}) on {}: {}", c.getId(), commit.name(),
c.getDest(), s);
String commitName = commit.getName();
// If mergeTip is null merge failed and mergeResultRev will not be read.
String mergeResultRev =
mergeTip != null ? mergeTip.getMergeResults().get(commitName) : null;
ObjectId mergeResultRev =
mergeTip != null ? mergeTip.getMergeResults().get(commit) : null;
try {
switch (s) {
case CLEAN_MERGE:
@@ -948,7 +947,7 @@ public class MergeOp {
return m;
}
private void setMerged(Change c, ChangeMessage msg, String mergeResultRev)
private void setMerged(Change c, ChangeMessage msg, ObjectId mergeResultRev)
throws OrmException, IOException {
logDebug("Setting change {} merged", c.getId());
ChangeUpdate update = null;
@@ -985,7 +984,7 @@ public class MergeOp {
try {
hooks.doChangeMergedHook(c,
accountCache.get(submitter.getAccountId()).getAccount(),
merged, db, mergeResultRev);
merged, db, mergeResultRev.name());
} catch (OrmException ex) {
logError("Cannot run hook for submitted patch set " + c.getId(), ex);
}

14
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeTip.java
View File

@@ -19,6 +19,8 @@ import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.collect.Maps;
import com.google.gerrit.common.Nullable;
import org.eclipse.jgit.lib.ObjectId;
import java.util.Collection;
import java.util.Map;
@@ -32,7 +34,7 @@ import java.util.Map;
*/
public class MergeTip {
private CodeReviewCommit branchTip;
private Map<String,String> mergeResults;
private Map<ObjectId, ObjectId> mergeResults;
/**
* @param initial Tip before the merge operation; may be null, indicating an
@@ -48,7 +50,7 @@ public class MergeTip {
this.branchTip = initial;
// Assume fast-forward merge until opposite is proven.
for (CodeReviewCommit commit : toMerge) {
mergeResults.put(commit.getName(), commit.getName());
mergeResults.put(commit.copy(), commit.copy());
}
}
@@ -56,12 +58,12 @@ public class MergeTip {
* Moves this MergeTip to newTip and appends mergeResult.
*
* @param newTip The new tip; may not be null.
* @param mergedFrom The result of the merge of newTip.
* @param mergedFrom The result of the merge of {@code newTip}.
*/
public void moveTipTo(CodeReviewCommit newTip, String mergedFrom) {
public void moveTipTo(CodeReviewCommit newTip, ObjectId mergedFrom) {
checkArgument(newTip != null);
branchTip = newTip;
mergeResults.put(mergedFrom, newTip.getName());
mergeResults.put(mergedFrom, newTip.copy());
}
/**
@@ -70,7 +72,7 @@ public class MergeTip {
* @return The merge results of the merge operation as a map of SHA-1 to be
* merged to SHA-1 of the merge result.
*/
public Map<String, String> getMergeResults() {
public Map<ObjectId, ObjectId> getMergeResults() {
return mergeResults;
}

6
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
View File

@@ -108,7 +108,7 @@ public class CherryPick extends SubmitStrategy {
try {
CodeReviewCommit merge =
writeCherryPickCommit(mergeTip.getCurrentTip(), n);
mergeTip.moveTipTo(merge, merge.getName());
mergeTip.moveTipTo(merge, merge);
newCommits.put(mergeTip.getCurrentTip().getPatchsetId()
.getParentKey(), mergeTip.getCurrentTip());
return mergeTip;
@@ -130,12 +130,12 @@ public class CherryPick extends SubmitStrategy {
// was configured.
if (!args.mergeUtil.hasMissingDependencies(args.mergeSorter, n)) {
if (args.rw.isMergedInto(mergeTip.getCurrentTip(), n)) {
mergeTip.moveTipTo(n, n.getName());
mergeTip.moveTipTo(n, n);
} else {
CodeReviewCommit result = args.mergeUtil.mergeOneCommit(
args.serverIdent.get(), args.repo, args.rw, args.inserter,
args.canMergeFlag, args.destBranch, mergeTip.getCurrentTip(), n);
mergeTip.moveTipTo(result, n.getName());
mergeTip.moveTipTo(result, n);
}
PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges(args.rw,
args.canMergeFlag, mergeTip.getCurrentTip(), args.alreadyAccepted);

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java
View File

@@ -36,7 +36,7 @@ public class FastForwardOnly extends SubmitStrategy {
args.mergeSorter, toMerge);
final CodeReviewCommit newMergeTipCommit =
args.mergeUtil.getFirstFastForward(branchTip, args.rw, sorted);
mergeTip.moveTipTo(newMergeTipCommit, newMergeTipCommit.getName());
mergeTip.moveTipTo(newMergeTipCommit, newMergeTipCommit);
while (!sorted.isEmpty()) {
final CodeReviewCommit n = sorted.remove(0);

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java
View File

@@ -46,7 +46,7 @@ public class MergeAlways extends SubmitStrategy {
args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw,
args.inserter, args.canMergeFlag, args.destBranch, mergeTip.getCurrentTip(),
mergedFrom);
mergeTip.moveTipTo(newTip, mergedFrom.getName());
mergeTip.moveTipTo(newTip, mergedFrom);
}
final PatchSetApproval submitApproval =

4
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java
View File

@@ -43,7 +43,7 @@ public class MergeIfNecessary extends SubmitStrategy {
branchTip =
args.mergeUtil.getFirstFastForward(branchTip, args.rw, sorted);
}
mergeTip.moveTipTo(branchTip, branchTip.getName());
mergeTip.moveTipTo(branchTip, branchTip);
// For every other commit do a pair-wise merge.
while (!sorted.isEmpty()) {
@@ -52,7 +52,7 @@ public class MergeIfNecessary extends SubmitStrategy {
args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo,
args.rw, args.inserter, args.canMergeFlag, args.destBranch,
branchTip, mergedFrom);
mergeTip.moveTipTo(branchTip, mergedFrom.getName());
mergeTip.moveTipTo(branchTip, mergedFrom);
}
final PatchSetApproval submitApproval =

14
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
View File

@@ -68,7 +68,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
// create the branch.
//
n.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
mergeTip.moveTipTo(n, n.getName());
mergeTip.moveTipTo(n, n);
} else if (n.getParentCount() == 0) {
// Refuse to merge a root commit into an existing branch,
@@ -80,7 +80,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
if (args.mergeUtil.canFastForward(args.mergeSorter,
mergeTip.getCurrentTip(), args.rw, n)) {
n.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
mergeTip.moveTipTo(n, n.getName());
mergeTip.moveTipTo(n, n);
} else {
try {
@@ -101,9 +101,9 @@ public class RebaseIfNecessary extends SubmitStrategy {
// rebaseChange.rebase() may already have copied some approvals,
// use upsert, not insert, to avoid constraint violation on database
args.db.patchSetApprovals().upsert(approvals);
mergeTip.moveTipTo((CodeReviewCommit) args.rw.parseCommit(ObjectId
.fromString(newPatchSet.getRevision().get())), newPatchSet
.getRevision().get());
CodeReviewCommit newTip = (CodeReviewCommit) args.rw.parseCommit(
ObjectId.fromString(newPatchSet.getRevision().get()));
mergeTip.moveTipTo(newTip, newTip);
n.change().setCurrentPatchSet(
patchSetInfoFactory.get(mergeTip.getCurrentTip(),
newPatchSet.getId()));
@@ -133,12 +133,12 @@ public class RebaseIfNecessary extends SubmitStrategy {
//
try {
if (args.rw.isMergedInto(mergeTip.getCurrentTip(), n)) {
mergeTip.moveTipTo(n, n.getName());
mergeTip.moveTipTo(n, n);
} else {
mergeTip.moveTipTo(
args.mergeUtil.mergeOneCommit(args.serverIdent.get(),
args.repo, args.rw, args.inserter, args.canMergeFlag,
args.destBranch, mergeTip.getCurrentTip(), n), n.getName());
args.destBranch, mergeTip.getCurrentTip(), n), n);
}
PatchSetApproval submitApproval =
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,