Simplify blame logic and improve coverage.

Move P4JavaException to unchecked conversion to outer try block,
allowing us to remove the inner try block and reduce complexity.  The
location of this conversion doesn't matter.

Iterate file annotations only once.

Add additional line annotation in test.  Ensure server.getChangelist is
invoked once to verify storing/retrieval from changelistMap works as
intended.

Add test for blame on last empty line.

src/main/java/org/sonar/plugins/scm/perforce/PerforceBlameCommand.java

@@ -59,51 +59,47 @@ 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<IFileAnnotation> fileAnnotations;
-    try {
-      // Get file annotations
-      List<IFileSpec> fileSpecs = Collections.singletonList(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 changelists
-      for (IFileAnnotation fileAnnotation : fileAnnotations) {
-        int lowerChangelistId = fileAnnotation.getLower();
-        if (!changelistMap.containsKey(lowerChangelistId)) {
-          changelistMap.put(lowerChangelistId, server.getChangelist(lowerChangelistId));
-        }
-      }
-    } catch (P4JavaException e) {
-      throw new IllegalStateException(e.getLocalizedMessage(), e);
-    }
+    List<IFileSpec> fileSpecs = Collections.singletonList(fileSpec);
 
-    computeBlame(inputFile, output, fileAnnotations);
-  }
+    // Get file annotations
+    List<IFileAnnotation> fileAnnotations = server.getFileAnnotations(fileSpecs, getFileAnnotationOptions());
+    if (fileAnnotations.size() == 1 && fileAnnotations.get(0).getDepotPath() == null) {
+      LOG.debug("File " + inputFile + " is not submitted. Skipping it.");
+      return;
+    }
 
-  private void computeBlame(InputFile inputFile, BlameOutput output, List<IFileAnnotation> fileAnnotations) {
+    // 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);
+      }
+
       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
     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);
   }
 

src/test/java/org/sonar/plugins/scm/perforce/PerforceBlameCommandTest.java

@@ -23,6 +23,8 @@
 import com.perforce.p4java.core.file.IFileAnnotation;
 import com.perforce.p4java.option.server.GetFileAnnotationsOptions;
 import com.perforce.p4java.server.IOptionsServer;
+
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Date;
 import org.junit.Test;
@@ -33,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;
@@ -65,6 +68,32 @@ 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, 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);
+    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);
@@ -74,9 +103,11 @@ public void testBlameSubmittedFile() throws Exception {
     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, Collections.singletonList(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));
   }
 
 }