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/gitblit.properties b/src/main/distrib/data/gitblit.properties index f8d6c6d..b29b1c7 100644 --- a/src/main/distrib/data/gitblit.properties +++ b/src/main/distrib/data/gitblit.properties
@@ -579,6 +579,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.7.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 279d3c9..1b1c24c 100644 --- a/src/main/java/com/gitblit/Constants.java +++ b/src/main/java/com/gitblit/Constants.java
@@ -609,6 +609,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 9e55524..7d81e61 100644 --- a/src/main/java/com/gitblit/git/PatchsetReceivePack.java +++ b/src/main/java/com/gitblit/git/PatchsetReceivePack.java
@@ -574,7 +574,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(""); @@ -1180,6 +1180,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 6a22db5..b967030 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; @@ -878,6 +879,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); @@ -1519,6 +1521,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 da51ea9..7d7ef6d 100644 --- a/src/main/java/com/gitblit/utils/JGitUtils.java +++ b/src/main/java/com/gitblit/utils/JGitUtils.java
@@ -84,6 +84,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.gitblit.Constants.MergeType; import com.gitblit.GitBlitException; import com.gitblit.models.GitNote; import com.gitblit.models.PathModel; @@ -2272,42 +2273,22 @@ public static enum MergeStatus { NOT_MERGEABLE, FAILED, ALREADY_MERGED, MERGEABLE, MERGED; } - + + /** * Determines if we can cleanly merge one branch into another. Returns true * if we can merge without conflict, otherwise returns false. * * @param repository * @param src - * @param toBranch + * @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); - RevCommit branchTip = revWalk.lookupCommit(repository.resolve(toBranch)); - RevCommit srcTip = revWalk.lookupCommit(repository.resolve(src)); - 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 (IOException e) { - LOGGER.error("Failed to determine canMerge", e); - } finally { - if (revWalk != null) { - revWalk.release(); - } - } - 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(); } @@ -2327,79 +2308,307 @@ * * @param repository * @param src - * @param toBranch + * @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)) { // branch ref doesn't start with ref, assume this is a branch head toBranch = Constants.R_HEADS + toBranch; } - - RevWalk revWalk = null; - 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); - } - 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(); - - // 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())); - } - - // return the merge commit id - return new MergeResult(MergeStatus.MERGED, mergeCommitId.getName()); - } finally { - odi.release(); - } - } - } catch (IOException e) { - LOGGER.error("Failed to merge", e); - } finally { - if (revWalk != null) { - revWalk.release(); - } - } - return new MergeResult(MergeStatus.FAILED, null); + + IntegrationStrategy strategy = IntegrationStrategyFactory.create(mergeType, repository, src, toBranch); + MergeResult mergeResult = strategy.merge(committer, message); + + if (mergeResult.status != MergeStatus.MERGED) { + return mergeResult; + } + + try { + // 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); + branchTip = revWalk.lookupCommit(repository.resolve(toBranch)); + srcTip = revWalk.lookupCommit(repository.resolve(src)); + } + + + abstract MergeStatus _canMerge() throws IOException; + + + MergeStatus canMerge() { + try { + prepare(); + if (revWalk.isMergedInto(srcTip, branchTip)) { + // already merged + return MergeStatus.ALREADY_MERGED; + } + // determined by specific integration strategy + return _canMerge(); + + } catch (IOException e) { + LOGGER.error("Failed to determine canMerge", e); + } finally { + if (revWalk != null) { + revWalk.release(); + } + } + + 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.release(); + } + } + + 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"; + 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"; + 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) { + // 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(); + 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.release(); + } + } + 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(); + 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.release(); + } + } + + 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; + } + } }
diff --git a/src/main/java/com/gitblit/wicket/pages/TicketPage.java b/src/main/java/com/gitblit/wicket/pages/TicketPage.java index ca1bf31..9360766 100644 --- a/src/main/java/com/gitblit/wicket/pages/TicketPage.java +++ b/src/main/java/com/gitblit/wicket/pages/TicketPage.java
@@ -1349,14 +1349,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