refactor after review

scm-core/src/main/java/sonia/scm/repository/api/MergeCommandBuilder.java

@@ -120,6 +120,9 @@ public class MergeCommandBuilder {
    * @return This builder instance.
    */
   public MergeCommandBuilder setMergeStrategy(MergeStrategy strategy) {
+    if (!mergeCommand.isSupported(strategy)) {
+      throw new IllegalArgumentException("merge strategy not supported: " + strategy);
+    }
     request.setMergeStrategy(strategy);
     return this;
   }

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitFastForwardIfPossible.java

@@ -10,35 +10,27 @@ import java.io.IOException;
 
 class GitFastForwardIfPossible extends GitMergeStrategy {
 
+  private GitMergeStrategy fallbackMerge;
+
   GitFastForwardIfPossible(Git clone, MergeCommandRequest request, GitContext context, Repository repository) {
     super(clone, request, context, repository);
+    fallbackMerge = new GitMergeCommit(clone, request, context, repository);
   }
 
   @Override
   MergeCommandResult run() throws IOException {
-    MergeResult fastForwardResult = mergeWithFastForwardMode(MergeCommand.FastForwardMode.FF_ONLY);
+    MergeResult fastForwardResult = mergeWithFastForwardOnlyMode();
     if (fastForwardResult.getMergeStatus().isSuccessful()) {
       push();
       return MergeCommandResult.success();
     } else {
-      return mergeWithCommit();
+      return fallbackMerge.run();
     }
   }
 
-  private MergeCommandResult mergeWithCommit() throws IOException {
+  private MergeResult mergeWithFastForwardOnlyMode() throws IOException {
-    MergeResult mergeCommitResult = mergeWithFastForwardMode(MergeCommand.FastForwardMode.NO_FF);
-    if (mergeCommitResult.getMergeStatus().isSuccessful()) {
-      doCommit();
-      push();
-      return MergeCommandResult.success();
-    } else {
-      return analyseFailure(mergeCommitResult);
-    }
-  }
-
-  private MergeResult mergeWithFastForwardMode(MergeCommand.FastForwardMode fastForwardMode) throws IOException {
     MergeCommand mergeCommand = getClone().merge();
-    mergeCommand.setFastForward(fastForwardMode);
+    mergeCommand.setFastForward(MergeCommand.FastForwardMode.FF_ONLY);
     return doMergeInClone(mergeCommand);
   }
 }

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeCommand.java

@@ -12,6 +12,8 @@ import sonia.scm.repository.api.MergeStrategy;
 import java.io.IOException;
 import java.util.Set;
 
+import static org.eclipse.jgit.merge.MergeStrategy.RECURSIVE;
+
 public class GitMergeCommand extends AbstractGitCommand implements MergeCommand {
 
   private final GitWorkdirFactory workdirFactory;
@@ -33,19 +35,26 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand
   }
 
   private MergeCommandResult mergeWithStrategy(MergeCommandRequest request) {
-    if (request.getMergeStrategy() == MergeStrategy.SQUASH) {
+    switch(request.getMergeStrategy()) {
+      case SQUASH:
         return inClone(clone -> new GitMergeWithSquash(clone, request, context, repository), workdirFactory, request.getTargetBranch());
-    } else if (request.getMergeStrategy() == MergeStrategy.FAST_FORWARD_IF_POSSIBLE) {
+
+      case FAST_FORWARD_IF_POSSIBLE:
         return inClone(clone -> new GitFastForwardIfPossible(clone, request, context, repository), workdirFactory, request.getTargetBranch());
-    }
+
+      case MERGE_COMMIT:
         return inClone(clone -> new GitMergeCommit(clone, request, context, repository), workdirFactory, request.getTargetBranch());
+
+      default:
+        throw new IllegalArgumentException("unknown merge strategy: " + request.getMergeStrategy());
+    }
   }
 
   @Override
   public MergeDryRunCommandResult dryRun(MergeCommandRequest request) {
     try {
       Repository repository = context.open();
-      ResolveMerger merger = (ResolveMerger) org.eclipse.jgit.merge.MergeStrategy.RECURSIVE.newMerger(repository, true);
+      ResolveMerger merger = (ResolveMerger) RECURSIVE.newMerger(repository, true);
       return new MergeDryRunCommandResult(
         merger.merge(
           resolveRevisionOrThrowNotFound(repository, request.getBranchToMerge()),

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeCommit.java

@@ -16,7 +16,7 @@ class GitMergeCommit extends GitMergeStrategy {
 
   @Override
   MergeCommandResult run() throws IOException {
-    org.eclipse.jgit.api.MergeCommand mergeCommand = getClone().merge();
+    MergeCommand mergeCommand = getClone().merge();
     mergeCommand.setFastForward(MergeCommand.FastForwardMode.NO_FF);
     MergeResult result = doMergeInClone(mergeCommand);
 

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeWithSquash.java

@@ -4,6 +4,7 @@ import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.MergeResult;
 import sonia.scm.repository.Repository;
 import sonia.scm.repository.api.MergeCommandResult;
+import org.eclipse.jgit.api.MergeCommand;
 
 import java.io.IOException;
 
@@ -15,7 +16,7 @@ class GitMergeWithSquash extends GitMergeStrategy {
 
   @Override
   MergeCommandResult run() throws IOException {
-    org.eclipse.jgit.api.MergeCommand mergeCommand = getClone().merge();
+   MergeCommand mergeCommand = getClone().merge();
     mergeCommand.setSquash(true);
     MergeResult result = doMergeInClone(mergeCommand);
 

scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMergeCommandTest.java

@@ -287,6 +287,29 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
     assertThat(mergeCommit.getId()).isEqualTo(featureBranchHead);
   }
 
+  @Test
+  public void shouldDoMergeCommitIfFastForwardIsNotPossible() throws IOException, GitAPIException {
+    GitMergeCommand command = createCommand();
+    MergeCommandRequest request = new MergeCommandRequest();
+    request.setTargetBranch("master");
+    request.setBranchToMerge("mergeable");
+    request.setMergeStrategy(MergeStrategy.FAST_FORWARD_IF_POSSIBLE);
+    request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det"));
+
+    MergeCommandResult mergeCommandResult = command.merge(request);
+
+    assertThat(mergeCommandResult.isSuccess()).isTrue();
+
+    Repository repository = createContext().open();
+    Iterable<RevCommit> commits = new Git(repository).log().add(repository.resolve("master")).setMaxCount(1).call();
+    RevCommit mergeCommit = commits.iterator().next();
+    PersonIdent mergeAuthor = mergeCommit.getAuthorIdent();
+    String message = mergeCommit.getFullMessage();
+    assertThat(mergeAuthor.getName()).isEqualTo("Dirk Gently");
+    assertThat(mergeAuthor.getEmailAddress()).isEqualTo("dirk@holistic.det");
+    assertThat(message).contains("master", "mergeable");
+  }
+
   @Test(expected = NotFoundException.class)
   public void shouldHandleNotExistingSourceBranchInMerge() {
     GitMergeCommand command = createCommand();