Skip to content

Commit

Permalink
Use p4 filelog and p4 annotate first, then get changelist
Browse files Browse the repository at this point in the history 
In order to try to optimize the blame process for a file in Perforce,
first run filelog which usually returns all revisions, then run
annotate. If we then encounter a changelist we don't know, attempt to
look it up. If that fails, too, fail gracefully.
  • Loading branch information
@tyrel @henryju
tyrel authored and henryju committed Feb 18, 2016
1 parent 367bd9e commit b88e6ea
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 36 deletions.
83 changes: 69 additions & 14 deletions src/main/java/org/sonar/plugins/scm/perforce/PerforceBlameCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,17 @@

import com.google.common.annotations.VisibleForTesting;
import com.perforce.p4java.core.IChangelist;
import com.perforce.p4java.core.file.FileSpecOpStatus;
import com.perforce.p4java.core.file.IFileAnnotation;
import com.perforce.p4java.core.file.IFileRevisionData;
import com.perforce.p4java.core.file.IFileSpec;
import com.perforce.p4java.exception.P4JavaException;
import com.perforce.p4java.impl.generic.core.file.FileSpec;
import com.perforce.p4java.option.server.GetFileAnnotationsOptions;
import com.perforce.p4java.option.server.GetRevisionHistoryOptions;
import com.perforce.p4java.server.IOptionsServer;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import java.util.*;
import javax.annotation.Nonnull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -44,6 +44,7 @@ public class PerforceBlameCommand extends BlameCommand {

private static final Logger LOG = LoggerFactory.getLogger(PerforceBlameCommand.class);
private final PerforceConfiguration config;
private final Map<Integer, IFileRevisionData> revisionDataMap = new HashMap<>();
private final Map<Integer, IChangelist> changelistMap = new HashMap<>();

public PerforceBlameCommand(PerforceConfiguration config) {
Expand Down Expand Up @@ -78,21 +79,61 @@ void blame(InputFile inputFile, IOptionsServer server, BlameOutput output) throw
return;
}

// Get history of file
Map<IFileSpec, List<IFileRevisionData>> revisionMap = server.getRevisionHistory(fileSpecs, getRevisionHistoryOptions());
for (Map.Entry<IFileSpec, List<IFileRevisionData>> entry : revisionMap.entrySet()) {
IFileSpec revisionFileSpec = entry.getKey();
if (!FileSpecOpStatus.VALID.equals(revisionFileSpec.getOpStatus()) && !FileSpecOpStatus.INFO.equals(revisionFileSpec.getOpStatus())) {
String statusMessage = fileSpec.getStatusMessage();
LOG.debug("Unable to get revisions of file " + inputFile + " [" + statusMessage + "]. Skipping it.");
return;
}
for (IFileRevisionData revisionData : entry.getValue()) {
revisionDataMap.put(revisionData.getChangelistId(), revisionData);
}
}

// Compute blame, getting changelist from server if not already retrieved
List<BlameLine> lines = new ArrayList<>();
for (IFileAnnotation fileAnnotation : fileAnnotations) {
int lowerChangelistId = fileAnnotation.getLower();

IChangelist changelist = changelistMap.get(lowerChangelistId);
if (changelist == null) {
changelist = server.getChangelist(lowerChangelistId);
changelistMap.put(lowerChangelistId, changelist);
IFileRevisionData data = revisionDataMap.get(lowerChangelistId);
if (data != null) {

lines.add(new BlameLine()
.revision(String.valueOf(lowerChangelistId))
.date(data.getDate())
.author(data.getUserName()));

} else {
// Sometimes they're missing from the revision history, so try to get it directly.

LOG.debug("Changelist " + lowerChangelistId + " was not found in history for " + inputFile + ". It will be fetched directly.");
IChangelist changelist = changelistMap.get(lowerChangelistId);
if (changelist == null) {
changelist = server.getChangelist(lowerChangelistId);
if (changelist != null) { // sometimes even that can fail due to cross-server imports
changelistMap.put(lowerChangelistId, changelist);
}
}

if (changelist != null) {
lines.add(new BlameLine()
.revision(String.valueOf(lowerChangelistId))
.date(changelist.getDate())
.author(changelist.getUsername()));
} else {
// We really couldn't get any information for this changelist!
// Unfortunately, blame information is required for every line...
lines.add(new BlameLine()
.revision(String.valueOf(lowerChangelistId))
.date(new Date(0))
.author("unknown"));
}

}

lines.add(new BlameLine()
.revision(String.valueOf(lowerChangelistId))
.date(changelist.getDate())
.author(changelist.getUsername()));
}

// SONARPLUGINS-3097: Perforce does not report blame on last empty line, so populate from last line with blame
Expand All @@ -111,11 +152,25 @@ void blame(InputFile inputFile, IOptionsServer server, BlameOutput output) throw
private static GetFileAnnotationsOptions getFileAnnotationOptions() {
GetFileAnnotationsOptions options = new GetFileAnnotationsOptions();
options.setUseChangeNumbers(true);
options.setFollowAllIntegrations(true);
options.setFollowBranches(true);
options.setIgnoreWhitespaceChanges(true);
return options;
}

/**
* Creating options for revision history command (filelog).
*
* @return options for requests.
*/
private static GetRevisionHistoryOptions getRevisionHistoryOptions() {
GetRevisionHistoryOptions options = new GetRevisionHistoryOptions();
options.setIncludeInherited(true);
options.setLongOutput(true);
options.setMaxRevs(1000);
options.setOmitNonContributaryIntegrations(true);
return options;
}

/**
* Creates file spec for the specified file taking into an account that we are interested in a revision that we have
* in the current client workspace.
Expand Down
69 changes: 47 additions & 22 deletions src/test/java/org/sonar/plugins/scm/perforce/PerforceBlameCommandTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,23 @@
package org.sonar.plugins.scm.perforce;

import com.perforce.p4java.core.IChangelist;
import com.perforce.p4java.core.file.FileSpecOpStatus;
import com.perforce.p4java.core.file.IFileAnnotation;
import com.perforce.p4java.core.file.IFileRevisionData;
import com.perforce.p4java.core.file.IFileSpec;
import com.perforce.p4java.option.server.GetFileAnnotationsOptions;
import com.perforce.p4java.option.server.GetRevisionHistoryOptions;
import com.perforce.p4java.server.IOptionsServer;

import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import org.junit.Test;
import org.sonar.api.batch.fs.InputFile;
import org.sonar.api.batch.scm.BlameCommand.BlameOutput;
import org.sonar.api.batch.scm.BlameLine;

import java.util.*;

import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyList;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;
import static org.mockito.Matchers.anyListOf;
import static org.mockito.Mockito.*;

public class PerforceBlameCommandTest {

Expand All @@ -51,7 +49,7 @@ public void testBlameUnSubmittedFile() throws Exception {
IFileAnnotation annotation = mock(IFileAnnotation.class);
when(annotation.getDepotPath()).thenReturn(null);

when(server.getFileAnnotations(anyList(), any(GetFileAnnotationsOptions.class))).thenReturn(Collections.singletonList(annotation));
when(server.getFileAnnotations(anyListOf(IFileSpec.class), any(GetFileAnnotationsOptions.class))).thenReturn(Collections.singletonList(annotation));

command.blame(mock(InputFile.class), server, blameOutput);

Expand All @@ -64,24 +62,51 @@ public void testBlameSubmittedFile() throws Exception {
IOptionsServer server = mock(IOptionsServer.class);
PerforceBlameCommand command = new PerforceBlameCommand(mock(PerforceConfiguration.class));

IFileAnnotation annotation = mock(IFileAnnotation.class);
when(annotation.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation.getLower()).thenReturn(3);
IFileAnnotation annotation1 = mock(IFileAnnotation.class);
when(annotation1.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation1.getLower()).thenReturn(3);

when(server.getFileAnnotations(anyList(), any(GetFileAnnotationsOptions.class))).thenReturn(Arrays.asList(annotation, annotation));
IFileAnnotation annotation2 = mock(IFileAnnotation.class);
when(annotation2.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation2.getLower()).thenReturn(3);

IChangelist changelist = mock(IChangelist.class);
IFileAnnotation annotation3 = mock(IFileAnnotation.class);
when(annotation3.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation3.getLower()).thenReturn(4);

IFileAnnotation annotation4 = mock(IFileAnnotation.class);
when(annotation4.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation4.getLower()).thenReturn(5);

Map<IFileSpec, List<IFileRevisionData>> result = new HashMap<>();
IFileSpec fileSpecResult = mock(IFileSpec.class);
when(fileSpecResult.getOpStatus()).thenReturn(FileSpecOpStatus.VALID);
IFileRevisionData revision3 = mock(IFileRevisionData.class);
when(revision3.getChangelistId()).thenReturn(3);
Date date = new Date();
when(revision3.getDate()).thenReturn(date);
when(revision3.getUserName()).thenReturn("jhenry");
result.put(fileSpecResult, Collections.singletonList(revision3));

when(server.getRevisionHistory(anyListOf(IFileSpec.class), any(GetRevisionHistoryOptions.class))).thenReturn(result);
when(server.getFileAnnotations(anyListOf(IFileSpec.class), any(GetFileAnnotationsOptions.class))).thenReturn(Arrays.asList(annotation1, annotation2, annotation3, annotation4));

IChangelist changelist = mock(IChangelist.class);
when(changelist.getDate()).thenReturn(date);
when(changelist.getUsername()).thenReturn("jhenry");
when(server.getChangelist(3)).thenReturn(changelist);
when(changelist.getUsername()).thenReturn("bgates");
when(server.getChangelist(4)).thenReturn(changelist);

when(server.getChangelist(5)).thenReturn(null);

InputFile inputFile = mock(InputFile.class);
command.blame(inputFile, server, blameOutput);

BlameLine line = new BlameLine().revision("3").date(date).author("jhenry");
verify(blameOutput).blameResult(inputFile, Arrays.asList(line, line));
verify(server, times(1)).getChangelist(3);
BlameLine line1 = new BlameLine().revision("3").date(date).author("jhenry");
BlameLine line2 = new BlameLine().revision("3").date(date).author("jhenry");
BlameLine line3 = new BlameLine().revision("4").date(date).author("bgates");
BlameLine line4 = new BlameLine().revision("5").date(new Date(0)).author("unknown");
verify(blameOutput).blameResult(inputFile, Arrays.asList(line1, line2, line3, line4));
verify(server, times(1)).getChangelist(4);
}

@Test
Expand All @@ -94,7 +119,7 @@ public void testBlameSubmittedFileLastEmptyLine() throws Exception {
when(annotation.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation.getLower()).thenReturn(3);

when(server.getFileAnnotations(anyList(), any(GetFileAnnotationsOptions.class))).thenReturn(Collections.singletonList(annotation));
when(server.getFileAnnotations(anyListOf(IFileSpec.class), any(GetFileAnnotationsOptions.class))).thenReturn(Collections.singletonList(annotation));

IChangelist changelist = mock(IChangelist.class);
Date date = new Date();
Expand Down

0 comments on commit b88e6ea

Please sign in to comment.