diff options
Diffstat (limited to 'sonar-scanner-engine/src')
14 files changed, 429 insertions, 195 deletions
diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ChangedLinesPublisher.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ChangedLinesPublisher.java index 56737d1e068..1d434618862 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ChangedLinesPublisher.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ChangedLinesPublisher.java @@ -20,12 +20,13 @@ package org.sonar.scanner.report; import java.nio.file.Path; +import java.util.Collection; import java.util.Collections; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.stream.Collectors; import java.util.stream.StreamSupport; +import javax.annotation.CheckForNull; import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.batch.fs.internal.DefaultInputProject; import org.sonar.api.batch.scm.ScmProvider; @@ -39,9 +40,12 @@ import org.sonar.scanner.repository.ReferenceBranchSupplier; import org.sonar.scanner.scan.branch.BranchConfiguration; import org.sonar.scanner.scan.filesystem.InputComponentStore; import org.sonar.scanner.scm.ScmConfiguration; +import org.sonar.scm.git.ChangedFile; import org.sonar.scm.git.GitScmProvider; import static java.util.Optional.empty; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; public class ChangedLinesPublisher implements ReportPublisherStep { private static final Logger LOG = Loggers.get(ChangedLinesPublisher.class); @@ -88,9 +92,9 @@ public class ChangedLinesPublisher implements ReportPublisherStep { private int writeChangedLines(ScmProvider provider, ScannerReportWriter writer, String targetScmBranch) { Path rootBaseDir = project.getBaseDir(); Map<Path, DefaultInputFile> changedFiles = StreamSupport.stream(inputComponentStore.allChangedFilesToPublish().spliterator(), false) - .collect(Collectors.toMap(DefaultInputFile::path, f -> f)); + .collect(toMap(DefaultInputFile::path, identity())); - Map<Path, Set<Integer>> pathSetMap = ((GitScmProvider) provider).branchChangedLines(targetScmBranch, rootBaseDir, changedFiles); // TODO: Extend ScmProvider abstract + Map<Path, Set<Integer>> pathSetMap = getBranchChangedLinesByScm(provider, targetScmBranch, rootBaseDir, toChangedFilesByPathMap(changedFiles.values())); int count = 0; if (pathSetMap == null) { @@ -126,4 +130,20 @@ public class ChangedLinesPublisher implements ReportPublisherStep { builder.addAllLine(changedLines); writer.writeComponentChangedLines(fileRef, builder.build()); } + + @CheckForNull + private static Map<Path, Set<Integer>> getBranchChangedLinesByScm(ScmProvider scmProvider, String targetScmBranch, Path rootBaseDir, Map<Path, ChangedFile> changedFiles) { + if (scmProvider instanceof GitScmProvider) { + return ((GitScmProvider) scmProvider).branchChangedLinesWithFileMovementDetection(targetScmBranch, rootBaseDir, changedFiles); + } + + return scmProvider.branchChangedLines(targetScmBranch, rootBaseDir, changedFiles.keySet()); + } + + private static Map<Path, ChangedFile> toChangedFilesByPathMap(Collection<DefaultInputFile> files) { + return files + .stream() + .map(ChangedFile::of) + .collect(toMap(ChangedFile::getAbsolutFilePath, identity())); + } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ComponentsPublisher.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ComponentsPublisher.java index b21da5f2f94..404d646e191 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ComponentsPublisher.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/report/ComponentsPublisher.java @@ -68,8 +68,9 @@ public class ComponentsPublisher implements ReportPublisherStep { fileBuilder.setStatus(convert(file.status())); fileBuilder.setMarkedAsUnchanged(file.isMarkedAsUnchanged()); - if (file.isMovedFile()) { - fileBuilder.setOldRelativeFilePath(file.oldPath()); + String oldRelativePath = file.oldRelativePath(); + if (oldRelativePath != null) { + fileBuilder.setOldRelativeFilePath(oldRelativePath); } String lang = getLanguageKey(file); diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/FileIndexer.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/FileIndexer.java index 4653c24d171..f62617d166a 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/FileIndexer.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scan/filesystem/FileIndexer.java @@ -136,7 +136,7 @@ public class FileIndexer { language != null ? language.key() : null, scannerComponentIdGenerator.getAsInt(), sensorStrategy, - scmChangedFiles.getFileOldPath(realAbsoluteFile) + scmChangedFiles.getOldRelativeFilePath(realAbsoluteFile) ); DefaultInputFile inputFile = new DefaultInputFile(indexedFile, f -> metadataGenerator.setMetadata(module.key(), f, module.getEncoding())); diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFiles.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFiles.java index 010f2de4cbf..a26f57ce2dd 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFiles.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFiles.java @@ -21,21 +21,26 @@ package org.sonar.scanner.scm; import java.nio.file.Path; import java.util.Collection; -import java.util.List; +import java.util.Map; import java.util.Optional; -import java.util.function.Predicate; import javax.annotation.CheckForNull; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; import org.sonar.scm.git.ChangedFile; +import static java.util.Collections.emptyMap; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; + @Immutable public class ScmChangedFiles { @Nullable private final Collection<ChangedFile> changedFiles; + private final Map<Path, ChangedFile> changedFilesByPath; public ScmChangedFiles(@Nullable Collection<ChangedFile> changedFiles) { this.changedFiles = changedFiles; + this.changedFilesByPath = toChangedFilesByPathMap(changedFiles); } public boolean isChanged(Path file) { @@ -43,7 +48,7 @@ public class ScmChangedFiles { throw new IllegalStateException("Scm didn't provide valid data"); } - return this.findFile(file).isPresent(); + return this.getChangedFile(file).isPresent(); } public boolean isValid() { @@ -56,20 +61,20 @@ public class ScmChangedFiles { } @CheckForNull - public String getFileOldPath(Path absoluteFilePath) { - return this.findFile(absoluteFilePath) - .filter(ChangedFile::isMoved) - .map(ChangedFile::getOldFilePath) + public String getOldRelativeFilePath(Path absoluteFilePath) { + return this.getChangedFile(absoluteFilePath) + .filter(ChangedFile::isMovedFile) + .map(ChangedFile::getOldRelativeFilePathReference) .orElse(null); } - private Optional<ChangedFile> findFile(Path absoluteFilePath) { - Predicate<ChangedFile> isTargetFile = file -> file.getAbsolutFilePath().equals(absoluteFilePath); + private Optional<ChangedFile> getChangedFile(Path absoluteFilePath) { + return Optional.ofNullable(changedFilesByPath.get(absoluteFilePath)); + } - return Optional.ofNullable(this.get()) - .orElseGet(List::of) - .stream() - .filter(isTargetFile) - .findFirst(); + private static Map<Path, ChangedFile> toChangedFilesByPathMap(@Nullable Collection<ChangedFile> changedFiles) { + return Optional.ofNullable(changedFiles) + .map(files -> files.stream().collect(toMap(ChangedFile::getAbsolutFilePath, identity()))) + .orElse(emptyMap()); } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java index 392996d7e74..4ec314a4566 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/scm/ScmChangedFilesProvider.java @@ -22,9 +22,10 @@ package org.sonar.scanner.scm; import java.nio.file.Path; import java.util.Collection; import java.util.Set; -import java.util.stream.Collectors; import javax.annotation.CheckForNull; +import javax.annotation.Nullable; import org.sonar.api.batch.fs.internal.DefaultInputProject; +import org.sonar.api.batch.scm.ScmProvider; import org.sonar.api.impl.utils.ScannerUtils; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; @@ -34,6 +35,8 @@ import org.sonar.scm.git.ChangedFile; import org.sonar.scm.git.GitScmProvider; import org.springframework.context.annotation.Bean; +import static java.util.stream.Collectors.toSet; + public class ScmChangedFilesProvider { private static final Logger LOG = Loggers.get(ScmChangedFilesProvider.class); private static final String LOG_MSG = "SCM collecting changed files in the branch"; @@ -44,33 +47,33 @@ public class ScmChangedFilesProvider { Collection<ChangedFile> changedFiles = loadChangedFilesIfNeeded(scmConfiguration, branchConfiguration, rootBaseDir); if (changedFiles != null) { - validatePaths(getFilePaths(changedFiles)); + validatePaths(getAbsoluteFilePaths(changedFiles)); } return new ScmChangedFiles(changedFiles); } private static void validatePaths(Set<Path> changedFilePaths) { - if (changedFilePaths != null && changedFilePaths.stream().anyMatch(p -> !p.isAbsolute())) { + if (changedFilePaths.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."); } } - private static Set<Path> getFilePaths(Collection<ChangedFile> changedFiles) { + private static Set<Path> getAbsoluteFilePaths(Collection<ChangedFile> changedFiles) { return changedFiles .stream() .map(ChangedFile::getAbsolutFilePath) - .collect(Collectors.toSet()); + .collect(toSet()); } @CheckForNull private static Collection<ChangedFile> loadChangedFilesIfNeeded(ScmConfiguration scmConfiguration, BranchConfiguration branchConfiguration, Path rootBaseDir) { final String targetBranchName = branchConfiguration.targetBranchName(); if (branchConfiguration.isPullRequest() && targetBranchName != null) { - GitScmProvider scmProvider = (GitScmProvider) scmConfiguration.provider(); + ScmProvider scmProvider = scmConfiguration.provider(); if (scmProvider != null) { Profiler profiler = Profiler.create(LOG).startInfo(LOG_MSG); - Collection<ChangedFile> changedFiles = scmProvider.branchModifiedFiles(targetBranchName, rootBaseDir); + Collection<ChangedFile> changedFiles = getChangedFilesByScm(scmProvider, targetBranchName, rootBaseDir); profiler.stopInfo(); if (changedFiles != null) { LOG.debug("SCM reported {} {} changed in the branch", changedFiles.size(), ScannerUtils.pluralize("file", changedFiles.size())); @@ -83,4 +86,23 @@ public class ScmChangedFilesProvider { return null; } + private static Collection<ChangedFile> getChangedFilesByScm(ScmProvider scmProvider, String targetBranchName, Path rootBaseDir) { + if (scmProvider instanceof GitScmProvider) { + return ((GitScmProvider) scmProvider).branchChangedFilesWithFileMovementDetection(targetBranchName, rootBaseDir); + } + + return toChangedFiles(scmProvider.branchChangedFiles(targetBranchName, rootBaseDir)); + } + + @CheckForNull + private static Collection<ChangedFile> toChangedFiles(@Nullable Set<Path> changedPaths) { + if (changedPaths == null) { + return null; + } + + return changedPaths + .stream() + .map(ChangedFile::of) + .collect(toSet()); + } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/ChangedFile.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/ChangedFile.java index 6f30d24e744..15b711d1fcb 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/ChangedFile.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/ChangedFile.java @@ -23,37 +23,62 @@ import java.nio.file.Path; import java.util.Objects; import javax.annotation.CheckForNull; import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; +import org.sonar.api.batch.fs.internal.DefaultInputFile; +@Immutable public class ChangedFile { - @Nullable - private final String oldFilePath; - private final String filePath; private final Path absoluteFilePath; + private final String oldRelativeFilePathReference; - public ChangedFile(String filePath, Path absoluteFilePath) { - this(filePath, absoluteFilePath, null); + private ChangedFile(Path absoluteFilePath, @Nullable String oldRelativeFilePathReference) { + this.absoluteFilePath = absoluteFilePath; + this.oldRelativeFilePathReference = oldRelativeFilePathReference; } - public ChangedFile(String filePath, Path absoluteFilePath, @Nullable String oldFilePath) { - this.filePath = filePath; - this.oldFilePath = oldFilePath; - this.absoluteFilePath = absoluteFilePath; + public Path getAbsolutFilePath() { + return absoluteFilePath; } @CheckForNull - public String getOldFilePath() { - return oldFilePath; + public String getOldRelativeFilePathReference() { + return oldRelativeFilePathReference; } - public boolean isMoved() { - return Objects.nonNull(this.getOldFilePath()); + public boolean isMovedFile() { + return this.getOldRelativeFilePathReference() != null; } - public String getFilePath() { - return filePath; + public static ChangedFile of(Path path) { + return new ChangedFile(path, null); } - public Path getAbsolutFilePath() { - return absoluteFilePath; + public static ChangedFile of(Path path, @Nullable String oldRelativeFilePathReference) { + return new ChangedFile(path, oldRelativeFilePathReference); + } + + public static ChangedFile of(DefaultInputFile file) { + return new ChangedFile(file.path(), file.oldRelativePath()); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + + if (o == null || getClass() != o.getClass()) { + return false; + } + + ChangedFile that = (ChangedFile) o; + + return Objects.equals(oldRelativeFilePathReference, that.oldRelativeFilePathReference) + && Objects.equals(absoluteFilePath, that.absoluteFilePath); + } + + @Override + public int hashCode() { + return Objects.hash(oldRelativeFilePathReference, absoluteFilePath); } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitScmProvider.java b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitScmProvider.java index c51035ee8f1..bfcbe5f3726 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitScmProvider.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scm/git/GitScmProvider.java @@ -27,16 +27,15 @@ import java.time.Instant; import java.util.Arrays; import java.util.Collection; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; import java.util.regex.Pattern; -import java.util.stream.Collectors; import javax.annotation.CheckForNull; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.GitAPIException; @@ -62,7 +61,6 @@ import org.eclipse.jgit.treewalk.FileTreeIterator; import org.eclipse.jgit.treewalk.filter.PathFilter; import org.eclipse.jgit.treewalk.filter.PathFilterGroup; import org.eclipse.jgit.treewalk.filter.TreeFilter; -import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.batch.scm.BlameCommand; import org.sonar.api.batch.scm.ScmProvider; import org.sonar.api.notifications.AnalysisWarnings; @@ -71,6 +69,11 @@ import org.sonar.api.utils.System2; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; +import static java.lang.String.format; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; +import static java.util.stream.Collectors.toSet; +import static java.util.stream.Collectors.toUnmodifiableMap; import static org.eclipse.jgit.diff.DiffEntry.ChangeType.ADD; import static org.eclipse.jgit.diff.DiffEntry.ChangeType.MODIFY; import static org.eclipse.jgit.diff.DiffEntry.ChangeType.RENAME; @@ -79,6 +82,7 @@ public class GitScmProvider extends ScmProvider { private static final Logger LOG = Loggers.get(GitScmProvider.class); private static final String COULD_NOT_FIND_REF = "Could not find ref '%s' in refs/heads, refs/remotes, refs/remotes/upstream or refs/remotes/origin"; + private static final String NO_MERGE_BASE_FOUND_MESSAGE = "No merge base found between HEAD and %s"; private final BlameCommand blameCommand; private final AnalysisWarnings analysisWarnings; private final GitIgnoreCommand gitIgnoreCommand; @@ -90,7 +94,7 @@ public class GitScmProvider extends ScmProvider { this.analysisWarnings = analysisWarnings; this.gitIgnoreCommand = gitIgnoreCommand; this.system2 = system2; - this.documentationLink = "/documentation/analysis/scm-integration/"; + this.documentationLink = "/documentation/analysis/scm-integration/"; } @Override @@ -117,48 +121,13 @@ public class GitScmProvider extends ScmProvider { @CheckForNull @Override public Set<Path> branchChangedFiles(String targetBranchName, Path rootBaseDir) { - try (Repository repo = buildRepo(rootBaseDir)) { - Ref targetRef = resolveTargetRef(targetBranchName, repo); - if (targetRef == null) { - addWarningTargetNotFound(targetBranchName); - return null; - } - - if (isDiffAlgoInvalid(repo.getConfig())) { - LOG.warn("The diff algorithm configured in git is not supported. " - + "No information regarding changes in the branch will be collected, which can lead to unexpected results."); - return null; - } - - Optional<RevCommit> mergeBaseCommit = findMergeBase(repo, targetRef); - if (!mergeBaseCommit.isPresent()) { - LOG.warn("No merge base found between HEAD and " + targetRef.getName()); - return null; - } - AbstractTreeIterator mergeBaseTree = prepareTreeParser(repo, mergeBaseCommit.get()); - - // we compare a commit with HEAD, so no point ignoring line endings (it will be whatever is committed) - try (Git git = newGit(repo)) { - List<DiffEntry> diffEntries = git.diff() - .setShowNameAndStatusOnly(true) - .setOldTree(mergeBaseTree) - .setNewTree(prepareNewTree(repo)) - .call(); - - return diffEntries.stream() - .filter(diffEntry -> diffEntry.getChangeType() == DiffEntry.ChangeType.ADD || diffEntry.getChangeType() == DiffEntry.ChangeType.MODIFY) - .map(diffEntry -> repo.getWorkTree().toPath().resolve(diffEntry.getNewPath())) - .collect(Collectors.toSet()); - } - } catch (IOException | GitAPIException e) { - LOG.warn(e.getMessage(), e); - } - return null; + return Optional.ofNullable((branchChangedFilesWithFileMovementDetection(targetBranchName, rootBaseDir))) + .map(GitScmProvider::extractAbsoluteFilePaths) + .orElse(null); } - // TODO: Adjust ScmProvider abstract @CheckForNull - public Collection<ChangedFile> branchModifiedFiles(String targetBranchName, Path rootBaseDir) { + public Collection<ChangedFile> branchChangedFilesWithFileMovementDetection(String targetBranchName, Path rootBaseDir) { try (Repository repo = buildRepo(rootBaseDir)) { Ref targetRef = resolveTargetRef(targetBranchName, repo); if (targetRef == null) { @@ -174,7 +143,7 @@ public class GitScmProvider extends ScmProvider { Optional<RevCommit> mergeBaseCommit = findMergeBase(repo, targetRef); if (mergeBaseCommit.isEmpty()) { - LOG.warn("No merge base found between HEAD and " + targetRef.getName()); + LOG.warn(composeNoMergeBaseFoundWarning(targetRef.getName())); return null; } AbstractTreeIterator mergeBaseTree = prepareTreeParser(repo, mergeBaseCommit.get()); @@ -201,11 +170,12 @@ public class GitScmProvider extends ScmProvider { Map<String, String> renamedFilePaths = computeRenamedFilePaths(repository, diffEntries); Set<String> changedFilePaths = computeChangedFilePaths(diffEntries); - List<ChangedFile> changedFiles = new LinkedList<>(); - - Consumer<String> collectChangedFiles = filePath -> changedFiles.add(new ChangedFile(filePath, workingDirectory.resolve(filePath), renamedFilePaths.getOrDefault(filePath, null))); - changedFilePaths.forEach(collectChangedFiles); + return collectChangedFiles(workingDirectory, renamedFilePaths, changedFilePaths); + } + private static List<ChangedFile> collectChangedFiles(Path workingDirectory, Map<String, String> renamedFilePaths, Set<String> changedFilePaths) { + List<ChangedFile> changedFiles = new LinkedList<>(); + changedFilePaths.forEach(filePath -> changedFiles.add(ChangedFile.of(workingDirectory.resolve(filePath), renamedFilePaths.get(filePath)))); return changedFiles; } @@ -217,7 +187,7 @@ public class GitScmProvider extends ScmProvider { .compute() .stream() .filter(entry -> RENAME.equals(entry.getChangeType())) - .collect(Collectors.toUnmodifiableMap(DiffEntry::getNewPath, DiffEntry::getOldPath)); + .collect(toUnmodifiableMap(DiffEntry::getNewPath, DiffEntry::getOldPath)); } private static Set<String> computeChangedFilePaths(List<DiffEntry> diffEntries) { @@ -225,10 +195,10 @@ public class GitScmProvider extends ScmProvider { .stream() .filter(isAllowedChangeType(ADD, MODIFY)) .map(DiffEntry::getNewPath) - .collect(Collectors.toSet()); + .collect(toSet()); } - private static Predicate<DiffEntry> isAllowedChangeType(ChangeType ...changeTypes) { + private static Predicate<DiffEntry> isAllowedChangeType(ChangeType... changeTypes) { Function<ChangeType, Predicate<DiffEntry>> isChangeType = type -> entry -> type.equals(entry.getChangeType()); return Arrays @@ -240,41 +210,11 @@ public class GitScmProvider extends ScmProvider { @CheckForNull @Override public Map<Path, Set<Integer>> branchChangedLines(String targetBranchName, Path projectBaseDir, Set<Path> changedFiles) { - try (Repository repo = buildRepo(projectBaseDir)) { - Ref targetRef = resolveTargetRef(targetBranchName, repo); - if (targetRef == null) { - addWarningTargetNotFound(targetBranchName); - return null; - } - - if (isDiffAlgoInvalid(repo.getConfig())) { - // we already print a warning when branchChangedFiles is called - return null; - } - - // force ignore different line endings when comparing a commit with the workspace - repo.getConfig().setBoolean("core", null, "autocrlf", true); - - Optional<RevCommit> mergeBaseCommit = findMergeBase(repo, targetRef); - if (!mergeBaseCommit.isPresent()) { - LOG.warn("No merge base found between HEAD and " + targetRef.getName()); - return null; - } - - Map<Path, Set<Integer>> changedLines = new HashMap<>(); - - for (Path path : changedFiles) { - collectChangedLines(repo, mergeBaseCommit.get(), changedLines, path); - } - return changedLines; - } catch (Exception e) { - LOG.warn("Failed to get changed lines from git", e); - } - return null; + return branchChangedLinesWithFileMovementDetection(targetBranchName, projectBaseDir, toChangedFileByPathsMap(changedFiles)); } - // TODO: Adjust ScmProvider abstract - public Map<Path, Set<Integer>> branchChangedLines(String targetBranchName, Path projectBaseDir, Map<Path, DefaultInputFile> changedFiles) { + @CheckForNull + public Map<Path, Set<Integer>> branchChangedLinesWithFileMovementDetection(String targetBranchName, Path projectBaseDir, Map<Path, ChangedFile> changedFiles) { try (Repository repo = buildRepo(projectBaseDir)) { Ref targetRef = resolveTargetRef(targetBranchName, repo); if (targetRef == null) { @@ -293,13 +233,13 @@ public class GitScmProvider extends ScmProvider { Optional<RevCommit> mergeBaseCommit = findMergeBase(repo, targetRef); if (mergeBaseCommit.isEmpty()) { - LOG.warn("No merge base found between HEAD and " + targetRef.getName()); + LOG.warn(composeNoMergeBaseFoundWarning(targetRef.getName())); return null; } Map<Path, Set<Integer>> changedLines = new HashMap<>(); - for (Map.Entry<Path, DefaultInputFile> entry : changedFiles.entrySet()) { + for (Map.Entry<Path, ChangedFile> entry : changedFiles.entrySet()) { collectChangedLines(repo, mergeBaseCommit.get(), changedLines, entry.getKey(), entry.getValue()); } @@ -307,56 +247,33 @@ public class GitScmProvider extends ScmProvider { } catch (Exception e) { LOG.warn("Failed to get changed lines from git", e); } + return null; } + private static String composeNoMergeBaseFoundWarning(String targetRef) { + return format(NO_MERGE_BASE_FOUND_MESSAGE, targetRef); + } + private void addWarningTargetNotFound(String targetBranchName) { - analysisWarnings.addUnique(String.format(COULD_NOT_FIND_REF + analysisWarnings.addUnique(format(COULD_NOT_FIND_REF + ". You may see unexpected issues and changes. " + "Please make sure to fetch this ref before pull request analysis and refer to" + " <a href=\"%s\" target=\"_blank\">the documentation</a>.", targetBranchName, documentationLink)); } - private void collectChangedLines(Repository repo, RevCommit mergeBaseCommit, Map<Path, Set<Integer>> changedLines, Path changedFile) { + private void collectChangedLines(Repository repo, RevCommit mergeBaseCommit, Map<Path, Set<Integer>> changedLines, Path changedFilePath, ChangedFile changedFile) { ChangedLinesComputer computer = new ChangedLinesComputer(); try (DiffFormatter diffFmt = new DiffFormatter(new BufferedOutputStream(computer.receiver()))) { - // copied from DiffCommand so that we can use a custom DiffFormatter which ignores white spaces. diffFmt.setRepository(repo); diffFmt.setProgressMonitor(NullProgressMonitor.INSTANCE); diffFmt.setDiffComparator(RawTextComparator.WS_IGNORE_ALL); - Path workTree = repo.getWorkTree().toPath(); - String relativePath = workTree.relativize(changedFile).toString(); - PathFilter pathFilter = PathFilter.create(toGitPath(relativePath)); - diffFmt.setPathFilter(pathFilter); - - AbstractTreeIterator mergeBaseTree = prepareTreeParser(repo, mergeBaseCommit); - List<DiffEntry> diffEntries = diffFmt.scan(mergeBaseTree, new FileTreeIterator(repo)); - diffFmt.format(diffEntries); - diffFmt.flush(); - diffEntries.stream() - .filter(diffEntry -> diffEntry.getChangeType() == DiffEntry.ChangeType.ADD || diffEntry.getChangeType() == DiffEntry.ChangeType.MODIFY) - .findAny() - .ifPresent(diffEntry -> changedLines.put(changedFile, computer.changedLines())); - } catch (Exception e) { - LOG.warn("Failed to get changed lines from git for file " + changedFile, e); - } - } - - // TODO: Adjust ScmProvider abstract - private void collectChangedLines(Repository repo, RevCommit mergeBaseCommit, Map<Path, Set<Integer>> changedLines, Path changedFilePath, DefaultInputFile changedFileData) { - ChangedLinesComputer computer = new ChangedLinesComputer(); - - try (DiffFormatter diffFmt = new DiffFormatter(new BufferedOutputStream(computer.receiver()))) { - diffFmt.setRepository(repo); - diffFmt.setProgressMonitor(NullProgressMonitor.INSTANCE); - diffFmt.setDiffComparator(RawTextComparator.WS_IGNORE_ALL); - - diffFmt.setDetectRenames(changedFileData.isMovedFile()); + diffFmt.setDetectRenames(changedFile.isMovedFile()); Path workTree = repo.getWorkTree().toPath(); - TreeFilter treeFilter = getTreeFilter(changedFileData, workTree); + TreeFilter treeFilter = getTreeFilter(changedFile, workTree); diffFmt.setPathFilter(treeFilter); AbstractTreeIterator mergeBaseTree = prepareTreeParser(repo, mergeBaseCommit); @@ -384,17 +301,24 @@ public class GitScmProvider extends ScmProvider { return path.replaceAll(Pattern.quote(File.separator), "/"); } - private TreeFilter getTreeFilter(DefaultInputFile changedFile, Path baseDir) { - String oldPath = toGitPath(changedFile.oldPath()); - String path = toGitPath(relativizeFilePath(baseDir, changedFile.path())); + private static TreeFilter getTreeFilter(ChangedFile changedFile, Path baseDir) { + String path = toGitPath(relativizeFilePath(baseDir, changedFile.getAbsolutFilePath())); + String oldRelativePath = changedFile.getOldRelativeFilePathReference(); - if (changedFile.isMovedFile()) { - return PathFilterGroup.createFromStrings(path, oldPath); + if (oldRelativePath != null) { + return PathFilterGroup.createFromStrings(path, toGitPath(oldRelativePath)); } return PathFilter.create(path); } + private static Set<Path> extractAbsoluteFilePaths(Collection<ChangedFile> changedFiles) { + return changedFiles + .stream() + .map(ChangedFile::getAbsolutFilePath) + .collect(toSet()); + } + @CheckForNull private Ref resolveTargetRef(String targetBranchName, Repository repo) throws IOException { String localRef = "refs/heads/" + targetBranchName; @@ -506,6 +430,12 @@ public class GitScmProvider extends ScmProvider { } } + private static Map<Path, ChangedFile> toChangedFileByPathsMap(Set<Path> changedFiles) { + return changedFiles + .stream() + .collect(toMap(identity(), ChangedFile::of, (x, y) -> y, LinkedHashMap::new)); + } + private static String relativizeFilePath(Path baseDirectory, Path filePath) { return baseDirectory.relativize(filePath).toString(); } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ChangedLinesPublisherTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ChangedLinesPublisherTest.java index 0c6e5d2c2cc..52cb1d1bd71 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ChangedLinesPublisherTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ChangedLinesPublisherTest.java @@ -43,10 +43,15 @@ import org.sonar.scanner.repository.ReferenceBranchSupplier; import org.sonar.scanner.scan.branch.BranchConfiguration; import org.sonar.scanner.scan.filesystem.InputComponentStore; import org.sonar.scanner.scm.ScmConfiguration; +import org.sonar.scm.git.GitScmProvider; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assumptions.assumeThat; +import static org.mockito.ArgumentMatchers.anyMap; +import static org.mockito.ArgumentMatchers.anySet; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -54,14 +59,14 @@ public class ChangedLinesPublisherTest { private static final String TARGET_BRANCH = "target"; private static final Path BASE_DIR = Paths.get("/root"); - private ScmConfiguration scmConfiguration = mock(ScmConfiguration.class); - private InputModuleHierarchy inputModuleHierarchy = mock(InputModuleHierarchy.class); - private InputComponentStore inputComponentStore = mock(InputComponentStore.class); - private BranchConfiguration branchConfiguration = mock(BranchConfiguration.class); - private ReferenceBranchSupplier referenceBranchSupplier = mock(ReferenceBranchSupplier.class); + private final ScmConfiguration scmConfiguration = mock(ScmConfiguration.class); + private final InputModuleHierarchy inputModuleHierarchy = mock(InputModuleHierarchy.class); + private final InputComponentStore inputComponentStore = mock(InputComponentStore.class); + private final BranchConfiguration branchConfiguration = mock(BranchConfiguration.class); + private final ReferenceBranchSupplier referenceBranchSupplier = mock(ReferenceBranchSupplier.class); private ScannerReportWriter writer; - private ScmProvider provider = mock(ScmProvider.class); - private DefaultInputProject project = mock(DefaultInputProject.class); + private final ScmProvider provider = mock(ScmProvider.class); + private final DefaultInputProject project = mock(DefaultInputProject.class); @Rule public TemporaryFolder temp = new TemporaryFolder(); @@ -69,7 +74,7 @@ public class ChangedLinesPublisherTest { @Rule public LogTester logTester = new LogTester(); - private ChangedLinesPublisher publisher = new ChangedLinesPublisher(scmConfiguration, project, inputComponentStore, branchConfiguration, referenceBranchSupplier); + private final ChangedLinesPublisher publisher = new ChangedLinesPublisher(scmConfiguration, project, inputComponentStore, branchConfiguration, referenceBranchSupplier); @Before public void setUp() { @@ -141,6 +146,19 @@ public class ChangedLinesPublisherTest { } @Test + public void write_changed_file_with_GitScmProvider() { + GitScmProvider provider = mock(GitScmProvider.class); + when(scmConfiguration.provider()).thenReturn(provider); + Set<Integer> lines = new HashSet<>(Arrays.asList(1, 10)); + when(provider.branchChangedLines(eq(TARGET_BRANCH), eq(BASE_DIR), anySet())) + .thenReturn(ImmutableMap.of(BASE_DIR.resolve("path1"), lines, BASE_DIR.resolve("path3"), Collections.emptySet())); + + publisher.publish(writer); + + verify(provider).branchChangedLinesWithFileMovementDetection(eq(TARGET_BRANCH), eq(BASE_DIR), anyMap()); + } + + @Test public void write_last_line_as_changed_if_all_other_lines_are_changed_and_last_line_is_empty() { DefaultInputFile fileWithChangedLines = createInputFile("path1", "l1\nl2\nl3\n"); DefaultInputFile fileWithoutChangedLines = createInputFile("path2", "l1\nl2\nl3\n"); @@ -156,7 +174,7 @@ public class ChangedLinesPublisherTest { } @Test - public void dont_write_last_line_as_changed_if_its_not_empty() { + public void do_not_write_last_line_as_changed_if_its_not_empty() { DefaultInputFile fileWithChangedLines = createInputFile("path1", "l1\nl2\nl3\nl4"); DefaultInputFile fileWithoutChangedLines = createInputFile("path2", "l1\nl2\nl3\nl4"); Set<Path> paths = new HashSet<>(Arrays.asList(BASE_DIR.resolve("path1"), BASE_DIR.resolve("path2"))); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ComponentsPublisherTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ComponentsPublisherTest.java index 97d98920dfa..c6ae8dc010c 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ComponentsPublisherTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/report/ComponentsPublisherTest.java @@ -31,11 +31,11 @@ import org.sonar.api.SonarRuntime; import org.sonar.api.batch.bootstrap.ProjectDefinition; import org.sonar.api.batch.fs.InputFile; import org.sonar.api.batch.fs.InputFile.Type; -import org.sonar.api.utils.DateUtils; -import org.sonar.scanner.ProjectInfo; import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.batch.fs.internal.DefaultInputProject; import org.sonar.api.batch.fs.internal.TestInputFileBuilder; +import org.sonar.api.utils.DateUtils; +import org.sonar.scanner.ProjectInfo; import org.sonar.scanner.protocol.output.FileStructure; import org.sonar.scanner.protocol.output.ScannerReport.Component; import org.sonar.scanner.protocol.output.ScannerReport.Component.FileStatus; @@ -107,6 +107,9 @@ public class ComponentsPublisherTest { DefaultInputFile testFile = new TestInputFileBuilder("foo", "module1/test/FooTest.java", 7).setType(Type.TEST).setStatus(InputFile.Status.ADDED).setLines(4).build(); store.put("module1", testFile); + DefaultInputFile movedFile = new TestInputFileBuilder("foo", "module1/src/MovedFile.java", "module0/src/MovedFile.java", 9).setStatus(InputFile.Status.CHANGED).setLines(4).build(); + store.put("module1", movedFile); + ComponentsPublisher publisher = new ComponentsPublisher(project, store); publisher.publish(writer); @@ -114,6 +117,7 @@ public class ComponentsPublisherTest { assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 4)).isTrue(); assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 6)).isTrue(); assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 7)).isTrue(); + assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 9)).isTrue(); // not marked for publishing assertThat(writer.hasComponentData(FileStructure.Domain.COMPONENT, 5)).isFalse(); @@ -126,6 +130,9 @@ public class ComponentsPublisherTest { assertThat(rootProtobuf.getDescription()).isEqualTo("Root description"); assertThat(rootProtobuf.getLinkCount()).isZero(); + Component movedFileProtobuf = reader.readComponent(9); + assertThat(movedFileProtobuf.getOldRelativeFilePath()).isEqualTo("module0/src/MovedFile.java"); + assertThat(reader.readComponent(4).getStatus()).isEqualTo(FileStatus.SAME); assertThat(reader.readComponent(6).getStatus()).isEqualTo(FileStatus.CHANGED); assertThat(reader.readComponent(7).getStatus()).isEqualTo(FileStatus.ADDED); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java index 94629f59b60..cc3c7ff31e9 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scan/filesystem/StatusDetectionTest.java @@ -65,7 +65,7 @@ public class StatusDetectionTest { @Test public void detect_status_branches_confirm() { Path filePath = Paths.get("module", "src", "Foo.java"); - ScmChangedFiles changedFiles = new ScmChangedFiles(List.of(new ChangedFile(filePath.toString(), filePath))); + ScmChangedFiles changedFiles = new ScmChangedFiles(List.of(ChangedFile.of(filePath))); StatusDetection statusDetection = new StatusDetection(projectRepositories, changedFiles); assertThat(statusDetection.status("foo", createFile("src/Foo.java"), "XXXXX")).isEqualTo(InputFile.Status.CHANGED); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java index abbc78a3819..bf3092a55e7 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesProviderTest.java @@ -28,9 +28,9 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.sonar.api.batch.fs.internal.DefaultInputProject; import org.sonar.api.batch.scm.ScmProvider; -import org.sonar.scanner.fs.InputModuleHierarchy; import org.sonar.scanner.scan.branch.BranchConfiguration; import org.sonar.scm.git.ChangedFile; +import org.sonar.scm.git.GitScmProvider; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -45,13 +45,12 @@ public class ScmChangedFilesProviderTest { @Mock private BranchConfiguration branchConfiguration; @Mock - private InputModuleHierarchy inputModuleHierarchy; - @Mock private ScmProvider scmProvider; - private Path rootBaseDir = Paths.get("root"); + private final Path rootBaseDir = Paths.get("root"); + private final DefaultInputProject project = mock(DefaultInputProject.class); + private ScmChangedFilesProvider provider; - private DefaultInputProject project = mock(DefaultInputProject.class); @Before public void setUp() { @@ -124,6 +123,21 @@ public class ScmChangedFilesProviderTest { } @Test + public void testGitScmProvider(){ + GitScmProvider gitScmProvider = mock(GitScmProvider.class); + + when(scmConfiguration.provider()).thenReturn(gitScmProvider); + when(branchConfiguration.isPullRequest()).thenReturn(true); + when(branchConfiguration.targetBranchName()).thenReturn("target"); + + ScmChangedFiles scmChangedFiles = provider.provide(scmConfiguration, branchConfiguration, project); + + assertThat(scmChangedFiles.get()).isEmpty(); + verify(scmConfiguration).provider(); + + } + + @Test public void testReturnChangedFiles() { when(branchConfiguration.targetBranchName()).thenReturn("target"); when(branchConfiguration.isPullRequest()).thenReturn(true); @@ -132,7 +146,7 @@ public class ScmChangedFilesProviderTest { ScmChangedFiles scmChangedFiles = provider.provide(scmConfiguration, branchConfiguration, project); Path filePath = Paths.get("changedFile").toAbsolutePath(); - ChangedFile changedFile = new ChangedFile(filePath.toString(), filePath); + ChangedFile changedFile = ChangedFile.of(filePath); assertThat(scmChangedFiles.get()).containsOnly(changedFile); verify(scmProvider).branchChangedFiles("target", rootBaseDir); } diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesTest.java index 856d0573124..766c8ec8457 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/scm/ScmChangedFilesTest.java @@ -35,7 +35,7 @@ public class ScmChangedFilesTest { @Test public void testGetter() { Path filePath = Paths.get("files"); - ChangedFile file = new ChangedFile(filePath.toString(), filePath); + ChangedFile file = ChangedFile.of(filePath); Collection<ChangedFile> files = Collections.singletonList(file); scmChangedFiles = new ScmChangedFiles(files); assertThat(scmChangedFiles.get()).containsOnly(file); @@ -54,7 +54,7 @@ public class ScmChangedFilesTest { @Test public void testConfirm() { Path filePath = Paths.get("files"); - Collection<ChangedFile> files = Collections.singletonList(new ChangedFile(filePath.toString(), filePath)); + Collection<ChangedFile> files = Collections.singletonList(ChangedFile.of(filePath)); scmChangedFiles = new ScmChangedFiles(files); assertThat(scmChangedFiles.isValid()).isTrue(); assertThat(scmChangedFiles.isChanged(Paths.get("files"))).isTrue(); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/ChangedFileTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/ChangedFileTest.java new file mode 100644 index 00000000000..99a73d8f0ba --- /dev/null +++ b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/ChangedFileTest.java @@ -0,0 +1,95 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.scm.git; + +import java.nio.file.Path; +import org.junit.Test; +import org.sonar.api.batch.fs.InputFile; +import org.sonar.api.batch.fs.internal.DefaultIndexedFile; +import org.sonar.api.batch.fs.internal.DefaultInputFile; +import org.sonar.api.batch.fs.internal.SensorStrategy; + +import static org.apache.commons.lang.RandomStringUtils.random; +import static org.apache.commons.lang.RandomStringUtils.randomNumeric; +import static org.assertj.core.api.Assertions.assertThat; + + +public class ChangedFileTest { + + @Test + public void test_unMovedFile(){ + Path absolutePath = Path.of("/absolutePath"); + ChangedFile changedFile = ChangedFile.of(absolutePath); + + assertThat(changedFile.getAbsolutFilePath()).isSameAs(absolutePath); + assertThat(changedFile.getOldRelativeFilePathReference()).isNull(); + assertThat(changedFile.isMovedFile()).isFalse(); + } + + @Test + public void test_movedFile(){ + Path absolutePath = Path.of("/absolutePath"); + ChangedFile changedFile = ChangedFile.of(absolutePath, "/oldRelativePath"); + + assertThat(changedFile.getAbsolutFilePath()).isSameAs(absolutePath); + assertThat(changedFile.getOldRelativeFilePathReference()).isEqualTo("/oldRelativePath"); + assertThat(changedFile.isMovedFile()).isTrue(); + } + + @Test + public void test_equalsAndHashCode(){ + Path absolutePath = Path.of("/absolutePath"); + ChangedFile changedFile1 = ChangedFile.of(absolutePath, "/oldRelativePath"); + ChangedFile changedFile2 = ChangedFile.of(absolutePath, "/oldRelativePath"); + + assertThat(changedFile1).isEqualTo(changedFile2).hasSameHashCodeAs(changedFile2); + } + + @Test + public void test_changed_file_is_created_properly_from_default_input_file() { + String oldRelativeReference = "old/relative/path"; + Path path = Path.of("absolute/path"); + DefaultInputFile file = composeDefaultInputFile(path, oldRelativeReference); + ChangedFile changedFile = ChangedFile.of(file); + + assertThat(changedFile.getAbsolutFilePath()).isEqualTo(path); + assertThat(changedFile.isMovedFile()).isTrue(); + assertThat(changedFile.getOldRelativeFilePathReference()).isEqualTo(oldRelativeReference); + } + + private DefaultInputFile composeDefaultInputFile(Path path, String oldRelativeReference) { + DefaultIndexedFile indexedFile = composeDefaultIndexFile(path, oldRelativeReference); + return new DefaultInputFile(indexedFile, f -> f.setPublished(true)); + } + + private DefaultIndexedFile composeDefaultIndexFile(Path path, String oldRelativePath) { + return new DefaultIndexedFile( + path, + random(5), + random(5), + random(5), + InputFile.Type.MAIN, + random(5), + Integer.parseInt(randomNumeric(5)), + new SensorStrategy(), + oldRelativePath); + } + +} diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitScmProviderTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitScmProviderTest.java index a41f2a508b9..e75152cdb74 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitScmProviderTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scm/git/GitScmProviderTest.java @@ -61,9 +61,11 @@ import org.sonar.api.utils.System2; import org.sonar.api.utils.log.LogAndArguments; import org.sonar.api.utils.log.LogTester; +import static java.lang.String.format; import static java.util.Collections.emptySet; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.AssertionsForClassTypes.tuple; import static org.assertj.core.data.MapEntry.entry; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -206,6 +208,35 @@ public class GitScmProviderTest { } @Test + public void branchChangedFilesWithFileMovementDetection_correctly_detects_several_file_moves_in_pull_request_base_branch() throws IOException, GitAPIException { + String fileM1 = "file-m1.xoo"; + String newFileM1 = "new-file-m1.xoo"; + String fileM2 = "file-m2.xoo"; + String newFileM2 = "new-file-m2.xoo"; + + Path newFileM1AbsolutPath = worktree.resolve(newFileM1); + Path newFileM2AbsolutPath = worktree.resolve(newFileM2); + + createAndCommitFile(fileM1); + createAndCommitFile(fileM2); + + createBranch(); + + editLineOfFile(fileM1, 1); + commit(fileM1); + + moveAndCommitFile(fileM1, newFileM1); + moveAndCommitFile(fileM2, newFileM2); + + assertThat(newScmProvider().branchChangedFilesWithFileMovementDetection("master", worktree)) + .extracting(ChangedFile::getAbsolutFilePath, ChangedFile::getOldRelativeFilePathReference) + .containsExactlyInAnyOrder( + tuple(newFileM1AbsolutPath, fileM1), + tuple(newFileM2AbsolutPath, fileM2) + ); + } + + @Test public void branchChangedFiles_should_not_fail_with_patience_diff_algo() throws IOException { Path gitConfig = worktree.resolve(".git").resolve("config"); Files.write(gitConfig, "[diff]\nalgorithm = patience\n".getBytes(StandardCharsets.UTF_8)); @@ -260,7 +291,7 @@ public class GitScmProviderTest { assertThat(newScmProvider().branchChangedLines("master", worktree, changedFiles)) .containsOnly( entry(worktree.resolve("lao.txt"), new HashSet<>(Arrays.asList(2, 3, 11, 12, 13))), - entry(worktree.resolve("file-m1.xoo"), new HashSet<>(Arrays.asList(4))), + entry(worktree.resolve("file-m1.xoo"), new HashSet<>(List.of(4))), entry(worktree.resolve("file-b2.xoo"), new HashSet<>(Arrays.asList(1, 2, 3)))); assertThat(newScmProvider().branchChangedLines("master", worktree, Collections.singleton(worktree.resolve("nonexistent")))) @@ -388,6 +419,45 @@ public class GitScmProviderTest { } @Test + public void branchChangedLinesWithFileMovementDetection_detects_modified_lines_on_renamed_pr_files_compared_to_original_files_on_target_branch() throws GitAPIException, IOException { + String fileM1 = "file-m1.xoo"; + String newFileM1 = "new-file-m1.xoo"; + String fileM2 = "file-m2.xoo"; + String newFileM2 = "new-file-m2.xoo"; + + Path newFileM1AbsolutPath = worktree.resolve(newFileM1); + Path newFileM2AbsolutPath = worktree.resolve(newFileM2); + + createAndCommitFile(fileM1); + createAndCommitFile(fileM2); + + createBranch(); + + editLineOfFile(fileM1, 2); + commit(fileM1); + + editLineOfFile(fileM2, 1); + commit(fileM2); + + moveAndCommitFile(fileM1, newFileM1); + moveAndCommitFile(fileM2, newFileM2); + + Map<Path, ChangedFile> changedFiles = Map.of( + newFileM1AbsolutPath, ChangedFile.of(newFileM1AbsolutPath, fileM1), + newFileM2AbsolutPath, ChangedFile.of(newFileM2AbsolutPath, fileM2) + ); + + Map<Path, Set<Integer>> changedLines = newScmProvider().branchChangedLinesWithFileMovementDetection("master", + worktree.resolve("project1"), changedFiles); + + assertThat(changedLines) + .containsOnly( + entry(newFileM1AbsolutPath, Set.of(2)), + entry(newFileM2AbsolutPath, Set.of(1)) + ); + } + + @Test public void branchChangedFiles_when_git_work_tree_is_above_project_basedir() throws IOException, GitAPIException { git.branchCreate().setName("b1").call(); git.checkout().setName("b1").call(); @@ -422,7 +492,7 @@ public class GitScmProviderTest { git.branchCreate().setName("b1").setStartPoint(forkPoint.getName()).call(); git.checkout().setName("b1").call(); - String newFileContent = new String(Files.readAllBytes(filePath), StandardCharsets.UTF_8).replaceAll("\n", "\r\n"); + String newFileContent = Files.readString(filePath).replaceAll("\n", "\r\n"); Files.write(filePath, newFileContent.getBytes(StandardCharsets.UTF_8), StandardOpenOption.TRUNCATE_EXISTING); commit("file-m1.xoo"); @@ -531,7 +601,7 @@ public class GitScmProviderTest { } @Test - public void branchChangedFiles_should_throw_when_repo_nonexistent() throws IOException { + public void branchChangedFiles_should_throw_when_repo_nonexistent() { assertThatThrownBy(() -> newScmProvider().branchChangedFiles("master", temp.newFolder().toPath())) .isInstanceOf(MessageException.class) .hasMessageContaining("Not inside a Git work tree: "); @@ -612,8 +682,10 @@ public class GitScmProviderTest { public void branchChangedLines_omits_files_with_git_api_errors() throws IOException, GitAPIException { String f1 = "file-in-first-commit.xoo"; String f2 = "file2-in-first-commit.xoo"; + String f3 = "file3-in-first-commit.xoo"; createAndCommitFile(f2); + createAndCommitFile(f3); git.branchCreate().setName("b1").call(); git.checkout().setName("b1").call(); @@ -621,26 +693,30 @@ public class GitScmProviderTest { // both files modified addLineToFile(f1, 1); addLineToFile(f2, 2); + addLineToFile(f3, 3); commit(f1); commit(f2); + commit(f3); AtomicInteger callCount = new AtomicInteger(0); GitScmProvider provider = new GitScmProvider(mockCommand(), analysisWarnings, gitIgnoreCommand, system2) { @Override AbstractTreeIterator prepareTreeParser(Repository repo, RevCommit commit) throws IOException { - if (callCount.getAndIncrement() == 1) { + if (callCount.getAndIncrement() == 2) { throw new RuntimeException("error"); } return super.prepareTreeParser(repo, commit); } }; - Set<Path> changedFiles = new LinkedHashSet<>(); - changedFiles.add(worktree.resolve(f1)); - changedFiles.add(worktree.resolve(f2)); + + Set<Path> changedFiles = new LinkedHashSet<>(List.of(worktree.resolve(f1), worktree.resolve(f2), worktree.resolve(f3))); assertThat(provider.branchChangedLines("master", worktree, changedFiles)) - .isEqualTo(Collections.singletonMap(worktree.resolve(f1), Collections.singleton(1))); + .containsOnly( + entry(worktree.resolve(f1), Collections.singleton(1)), + entry(worktree.resolve(f2), Collections.singleton(2)) + ); } @Test @@ -798,6 +874,27 @@ public class GitScmProviderTest { git.commit().setAuthor(person).setCommitter(person).setMessage(relativePath).call(); } + private void createBranch() throws IOException, GitAPIException { + ObjectId forkPoint = git.getRepository().exactRef("HEAD").getObjectId(); + git.branchCreate().setName(BRANCH_NAME).setStartPoint(forkPoint.getName()).call(); + git.checkout().setName(BRANCH_NAME).call(); + } + + private void editLineOfFile(String relativePath, int lineNumber) throws IOException { + Path filePath = worktree.resolve(relativePath); + List<String> lines = Files.readAllLines(filePath); + lines.set(lineNumber - 1, randomizedLine(relativePath)); + Files.write(filePath, lines, StandardOpenOption.TRUNCATE_EXISTING); + } + + private void moveAndCommitFile(String filename, String newFilename) throws GitAPIException, IOException { + Path file = worktree.resolve(filename); + Files.move(file, file.resolveSibling(newFilename)); + git.add().addFilepattern(filename).setUpdate(true).call(); + git.add().addFilepattern(newFilename).call(); + git.commit().setAuthor("joe", "joe@example.com").setMessage(format("Move file from [%s] to [%s]", filename, newFilename)).call(); + } + private GitScmProvider newScmProvider() { return new GitScmProvider(mockCommand(), analysisWarnings, gitIgnoreCommand, system2); } |