From 33037385e4a5fc33ad43074f7ea303acbfbdf60d Mon Sep 17 00:00:00 2001
From: Rene Pfeuffer <rene.pfeuffer@cloudogu.com>
Date: Thu, 27 Feb 2020 09:56:25 +0100
Subject: [PATCH] Handle unexpected merge results

It is possible that a git work tree is dirty directly after the clone
of a repository, eg. when files are not changed correctly due to bogous
.gitattribute files (though this is just a guess). In these cases a
merge might fail due to these dirty files and not due to merge
conflicts. Without this change such results lead to null pointer
exceptions, because result.getConflicts() is null.
---
 .../scm/repository/spi/GitMergeStrategy.java  |  9 +++-
 .../spi/UnexpectedMergeResultException.java   | 27 ++++++++++
 .../spi/AbstractGitCommandTestBase.java       |  1 -
 .../repository/spi/GitMergeCommandTest.java   | 49 ++++++++++++++++++-
 .../main/resources/locales/de/plugins.json    |  4 ++
 .../main/resources/locales/en/plugins.json    |  4 ++
 6 files changed, 90 insertions(+), 4 deletions(-)
 create mode 100644 scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/UnexpectedMergeResultException.java

diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeStrategy.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeStrategy.java
index 8e9f79d1b9..4b8280c669 100644
--- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeStrategy.java
+++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeStrategy.java
@@ -88,7 +88,14 @@ abstract class GitMergeStrategy extends AbstractGitCommand.GitCloneWorker<MergeC
   }
 
   MergeCommandResult analyseFailure(MergeResult result) {
-    logger.info("could not merge branch {} into {} due to conflict in paths {}", branchToMerge, targetBranch, result.getConflicts().keySet());
+    logger.info("could not merge branch {} into {} with merge status '{}' due to ...", branchToMerge, targetBranch, result.getMergeStatus());
+    logger.info("... conflicts: {}", result.getConflicts());
+    logger.info("... checkout conflicts: {}", result.getCheckoutConflicts());
+    logger.info("... failing paths: {}", result.getFailingPaths());
+    logger.info("... message: {}", result);
+    if (result.getConflicts() == null) {
+      throw new UnexpectedMergeResultException(getRepository(), result);
+    }
     return MergeCommandResult.failure(targetRevision.name(), revisionToMerge.name(), result.getConflicts().keySet());
   }
 }
diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/UnexpectedMergeResultException.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/UnexpectedMergeResultException.java
new file mode 100644
index 0000000000..7fea7b39e0
--- /dev/null
+++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/UnexpectedMergeResultException.java
@@ -0,0 +1,27 @@
+package sonia.scm.repository.spi;
+
+import org.eclipse.jgit.api.MergeResult;
+import sonia.scm.ContextEntry;
+import sonia.scm.ExceptionWithContext;
+import sonia.scm.repository.Repository;
+
+class UnexpectedMergeResultException extends ExceptionWithContext {
+
+  public static final String CODE = "4GRrgkSC01";
+
+  public UnexpectedMergeResultException(Repository repository, MergeResult result) {
+    super(ContextEntry.ContextBuilder.entity(repository).build(), createMessage(result));
+  }
+
+  private static String createMessage(MergeResult result) {
+    return "unexpected merge result: " + result
+      + "\nconflicts: " + result.getConflicts()
+      + "\ncheckout conflicts: " + result.getCheckoutConflicts()
+      + "\nfailing paths: " + result.getFailingPaths();
+  }
+
+  @Override
+  public String getCode() {
+    return CODE;
+  }
+}
diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/AbstractGitCommandTestBase.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/AbstractGitCommandTestBase.java
index 8c4b682b18..2d654ae30c 100644
--- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/AbstractGitCommandTestBase.java
+++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/AbstractGitCommandTestBase.java
@@ -113,5 +113,4 @@ public class AbstractGitCommandTestBase extends ZippedRepositoryTestBase
 
   /** Field description */
   private GitContext context;
-  private ScmTransportProtocol scmTransportProtocol;
 }
diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMergeCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMergeCommandTest.java
index 2616d5b6e5..8699533c00 100644
--- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMergeCommandTest.java
+++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitMergeCommandTest.java
@@ -12,16 +12,23 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.jupiter.api.Assertions;
 import sonia.scm.NoChangesMadeException;
 import sonia.scm.NotFoundException;
+import sonia.scm.repository.GitWorkdirFactory;
 import sonia.scm.repository.Person;
 import sonia.scm.repository.api.MergeCommandResult;
 import sonia.scm.repository.api.MergeStrategy;
 import sonia.scm.repository.util.WorkdirProvider;
 import sonia.scm.user.User;
 
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
 import java.io.IOException;
 import java.util.List;
