Add integration strategy to merge tickes fast-forward or with commit.
Add the option to merge a ticket branch to the integration branch
only when it can be fast-forwarded, or
always with a merge commit, or
by fast-foward if possible, otherwise with a merge commit.
Adds a new property ticket.mergeType with the valid values
FAST_FOWARD_ONLY, MERGE_ALWAYS and MERGE_IF_NECESSARY.
Merging and canMerge were refactored to make use of a new
IntegrationStrategy class for each type of strategy.
diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties
index 0c7d6cd..208fd99 100644
--- a/src/main/distrib/data/defaults.properties
+++ b/src/main/distrib/data/defaults.properties
@@ -567,6 +567,21 @@
# SINCE 1.4.0
tickets.requireApproval = false
+# Default setting to control how patchsets are merged to the integration branch.
+# Valid values:
+# MERGE_ALWAYS - Always merge with a merge commit. Every ticket will show up as a branch,
+# even if it could have been fast-forward merged. This is the default.
+# MERGE_IF_NECESSARY - If possible, fast-forward the integration branch,
+# if not, merge with a merge commit.
+# FAST_FORWARD_ONLY - Only merge when a fast-forward is possible. This produces a strictly
+# linear history of the integration branch.
+#
+# This setting can be overriden per-repository.
+#
+# RESTART REQUIRED
+# SINCE 1.9.0
+tickets.mergeType = MERGE_ALWAYS
+
# The case-insensitive regular expression used to identify and close tickets on
# push to the integration branch for commits that are NOT already referenced as
# a patchset tip.
diff --git a/src/main/java/com/gitblit/Constants.java b/src/main/java/com/gitblit/Constants.java
index 6232552..321f84f 100644
--- a/src/main/java/com/gitblit/Constants.java
+++ b/src/main/java/com/gitblit/Constants.java
@@ -639,6 +639,37 @@
}
}
+ /**
+ * The type of merge Gitblit will use when merging a ticket to the integration branch.
+ * <p>
+ * The default type is MERGE_ALWAYS.
+ * <p>
+ * This is modeled after the Gerrit SubmitType.
+ */
+ public static enum MergeType {
+ /** Allows a merge only if it can be fast-forward merged into the integration branch. */
+ FAST_FORWARD_ONLY,
+ /** Uses a fast-forward merge if possible, other wise a merge commit is created. */
+ MERGE_IF_NECESSARY,
+ // Future REBASE_IF_NECESSARY,
+ /** Always merge with a merge commit, even when a fast-forward would be possible. */
+ MERGE_ALWAYS,
+ // Future? CHERRY_PICK
+ ;
+
+ public static final MergeType DEFAULT_MERGE_TYPE = MERGE_ALWAYS;
+
+ public static MergeType fromName(String name) {
+ for (MergeType type : values()) {
+ if (type.name().equalsIgnoreCase(name)) {
+ return type;
+ }
+ }
+ return DEFAULT_MERGE_TYPE;
+ }
+ }
+
+
@Documented
@Retention(RetentionPolicy.RUNTIME)
public @interface Unused {
diff --git a/src/main/java/com/gitblit/git/PatchsetReceivePack.java b/src/main/java/com/gitblit/git/PatchsetReceivePack.java
index 33fa470..4a09139 100644
--- a/src/main/java/com/gitblit/git/PatchsetReceivePack.java
+++ b/src/main/java/com/gitblit/git/PatchsetReceivePack.java
@@ -599,7 +599,7 @@
}
// ensure that the patchset can be cleanly merged right now
- MergeStatus status = JGitUtils.canMerge(getRepository(), tipCommit.getName(), forBranch);
+ MergeStatus status = JGitUtils.canMerge(getRepository(), tipCommit.getName(), forBranch, repository.mergeType);
switch (status) {
case ALREADY_MERGED:
sendError("");
@@ -1279,6 +1279,7 @@
getRepository(),
patchset.tip,
ticket.mergeTo,
+ getRepositoryModel().mergeType,
committer,
message);
diff --git a/src/main/java/com/gitblit/manager/RepositoryManager.java b/src/main/java/com/gitblit/manager/RepositoryManager.java
index e9bf5b8..baccfcf 100644
--- a/src/main/java/com/gitblit/manager/RepositoryManager.java
+++ b/src/main/java/com/gitblit/manager/RepositoryManager.java
@@ -63,6 +63,7 @@
import com.gitblit.Constants.AuthorizationControl;
import com.gitblit.Constants.CommitMessageRenderer;
import com.gitblit.Constants.FederationStrategy;
+import com.gitblit.Constants.MergeType;
import com.gitblit.Constants.PermissionType;
import com.gitblit.Constants.RegistrantType;
import com.gitblit.GitBlitException;
@@ -899,6 +900,7 @@
model.acceptNewTickets = getConfig(config, "acceptNewTickets", true);
model.requireApproval = getConfig(config, "requireApproval", settings.getBoolean(Keys.tickets.requireApproval, false));
model.mergeTo = getConfig(config, "mergeTo", null);
+ model.mergeType = MergeType.fromName(getConfig(config, "mergeType", settings.getString(Keys.tickets.mergeType, null)));
model.useIncrementalPushTags = getConfig(config, "useIncrementalPushTags", false);
model.incrementalPushTagPrefix = getConfig(config, "incrementalPushTagPrefix", null);
model.allowForks = getConfig(config, "allowForks", true);
@@ -1557,6 +1559,13 @@
if (!StringUtils.isEmpty(repository.mergeTo)) {
config.setString(Constants.CONFIG_GITBLIT, null, "mergeTo", repository.mergeTo);
}
+ if (repository.mergeType == null || repository.mergeType == MergeType.fromName(settings.getString(Keys.tickets.mergeType, null))) {
+ // use default
+ config.unset(Constants.CONFIG_GITBLIT, null, "mergeType");
+ } else {
+ // override default
+ config.setString(Constants.CONFIG_GITBLIT, null, "mergeType", repository.mergeType.name());
+ }
config.setBoolean(Constants.CONFIG_GITBLIT, null, "useIncrementalPushTags", repository.useIncrementalPushTags);
if (StringUtils.isEmpty(repository.incrementalPushTagPrefix) ||
repository.incrementalPushTagPrefix.equals(settings.getString(Keys.git.defaultIncrementalPushTagPrefix, "r"))) {
diff --git a/src/main/java/com/gitblit/models/RepositoryModel.java b/src/main/java/com/gitblit/models/RepositoryModel.java
index a81c622..67ee1c7 100644
--- a/src/main/java/com/gitblit/models/RepositoryModel.java
+++ b/src/main/java/com/gitblit/models/RepositoryModel.java
@@ -28,6 +28,7 @@
import com.gitblit.Constants.AuthorizationControl;
import com.gitblit.Constants.CommitMessageRenderer;
import com.gitblit.Constants.FederationStrategy;
+import com.gitblit.Constants.MergeType;
import com.gitblit.utils.ArrayUtils;
import com.gitblit.utils.ModelUtils;
import com.gitblit.utils.StringUtils;
@@ -89,6 +90,7 @@
public boolean acceptNewTickets;
public boolean requireApproval;
public String mergeTo;
+ public MergeType mergeType;
public transient boolean isCollectingGarbage;
public Date lastGC;
@@ -111,6 +113,7 @@
this.isBare = true;
this.acceptNewTickets = true;
this.acceptNewPatchsets = true;
+ this.mergeType = MergeType.DEFAULT_MERGE_TYPE;
addOwner(owner);
}
diff --git a/src/main/java/com/gitblit/utils/JGitUtils.java b/src/main/java/com/gitblit/utils/JGitUtils.java
index a02fc3f..0eea1d6 100644
--- a/src/main/java/com/gitblit/utils/JGitUtils.java
+++ b/src/main/java/com/gitblit/utils/JGitUtils.java
@@ -99,6 +99,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.gitblit.Constants.MergeType;
import com.gitblit.GitBlitException;
import com.gitblit.IStoredSettings;
import com.gitblit.Keys;
@@ -2453,44 +2454,13 @@
* @param repository
* @param src
* @param toBranch
+ * @param mergeType
+ * Defines the integration strategy to use for merging.
* @return true if we can merge without conflict
*/
- public static MergeStatus canMerge(Repository repository, String src, String toBranch) {
- RevWalk revWalk = null;
- try {
- revWalk = new RevWalk(repository);
- ObjectId branchId = repository.resolve(toBranch);
- if (branchId == null) {
- return MergeStatus.MISSING_INTEGRATION_BRANCH;
- }
- ObjectId srcId = repository.resolve(src);
- if (srcId == null) {
- return MergeStatus.MISSING_SRC_BRANCH;
- }
- RevCommit branchTip = revWalk.lookupCommit(branchId);
- RevCommit srcTip = revWalk.lookupCommit(srcId);
- if (revWalk.isMergedInto(srcTip, branchTip)) {
- // already merged
- return MergeStatus.ALREADY_MERGED;
- } else if (revWalk.isMergedInto(branchTip, srcTip)) {
- // fast-forward
- return MergeStatus.MERGEABLE;
- }
- RecursiveMerger merger = (RecursiveMerger) MergeStrategy.RECURSIVE.newMerger(repository, true);
- boolean canMerge = merger.merge(branchTip, srcTip);
- if (canMerge) {
- return MergeStatus.MERGEABLE;
- }
- } catch (NullPointerException e) {
- LOGGER.error("Failed to determine canMerge", e);
- } catch (IOException e) {
- LOGGER.error("Failed to determine canMerge", e);
- } finally {
- if (revWalk != null) {
- revWalk.close();
- }
- }
- return MergeStatus.NOT_MERGEABLE;
+ public static MergeStatus canMerge(Repository repository, String src, String toBranch, MergeType mergeType) {
+ IntegrationStrategy strategy = IntegrationStrategyFactory.create(mergeType, repository, src, toBranch);
+ return strategy.canMerge();
}
@@ -2511,11 +2481,13 @@
* @param repository
* @param src
* @param toBranch
+ * @param mergeType
+ * Defines the integration strategy to use for merging.
* @param committer
* @param message
* @return the merge result
*/
- public static MergeResult merge(Repository repository, String src, String toBranch,
+ public static MergeResult merge(Repository repository, String src, String toBranch, MergeType mergeType,
PersonIdent committer, String message) {
if (!toBranch.startsWith(Constants.R_REFS)) {
@@ -2523,15 +2495,202 @@
toBranch = Constants.R_HEADS + toBranch;
}
- RevWalk revWalk = null;
+ IntegrationStrategy strategy = IntegrationStrategyFactory.create(mergeType, repository, src, toBranch);
+ MergeResult mergeResult = strategy.merge(committer, message);
+
+ if (mergeResult.status != MergeStatus.MERGED) {
+ return mergeResult;
+ }
+
try {
- revWalk = new RevWalk(repository);
- RevCommit branchTip = revWalk.lookupCommit(repository.resolve(toBranch));
- RevCommit srcTip = revWalk.lookupCommit(repository.resolve(src));
- if (revWalk.isMergedInto(srcTip, branchTip)) {
- // already merged
- return new MergeResult(MergeStatus.ALREADY_MERGED, null);
+ // Update the integration branch ref
+ RefUpdate mergeRefUpdate = repository.updateRef(toBranch);
+ mergeRefUpdate.setNewObjectId(strategy.getMergeCommit());
+ mergeRefUpdate.setRefLogMessage(strategy.getRefLogMessage(), false);
+ mergeRefUpdate.setExpectedOldObjectId(strategy.branchTip);
+ RefUpdate.Result rc = mergeRefUpdate.update();
+ switch (rc) {
+ case FAST_FORWARD:
+ // successful, clean merge
+ break;
+ default:
+ mergeResult = new MergeResult(MergeStatus.FAILED, null);
+ throw new GitBlitException(MessageFormat.format("Unexpected result \"{0}\" when {1} in {2}",
+ rc.name(), strategy.getOperationMessage(), repository.getDirectory()));
}
+ } catch (IOException e) {
+ LOGGER.error("Failed to merge", e);
+ }
+
+ return mergeResult;
+ }
+
+
+ private static abstract class IntegrationStrategy {
+ Repository repository;
+ String src;
+ String toBranch;
+
+ RevWalk revWalk;
+ RevCommit branchTip;
+ RevCommit srcTip;
+
+ RevCommit mergeCommit;
+ String refLogMessage;
+ String operationMessage;
+
+ RevCommit getMergeCommit() {
+ return mergeCommit;
+ }
+
+ String getRefLogMessage() {
+ return refLogMessage;
+ }
+
+ String getOperationMessage() {
+ return operationMessage;
+ }
+
+ IntegrationStrategy(Repository repository, String src, String toBranch) {
+ this.repository = repository;
+ this.src = src;
+ this.toBranch = toBranch;
+ }
+
+ void prepare() throws IOException {
+ if (revWalk == null) revWalk = new RevWalk(repository);
+ ObjectId branchId = repository.resolve(toBranch);
+ if (branchId != null) {
+ branchTip = revWalk.lookupCommit(branchId);
+ }
+ ObjectId srcId = repository.resolve(src);
+ if (srcId != null) {
+ srcTip = revWalk.lookupCommit(srcId);
+ }
+ }
+
+
+ abstract MergeStatus _canMerge() throws IOException;
+
+
+ MergeStatus canMerge() {
+ try {
+ prepare();
+ if (branchTip == null) {
+ return MergeStatus.MISSING_INTEGRATION_BRANCH;
+ }
+ if (srcTip == null) {
+ return MergeStatus.MISSING_SRC_BRANCH;
+ }
+ if (revWalk.isMergedInto(srcTip, branchTip)) {
+ // already merged
+ return MergeStatus.ALREADY_MERGED;
+ }
+ // determined by specific integration strategy
+ return _canMerge();
+
+ } catch (NullPointerException e) {
+ LOGGER.error("Failed to determine canMerge", e);
+ } catch (IOException e) {
+ LOGGER.error("Failed to determine canMerge", e);
+ } finally {
+ if (revWalk != null) {
+ revWalk.close();
+ }
+ }
+
+ return MergeStatus.NOT_MERGEABLE;
+ }
+
+
+ abstract MergeResult _merge(PersonIdent committer, String message) throws IOException;
+
+
+ MergeResult merge(PersonIdent committer, String message) {
+ try {
+ prepare();
+ if (revWalk.isMergedInto(srcTip, branchTip)) {
+ // already merged
+ return new MergeResult(MergeStatus.ALREADY_MERGED, null);
+ }
+ // determined by specific integration strategy
+ return _merge(committer, message);
+
+ } catch (IOException e) {
+ LOGGER.error("Failed to merge", e);
+ } finally {
+ if (revWalk != null) {
+ revWalk.close();
+ }
+ }
+
+ return new MergeResult(MergeStatus.FAILED, null);
+ }
+ }
+
+
+ private static class FastForwardOnly extends IntegrationStrategy {
+ FastForwardOnly(Repository repository, String src, String toBranch) {
+ super(repository, src, toBranch);
+ }
+
+ @Override
+ MergeStatus _canMerge() throws IOException {
+ if (revWalk.isMergedInto(branchTip, srcTip)) {
+ // fast-forward
+ return MergeStatus.MERGEABLE;
+ }
+
+ return MergeStatus.NOT_MERGEABLE;
+ }
+
+ @Override
+ MergeResult _merge(PersonIdent committer, String message) throws IOException {
+ if (! revWalk.isMergedInto(branchTip, srcTip)) {
+ // is not fast-forward
+ return new MergeResult(MergeStatus.FAILED, null);
+ }
+
+ mergeCommit = srcTip;
+ refLogMessage = "merge " + src + ": Fast-forward";
+ operationMessage = MessageFormat.format("fast-forwarding {0} to commit {1}", srcTip.getName(), branchTip.getName());
+
+ return new MergeResult(MergeStatus.MERGED, srcTip.getName());
+ }
+ }
+
+ private static class MergeIfNecessary extends IntegrationStrategy {
+ MergeIfNecessary(Repository repository, String src, String toBranch) {
+ super(repository, src, toBranch);
+ }
+
+ @Override
+ MergeStatus _canMerge() throws IOException {
+ if (revWalk.isMergedInto(branchTip, srcTip)) {
+ // fast-forward
+ return MergeStatus.MERGEABLE;
+ }
+
+ RecursiveMerger merger = (RecursiveMerger) MergeStrategy.RECURSIVE.newMerger(repository, true);
+ boolean canMerge = merger.merge(branchTip, srcTip);
+ if (canMerge) {
+ return MergeStatus.MERGEABLE;
+ }
+
+ return MergeStatus.NOT_MERGEABLE;
+ }
+
+ @Override
+ MergeResult _merge(PersonIdent committer, String message) throws IOException {
+ if (revWalk.isMergedInto(branchTip, srcTip)) {
+ // fast-forward
+ mergeCommit = srcTip;
+ refLogMessage = "merge " + src + ": Fast-forward";
+ operationMessage = MessageFormat.format("fast-forwarding {0} to commit {1}", branchTip.getName(), srcTip.getName());
+
+ return new MergeResult(MergeStatus.MERGED, srcTip.getName());
+ }
+
RecursiveMerger merger = (RecursiveMerger) MergeStrategy.RECURSIVE.newMerger(repository, true);
boolean merged = merger.merge(branchTip, srcTip);
if (merged) {
@@ -2555,20 +2714,9 @@
ObjectId mergeCommitId = odi.insert(commitBuilder);
odi.flush();
- // set the merge ref to the merge commit
- RevCommit mergeCommit = revWalk.parseCommit(mergeCommitId);
- RefUpdate mergeRefUpdate = repository.updateRef(toBranch);
- mergeRefUpdate.setNewObjectId(mergeCommitId);
- mergeRefUpdate.setRefLogMessage("commit: " + mergeCommit.getShortMessage(), false);
- RefUpdate.Result rc = mergeRefUpdate.update();
- switch (rc) {
- case FAST_FORWARD:
- // successful, clean merge
- break;
- default:
- throw new GitBlitException(MessageFormat.format("Unexpected result \"{0}\" when merging commit {1} into {2} in {3}",
- rc.name(), srcTip.getName(), branchTip.getName(), repository.getDirectory()));
- }
+ mergeCommit = revWalk.parseCommit(mergeCommitId);
+ refLogMessage = "commit: " + mergeCommit.getShortMessage();
+ operationMessage = MessageFormat.format("merging commit {0} into {1}", srcTip.getName(), branchTip.getName());
// return the merge commit id
return new MergeResult(MergeStatus.MERGED, mergeCommitId.getName());
@@ -2576,17 +2724,82 @@
odi.close();
}
}
- } catch (IOException e) {
- LOGGER.error("Failed to merge", e);
- } finally {
- if (revWalk != null) {
- revWalk.close();
- }
+ return new MergeResult(MergeStatus.FAILED, null);
}
- return new MergeResult(MergeStatus.FAILED, null);
}
-
-
+
+ private static class MergeAlways extends IntegrationStrategy {
+ MergeAlways(Repository repository, String src, String toBranch) {
+ super(repository, src, toBranch);
+ }
+
+ @Override
+ MergeStatus _canMerge() throws IOException {
+ RecursiveMerger merger = (RecursiveMerger) MergeStrategy.RECURSIVE.newMerger(repository, true);
+ boolean canMerge = merger.merge(branchTip, srcTip);
+ if (canMerge) {
+ return MergeStatus.MERGEABLE;
+ }
+
+ return MergeStatus.NOT_MERGEABLE;
+ }
+
+ @Override
+ MergeResult _merge(PersonIdent committer, String message) throws IOException {
+ RecursiveMerger merger = (RecursiveMerger) MergeStrategy.RECURSIVE.newMerger(repository, true);
+ boolean merged = merger.merge(branchTip, srcTip);
+ if (merged) {
+ // create a merge commit and a reference to track the merge commit
+ ObjectId treeId = merger.getResultTreeId();
+ ObjectInserter odi = repository.newObjectInserter();
+ try {
+ // Create a commit object
+ CommitBuilder commitBuilder = new CommitBuilder();
+ commitBuilder.setCommitter(committer);
+ commitBuilder.setAuthor(committer);
+ commitBuilder.setEncoding(Constants.CHARSET);
+ if (StringUtils.isEmpty(message)) {
+ message = MessageFormat.format("merge {0} into {1}", srcTip.getName(), branchTip.getName());
+ }
+ commitBuilder.setMessage(message);
+ commitBuilder.setParentIds(branchTip.getId(), srcTip.getId());
+ commitBuilder.setTreeId(treeId);
+
+ // Insert the merge commit into the repository
+ ObjectId mergeCommitId = odi.insert(commitBuilder);
+ odi.flush();
+
+ mergeCommit = revWalk.parseCommit(mergeCommitId);
+ refLogMessage = "commit: " + mergeCommit.getShortMessage();
+ operationMessage = MessageFormat.format("merging commit {0} into {1}", srcTip.getName(), branchTip.getName());
+
+ // return the merge commit id
+ return new MergeResult(MergeStatus.MERGED, mergeCommitId.getName());
+ } finally {
+ odi.close();
+ }
+ }
+
+ return new MergeResult(MergeStatus.FAILED, null);
+ }
+ }
+
+
+ private static class IntegrationStrategyFactory {
+ static IntegrationStrategy create(MergeType mergeType, Repository repository, String src, String toBranch) {
+ switch(mergeType) {
+ case FAST_FORWARD_ONLY:
+ return new FastForwardOnly(repository, src, toBranch);
+ case MERGE_IF_NECESSARY:
+ return new MergeIfNecessary(repository, src, toBranch);
+ case MERGE_ALWAYS:
+ return new MergeAlways(repository, src, toBranch);
+ }
+ return null;
+ }
+ }
+
+
/**
* Returns the LFS URL for the given oid
* Currently assumes that the Gitblit Filestore is used
diff --git a/src/main/java/com/gitblit/wicket/pages/TicketPage.java b/src/main/java/com/gitblit/wicket/pages/TicketPage.java
index cd049f4..e213396 100644
--- a/src/main/java/com/gitblit/wicket/pages/TicketPage.java
+++ b/src/main/java/com/gitblit/wicket/pages/TicketPage.java
@@ -1405,14 +1405,14 @@
boolean allowMerge;
if (repository.requireApproval) {
- // rpeository requires approval
+ // repository requires approval
allowMerge = ticket.isOpen() && ticket.isApproved(patchset);
} else {
- // vetos are binding
+ // vetoes are binding
allowMerge = ticket.isOpen() && !ticket.isVetoed(patchset);
}
- MergeStatus mergeStatus = JGitUtils.canMerge(getRepository(), patchset.tip, ticket.mergeTo);
+ MergeStatus mergeStatus = JGitUtils.canMerge(getRepository(), patchset.tip, ticket.mergeTo, repository.mergeType);
if (allowMerge) {
if (MergeStatus.MERGEABLE == mergeStatus) {
// patchset can be cleanly merged to integration branch OR has already been merged
