SONAR-16290 Fallback to jgit on first git failure

7 files changed, 132 insertions, 80 deletions

diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/CompositeBlameCommand.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/CompositeBlameCommand.java
index 83223959ee8..85f0bb26949 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/CompositeBlameCommand.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/CompositeBlameCommand.java
@@ -20,14 +20,20 @@
 package org.sonar.scm.git;
 
 import java.io.File;
+import java.io.IOException;
 import java.nio.file.Files;
+import java.util.HashSet;
 import java.util.List;
-import java.util.concurrent.ForkJoinPool;
+import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
-import java.util.stream.Stream;
-import java.util.stream.StreamSupport;
 import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.TreeWalk;
 import org.sonar.api.batch.fs.InputFile;
 import org.sonar.api.batch.scm.BlameCommand;
 import org.sonar.api.batch.scm.BlameLine;
@@ -60,29 +66,60 @@ public class CompositeBlameCommand extends BlameCommand {
       if (cloneIsInvalid(gitBaseDir)) {
         return;
       }
-      nativeGitEnabled = nativeCmd.isEnabled(basedir.toPath());
-      Stream<InputFile> stream = StreamSupport.stream(input.filesToBlame().spliterator(), true);
-      ForkJoinPool forkJoinPool = new ForkJoinPool(Runtime.getRuntime().availableProcessors(), new GitThreadFactory(), null, false);
-      // exceptions thrown by the blame method will be ignored
-      forkJoinPool.submit(() -> stream.forEach(inputFile -> blame(output, git, gitBaseDir, inputFile)));
+
+      Set<String> committedFiles = collectAllCommittedFiles(repo);
+      nativeGitEnabled = nativeCmd.isEnabled();
+      ExecutorService executorService = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors(), new GitThreadFactory());
+
+      for (InputFile inputFile : input.filesToBlame()) {
+        String filename = pathResolver.relativePath(gitBaseDir, inputFile.file());
+        if (filename == null || !committedFiles.contains(filename)) {
+          continue;
+        }
+        // exceptions thrown by the blame method will be ignored
+        executorService.submit(() -> blame(output, git, gitBaseDir, inputFile, filename));
+      }
+
+      executorService.shutdown();
       try {
-        forkJoinPool.shutdown();
-        forkJoinPool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
+        executorService.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);
       } catch (InterruptedException e) {
         LOG.info("Git blame interrupted");
       }
     }
   }
 
