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

Programming exercises: Fix inconsistencies between diff viewer and diff line stats #9984

Merged
merged 7 commits into from
Dec 20, 2024
Merged

Programming exercises: Fix inconsistencies between diff viewer and diff line stats #9984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
29cc46b
add an option to ignore whitespace during diffs
krusche Dec 8, 2024
b137ea9
Allow hunk headers
magaupp Dec 8, 2024
7989b41
Only ignore trimmed whitespace
magaupp Dec 8, 2024
e31ccc3
Merge branch 'develop' into feature/programming/diff-whitespace-option
magaupp Dec 9, 2024
234849c
Add missing JavaDoc
magaupp Dec 10, 2024
fe91f75
Add additional testing assertions
magaupp Dec 10, 2024
a248dc8
Remove unused throws clause
magaupp Dec 10, 2024
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
2 changes: 1 addition & 1 deletion src/main/java/de/tum/cit/aet/artemis/programming/service/CommitHistoryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ private ProgrammingExerciseGitDiffReport createReport(Repository repository, Rev

diffs.append(out.toString(StandardCharsets.UTF_8));
}
var programmingExerciseGitDiffEntries = gitDiffReportParserService.extractDiffEntries(diffs.toString(), false);
var programmingExerciseGitDiffEntries = gitDiffReportParserService.extractDiffEntries(diffs.toString(), false, false);
var report = new ProgrammingExerciseGitDiffReport();
for (ProgrammingExerciseGitDiffEntry gitDiffEntry : programmingExerciseGitDiffEntries) {
gitDiffEntry.setGitDiffReport(report);
Expand Down
22 changes: 12 additions & 10 deletions ...m/cit/aet/artemis/programming/service/hestia/ProgrammingExerciseGitDiffReportService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public ProgrammingExerciseGitDiffReport updateReport(ProgrammingExercise program

var templateHash = templateSubmission.getCommitHash();
var solutionHash = solutionSubmission.getCommitHash();
var existingReport = this.getReportOfExercise(programmingExercise);
var existingReport = getReportOfExercise(programmingExercise);
if (existingReport != null && canUseExistingReport(existingReport, templateHash, solutionHash)) {
return existingReport;
}
Expand Down Expand Up @@ -164,7 +164,7 @@ else if (reports.size() == 1) {
* @return The report or null if none can be generated
*/
public ProgrammingExerciseGitDiffReport getOrCreateReportOfExercise(ProgrammingExercise programmingExercise) {
var report = this.getReportOfExercise(programmingExercise);
var report = getReportOfExercise(programmingExercise);
if (report == null) {
return updateReport(programmingExercise);
}
Expand Down Expand Up @@ -215,7 +215,7 @@ public int calculateNumberOfDiffLinesBetweenRepos(VcsRepositoryUri urlRepoA, Pat
try (var diffOutputStream = new ByteArrayOutputStream(); var git = Git.wrap(repoB)) {
git.diff().setOldTree(treeParserRepoB).setNewTree(treeParserRepoA).setOutputStream(diffOutputStream).call();
var diff = diffOutputStream.toString();
return gitDiffReportParserService.extractDiffEntries(diff, true).stream().mapToInt(ProgrammingExerciseGitDiffEntry::getLineCount).sum();
return gitDiffReportParserService.extractDiffEntries(diff, true, false).stream().mapToInt(ProgrammingExerciseGitDiffEntry::getLineCount).sum();
}
catch (IOException | GitAPIException e) {
log.error("Error calculating number of diff lines between repositories: urlRepoA={}, urlRepoB={}.", urlRepoA, urlRepoB, e);
Expand All @@ -234,6 +234,7 @@ public int calculateNumberOfDiffLinesBetweenRepos(VcsRepositoryUri urlRepoA, Pat
*/
private ProgrammingExerciseGitDiffReport generateReport(TemplateProgrammingExerciseParticipation templateParticipation,
SolutionProgrammingExerciseParticipation solutionParticipation) throws GitAPIException, IOException {
// TODO: in case of LocalVC, we should calculate the diff in the bare origin repository
Repository templateRepo = prepareTemplateRepository(templateParticipation);
var solutionRepo = gitService.getOrCheckoutRepository(solutionParticipation.getVcsRepositoryUri(), true);
gitService.resetToOriginHead(solutionRepo);
Expand Down Expand Up @@ -306,19 +307,20 @@ private ProgrammingExerciseGitDiffReport parseFilesAndCreateReport(Repository re
* It parses all files of the repositories in their directories on the file system and creates a report containing the changes.
* Both repositories have to be checked out at the commit that should be compared and be in different directories
*
* @param repo1 The first repository
* @param oldTreeParser The tree parser for the first repository
* @param newTreeParser The tree parser for the second repository
* @param firstRepo The first repository
* @param firstRepoTreeParser The tree parser for the first repository
* @param secondRepoTreeParser The tree parser for the second repository
* @return The report with the changes between the two repositories at their checked out state
* @throws IOException If an error occurs while accessing the file system
* @throws GitAPIException If an error occurs while accessing the git repository
*/
@NotNull
private ProgrammingExerciseGitDiffReport createReport(Repository repo1, FileTreeIterator oldTreeParser, FileTreeIterator newTreeParser) throws IOException, GitAPIException {
try (ByteArrayOutputStream diffOutputStream = new ByteArrayOutputStream(); Git git = Git.wrap(repo1)) {
git.diff().setOldTree(oldTreeParser).setNewTree(newTreeParser).setOutputStream(diffOutputStream).call();
private ProgrammingExerciseGitDiffReport createReport(Repository firstRepo, FileTreeIterator firstRepoTreeParser, FileTreeIterator secondRepoTreeParser)
throws IOException, GitAPIException {
try (ByteArrayOutputStream diffOutputStream = new ByteArrayOutputStream(); Git git = Git.wrap(firstRepo)) {
git.diff().setOldTree(firstRepoTreeParser).setNewTree(secondRepoTreeParser).setOutputStream(diffOutputStream).call();
var diff = diffOutputStream.toString();
var programmingExerciseGitDiffEntries = gitDiffReportParserService.extractDiffEntries(diff, false);
var programmingExerciseGitDiffEntries = gitDiffReportParserService.extractDiffEntries(diff, false, true);
var report = new ProgrammingExerciseGitDiffReport();
for (ProgrammingExerciseGitDiffEntry gitDiffEntry : programmingExerciseGitDiffEntries) {
gitDiffEntry.setGitDiffReport(report);
Expand Down
111 changes: 74 additions & 37 deletions src/main/java/de/tum/cit/aet/artemis/programming/web/GitDiffReportParserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,17 @@ public class GitDiffReportParserService {

private static final String PREFIX_RENAME_TO = "rename to ";

private final Pattern gitDiffLinePattern = Pattern.compile("@@ -(?<previousLine>\\d+)(,(?<previousLineCount>\\d+))? \\+(?<newLine>\\d+)(,(?<newLineCount>\\d+))? @@");
private final Pattern gitDiffLinePattern = Pattern.compile("@@ -(?<previousLine>\\d+)(,(?<previousLineCount>\\d+))? \\+(?<newLine>\\d+)(,(?<newLineCount>\\d+))? @@.*");

/**
* Extracts the ProgrammingExerciseGitDiffEntry from the raw git-diff output
*
* @param diff The raw git-diff output
* @param useAbsoluteLineCount Whether to use absolute line count or previous line count
* @param ignoreWhitespace Whether to ignore entries where only leading and trailing whitespace differ
* @return The extracted ProgrammingExerciseGitDiffEntries
*/
public List<ProgrammingExerciseGitDiffEntry> extractDiffEntries(String diff, boolean useAbsoluteLineCount) {
public List<ProgrammingExerciseGitDiffEntry> extractDiffEntries(String diff, boolean useAbsoluteLineCount, boolean ignoreWhitespace) {
var lines = diff.split("\n");
var parserState = new ParserState();
Map<String, String> renamedFilePaths = new HashMap<>();
Expand All @@ -44,8 +45,7 @@ public List<ProgrammingExerciseGitDiffEntry> extractDiffEntries(String diff, boo
continue;
}

// Files may be renamed without changes, in which case the lineMatcher will never match the entry
// We store this information separately so it is not lost
// Check for renamed files
if (line.startsWith(PREFIX_RENAME_FROM) && i + 1 < lines.length) {
var nextLine = lines[i + 1];
if (nextLine.startsWith(PREFIX_RENAME_TO)) {
Expand All @@ -57,75 +57,81 @@ public List<ProgrammingExerciseGitDiffEntry> extractDiffEntries(String diff, boo

var lineMatcher = gitDiffLinePattern.matcher(line);
if (lineMatcher.matches()) {
handleNewDiffBlock(lines, i, parserState, lineMatcher);
handleNewDiffBlock(lines, i, parserState, lineMatcher, ignoreWhitespace);
}
else if (!parserState.deactivateCodeReading) {
else if (!parserState.deactivateCodeReading && !line.isEmpty()) {
switch (line.charAt(0)) {
case '+' -> handleAddition(parserState);
case '-' -> handleRemoval(parserState, useAbsoluteLineCount);
case ' ' -> handleUnchanged(parserState);
case '+' -> handleAddition(parserState, line);
case '-' -> handleRemoval(parserState, useAbsoluteLineCount, line);
case ' ' -> handleUnchanged(parserState, ignoreWhitespace);
default -> parserState.deactivateCodeReading = true;
}
}
}
if (!parserState.currentEntry.isEmpty()) {
parserState.entries.add(parserState.currentEntry);
}
// Add an empty diff entry for renamed files without changes

// Check the last entry
finalizeEntry(parserState, ignoreWhitespace);

// Add empty entries for renamed files without changes
for (var entry : renamedFilePaths.entrySet()) {
var diffEntry = new ProgrammingExerciseGitDiffEntry();
diffEntry.setFilePath(entry.getValue());
diffEntry.setPreviousFilePath(entry.getKey());
parserState.entries.add(diffEntry);
}

return parserState.entries;
}

private void handleNewDiffBlock(String[] lines, int currentLine, ParserState parserState, Matcher lineMatcher) {
if (!parserState.currentEntry.isEmpty()) {
parserState.entries.add(parserState.currentEntry);
}
private void handleNewDiffBlock(String[] lines, int currentLine, ParserState parserState, Matcher lineMatcher, boolean ignoreWhitespace) {
finalizeEntry(parserState, ignoreWhitespace);

// Start of a new file
var newFilePath = getFilePath(lines, currentLine);
var newPreviousFilePath = getPreviousFilePath(lines, currentLine);
if (newFilePath != null || newPreviousFilePath != null) {
parserState.currentFilePath = newFilePath;
parserState.currentPreviousFilePath = newPreviousFilePath;
}

parserState.currentEntry = new ProgrammingExerciseGitDiffEntry();
parserState.currentEntry.setFilePath(parserState.currentFilePath);
parserState.currentEntry.setPreviousFilePath(parserState.currentPreviousFilePath);
parserState.currentLineCount = Integer.parseInt(lineMatcher.group("newLine"));
parserState.currentPreviousLineCount = Integer.parseInt(lineMatcher.group("previousLine"));
parserState.deactivateCodeReading = false;
parserState.addedLines.clear();
parserState.removedLines.clear();
}

private void handleUnchanged(ParserState parserState) {
var entry = parserState.currentEntry;
if (!entry.isEmpty()) {
parserState.entries.add(entry);
}
entry = new ProgrammingExerciseGitDiffEntry();
entry.setFilePath(parserState.currentFilePath);
entry.setPreviousFilePath(parserState.currentPreviousFilePath);
private void handleUnchanged(ParserState parserState, boolean ignoreWhitespace) {
finalizeEntry(parserState, ignoreWhitespace);
parserState.currentEntry = new ProgrammingExerciseGitDiffEntry();
parserState.currentEntry.setFilePath(parserState.currentFilePath);
parserState.currentEntry.setPreviousFilePath(parserState.currentPreviousFilePath);

parserState.currentEntry = entry;
parserState.lastLineRemoveOperation = false;
parserState.currentLineCount++;
parserState.currentPreviousLineCount++;
parserState.addedLines.clear();
parserState.removedLines.clear();
}

private void handleRemoval(ParserState parserState, boolean useAbsoluteLineCount) {
private void handleRemoval(ParserState parserState, boolean useAbsoluteLineCount, String line) {
var entry = parserState.currentEntry;
if (!parserState.lastLineRemoveOperation && !entry.isEmpty()) {
parserState.entries.add(entry);
entry = new ProgrammingExerciseGitDiffEntry();
entry.setFilePath(parserState.currentFilePath);
entry.setPreviousFilePath(parserState.currentPreviousFilePath);
finalizeEntry(parserState, false);
parserState.currentEntry = new ProgrammingExerciseGitDiffEntry();
parserState.currentEntry.setFilePath(parserState.currentFilePath);
parserState.currentEntry.setPreviousFilePath(parserState.currentPreviousFilePath);
}
if (entry.getPreviousLineCount() == null) {
entry.setPreviousLineCount(0);
entry.setPreviousStartLine(parserState.currentPreviousLineCount);

// Store removed line
parserState.removedLines.add(line.substring(1));

if (parserState.currentEntry.getPreviousLineCount() == null) {
parserState.currentEntry.setPreviousLineCount(0);
parserState.currentEntry.setPreviousStartLine(parserState.currentPreviousLineCount);
}
if (useAbsoluteLineCount) {
if (parserState.currentEntry.getLineCount() == null) {
Expand All @@ -135,15 +141,17 @@ private void handleRemoval(ParserState parserState, boolean useAbsoluteLineCount
parserState.currentEntry.setLineCount(parserState.currentEntry.getLineCount() + 1);
}
else {
entry.setPreviousLineCount(entry.getPreviousLineCount() + 1);
parserState.currentEntry.setPreviousLineCount(parserState.currentEntry.getPreviousLineCount() + 1);
}

parserState.currentEntry = entry;
parserState.lastLineRemoveOperation = true;
parserState.currentPreviousLineCount++;
}

private void handleAddition(ParserState parserState) {
private void handleAddition(ParserState parserState, String line) {
// Store added line
parserState.addedLines.add(line.substring(1));

if (parserState.currentEntry.getLineCount() == null) {
parserState.currentEntry.setLineCount(0);
parserState.currentEntry.setStartLine(parserState.currentLineCount);
Expand All @@ -154,6 +162,29 @@ private void handleAddition(ParserState parserState) {
parserState.currentLineCount++;
}

private void finalizeEntry(ParserState parserState, boolean ignoreWhitespace) {
if (!parserState.currentEntry.isEmpty()) {
if (!ignoreWhitespace || !isWhitespaceOnlyChange(parserState.addedLines, parserState.removedLines)) {
parserState.entries.add(parserState.currentEntry);
}
}
}

private boolean isWhitespaceOnlyChange(List<String> addedLines, List<String> removedLines) {
if (addedLines.size() != removedLines.size()) {
return false; // Different number of lines changed, definitely not whitespace only
}

for (int i = 0; i < addedLines.size(); i++) {
String added = addedLines.get(i).trim();
String removed = removedLines.get(i).trim();
if (!added.equals(removed)) {
return false;
}
}
return true;
}

/**
* Extracts the file path from the raw git-diff for a specified diff block
*
Expand Down Expand Up @@ -219,13 +250,19 @@ private static class ParserState {

private int currentPreviousLineCount;

private final List<String> addedLines;

private final List<String> removedLines;

public ParserState() {
entries = new ArrayList<>();
currentEntry = new ProgrammingExerciseGitDiffEntry();
deactivateCodeReading = true;
lastLineRemoveOperation = false;
currentLineCount = 0;
currentPreviousLineCount = 0;
addedLines = new ArrayList<>();
removedLines = new ArrayList<>();
}
}
}
2 changes: 1 addition & 1 deletion ...de/tum/cit/aet/artemis/UnionFindTest.java → .../cit/aet/artemis/atlas/UnionFindTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package de.tum.cit.aet.artemis;
package de.tum.cit.aet.artemis.atlas;

import static org.assertj.core.api.Assertions.assertThat;

Expand Down
Loading
Loading