Do not modify git repository when checking if a change is mergeable

If testing of mergeability of changes is enabled (see changeMerge.test
config parameter) the merge algorithm is executed without updating
the destination branch and the change status. However running the merge
algorithm creates new objects and even commits in the git repository.
The created objects/commits are never used and it's just waste to
create them and to have them.

Rewrite the mergeability test so that it is a read-only operation and
the git repository is not modified.

If CHERRY_PICK is chosen as submit type the merge algorithm also
creates a new patch set for the commit that was created by the cherry
pick. When testing the mergeability creating the new patch set
currently fails with an NullPointerException since the code trys to set
the submitter as uploader of the new patch set, but there is no
submitter when the mergeability is tested.

Change-Id: I613f45e7d173b33db3e400fba461bb94c86320ec
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
This commit is contained in:
Edwin Kempin
2012-09-06 15:16:15 +02:00
parent ba02b61480
commit a085d501c0
7 changed files with 175 additions and 23 deletions

View File

@@ -14,7 +14,10 @@
package com.google.gerrit.server.git; package com.google.gerrit.server.git;
import static com.google.gerrit.server.git.MergeUtil.canFastForward;
import static com.google.gerrit.server.git.MergeUtil.canMerge;
import static com.google.gerrit.server.git.MergeUtil.commit; import static com.google.gerrit.server.git.MergeUtil.commit;
import static com.google.gerrit.server.git.MergeUtil.createDryRunInserter;
import static com.google.gerrit.server.git.MergeUtil.hasMissingDependencies; import static com.google.gerrit.server.git.MergeUtil.hasMissingDependencies;
import static com.google.gerrit.server.git.MergeUtil.markCleanMerges; import static com.google.gerrit.server.git.MergeUtil.markCleanMerges;
import static com.google.gerrit.server.git.MergeUtil.mergeOneCommit; import static com.google.gerrit.server.git.MergeUtil.mergeOneCommit;
@@ -163,6 +166,54 @@ public class CherryPick extends SubmitStrategy {
return newCommits; return newCommits;
} }
@Override
public boolean dryRun(final CodeReviewCommit mergeTip,
final CodeReviewCommit toMerge) throws MergeException {
return canCherryPick(mergeTip, toMerge);
}
private boolean canCherryPick(final CodeReviewCommit mergeTip,
final CodeReviewCommit toMerge) throws MergeException {
if (mergeTip == null) {
// The branch is unborn. Fast-forward is possible.
//
return true;
}
if (toMerge.getParentCount() == 0) {
// Refuse to merge a root commit into an existing branch,
// we cannot obtain a delta for the cherry-pick to apply.
//
return false;
}
if (toMerge.getParentCount() == 1) {
// If there is only one parent, a cherry-pick can be done by
// taking the delta relative to that one parent and redoing
// that on the current merge tip.
//
try {
final ThreeWayMerger m =
newThreeWayMerger(args.repo, createDryRunInserter(),
args.useContentMerge);
m.setBase(toMerge.getParent(0));
return m.merge(mergeTip, toMerge);
} catch (IOException e) {
throw new MergeException("Cannot merge " + toMerge.name(), e);
}
}
// There are multiple parents, so this is a merge commit. We
// don't want to cherry-pick the merge as clients can't easily
// rebase their history with that merge present and replaced
// by an equivalent merge with a different first parent. So
// instead behave as though MERGE_IF_NECESSARY was configured.
//
return canFastForward(args.mergeSorter, mergeTip, args.rw, toMerge)
|| canMerge(args.mergeSorter, args.repo, args.useContentMerge,
mergeTip, toMerge);
}
private CodeReviewCommit writeCherryPickCommit(final Merger m, private CodeReviewCommit writeCherryPickCommit(final Merger m,
final CodeReviewCommit mergeTip, final CodeReviewCommit n) final CodeReviewCommit mergeTip, final CodeReviewCommit n)
throws IOException, OrmException { throws IOException, OrmException {

View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.git; package com.google.gerrit.server.git;
import static com.google.gerrit.server.git.MergeUtil.canFastForward;
import static com.google.gerrit.server.git.MergeUtil.getFirstFastForward; import static com.google.gerrit.server.git.MergeUtil.getFirstFastForward;
import static com.google.gerrit.server.git.MergeUtil.markCleanMerges; import static com.google.gerrit.server.git.MergeUtil.markCleanMerges;
import static com.google.gerrit.server.git.MergeUtil.reduceToMinimalMerge; import static com.google.gerrit.server.git.MergeUtil.reduceToMinimalMerge;
@@ -52,4 +53,9 @@ public class FastForwardOnly extends SubmitStrategy {
public boolean retryOnLockFailure() { public boolean retryOnLockFailure() {
return false; return false;
} }
public boolean dryRun(final CodeReviewCommit mergeTip,
final CodeReviewCommit toMerge) throws MergeException {
return canFastForward(args.mergeSorter, mergeTip, args.rw, toMerge);
}
} }

View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.git; package com.google.gerrit.server.git;
import static com.google.gerrit.server.git.MergeUtil.markCleanMerges; import static com.google.gerrit.server.git.MergeUtil.markCleanMerges;
import static com.google.gerrit.server.git.MergeUtil.canMerge;
import static com.google.gerrit.server.git.MergeUtil.mergeOneCommit; import static com.google.gerrit.server.git.MergeUtil.mergeOneCommit;
import static com.google.gerrit.server.git.MergeUtil.reduceToMinimalMerge; import static com.google.gerrit.server.git.MergeUtil.reduceToMinimalMerge;
@@ -49,4 +50,11 @@ public class MergeAlways extends SubmitStrategy {
return newMergeTip; return newMergeTip;
} }
@Override
public boolean dryRun(final CodeReviewCommit mergeTip,
final CodeReviewCommit toMerge) throws MergeException {
return canMerge(args.mergeSorter, args.repo, args.useContentMerge,
mergeTip, toMerge);
}
} }

