SONAR-9837 Detect files changed in the current branch with SCM

sonar-plugin-api/src/main/java/org/sonar/api/batch/scm/ScmBranchProvider.java

@@ -21,7 +21,7 @@
 package org.sonar.api.batch.scm;
 
 import java.nio.file.Path;
-import java.util.Collection;
+import java.util.Set;
 import javax.annotation.Nullable;
 import org.sonar.api.ExtensionPoint;
 import org.sonar.api.batch.InstantiationStrategy;
@@ -37,11 +37,11 @@
 public abstract class ScmBranchProvider extends ScmProvider {
 
   /**
-   * Return absolute path of files changed in the current branch, compared to the provided target branch.
+   * Return absolute path of the files changed in the current branch, compared to the provided target branch.
    * @return null if SCM provider was not able to compute the list of files.
    */
   @Nullable
-  public Collection<Path> branchChangedFiles(String targetBranchName, Path rootBaseDir) {
+  public Set<Path> branchChangedFiles(String targetBranchName, Path rootBaseDir) {
     return null;
   }
 }

sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/DefaultModuleFileSystem.java

@@ -23,29 +23,28 @@
 import org.sonar.api.batch.fs.internal.DefaultFileSystem;
 import org.sonar.api.batch.fs.internal.DefaultInputModule;
 import org.sonar.scanner.analysis.DefaultAnalysisMode;
-import org.sonar.scanner.repository.ProjectRepositories;
 
 public class DefaultModuleFileSystem extends DefaultFileSystem {
 
   public DefaultModuleFileSystem(ModuleInputComponentStore moduleInputFileCache, DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode,
-    ProjectRepositories projectRepositories) {
+    StatusDetection statusDetection) {
     super(module.getBaseDir(), moduleInputFileCache);
-    setFields(module, initializer, mode, projectRepositories);
+    setFields(module, initializer, mode, statusDetection);
   }
 
   @VisibleForTesting
-  public DefaultModuleFileSystem(DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, ProjectRepositories projectRepositories) {
+  public DefaultModuleFileSystem(DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, StatusDetection statusDetection) {
     super(module.getBaseDir());
-    setFields(module, initializer, mode, projectRepositories);
+    setFields(module, initializer, mode, statusDetection);
   }
 
-  private void setFields(DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, ProjectRepositories projectRepositories) {
+  private void setFields(DefaultInputModule module, ModuleFileSystemInitializer initializer, DefaultAnalysisMode mode, StatusDetection statusDetection) {
     setWorkDir(module.getWorkDir());
     setEncoding(initializer.defaultEncoding());
 
     // filter the files sensors have access to
     if (!mode.scanAllFiles()) {
-      setDefaultPredicate(p -> new SameInputFilePredicate(p, projectRepositories, module.definition().getKeyWithBranch()));
+      setDefaultPredicate(p -> new SameInputFilePredicate(p, statusDetection, module.definition().getKeyWithBranch()));
     }
   }
 

sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/SameInputFilePredicate.java

@@ -20,45 +20,39 @@
 package org.sonar.scanner.scan.filesystem;
 
 import java.util.function.Predicate;
-import org.apache.commons.lang.StringUtils;
 import org.sonar.api.batch.fs.FilePredicate;
 import org.sonar.api.batch.fs.InputFile;
+import org.sonar.api.batch.fs.InputFile.Status;
+import org.sonar.api.batch.fs.internal.DefaultInputFile;
 import org.sonar.api.batch.fs.internal.OperatorPredicate;
 import org.sonar.api.batch.fs.internal.StatusPredicate;
-import org.sonar.scanner.repository.FileData;
-import org.sonar.scanner.repository.ProjectRepositories;
 
 public class SameInputFilePredicate implements Predicate<InputFile> {
-  private final ProjectRepositories projectRepositories;
+  private final StatusDetection statusDetection;
   private final String moduleKeyWithBranch;
   private final FilePredicate currentPredicate;
 
-  public SameInputFilePredicate(FilePredicate currentPredicate, ProjectRepositories projectRepositories, String moduleKeyWithBranch) {
+  public SameInputFilePredicate(FilePredicate currentPredicate, StatusDetection statusDetection, String moduleKeyWithBranch) {
     this.currentPredicate = currentPredicate;
-    this.projectRepositories = projectRepositories;
+    this.statusDetection = statusDetection;
     this.moduleKeyWithBranch = moduleKeyWithBranch;
   }
 
   @Override
   public boolean test(InputFile inputFile) {
     if (hasExplicitFilterOnStatus(currentPredicate)) {
-      // If user explicitely requested a given status, don't change the result
+      // If user explicitly requested a given status, don't change the result
       return true;
     }
-    // Try to avoid initializing metadata
-    FileData fileDataPerPath = projectRepositories.fileData(moduleKeyWithBranch, inputFile.relativePath());
-    if (fileDataPerPath == null) {
-      // ADDED
-      return true;
-    }
-    String previousHash = fileDataPerPath.hash();
-    if (StringUtils.isEmpty(previousHash)) {
-      // ADDED
-      return true;
+
+    // TODO: the inputFile could try to calculate the status itself without generating metadata
+    Status status = statusDetection.getStatusWithoutMetadata(moduleKeyWithBranch, (DefaultInputFile) inputFile);
+    if (status != null) {
+      return status != Status.SAME;
     }
 
     // this will trigger computation of metadata
-    return inputFile.status() != InputFile.Status.SAME;
+    return inputFile.status() != Status.SAME;
   }
 
   static boolean hasExplicitFilterOnStatus(FilePredicate predicate) {

sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/StatusDetection.java

@@ -19,6 +19,7 @@
  */
 package org.sonar.scanner.scan.filesystem;
 
+import javax.annotation.CheckForNull;
 import javax.annotation.concurrent.Immutable;
 import org.apache.commons.lang.StringUtils;
 import org.sonar.api.batch.fs.InputFile;
@@ -27,6 +28,8 @@
 import org.sonar.scanner.repository.ProjectRepositories;
 import org.sonar.scanner.scm.ScmChangedFiles;
 
+import static org.sonar.api.batch.fs.InputFile.Status.*;
+
 @Immutable
 public class StatusDetection {
 
@@ -41,18 +44,39 @@ public StatusDetection(ProjectRepositories projectSettings, ScmChangedFiles scmC
   InputFile.Status status(String projectKeyWithBranch, DefaultInputFile inputFile, String hash) {
     FileData fileDataPerPath = projectRepositories.fileData(projectKeyWithBranch, inputFile.relativePath());
     if (fileDataPerPath == null) {
-      return InputFile.Status.ADDED;
+      return checkChanged(ADDED, inputFile);
     }
     String previousHash = fileDataPerPath.hash();
     if (StringUtils.equals(hash, previousHash)) {
-      return InputFile.Status.SAME;
+      return SAME;
     }
     if (StringUtils.isEmpty(previousHash)) {
-      return InputFile.Status.ADDED;
+      return checkChanged(ADDED, inputFile);
+    }
+    return checkChanged(CHANGED, inputFile);
+  }
+
+  /**
+   * If possible, get the status of the provided file without initializing metadata of the file.
+   * @return null if it was not possible to get the status without calculating metadata
+   */
+  @CheckForNull
+  public InputFile.Status getStatusWithoutMetadata(String projectKeyWithBranch, DefaultInputFile inputFile) {
+    FileData fileDataPerPath = projectRepositories.fileData(projectKeyWithBranch, inputFile.relativePath());
+    if (fileDataPerPath == null) {
+      return checkChanged(ADDED, inputFile);
     }
+    String previousHash = fileDataPerPath.hash();
+    if (StringUtils.isEmpty(previousHash)) {
+      return checkChanged(ADDED, inputFile);
+    }
+    return null;
+  }
+
+  private InputFile.Status checkChanged(InputFile.Status status, DefaultInputFile inputFile) {
     if (!scmChangedFiles.verifyChanged(inputFile.path())) {
-      return InputFile.Status.SAME;
+      return SAME;
     }
-    return InputFile.Status.CHANGED;
+    return status;
   }
 }

sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java

@@ -24,15 +24,17 @@
 import javax.annotation.CheckForNull;
 import org.picocontainer.annotations.Nullable;
 import org.picocontainer.injectors.ProviderAdapter;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import org.sonar.api.batch.fs.internal.InputModuleHierarchy;
 import org.sonar.api.batch.scm.ScmBranchProvider;
 import org.sonar.api.batch.scm.ScmProvider;
+import org.sonar.api.utils.log.Logger;
+import org.sonar.api.utils.log.Loggers;
+import org.sonar.api.utils.log.Profiler;
 import org.sonar.scanner.scan.branch.BranchConfiguration;
 
 public class ScmChangedFilesProvider extends ProviderAdapter {
-  private static final Logger LOG = LoggerFactory.getLogger(ScmChangedFilesProvider.class);
+  private static final Logger LOG = Loggers.get(ScmChangedFilesProvider.class);
+  private static final String LOG_MSG = "SCM collecting changed files in the branch";
 
   private ScmChangedFiles scmBranchChangedFiles;
 
@@ -46,21 +48,30 @@ public ScmChangedFiles provide(@Nullable ScmConfiguration scmConfiguration, Bran
       } else {
         Path rootBaseDir = inputModuleHierarchy.root().getBaseDir();
         Collection<Path> changedFiles = loadChangedFilesIfNeeded(scmConfiguration, branchConfiguration, rootBaseDir);
+        validatePaths(changedFiles);
         scmBranchChangedFiles = new ScmChangedFiles(changedFiles);
       }
     }
     return scmBranchChangedFiles;
   }
 
+  private static void validatePaths(@javax.annotation.Nullable Collection<Path> paths) {
+    if (paths != null && paths.stream().anyMatch(p -> !p.isAbsolute())) {
+      throw new IllegalStateException("SCM provider returned a changed file with a relative path but paths must be absolute. Please fix the provider.");
+    }
+  }
+
   @CheckForNull
   private static Collection<Path> loadChangedFilesIfNeeded(ScmConfiguration scmConfiguration, BranchConfiguration branchConfiguration, Path rootBaseDir) {
     if (branchConfiguration.isShortLivingBranch()) {
       ScmProvider scmProvider = scmConfiguration.provider();
       if (scmProvider != null && (scmProvider instanceof ScmBranchProvider)) {
+        Profiler profiler = Profiler.create(LOG).startInfo(LOG_MSG);
         ScmBranchProvider scmBranchProvider = (ScmBranchProvider) scmProvider;
         Collection<Path> changedFiles = scmBranchProvider.branchChangedFiles(branchConfiguration.branchTarget(), rootBaseDir);
+        profiler.stopInfo();
         if (changedFiles != null) {
-          LOG.debug("SCM reported {} files changed in the branch", changedFiles.size());
+          LOG.debug("SCM reported {} {} changed in the branch", changedFiles.size(), pluralize("file", changedFiles.size()));
           return changedFiles;
         }
       }
@@ -70,4 +81,11 @@ private static Collection<Path> loadChangedFilesIfNeeded(ScmConfiguration scmCon
     return null;
   }
 
+  private static String pluralize(String str, int i) {
+    if (i == 1) {
+      return str;
+    }
+    return str + "s";
+  }
+
 }

sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java

@@ -33,6 +33,10 @@
 import org.sonar.scanner.scm.ScmChangedFiles;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
+import static org.mockito.Mockito.when;
 
 public class StatusDetectionTest {
   @Test
@@ -54,6 +58,26 @@ public void detect_status_branches_exclude() {
 
     // normally changed
     assertThat(statusDetection.status("foo", createFile("src/Foo.java"), "XXXXX")).isEqualTo(InputFile.Status.SAME);
+
+    // normally added
+    assertThat(statusDetection.status("foo", createFile("src/Other.java"), "QWERT")).isEqualTo(InputFile.Status.SAME);
+  }
+
+  @Test
+  public void detect_status_without_metadata() {
+    DefaultInputFile mockedFile = mock(DefaultInputFile.class);
+    when(mockedFile.relativePath()).thenReturn("module/src/Foo.java");
+    when(mockedFile.path()).thenReturn(Paths.get("module", "src", "Foo.java"));
+
+    ProjectRepositories ref = new ProjectRepositories(ImmutableTable.of(), createTable(), null);
+    ScmChangedFiles changedFiles = new ScmChangedFiles(Collections.singletonList(Paths.get("module", "src", "Foo.java")));
+    StatusDetection statusDetection = new StatusDetection(ref, changedFiles);
+
+    assertThat(statusDetection.getStatusWithoutMetadata("foo", mockedFile)).isEqualTo(InputFile.Status.ADDED);
+
+    verify(mockedFile).path();
+    verify(mockedFile).relativePath();
+    verifyNoMoreInteractions(mockedFile);
   }
 
   @Test

sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java

@@ -23,7 +23,9 @@
 import java.nio.file.Paths;
 import java.util.Collections;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.sonar.api.batch.fs.internal.DefaultInputModule;
@@ -48,6 +50,9 @@ public class ScmChangedFilesProviderTest {
   @Mock
   private ScmBranchProvider scmProvider;
 
+  @Rule
+  public ExpectedException exception = ExpectedException.none();
+
   private Path rootBaseDir = Paths.get("root");
   private ScmChangedFilesProvider provider;
 
@@ -69,6 +74,18 @@ public void testNoScmProvider() {
     verify(scmConfiguration).provider();
   }
 
+  @Test
+  public void testFailIfRelativePath() {
+    when(branchConfiguration.branchTarget()).thenReturn("target");
+    when(branchConfiguration.isShortLivingBranch()).thenReturn(true);
+    when(scmConfiguration.provider()).thenReturn(scmProvider);
+    when(scmProvider.branchChangedFiles("target", rootBaseDir)).thenReturn(Collections.singleton(Paths.get("changedFile")));
+
+    exception.expect(IllegalStateException.class);
+    exception.expectMessage("changed file with a relative path");
+    provider.provide(scmConfiguration, branchConfiguration, inputModuleHierarchy);
+  }
+
   @Test
   public void testProviderDoesntSupport() {
     when(branchConfiguration.branchTarget()).thenReturn("target");
@@ -108,10 +125,10 @@ public void testReturnChangedFiles() {
     when(branchConfiguration.branchTarget()).thenReturn("target");
     when(branchConfiguration.isShortLivingBranch()).thenReturn(true);
     when(scmConfiguration.provider()).thenReturn(scmProvider);
-    when(scmProvider.branchChangedFiles("target", rootBaseDir)).thenReturn(Collections.singletonList(Paths.get("changedFile")));
+    when(scmProvider.branchChangedFiles("target", rootBaseDir)).thenReturn(Collections.singleton(Paths.get("changedFile").toAbsolutePath()));
     ScmChangedFiles scmChangedFiles = provider.provide(scmConfiguration, branchConfiguration, inputModuleHierarchy);
 
-    assertThat(scmChangedFiles.get()).containsOnly(Paths.get("changedFile"));
+    assertThat(scmChangedFiles.get()).containsOnly(Paths.get("changedFile").toAbsolutePath());
     verify(scmProvider).branchChangedFiles("target", rootBaseDir);
   }