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.

7 files changed, 341 insertions, 69 deletions

diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties
index 0c7d6cd4..208fd992 100644
--- a/src/main/distrib/data/defaults.properties
+++ b/src/main/distrib/data/defaults.properties
@@ -567,6 +567,21 @@ tickets.acceptNewPatchsets = true
 # 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 6232552e..321f84f9 100644
--- a/src/main/java/com/gitblit/Constants.java
+++ b/src/main/java/com/gitblit/Constants.java
@@ -639,6 +639,37 @@ public class Constants {
 		}

 	}

 

+	/**

+	 * 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 33fa4705..4a09139a 100644
--- a/src/main/java/com/gitblit/git/PatchsetReceivePack.java
+++ b/src/main/java/com/gitblit/git/PatchsetReceivePack.java
@@ -599,7 +599,7 @@ public class PatchsetReceivePack extends GitblitReceivePack {
 		}

 

 		// 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 @@ public class PatchsetReceivePack extends GitblitReceivePack {
 				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 e9bf5b84..baccfcfa 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.AccessRestrictionType;
 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 @@ public class RepositoryManager implements IRepositoryManager {
 			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 @@ public class RepositoryManager implements IRepositoryManager {
 		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 a81c622a..67ee1c7e 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.AccessRestrictionType;
 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 class RepositoryModel implements Serializable, Comparable<RepositoryModel
 	public boolean acceptNewTickets;

 	public boolean requireApproval;
 	public String mergeTo;

+	public MergeType mergeType;

 

 	public transient boolean isCollectingGarbage;

 	public Date lastGC;

@@ -111,6 +113,7 @@ public class RepositoryModel implements Serializable, Comparable<RepositoryModel
 		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 a02fc3ff..0eea1d61 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.jetbrains.annotations.NotNull;
 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 @@ public class JGitUtils {
 	 * @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 @@ public class JGitUtils {
 	 * @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 @@ public class JGitUtils {
 			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,64 @@ public class JGitUtils {
 					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());

+				} finally {

+					odi.close();

+				}

+			}

+			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());

@@ -2576,17 +2779,27 @@ public class JGitUtils {
 					odi.close();

 				}

 			}

-		} catch (IOException e) {

-			LOGGER.error("Failed to merge", e);

-		} finally {

-			if (revWalk != null) {

-				revWalk.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;

 		}

-		return new MergeResult(MergeStatus.FAILED, 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 cd049f4d..e2133966 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 @@ public class TicketPage extends RepositoryPage {
 

 		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