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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

SLCORE-1142 Improve Git blaming for the analysis #1247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
625d54b
SLCORE-1142 Improve Git blaming for the analysis
kirill-knize-sonarsource Feb 13, 2025
ab9b396
use new code definition period as git blame limit
kirill-knize-sonarsource Feb 18, 2025
f97e0ce
change default git blame limit from 6 months to Instant.now()
kirill-knize-sonarsource Feb 18, 2025
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
19 changes: 19 additions & 0 deletions backend/commons/src/main/java/org/sonarsource/sonarlint/core/commons/NewCodeDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ static NewCodeDefinition withNumberOfDaysWithDate(int days, long thresholdDate)
return new NewCodeNumberOfDaysWithDate(days, thresholdDate);
}

long getThresholdDate();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not returning an Instant from here? You can make the conversion later when you use it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think threshold date is maybe not precise enough. I am bad at naming, but what about something like "period start date"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I would move the method somewhere else, to not be in the middle of with* methods.


static NewCodeDefinition withPreviousVersion(long thresholdDate, @Nullable String version) {
return new NewCodePreviousVersion(thresholdDate, version);
}
Expand Down Expand Up @@ -116,6 +118,11 @@ public boolean isSupported() {
return true;
}

@Override
public long getThresholdDate() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this method also be implemented in NewCodeDefinitionWithDate? Maybe it's the case.
EDIT: I see this abstract class is not used at all, maybe we could remove it?

return Instant.now().minus(days, ChronoUnit.DAYS).toEpochMilli();
}

// 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 long 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().toEpochMilli();
}

// 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 long 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().toEpochMilli();
}

@Override
public boolean isSupported() {
return true;
Expand Down
22 changes: 15 additions & 7 deletions backend/commons/src/main/java/org/sonarsource/sonarlint/core/commons/util/git/GitUtils.java
View file
Open in desktop
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, long 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, long 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, long thresholdDate) {
var nativeExecutable = getNativeGitExecutable();
if (nativeExecutable != null) {
for (var relativeFilePath : projectBaseRelativeFilePaths) {
try {
var blameHistoryWindow = getBlameHistoryWindow(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 All @@ -189,6 +192,11 @@ public static SonarLintBlameResult blameFromNativeCommand(Path projectBaseDir, S
throw new IllegalStateException("There is no native Git available");
}

private static String getBlameHistoryWindow(long thresholdDate) {
var blameLimit = Instant.ofEpochMilli(thresholdDate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to what I said earlier, we create Instants that we convert to a timestamp, then back to an Instant

return "--since='" + blameLimit + "'";
}

public static boolean checkIfEnabled(Path projectBaseDir) {
var nativeExecutable = getNativeGitExecutable();
if (nativeExecutable == null) {
Expand Down
90 changes: 88 additions & 2 deletions ...d/commons/src/test/java/org/sonarsource/sonarlint/core/commons/util/git/GitUtilsTest.java
View file
Open in desktop
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, 0);
assertThat(IntStream.of(1, 2, 3)
.mapToObj(lineNumber -> sonarLintBlameResult.getLatestChangeDateForLinesInFile(Path.of("fileA"), List.of(lineNumber))))
.map(Optional::get)
Expand All @@ -131,7 +138,7 @@ 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));
assertThrows(IllegalStateException.class, () -> getBlameResult(projectDirPath, files, null, path -> true, 0));
}

@Test
Expand Down Expand Up @@ -350,6 +357,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().toEpochMilli());

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)).toEpochMilli());

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
11 changes: 9 additions & 2 deletions backend/core/src/main/java/org/sonarsource/sonarlint/core/tracking/TrackingService.java
View file
Open in desktop
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