+import java.util.function.Consumer;
+import java.util.function.Function;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -163,11 +170,35 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
     assertThat(mergeCommandResult.getFilesWithConflict()).containsExactly("a.txt");
   }
 
+  @Test
+  public void shouldHandleUnexpectedMergeResults() {
+    GitMergeCommand command = createCommand(git -> {
+      FileWriter fw = null;
+      try {
+        fw = new FileWriter(new File(git.getRepository().getWorkTree(), "b.txt"), true);
+        BufferedWriter bw = new BufferedWriter(fw);
+        bw.write("change");
+        bw.newLine();
+        bw.close();
+      } catch (IOException e) {
+        e.printStackTrace();
+      }
+    });
+    MergeCommandRequest request = new MergeCommandRequest();
+    request.setBranchToMerge("mergeable");
+    request.setTargetBranch("master");
+    request.setMergeStrategy(MergeStrategy.MERGE_COMMIT);
+    request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det"));
+    request.setMessageTemplate("simple");
+
+    Assertions.assertThrows(UnexpectedMergeResultException.class, () -> command.merge(request));
+  }
+
   @Test
   public void shouldTakeAuthorFromSubjectIfNotSet() throws IOException, GitAPIException {
     SimplePrincipalCollection principals = new SimplePrincipalCollection();
     principals.add("admin", REALM);
-    principals.add( new User("dirk", "Dirk Gently", "dirk@holistic.det"), REALM);
+    principals.add(new User("dirk", "Dirk Gently", "dirk@holistic.det"), REALM);
     shiro.setSubject(
       new Subject.Builder()
         .principals(principals)
@@ -364,6 +395,20 @@ public class GitMergeCommandTest extends AbstractGitCommandTestBase {
   }
 
   private GitMergeCommand createCommand() {
-    return new GitMergeCommand(createContext(), repository, new SimpleGitWorkdirFactory(new WorkdirProvider()));
+    return createCommand(git -> {
+    });
+  }
+
+  private GitMergeCommand createCommand(Consumer<Git> interceptor) {
+    return new GitMergeCommand(createContext(), repository, new SimpleGitWorkdirFactory(new WorkdirProvider())) {
+      @Override
+      <R, W extends GitCloneWorker<R>> R inClone(Function<Git, W> workerSupplier, GitWorkdirFactory workdirFactory, String initialBranch) {
+        Function<Git, W> interceptedWorkerSupplier = git -> {
+          interceptor.accept(git);
+          return workerSupplier.apply(git);
+        };
+        return super.inClone(interceptedWorkerSupplier, workdirFactory, initialBranch);
+      }
+    };
   }
 }
diff --git a/scm-webapp/src/main/resources/locales/de/plugins.json b/scm-webapp/src/main/resources/locales/de/plugins.json
index 95179a94e2..809da9d0df 100644
--- a/scm-webapp/src/main/resources/locales/de/plugins.json
+++ b/scm-webapp/src/main/resources/locales/de/plugins.json
@@ -199,6 +199,10 @@
     "8LRncum0S1": {
       "displayName": "Interner Fehler im Repository",
       "description": "Bei der Bearbeitung des internen Repositories ist ein Fehler oder ein unerwarteter Zustand aufgetreten. Bitte prüfen Sie die Logs für weitere Informationen."
+    },
+    "4GRrgkSC01": {
+      "displayName": "Unerwartetes Merge-Ergebnis",
+      "description": "Der Merge hatte ein unerwartetes Ergebis, das nicht automatisiert behandelt werden konnte. Nähere Details sind im Log zu finden. Führen Sie den Merge ggf. manuell durch."
     }
   },
   "namespaceStrategies": {
diff --git a/scm-webapp/src/main/resources/locales/en/plugins.json b/scm-webapp/src/main/resources/locales/en/plugins.json
index 2353184ada..69e8d057ee 100644
--- a/scm-webapp/src/main/resources/locales/en/plugins.json
+++ b/scm-webapp/src/main/resources/locales/en/plugins.json
@@ -199,6 +199,10 @@
     "8LRncum0S1": {
       "displayName": "Internal repository error",
       "description": "There was an error or an unexpected condition while handling the native repository. Please consult the logs for further information."
+    },
+    "4GRrgkSC01": {
+      "displayName": "Unexpected merge result",
+      "description": "The merge led to an unexpected result, that could not be handled automatically. More details could be found in the log. Please merge the branches manually."
     }
   },
   "namespaceStrategies": {