-  private void blame(BlameOutput output, Git git, File gitBaseDir, InputFile inputFile) {
-    String filename = pathResolver.relativePath(gitBaseDir, inputFile.file());
+  private static Set<String> collectAllCommittedFiles(Repository repo) {
+    try {
+      Set<String> files = new HashSet<>();
+
+      try (RevWalk revWalk = new RevWalk(repo)) {
+        RevCommit head = revWalk.parseCommit(repo.resolve(Constants.HEAD));
+        try (TreeWalk treeWalk = new TreeWalk(repo)) {
+          treeWalk.addTree(head.getTree());
+          treeWalk.setRecursive(true);
+
+          while (treeWalk.next()) {
+            String path = treeWalk.getPathString();
+            files.add(path);
+          }
+        }
+      }
+      return files;
+    } catch (IOException e) {
+      throw new IllegalStateException("Failed to find all committed files", e);
+    }
+  }
+
+  private void blame(BlameOutput output, Git git, File gitBaseDir, InputFile inputFile, String filename) {
     LOG.debug("Blame file {}", filename);
     List<BlameLine> blame = null;
     if (nativeGitEnabled) {
       try {
         blame = nativeCmd.blame(gitBaseDir.toPath(), filename);
       } catch (Exception e) {
-        // fallback to jgit
+        LOG.debug("Native git blame failed. Falling back to jgit: " + filename, e);
+        nativeGitEnabled = false;
       }
     }
 
@@ -92,7 +129,7 @@ public class CompositeBlameCommand extends BlameCommand {
 
     if (!blame.isEmpty()) {
       if (blame.size() == inputFile.lines() - 1) {
-        // SONARPLUGINS-3097 Git do not report blame on last empty line
+        // SONARPLUGINS-3097 Git does not report blame on last empty line
         blame.add(blame.get(blame.size() - 1));
       }
       output.blameResult(inputFile, blame);
@@ -106,10 +143,8 @@ public class CompositeBlameCommand extends BlameCommand {
     }
 
     if (Files.isRegularFile(gitBaseDir.toPath().resolve(".git/shallow"))) {
-      LOG.warn("Shallow clone detected, no blame information will be provided. "
-        + "You can convert to non-shallow with 'git fetch --unshallow'.");
-      analysisWarnings.addUnique("Shallow clone detected during the analysis. "
-        + "Some files will miss SCM information. This will affect features like auto-assignment of issues. "
+      LOG.warn("Shallow clone detected, no blame information will be provided. " + "You can convert to non-shallow with 'git fetch --unshallow'.");
+      analysisWarnings.addUnique("Shallow clone detected during the analysis. " + "Some files will miss SCM information. This will affect features like auto-assignment of issues. "
         + "Please configure your build to disable shallow clone.");
       return true;
     }
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java
index 3fffbe0ea83..1f3dbdaf0d6 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitBlameCommand.java
@@ -24,12 +24,13 @@ import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.nio.file.Path;
 import java.time.Instant;
-import java.util.ArrayList;
 import java.util.Date;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.function.Consumer;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import javax.annotation.Nullable;
 import org.sonar.api.batch.scm.BlameLine;
 import org.sonar.api.utils.log.Logger;
 import org.sonar.api.utils.log.Loggers;
@@ -37,7 +38,6 @@ import org.springframework.beans.factory.annotation.Autowired;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Collections.emptyList;
-import static java.util.Collections.unmodifiableList;
 import static org.sonar.api.utils.Preconditions.checkState;
 
 public class GitBlameCommand {
@@ -62,10 +62,10 @@ public class GitBlameCommand {
     this.gitCommand = gitCommand;
   }
 
-  public boolean isEnabled(Path baseDir) {
+  public boolean isEnabled() {
     try {
       MutableString stdOut = new MutableString();
-      executeCommand(baseDir, l -> stdOut.string = l, gitCommand, "--version");
+      executeCommand(null, l -> stdOut.string = l, gitCommand, "--version");
       return stdOut.string != null && stdOut.string.startsWith("git version");
     } catch (Exception e) {
       LOG.debug("Failed to find git native client", e);
@@ -73,21 +73,21 @@ public class GitBlameCommand {
     }
   }
 
-  public List<BlameLine> blame(Path baseDir, String fileName) {
+  public List<BlameLine> blame(Path baseDir, String fileName) throws Exception {
     BlameOutputProcessor outputProcessor = new BlameOutputProcessor();
     try {
       executeCommand(baseDir, outputProcessor::process, gitCommand, BLAME_COMMAND, BLAME_LINE_PORCELAIN_FLAG, IGNORE_WHITESPACES, fileName);
-      return outputProcessor.getBlameLines();
-    } catch (Exception e) {
-      LOG.debug("Blame failed for " + fileName, e);
+    } catch (UncommittedLineException e) {
+      LOG.debug("Unable to blame file '{}' - it has uncommitted changes", fileName);
       return emptyList();
     }
+    return outputProcessor.getBlameLines();
   }
 
-  private void executeCommand(Path baseDir, Consumer<String> stdOutLineConsumer, String... command) throws Exception {
+  private static void executeCommand(@Nullable Path baseDir, Consumer<String> stdOutLineConsumer, String... command) throws Exception {
     ProcessBuilder pb = new ProcessBuilder()
       .command(command)
-      .directory(baseDir.toFile());
+      .directory(baseDir != null ? baseDir.toFile() : null);
 
     Process p = pb.start();
     try {
@@ -103,20 +103,19 @@ public class GitBlameCommand {
       if (exit != 0) {
         throw new IllegalStateException(String.format("Command execution exited with code: %d", exit));
       }
-
     } finally {
       p.destroy();
     }
   }
 
   private static class BlameOutputProcessor {
-    private final List<BlameLine> blameLines = new ArrayList<>();
-    private String sha1;
-    private String committerTime;
-    private String committerMail;
+    private final List<BlameLine> blameLines = new LinkedList<>();
+    private String sha1 = null;
+    private String committerTime = null;
+    private String committerMail = null;
 
     public List<BlameLine> getBlameLines() {
-      return unmodifiableList(blameLines);
+      return blameLines;
     }
 
     public void process(String line) {
@@ -133,7 +132,7 @@ public class GitBlameCommand {
         }
         committerMail = matcher.group(1);
         if (committerMail.equals("not.committed.yet")) {
-          throw new IllegalStateException("Uncommitted line found");
+          throw new UncommittedLineException();
         }
       }
     }
@@ -159,4 +158,8 @@ public class GitBlameCommand {
   private static class MutableString {
     String string;
   }
+
+  private static class UncommittedLineException extends RuntimeException {
+
+  }
 }
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitThreadFactory.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitThreadFactory.java
index 6579c82564f..5316c86e7b6 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitThreadFactory.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitThreadFactory.java
@@ -22,15 +22,17 @@ package org.sonar.scm.git;
 import java.util.concurrent.ForkJoinPool;
 import java.util.concurrent.ForkJoinPool.ForkJoinWorkerThreadFactory;
 import java.util.concurrent.ForkJoinWorkerThread;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.atomic.AtomicInteger;
 
-public class GitThreadFactory implements ForkJoinWorkerThreadFactory {
+public class GitThreadFactory implements ThreadFactory {
   private static final String NAME_PREFIX = "git-scm-";
-  private int i = 0;
+  private final AtomicInteger count = new AtomicInteger(0);
 
   @Override
-  public ForkJoinWorkerThread newThread(ForkJoinPool pool) {
-    ForkJoinWorkerThread thread = ForkJoinPool.defaultForkJoinWorkerThreadFactory.newThread(pool);
-    thread.setName(NAME_PREFIX + i++);
-    return thread;
+  public Thread newThread(Runnable r) {
+    Thread t = new Thread(r);
+    t.setName(NAME_PREFIX + count.getAndIncrement());
+    return t;
   }
 }
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/JGitBlameCommand.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/JGitBlameCommand.java
index e9868a03c52..97d797785e9 100644
--- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/JGitBlameCommand.java
+++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/JGitBlameCommand.java
@@ -34,7 +34,6 @@ public class JGitBlameCommand {
   private static final Logger LOG = Loggers.get(JGitBlameCommand.class);
 
   public List<BlameLine> blame(Git git, String filename) {
-    LOG.debug("Blame file {}", filename);
     BlameResult blameResult;
     try {
       blameResult = git.blame()
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/CompositeBlameCommandTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/CompositeBlameCommandTest.java
index 5b566b72724..0604b456b93 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/CompositeBlameCommandTest.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/CompositeBlameCommandTest.java
@@ -50,6 +50,7 @@ import org.sonar.api.utils.log.LogTester;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.Assume.assumeTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.startsWith;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -88,30 +89,54 @@ public class CompositeBlameCommandTest {
   }
 
   @Test
-  public void fallback_to_jgit_if_native_git_fails() throws IOException {
+  public void fallback_to_jgit_if_native_git_fails() throws Exception {
     GitBlameCommand gitCmd = mock(GitBlameCommand.class);
     BlameCommand blameCmd = new CompositeBlameCommand(analysisWarnings, pathResolver, jGitBlameCommand, gitCmd);
     File projectDir = createNewTempFolder();
     javaUnzip("dummy-git.zip", projectDir);
 
     File baseDir = new File(projectDir, "dummy-git");
-    when(gitCmd.isEnabled(baseDir.toPath())).thenReturn(true);
+    when(gitCmd.isEnabled()).thenReturn(true);
     when(gitCmd.blame(baseDir.toPath(), DUMMY_JAVA)).thenThrow(new IllegalStateException());
     setUpBlameInputWithFile(baseDir.toPath());
     TestBlameOutput output = new TestBlameOutput();
     blameCmd.blame(input, output);
     assertThat(output.blame).hasSize(1);
     assertThat(output.blame.get(input.filesToBlame().iterator().next())).hasSize(29);
+
+    // only tried once
+    verify(gitCmd).blame(any(Path.class), any(String.class));
+    assertThat(logTester.logs()).contains("Native git blame failed. Falling back to jgit: src/main/java/org/dummy/Dummy.java");
   }
 
   @Test
-  public void use_native_git_by_default() throws IOException {
+  public void skip_files_not_committed() throws Exception {
+    // skip if git not installed
+    assumeTrue(gitBlameCommand.isEnabled());
+
+    JGitBlameCommand jgit = mock(JGitBlameCommand.class);
+    BlameCommand blameCmd = new CompositeBlameCommand(analysisWarnings, pathResolver, jgit, gitBlameCommand);
     File projectDir = createNewTempFolder();
     javaUnzip("dummy-git.zip", projectDir);
+
     File baseDir = new File(projectDir, "dummy-git");
+    setUpBlameInputWithFile(baseDir.toPath());
+    TestBlameOutput output = new TestBlameOutput();
+    blameCmd.blame(input, output);
+    assertThat(output.blame).hasSize(1);
+    assertThat(output.blame.get(input.filesToBlame().iterator().next())).hasSize(29);
 
+    // never had to fall back to jgit
+    verifyNoInteractions(jgit);
+  }
+
+  @Test
+  public void use_native_git_by_default() throws IOException {
     // skip test if git is not installed
-    assumeTrue(gitBlameCommand.isEnabled(baseDir.toPath()));
+    assumeTrue(gitBlameCommand.isEnabled());
+    File projectDir = createNewTempFolder();
+    javaUnzip("dummy-git.zip", projectDir);
+    File baseDir = new File(projectDir, "dummy-git");
 
     JGitBlameCommand jgit = mock(JGitBlameCommand.class);
     BlameCommand blameCmd = new CompositeBlameCommand(analysisWarnings, pathResolver, jgit, gitBlameCommand);
@@ -188,7 +213,6 @@ public class CompositeBlameCommandTest {
     blameCommand.blame(input, output);
   }
 
-
   @Test
   public void return_early_when_clone_with_reference_detected() throws IOException {
     File projectDir = createNewTempFolder();
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitBlameCommandTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitBlameCommandTest.java
index 0de3252668c..4961048e7e2 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitBlameCommandTest.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitBlameCommandTest.java
@@ -30,6 +30,7 @@ import java.util.List;
 import org.apache.commons.io.FileUtils;
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.GitAPIException;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -39,6 +40,7 @@ import org.sonar.api.utils.System2;
 import org.sonar.api.utils.log.LogTester;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.Assume.assumeTrue;
 import static org.sonar.scm.git.GitUtils.createFile;
 import static org.sonar.scm.git.GitUtils.createRepository;
@@ -53,8 +55,13 @@ public class GitBlameCommandTest {
   public LogTester logTester = new LogTester();
   private final GitBlameCommand blameCommand = new GitBlameCommand();
 
+  @Before
+  public void skipTestsIfNoGitFound() {
+    assumeTrue(blameCommand.isEnabled());
+  }
+
   @Test
-  public void blame_collects_all_lines() throws IOException {
+  public void blame_collects_all_lines() throws Exception {
     File projectDir = createNewTempFolder();
     javaUnzip("dummy-git.zip", projectDir);
     File baseDir = new File(projectDir, "dummy-git");
@@ -85,7 +92,7 @@ public class GitBlameCommandTest {
   }
 
   @Test
-  public void modified_file_returns_no_blame() throws IOException {
+  public void modified_file_returns_no_blame() throws Exception {
     File projectDir = createNewTempFolder();
     javaUnzip("dummy-git.zip", projectDir);
 
@@ -98,22 +105,7 @@ public class GitBlameCommandTest {
   }
 
   @Test
-  public void new_file_returns_no_blame() throws IOException {
-    File projectDir = createNewTempFolder();
-    javaUnzip("dummy-git.zip", projectDir);
-
-    File baseDir = new File(projectDir, "dummy-git");
-    String relativePath2 = "src/main/java/org/dummy/Dummy2.java";
-
-    // Emulate a new file
-    FileUtils.copyFile(new File(baseDir, DUMMY_JAVA), new File(baseDir, relativePath2));
-
-    assertThat(blameCommand.blame(baseDir.toPath(), DUMMY_JAVA)).hasSize(29);
-    assertThat(blameCommand.blame(baseDir.toPath(), relativePath2)).isEmpty();
-  }
-
-  @Test
-  public void symlink_doesnt_fail() throws IOException {
+  public void throw_exception_if_symlink_found() throws Exception {
     assumeTrue(!System2.INSTANCE.isOsWindows());
     File projectDir = temp.newFolder();
     javaUnzip("dummy-git.zip", projectDir);
@@ -125,32 +117,30 @@ public class GitBlameCommandTest {
     Files.createSymbolicLink(baseDir.resolve(relativePath2), baseDir.resolve(DUMMY_JAVA));
 
     blameCommand.blame(baseDir, DUMMY_JAVA);
-    blameCommand.blame(baseDir, relativePath2);
+    assertThatThrownBy(() -> blameCommand.blame(baseDir, relativePath2)).isInstanceOf(IllegalStateException.class);
   }
 
   @Test
-  public void git_should_be_detected() throws IOException {
-    Path baseDir = temp.newFolder().toPath();
+  public void git_should_be_detected() {
     GitBlameCommand blameCommand = new GitBlameCommand();
-    assertThat(blameCommand.isEnabled(baseDir)).isTrue();
+    assertThat(blameCommand.isEnabled()).isTrue();
   }
 
   @Test
-  public void git_should_not_be_detected() throws IOException {
-    Path baseDir = temp.newFolder().toPath();
+  public void git_should_not_be_detected() {
     GitBlameCommand blameCommand = new GitBlameCommand("randomcmdthatwillneverbefound");
-    assertThat(blameCommand.isEnabled(baseDir)).isFalse();
+    assertThat(blameCommand.isEnabled()).isFalse();
   }
 
   @Test
-  public void return_empty_if_command_fails() throws IOException {
+  public void throw_exception_if_command_fails() throws Exception {
     Path baseDir = temp.newFolder().toPath();
     GitBlameCommand blameCommand = new GitBlameCommand("randomcmdthatwillneverbefound");
-    assertThat(blameCommand.blame(baseDir, "file")).isEmpty();
+    assertThatThrownBy(() -> blameCommand.blame(baseDir, "file")).isInstanceOf(IOException.class);
   }
 
   @Test
-  public void blame_without_email_doesnt_fail() throws IOException, GitAPIException {
+  public void blame_without_email_doesnt_fail() throws Exception {
     Path baseDir = temp.newFolder().toPath();
     Git git = createRepository(baseDir);
     String filePath = "file.txt";
diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitThreadFactoryTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitThreadFactoryTest.java
index b1793cae2ac..b14d97bf58d 100644
--- a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitThreadFactoryTest.java
+++ b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitThreadFactoryTest.java
@@ -19,7 +19,6 @@
  */
 package org.sonar.scm.git;
 
-import java.util.concurrent.ForkJoinPool;
 import org.junit.Test;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -28,9 +27,9 @@ public class GitThreadFactoryTest {
   @Test
   public void testName() {
     GitThreadFactory factory = new GitThreadFactory();
-    ForkJoinPool pool = new ForkJoinPool();
-    assertThat(factory.newThread(pool).getName()).isEqualTo("git-scm-0");
-    assertThat(factory.newThread(pool).getName()).isEqualTo("git-scm-1");
-
+    assertThat(factory.newThread(() -> {
+    }).getName()).isEqualTo("git-scm-0");
+    assertThat(factory.newThread(() -> {
+    }).getName()).isEqualTo("git-scm-1");
   }
 }