View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server.git; package com.google.gerrit.server.git;
import static com.google.gerrit.server.git.MergeUtil.canFastForward;
import static com.google.gerrit.server.git.MergeUtil.canMerge;
import static com.google.gerrit.server.git.MergeUtil.getFirstFastForward; import static com.google.gerrit.server.git.MergeUtil.getFirstFastForward;
import static com.google.gerrit.server.git.MergeUtil.markCleanMerges; import static com.google.gerrit.server.git.MergeUtil.markCleanMerges;
import static com.google.gerrit.server.git.MergeUtil.mergeOneCommit; import static com.google.gerrit.server.git.MergeUtil.mergeOneCommit;
@@ -52,4 +54,12 @@ public class MergeIfNecessary extends SubmitStrategy {
return newMergeTip; return newMergeTip;
} }
@Override
public boolean dryRun(final CodeReviewCommit mergeTip,
final CodeReviewCommit toMerge) throws MergeException {
return canFastForward(args.mergeSorter, mergeTip, args.rw, toMerge)
|| canMerge(args.mergeSorter, args.repo, args.useContentMerge,
mergeTip, toMerge);
}
} }

View File

@@ -86,6 +86,7 @@ import java.util.HashSet;
import java.util.Iterator; import java.util.Iterator;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Map.Entry;
import java.util.Set; import java.util.Set;
/** /**
@@ -212,8 +213,11 @@ public class MergeOp {
openSchema(); openSchema();
openBranch(); openBranch();
validateChangeList(Collections.singletonList(change)); validateChangeList(Collections.singletonList(change));
final SubmitType submitType = toMerge.keySet().iterator().next(); if (!toMerge.isEmpty()) {
preMerge(createStrategy(submitType), toMerge.get(submitType)); final Entry<SubmitType, CodeReviewCommit> e =
toMerge.entries().iterator().next();
final boolean isMergeable =
createStrategy(e.getKey()).dryRun(branchTip, e.getValue());
// update sha1 tested merge. // update sha1 tested merge.
if (destBranchRef != null) { if (destBranchRef != null) {
@@ -222,8 +226,12 @@ public class MergeOp {
} else { } else {
change.setLastSha1MergeTested(new RevId("")); change.setLastSha1MergeTested(new RevId(""));
} }
change.setMergeable(isMergeable(change)); change.setMergeable(isMergeable);
db.changes().update(Collections.singleton(change)); db.changes().update(Collections.singleton(change));
} else {
log.error("Test merge attempt for change: " + change.getId()
+ " failed");
}
} }
} catch (MergeException e) { } catch (MergeException e) {
log.error("Test merge attempt for change: " + change.getId() log.error("Test merge attempt for change: " + change.getId()
@@ -654,20 +662,6 @@ public class MergeOp {
} }
} }
private boolean isMergeable(Change c) {
final CodeReviewCommit commit = commits.get(c.getId());
final CommitMergeStatus s = commit != null ? commit.statusCode : null;
boolean isMergeable = false;
if (s != null
&& (s.equals(CommitMergeStatus.CLEAN_MERGE)
|| s.equals(CommitMergeStatus.CLEAN_PICK) || s
.equals(CommitMergeStatus.ALREADY_MERGED))) {
isMergeable = true;
}
return isMergeable;
}
private void updateChangeStatus(final List<Change> submitted) { private void updateChangeStatus(final List<Change> submitted) {
List<CodeReviewCommit> merged = new ArrayList<CodeReviewCommit>(); List<CodeReviewCommit> merged = new ArrayList<CodeReviewCommit>();

View File

@@ -27,6 +27,7 @@ import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.MutableObjectId;
import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.PersonIdent;
@@ -37,10 +38,12 @@ import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevFlag; import org.eclipse.jgit.revwalk.RevFlag;
import org.eclipse.jgit.revwalk.RevSort; import org.eclipse.jgit.revwalk.RevSort;
import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.PackParser;
import org.slf4j.Logger; import org.slf4j.Logger;
import org.slf4j.LoggerFactory; import org.slf4j.LoggerFactory;
import java.io.IOException; import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException; import java.io.UnsupportedEncodingException;
import java.sql.Timestamp; import java.sql.Timestamp;
import java.util.ArrayList; import java.util.ArrayList;
@@ -167,6 +170,40 @@ public class MergeUtil {
return authorIdent; return authorIdent;
} }
public static boolean canMerge(final MergeSorter mergeSorter,
final Repository repo, final boolean useContentMerge,
final CodeReviewCommit mergeTip, final CodeReviewCommit toMerge)
throws MergeException {
if (hasMissingDependencies(mergeSorter, toMerge)) {
return false;
}
final ThreeWayMerger m =
newThreeWayMerger(repo, createDryRunInserter(), useContentMerge);
try {
return m.merge(new AnyObjectId[] {mergeTip, toMerge});
} catch (IOException e) {
if (e.getMessage().startsWith("Multiple merge bases for")) {
return false;
}
throw new MergeException("Cannot merge " + toMerge.name(), e);
}
}
public static boolean canFastForward(final MergeSorter mergeSorter,
final CodeReviewCommit mergeTip, final RevWalk rw,
final CodeReviewCommit toMerge) throws MergeException {
if (hasMissingDependencies(mergeSorter, toMerge)) {
return false;
}
try {
return mergeTip == null || rw.isMergedInto(mergeTip, toMerge);
} catch (IOException e) {
throw new MergeException("Cannot fast-forward test during merge", e);
}
}
public static boolean hasMissingDependencies(final MergeSorter mergeSorter, public static boolean hasMissingDependencies(final MergeSorter mergeSorter,
final CodeReviewCommit toMerge) throws MergeException { final CodeReviewCommit toMerge) throws MergeException {
try { try {
@@ -176,6 +213,36 @@ public class MergeUtil {
} }
} }
public static ObjectInserter createDryRunInserter() {
return new ObjectInserter() {
private final MutableObjectId buf = new MutableObjectId();
private final static int LAST_BYTE = Constants.OBJECT_ID_LENGTH - 1;
@Override
public ObjectId insert(int objectType, long length, InputStream in)
throws IOException {
// create non-existing dummy ID
buf.setByte(LAST_BYTE, buf.getByte(LAST_BYTE) + 1);
return buf.copy();
}
@Override
public PackParser newPackParser(InputStream in) throws IOException {
throw new UnsupportedOperationException();
}
@Override
public void flush() throws IOException {
// Do nothing.
}
@Override
public void release() {
// Do nothing.
}
};
}
public static CodeReviewCommit mergeOneCommit(final ReviewDb reviewDb, public static CodeReviewCommit mergeOneCommit(final ReviewDb reviewDb,
final IdentifiedUser.GenericFactory identifiedUserFactory, final IdentifiedUser.GenericFactory identifiedUserFactory,
final PersonIdent myIdent, final Repository repo, final RevWalk rw, final PersonIdent myIdent, final Repository repo, final RevWalk rw,

View File

@@ -112,6 +112,22 @@ public abstract class SubmitStrategy {
protected abstract CodeReviewCommit _run(CodeReviewCommit mergeTip, protected abstract CodeReviewCommit _run(CodeReviewCommit mergeTip,
List<CodeReviewCommit> toMerge) throws MergeException; List<CodeReviewCommit> toMerge) throws MergeException;
/**
* Checks whether the given commit can be merged.
*
* Subclasses must ensure that invoking this method does neither modify the
* git repository nor the Gerrit database.
*
* @param mergeTip the mergeTip
* @param toMerge the commit for which it should be checked whether it can be
* merged or not
* @return <code>true</code> if the given commit can be merged, otherwise
* <code>false</code>
* @throws MergeException
*/
public abstract boolean dryRun(CodeReviewCommit mergeTip,
CodeReviewCommit toMerge) throws MergeException;
/** /**
* Returns the PersonIdent that should be used for the ref log entries when * Returns the PersonIdent that should be used for the ref log entries when
* updating the destination branch. The ref log identity may be set after the * updating the destination branch. The ref log identity may be set after the