Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SLCORE-1142 Improve Git blaming for the analysis #1247

Merged
merged 4 commits into from
Feb 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ static NewCodeDefinition withSpecificAnalysis(long thresholdDate) {
return new NewCodeSpecificAnalysis(thresholdDate);
}

Instant getThresholdDate();

abstract class NewCodeDefinitionWithDate implements NewCodeDefinition {
protected final long thresholdDate;

Expand All @@ -89,8 +91,8 @@ public boolean isSupported() {
return true;
}

public long getThresholdDate() {
return thresholdDate;
public Instant getThresholdDate() {
return Instant.ofEpochMilli(thresholdDate);
}
}

Expand All @@ -116,6 +118,11 @@ public boolean isSupported() {
return true;
}

@Override
public Instant getThresholdDate() {
return Instant.now().minus(days, ChronoUnit.DAYS);
}

// Text used by IDEs in the UI. Communicate changes to IDE squad prior to changing the wording.
@Override
public String toString() {
Expand Down Expand Up @@ -216,6 +223,12 @@ public String getBranchName() {
return branchName;
}

@Override
public Instant getThresholdDate() {
// instead of Long.MAX_VALUE it's set for Instant.now() in case it will be used for git blame limit
return Instant.now();
}

// Text used by IDEs in the UI. Communicate changes to IDE squad prior to changing the wording.
@Override
public String toString() {
Expand All @@ -239,6 +252,12 @@ public boolean isOnNewCode(long creationDate) {
return true;
}

@Override
public Instant getThresholdDate() {
// instead of 0L it's set for Instant.now() in case it will be used for git blame limit (which shouldn't normally happen)
return Instant.now();
}

@Override
public boolean isSupported() {
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.io.InputStream;
import java.net.URI;
import java.nio.file.Path;
import java.time.Instant;
import java.util.LinkedList;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -97,15 +98,16 @@ public static List<URI> getVSCChangedFiles(@Nullable Path baseDir) {
}
}

public static SonarLintBlameResult getBlameResult(Path projectBaseDir, Set<Path> projectBaseRelativeFilePaths, @Nullable UnaryOperator<String> fileContentProvider) {
return getBlameResult(projectBaseDir, projectBaseRelativeFilePaths, fileContentProvider, GitUtils::checkIfEnabled);
public static SonarLintBlameResult getBlameResult(Path projectBaseDir, Set<Path> projectBaseRelativeFilePaths,
@Nullable UnaryOperator<String> fileContentProvider, Instant thresholdDate) {
return getBlameResult(projectBaseDir, projectBaseRelativeFilePaths, fileContentProvider, GitUtils::checkIfEnabled, thresholdDate);
}

public static SonarLintBlameResult getBlameResult(Path projectBaseDir, Set<Path> projectBaseRelativeFilePaths, @Nullable UnaryOperator<String> fileContentProvider,
Predicate<Path> isEnabled) {
static SonarLintBlameResult getBlameResult(Path projectBaseDir, Set<Path> projectBaseRelativeFilePaths, @Nullable UnaryOperator<String> fileContentProvider,
Predicate<Path> isEnabled, Instant thresholdDate) {
if (isEnabled.test(projectBaseDir)) {
LOG.debug("Using native git blame");
return blameFromNativeCommand(projectBaseDir, projectBaseRelativeFilePaths);
return blameFromNativeCommand(projectBaseDir, projectBaseRelativeFilePaths, thresholdDate);
} else {
LOG.debug("Falling back to JGit git blame");
return blameWithFilesGitCommand(projectBaseDir, projectBaseRelativeFilePaths, fileContentProvider);
Expand Down Expand Up @@ -173,13 +175,14 @@ private static String executeGitCommand(Path workingDir, String... command) thro
return String.join(System.lineSeparator(), commandResult);
}

public static SonarLintBlameResult blameFromNativeCommand(Path projectBaseDir, Set<Path> projectBaseRelativeFilePaths) {
public static SonarLintBlameResult blameFromNativeCommand(Path projectBaseDir, Set<Path> projectBaseRelativeFilePaths, Instant thresholdDate) {
var nativeExecutable = getNativeGitExecutable();
if (nativeExecutable != null) {
for (var relativeFilePath : projectBaseRelativeFilePaths) {
try {
var blameHistoryWindow = "--since='" + thresholdDate + "'";
return parseBlameOutput(executeGitCommand(projectBaseDir,
nativeExecutable, "blame", projectBaseDir.resolve(relativeFilePath).toString(), "--line-porcelain", "--encoding=UTF-8"),
nativeExecutable, "blame", blameHistoryWindow, projectBaseDir.resolve(relativeFilePath).toString(), "--line-porcelain", "--encoding=UTF-8"),
projectBaseDir.resolve(relativeFilePath).toString().replace("\\", "/"), projectBaseDir);
} catch (IOException e) {
throw new IllegalStateException("Failed to blame repository files", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,15 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.time.Instant;
import java.time.Period;
import java.time.temporal.ChronoUnit;
import java.util.Calendar;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.TimeZone;
import java.util.function.UnaryOperator;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
Expand Down Expand Up @@ -55,9 +60,11 @@
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.sonarsource.sonarlint.core.commons.testutils.GitUtils.addFileToGitIgnoreAndCommit;
import static org.sonarsource.sonarlint.core.commons.testutils.GitUtils.commit;
import static org.sonarsource.sonarlint.core.commons.testutils.GitUtils.commitAtDate;
import static org.sonarsource.sonarlint.core.commons.testutils.GitUtils.createFile;
import static org.sonarsource.sonarlint.core.commons.testutils.GitUtils.createRepository;
import static org.sonarsource.sonarlint.core.commons.testutils.GitUtils.modifyFile;
import static org.sonarsource.sonarlint.core.commons.util.git.GitUtils.blameFromNativeCommand;
import static org.sonarsource.sonarlint.core.commons.util.git.GitUtils.blameWithFilesGitCommand;
import static org.sonarsource.sonarlint.core.commons.util.git.GitUtils.getBlameResult;
import static org.sonarsource.sonarlint.core.commons.util.git.GitUtils.getVSCChangedFiles;
Expand Down Expand Up @@ -120,7 +127,7 @@ void it_should_fallback_to_jgit_blame() throws IOException, GitAPIException {
createFile(projectDirPath, "fileA", "line1", "line2", "line3");
var c1 = commit(git, "fileA");

var sonarLintBlameResult = getBlameResult(projectDirPath, Set.of(Path.of("fileA")), null, path -> false);
var sonarLintBlameResult = getBlameResult(projectDirPath, Set.of(Path.of("fileA")), null, path -> false, Instant.now());
assertThat(IntStream.of(1, 2, 3)
.mapToObj(lineNumber -> sonarLintBlameResult.getLatestChangeDateForLinesInFile(Path.of("fileA"), List.of(lineNumber))))
.map(Optional::get)
Expand All @@ -131,7 +138,8 @@ void it_should_fallback_to_jgit_blame() throws IOException, GitAPIException {
void it_should_throw_if_no_files() {
Set<Path> files = Set.of();

assertThrows(IllegalStateException.class, () -> getBlameResult(projectDirPath, files, null, path -> true));
var now = Instant.now();
assertThrows(IllegalStateException.class, () -> getBlameResult(projectDirPath, files, null, path -> true, now));
}

@Test
Expand Down Expand Up @@ -350,6 +358,85 @@ void git_blame_works_for_bare_repos_too() {
assertThat(sonarLintBlameResult.getLatestChangeDateForLinesInFile(Path.of("fileB"), List.of(3))).isEmpty();
}

@Test
void it_should_default_to_instant_now_git_blame_history_limit() throws IOException, GitAPIException {
var calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
calendar.add(Calendar.YEAR, -1);
var fileAStr = "fileA";
createFile(projectDirPath, fileAStr, "line1");
var yearAgo = calendar.toInstant();
// initial commit 1 year ago
commitAtDate(git, yearAgo, fileAStr);
var lines = new String[3];

// second commit 4 months after initial commit
calendar.add(Calendar.MONTH, 4);
lines[0] = "line1";
lines[1] = "line2";
var eightMonthsAgo = calendar.toInstant();
modifyFile(projectDirPath.resolve(fileAStr), lines);
commitAtDate(git, eightMonthsAgo, fileAStr);

// third commit 4 months after second commit
calendar.add(Calendar.MONTH, 4);
lines[2] = "line3";
var fourMonthsAgo = calendar.toInstant();
modifyFile(projectDirPath.resolve(fileAStr), lines);
commitAtDate(git, fourMonthsAgo, fileAStr);
var fileA = Path.of(fileAStr);

var blameResult = blameFromNativeCommand(projectDirPath, Set.of(fileA), Instant.now());

var line1Date = blameResult.getLatestChangeDateForLinesInFile(fileA, List.of(1)).get();
var line2Date = blameResult.getLatestChangeDateForLinesInFile(fileA, List.of(2)).get();
var line3Date = blameResult.getLatestChangeDateForLinesInFile(fileA, List.of(3)).get();
// provided blame time limit is 30 days, so all lines are blamed by the latest commit that happened 4 months ago
assertThat(ChronoUnit.MINUTES.between(line1Date.toInstant(), fourMonthsAgo)).isZero();
assertThat(ChronoUnit.MINUTES.between(line2Date.toInstant(), fourMonthsAgo)).isZero();
assertThat(ChronoUnit.MINUTES.between(line3Date.toInstant(), fourMonthsAgo)).isZero();
}

@Test
void it_should_blame_file_since_provided_period() throws IOException, GitAPIException {
var calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
calendar.add(Calendar.YEAR, -1);
var fileAStr = "fileA";
createFile(projectDirPath, fileAStr, "line1");
var yearAgo = calendar.toInstant();
// initial commit 1 year ago
commitAtDate(git, yearAgo, fileAStr);
var lines = new String[3];

// second commit 4 months after initial commit
calendar.add(Calendar.MONTH, 4);
lines[0] = "line1";
lines[1] = "line2";
var eightMonthsAgo = calendar.toInstant();
modifyFile(projectDirPath.resolve(fileAStr), lines);
commitAtDate(git, eightMonthsAgo, fileAStr);

// third commit 4 months after second commit
calendar.add(Calendar.MONTH, 4);
lines[2] = "line3";
var fourMonthsAgo = calendar.toInstant();
modifyFile(projectDirPath.resolve(fileAStr), lines);
commitAtDate(git, fourMonthsAgo, fileAStr);
var fileA = Path.of(fileAStr);

var blameResult = blameFromNativeCommand(projectDirPath, Set.of(fileA), Instant.now().minus(Period.ofDays(180)));

var line1Date = blameResult.getLatestChangeDateForLinesInFile(fileA, List.of(1)).get();
var line2Date = blameResult.getLatestChangeDateForLinesInFile(fileA, List.of(2)).get();
var line3Date = blameResult.getLatestChangeDateForLinesInFile(fileA, List.of(3)).get();
// provided blame time limit is 180 days
// line 1 was committed 1 year ago but should have commit date of the first commit made earlier than blame time window - 8 months ago
assertThat(ChronoUnit.MINUTES.between(line2Date.toInstant(), line1Date.toInstant())).isZero();
// line 2 was committed 8 months ago, it's outside the blame time window, but it's a first commit outside the range, so it has real commit date
assertThat(ChronoUnit.MINUTES.between(line2Date.toInstant(), eightMonthsAgo)).isZero();
// line 3 was committed 4 months ago, it's inside the blame time window, so it has real commit date
assertThat(ChronoUnit.MINUTES.between(line3Date.toInstant(), fourMonthsAgo)).isZero();
}

private static void setUpBareRepo(Map<String, String> filePathContentMap) throws IOException, GitAPIException {
bareRepoPath = Files.createTempDirectory("bare-repo");
workingRepoPath = Files.createTempDirectory("working-repo");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@
import org.sonarsource.sonarlint.core.branch.SonarProjectBranchTrackingService;
import org.sonarsource.sonarlint.core.commons.KnownFinding;
import org.sonarsource.sonarlint.core.commons.LocalOnlyIssue;
import org.sonarsource.sonarlint.core.commons.NewCodeDefinition;
import org.sonarsource.sonarlint.core.commons.RuleType;
import org.sonarsource.sonarlint.core.commons.SonarLintBlameResult;
import org.sonarsource.sonarlint.core.commons.log.SonarLintLogger;
import org.sonarsource.sonarlint.core.commons.util.git.GitRepoNotFoundException;
import org.sonarsource.sonarlint.core.file.PathTranslationService;
import org.sonarsource.sonarlint.core.local.only.LocalOnlyIssueStorageService;
import org.sonarsource.sonarlint.core.newcode.NewCodeService;
import org.sonarsource.sonarlint.core.reporting.FindingReportingService;
import org.sonarsource.sonarlint.core.repository.config.ConfigurationRepository;
import org.sonarsource.sonarlint.core.rpc.protocol.SonarLintRpcClient;
Expand Down Expand Up @@ -79,10 +81,12 @@ public class TrackingService {
private final LocalOnlyIssueRepository localOnlyIssueRepository;
private final LocalOnlyIssueStorageService localOnlyIssueStorageService;
private final FindingsSynchronizationService findingsSynchronizationService;
private final NewCodeService newCodeService;

public TrackingService(SonarLintRpcClient client, ConfigurationRepository configurationRepository, SonarProjectBranchTrackingService branchTrackingService,
PathTranslationService pathTranslationService, FindingReportingService reportingService, KnownFindingsStorageService knownFindingsStorageService, StorageService storageService,
LocalOnlyIssueRepository localOnlyIssueRepository, LocalOnlyIssueStorageService localOnlyIssueStorageService, FindingsSynchronizationService findingsSynchronizationService) {
LocalOnlyIssueRepository localOnlyIssueRepository, LocalOnlyIssueStorageService localOnlyIssueStorageService, FindingsSynchronizationService findingsSynchronizationService,
NewCodeService newCodeService) {
this.client = client;
this.configurationRepository = configurationRepository;
this.branchTrackingService = branchTrackingService;
Expand All @@ -93,6 +97,7 @@ public TrackingService(SonarLintRpcClient client, ConfigurationRepository config
this.localOnlyIssueRepository = localOnlyIssueRepository;
this.localOnlyIssueStorageService = localOnlyIssueStorageService;
this.findingsSynchronizationService = findingsSynchronizationService;
this.newCodeService = newCodeService;
}

@EventListener
Expand Down Expand Up @@ -271,7 +276,9 @@ private IntroductionDateProvider getIntroductionDateProvider(String configuratio
var baseDir = getBaseDir(configurationScopeId);
if (baseDir != null) {
try {
var sonarLintBlameResult = getBlameResult(baseDir, fileRelativePaths, fileContentProvider);
var newCodeDefinition = newCodeService.getFullNewCodeDefinition(configurationScopeId);
var thresholdDate = newCodeDefinition.map(NewCodeDefinition::getThresholdDate).orElse(NewCodeDefinition.withAlwaysNew().getThresholdDate());
var sonarLintBlameResult = getBlameResult(baseDir, fileRelativePaths, fileContentProvider, thresholdDate);
return (filePath, lineNumbers) -> determineIntroductionDate(filePath, lineNumbers, sonarLintBlameResult);
} catch (GitRepoNotFoundException e) {
LOG.info("Git Repository not found for {}. The path {} is not in a Git repository", configurationScopeId, e.getPath());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ static Sonarlint.NewCodeDefinition adapt(NewCodeDefinition newCodeDefinition) {
}
if (newCodeDefinition.getMode() != NewCodeMode.REFERENCE_BRANCH) {
var newCodeDefinitionWithDate = (NewCodeDefinition.NewCodeDefinitionWithDate) newCodeDefinition;
builder.setThresholdDate(newCodeDefinitionWithDate.getThresholdDate());
builder.setThresholdDate(newCodeDefinitionWithDate.getThresholdDate().toEpochMilli());
} else {
var newCodeReferenceBranch = (NewCodeDefinition.NewCodeReferenceBranch) newCodeDefinition;
builder.setReferenceBranch(newCodeReferenceBranch.getBranchName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ void shouldAdaptFromProtobuf() {
var days = adapt(daysProto);
assertThat(days.isSupported()).isTrue();
assertThat(days).isInstanceOf(NewCodeDefinition.NewCodeNumberOfDaysWithDate.class);
assertThat(((NewCodeDefinition.NewCodeNumberOfDaysWithDate) days).getThresholdDate()).isEqualTo(1000);
assertThat(days.getThresholdDate().toEpochMilli()).isEqualTo(1000);
assertThat(((NewCodeDefinition.NewCodeNumberOfDaysWithDate) days).getDays()).isEqualTo(30);

var previousWithVersionProto = Sonarlint.NewCodeDefinition.newBuilder()
Expand All @@ -68,7 +68,7 @@ void shouldAdaptFromProtobuf() {
var previousWithVersion = adapt(previousWithVersionProto);
assertThat(previousWithVersion.isSupported()).isTrue();
assertThat(previousWithVersion).isInstanceOf(NewCodeDefinition.NewCodePreviousVersion.class);
assertThat(((NewCodeDefinition.NewCodePreviousVersion) previousWithVersion).getThresholdDate()).isEqualTo(1000);
assertThat(previousWithVersion.getThresholdDate().toEpochMilli()).isEqualTo(1000);
assertThat(((NewCodeDefinition.NewCodePreviousVersion) previousWithVersion).getVersion()).isEqualTo("1.0-SNAPSHOT");

var previousWithoutVersionProto = Sonarlint.NewCodeDefinition.newBuilder()
Expand All @@ -78,7 +78,7 @@ void shouldAdaptFromProtobuf() {
var previousWithoutVersion = adapt(previousWithoutVersionProto);
assertThat(previousWithoutVersion.isSupported()).isTrue();
assertThat(previousWithoutVersion).isInstanceOf(NewCodeDefinition.NewCodePreviousVersion.class);
assertThat(((NewCodeDefinition.NewCodePreviousVersion) previousWithoutVersion).getThresholdDate()).isEqualTo(1000);
assertThat(previousWithoutVersion.getThresholdDate().toEpochMilli()).isEqualTo(1000);
assertThat(((NewCodeDefinition.NewCodePreviousVersion) previousWithoutVersion).getVersion()).isNull();

var branchProto = Sonarlint.NewCodeDefinition.newBuilder()
Expand All @@ -97,7 +97,7 @@ void shouldAdaptFromProtobuf() {
var analysis = adapt(analysisProto);
assertThat(analysis.isSupported()).isTrue();
assertThat(analysis).isInstanceOf(NewCodeDefinition.NewCodeSpecificAnalysis.class);
assertThat(((NewCodeDefinition.NewCodeSpecificAnalysis) analysis).getThresholdDate()).isEqualTo(1000);
assertThat(analysis.getThresholdDate().toEpochMilli()).isEqualTo(1000);

var unknownProto = Sonarlint.NewCodeDefinition.newBuilder()
.setMode(Sonarlint.NewCodeDefinitionMode.UNKNOWN)
Expand Down