diff --git a/src/main/java/org/sonar/plugins/scm/perforce/PerforceBlameCommand.java b/src/main/java/org/sonar/plugins/scm/perforce/PerforceBlameCommand.java index efa8847..57e637d 100644 --- a/src/main/java/org/sonar/plugins/scm/perforce/PerforceBlameCommand.java +++ b/src/main/java/org/sonar/plugins/scm/perforce/PerforceBlameCommand.java @@ -20,22 +20,19 @@ package org.sonar.plugins.scm.perforce; import com.google.common.annotations.VisibleForTesting; -import com.perforce.p4java.core.file.FileSpecOpStatus; +import com.perforce.p4java.core.IChangelist; 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.Arrays; -import java.util.Date; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Map.Entry; +import javax.annotation.Nonnull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sonar.api.batch.fs.FileSystem; @@ -47,6 +44,7 @@ public class PerforceBlameCommand extends BlameCommand { private static final Logger LOG = LoggerFactory.getLogger(PerforceBlameCommand.class); private final PerforceConfiguration config; + private final Map changelistMap = new HashMap<>(); public PerforceBlameCommand(PerforceConfiguration config) { this.config = config; @@ -61,78 +59,55 @@ public void blame(BlameInput input, BlameOutput output) { for (InputFile inputFile : input.filesToBlame()) { blame(inputFile, executor.getServer(), output); } + } catch (P4JavaException e) { + throw new IllegalStateException(e.getLocalizedMessage(), e); } finally { executor.clean(); } } @VisibleForTesting - void blame(InputFile inputFile, IOptionsServer server, BlameOutput output) { + void blame(InputFile inputFile, IOptionsServer server, BlameOutput output) throws P4JavaException { IFileSpec fileSpec = createFileSpec(inputFile); - List fileAnnotations; - List revisions; - try { - // Get file annotations - List fileSpecs = Arrays.asList(fileSpec); - fileAnnotations = server.getFileAnnotations(fileSpecs, getFileAnnotationOptions()); - if (fileAnnotations.size() == 1 && fileAnnotations.get(0).getDepotPath() == null) { - LOG.debug("File " + inputFile + " is not submitted. Skipping it."); - return; - } - // Get revision history - Map> revisionMap = server.getRevisionHistory(fileSpecs, getRevisionHistoryOptions()); - Entry> singleEntry = revisionMap.entrySet().iterator().next(); - IFileSpec resultFileSpec = singleEntry.getKey(); - if (!FileSpecOpStatus.VALID.equals(resultFileSpec.getOpStatus()) && !FileSpecOpStatus.INFO.equals(resultFileSpec.getOpStatus())) { - String statusMessage = resultFileSpec.getStatusMessage(); - LOG.debug("Unable to get revisions of file " + inputFile + " [" + statusMessage + "]. Skipping it."); - return; - } - revisions = singleEntry.getValue(); - } catch (P4JavaException e) { - throw new IllegalStateException(e.getLocalizedMessage(), e); - } + List fileSpecs = Collections.singletonList(fileSpec); - computeBlame(inputFile, output, fileAnnotations, revisions); - } - - private void computeBlame(InputFile inputFile, BlameOutput output, List fileAnnotations, List revisions) { - Map changelistDates = new HashMap<>(); - Map changelistAuthors = new HashMap<>(); - for (IFileRevisionData revision : revisions) { - changelistDates.put(revision.getChangelistId(), revision.getDate()); - changelistAuthors.put(revision.getChangelistId(), revision.getUserName()); + // Get file annotations + List fileAnnotations = server.getFileAnnotations(fileSpecs, getFileAnnotationOptions()); + if (fileAnnotations.size() == 1 && fileAnnotations.get(0).getDepotPath() == null) { + LOG.debug("File " + inputFile + " is not submitted. Skipping it."); + return; } + // Compute blame, getting changelist from server if not already retrieved List 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); + } + lines.add(new BlameLine() .revision(String.valueOf(lowerChangelistId)) - .date(changelistDates.get(lowerChangelistId)) - .author(changelistAuthors.get(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 if (lines.size() == (inputFile.lines() - 1)) { - // SONARPLUGINS-3097 Perforce do not report blame on last empty line lines.add(lines.get(lines.size() - 1)); } - output.blameResult(inputFile, lines); - } - /** - * Creating options for revision history command (filelog). - * @return options for requests. - */ - private static GetRevisionHistoryOptions getRevisionHistoryOptions() { - GetRevisionHistoryOptions options = new GetRevisionHistoryOptions(); - options.setIncludeInherited(true); - return options; + output.blameResult(inputFile, lines); } /** * Creating options for file annotation command. * @return options for requests. */ + @Nonnull private static GetFileAnnotationsOptions getFileAnnotationOptions() { GetFileAnnotationsOptions options = new GetFileAnnotationsOptions(); options.setUseChangeNumbers(true); @@ -146,7 +121,8 @@ private static GetFileAnnotationsOptions getFileAnnotationOptions() { * in the current client workspace. * @param inputFile file to create file spec for */ - private static IFileSpec createFileSpec(InputFile inputFile) { + @Nonnull + private static IFileSpec createFileSpec(@Nonnull InputFile inputFile) { IFileSpec fileSpec = new FileSpec(PerforceExecutor.encodeWildcards(inputFile.absolutePath())); fileSpec.setEndRevision(IFileSpec.HAVE_REVISION); return fileSpec; diff --git a/src/main/java/org/sonar/plugins/scm/perforce/PerforceExecutor.java b/src/main/java/org/sonar/plugins/scm/perforce/PerforceExecutor.java index ab7b846..e47b398 100644 --- a/src/main/java/org/sonar/plugins/scm/perforce/PerforceExecutor.java +++ b/src/main/java/org/sonar/plugins/scm/perforce/PerforceExecutor.java @@ -37,6 +37,7 @@ import java.net.URISyntaxException; import java.util.List; import java.util.Properties; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.apache.commons.lang.StringUtils; import org.slf4j.Logger; @@ -58,14 +59,10 @@ public class PerforceExecutor { /** * Instantiates a new p4 command helper. * - * @param repository - * the repository - * @param fileSet - * the file set - * @param logger - * the logger - * @throws ScmException - * the scm exception + * @param config + * the plugin configuration + * @param workDir + * the working directory */ public PerforceExecutor(PerforceConfiguration config, File workDir) { this.config = config; @@ -81,25 +78,6 @@ public IOptionsServer getServer() { return server; } - /** - * Gets the client. - * - * @return the client - */ - public IClient getClient() { - return client; - } - - /** - * Sets the client. - * - * @param client - * the new client - */ - public void setClient(IClient client) { - this.client = client; - } - /** * Initialize Perforce server and client instances. * @@ -167,11 +145,7 @@ private static boolean isLogin(IOptionsServer connection) throws P4JavaException return true; } // If there is a broker or something else that swallows the message - if (status.isEmpty()) { - return true; - } - - return false; + return status.isEmpty(); } private void createServer() throws URISyntaxException, P4JavaException { @@ -295,10 +269,10 @@ private void initClient(File workDir) { /** * Creates the client view mapping. * - * @param repo - * the repo * @param basedir * the basedir + * @param p4ClientName + * the Perforce client name * @return the client view mapping */ private ClientViewMapping createClientViewMapping(File basedir, String p4ClientName) { @@ -315,7 +289,8 @@ private ClientViewMapping createClientViewMapping(File basedir, String p4ClientN * the path * @return the repo location */ - private String getRepoLocation(String path) { + @Nullable + private String getRepoLocation(@Nonnull String path) { String location = null; if (StringUtils.isNotBlank(path) && client != null) { try { @@ -340,7 +315,8 @@ private String getRepoLocation(String path) { * the repo path * @return the canonical repo path */ - private static String getCanonicalRepoPath(String repoPath) { + @Nullable + private static String getCanonicalRepoPath(@Nullable String repoPath) { if (repoPath == null) { return null; } @@ -359,6 +335,7 @@ private static String getCanonicalRepoPath(String repoPath) { * @param filePath the file path * @return the string */ + @Nonnull static String encodeWildcards(@Nullable String filePath) { if (filePath != null) { return filePath.replaceAll("%", "%25").replaceAll("\\*", "%2A").replaceAll("#", "%23").replaceAll("@", "%40"); diff --git a/src/test/java/org/sonar/plugins/scm/perforce/PerforceBlameCommandTest.java b/src/test/java/org/sonar/plugins/scm/perforce/PerforceBlameCommandTest.java index 5ab1a75..db707a7 100644 --- a/src/test/java/org/sonar/plugins/scm/perforce/PerforceBlameCommandTest.java +++ b/src/test/java/org/sonar/plugins/scm/perforce/PerforceBlameCommandTest.java @@ -19,18 +19,14 @@ */ package org.sonar.plugins.scm.perforce; -import com.perforce.p4java.core.file.FileSpecOpStatus; +import com.perforce.p4java.core.IChangelist; 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 java.util.HashMap; -import java.util.List; -import java.util.Map; import org.junit.Test; import org.sonar.api.batch.fs.InputFile; import org.sonar.api.batch.scm.BlameCommand.BlameOutput; @@ -39,6 +35,7 @@ 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; @@ -54,7 +51,7 @@ public void testBlameUnSubmittedFile() throws Exception { IFileAnnotation annotation = mock(IFileAnnotation.class); when(annotation.getDepotPath()).thenReturn(null); - when(server.getFileAnnotations(anyList(), any(GetFileAnnotationsOptions.class))).thenReturn(Arrays.asList(annotation)); + when(server.getFileAnnotations(anyList(), any(GetFileAnnotationsOptions.class))).thenReturn(Collections.singletonList(annotation)); command.blame(mock(InputFile.class), server, blameOutput); @@ -71,24 +68,46 @@ public void testBlameSubmittedFile() throws Exception { when(annotation.getDepotPath()).thenReturn("foo/bar/src/Foo.java"); when(annotation.getLower()).thenReturn(3); - when(server.getFileAnnotations(anyList(), any(GetFileAnnotationsOptions.class))).thenReturn(Arrays.asList(annotation)); + when(server.getFileAnnotations(anyList(), any(GetFileAnnotationsOptions.class))).thenReturn(Arrays.asList(annotation, annotation)); - Map> result = new HashMap<>(); - IFileSpec fileSpecResult = mock(IFileSpec.class); - when(fileSpecResult.getOpStatus()).thenReturn(FileSpecOpStatus.VALID); - IFileRevisionData revision3 = mock(IFileRevisionData.class); - when(revision3.getChangelistId()).thenReturn(3); + IChangelist changelist = mock(IChangelist.class); Date date = new Date(); - when(revision3.getDate()).thenReturn(date); - when(revision3.getUserName()).thenReturn("jhenry"); - result.put(fileSpecResult, Arrays.asList(revision3)); + when(changelist.getDate()).thenReturn(date); + when(changelist.getUsername()).thenReturn("jhenry"); + when(server.getChangelist(3)).thenReturn(changelist); - when(server.getRevisionHistory(anyList(), any(GetRevisionHistoryOptions.class))).thenReturn(result); + 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); + } + + @Test + public void testBlameSubmittedFileLastEmptyLine() throws Exception { + BlameOutput blameOutput = mock(BlameOutput.class); + 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); + + when(server.getFileAnnotations(anyList(), any(GetFileAnnotationsOptions.class))).thenReturn(Collections.singletonList(annotation)); + + IChangelist changelist = mock(IChangelist.class); + Date date = new Date(); + when(changelist.getDate()).thenReturn(date); + when(changelist.getUsername()).thenReturn("jhenry"); + when(server.getChangelist(3)).thenReturn(changelist); InputFile inputFile = mock(InputFile.class); + when(inputFile.lines()).thenReturn(2); command.blame(inputFile, server, blameOutput); - verify(blameOutput).blameResult(inputFile, Arrays.asList(new BlameLine().revision("3").date(date).author("jhenry"))); + BlameLine line = new BlameLine().revision("3").date(date).author("jhenry"); + verify(blameOutput).blameResult(inputFile, Arrays.asList(line, line)); } }