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