Merge pull request #6 from mixalturek/master

[JENKINS-14481] Quickly identify only the new errors
jenkinsci · Mar 10, 2014 · ce2c74b · ce2c74b
2 parents 7f9f96c + 0ee027c
commit ce2c74b
Show file tree

Hide file tree

Showing 7 changed files with 221 additions and 9 deletions.
diff --git a/src/main/java/com/thalesgroup/hudson/plugins/cppcheck/model/CppcheckFile.java b/src/main/java/com/thalesgroup/hudson/plugins/cppcheck/model/CppcheckFile.java
@@ -51,6 +51,15 @@ public String getFileName() {
         return fileName;
     }
 
+    /**
+     * Get the filename.
+     * 
+     * @return the filename or empty string if the filename is null
+     */
+    public String getFileNameNotNull() {
+        return (fileName != null) ? fileName : "";
+    }
+
     public void setFileName(String filename) {
         this.fileName = filename;
     }
@@ -60,6 +69,15 @@ public int getLineNumber() {
         return lineNumber;
     }
 
+    /**
+     * Get line number depending on availability of the file name.
+     * 
+     * @return the line number or empty string if the file name is empty
+     */
+    public String getLineNumberString() {
+        return ("".equals(getFileNameNotNull())) ? "" : String.valueOf(lineNumber);
+    }
+
     /**
      * Returns the line number that should be shown on top of the source code view.
      *
@@ -112,5 +130,4 @@ public void setKey(Integer key) {
     public String getDisplayName() {
         return "cppcheckFile";
     }
-
 }
diff --git a/src/main/java/com/thalesgroup/hudson/plugins/cppcheck/model/CppcheckSourceContainer.java b/src/main/java/com/thalesgroup/hudson/plugins/cppcheck/model/CppcheckSourceContainer.java
@@ -67,7 +67,7 @@ public CppcheckSourceContainer(BuildListener listener, FilePath basedir, List<Cp
             cppcheckFile.setKey(key);
             cppcheckWorkspaceFile.setCppcheckFile(cppcheckFile);
             internalMap.put(key, cppcheckWorkspaceFile);
-            key = ++key;
+            ++key;
         }
     }
 

diff --git a/src/main/java/com/thalesgroup/hudson/plugins/cppcheck/model/CppcheckWorkspaceFile.java b/src/main/java/com/thalesgroup/hudson/plugins/cppcheck/model/CppcheckWorkspaceFile.java
@@ -25,11 +25,11 @@
 
 import hudson.model.AbstractBuild;
 import org.apache.commons.lang.StringUtils;
+import org.jenkinsci.plugins.cppcheck.CppcheckDiffState;
 
 import java.io.File;
 
 public class CppcheckWorkspaceFile {
-
     /**
      * Temporary directory holding the workspace files.
      */
@@ -40,10 +40,18 @@ public class CppcheckWorkspaceFile {
     private CppcheckFile cppcheckFile;
 
     /**
-     * Useful for files that are not found on the buid filestystem
+     * Useful for files that are not found on the build file system
      */
     private boolean sourceIgnored;
 
+    /**
+     * State of compare. It is a runtime parameter, don't store it anywhere
+     * (transient).
+     * 
+     * @since 1.15
+     */
+    private transient CppcheckDiffState diffState = null;
+
     public CppcheckWorkspaceFile(File file) {
         if (file != null)
             this.fileName = file.getAbsolutePath().replace('\\', '/');
@@ -104,4 +112,12 @@ public boolean isSourceIgnored() {
     public void setSourceIgnored(boolean sourceIgnored) {
         this.sourceIgnored = sourceIgnored;
     }
+
+    public CppcheckDiffState getDiffState() {
+        return diffState;
+    }
+
+    public void setDiffState(CppcheckDiffState diffState) {
+        this.diffState = diffState;
+    }
 }
diff --git a/src/main/java/org/jenkinsci/plugins/cppcheck/CppcheckDiffState.java b/src/main/java/org/jenkinsci/plugins/cppcheck/CppcheckDiffState.java
@@ -0,0 +1,65 @@
+package org.jenkinsci.plugins.cppcheck;
+
+/**
+ * Status of comparison of two reports.
+ * 
+ * Implementation note: The declaration order of the constants is significant,
+ * it's used in sorting.
+ * 
+ * @see CppcheckResult#diffCurrentAndPrevious()
+ * @author Michal Turek
+ */
+public enum CppcheckDiffState {
+    /** The issue is present only in the current report. */
+    NEW {
+        @Override
+        public String getCss() {
+            return "new";
+        }
+
+        @Override
+        public String getText() {
+            return Messages.cppcheck_DiffNew();
+        }
+    },
+
+    /** The issue is present only in the previous report. */
+    SOLVED {
+        @Override
+        public String getCss() {
+            return "solved";
+        }
+
+        @Override
+        public String getText() {
+            return Messages.cppcheck_DiffSolved();
+        }
+    },
+
+    /** The issue is present in both the current and the previous report. */
+    UNCHANGED {
+        @Override
+        public String getCss() {
+            return "unchanged";
+        }
+
+        @Override
+        public String getText() {
+            return Messages.cppcheck_DiffUnchanged();
+        }
+    };
+
+    /**
+     * Get CSS class.
+     * 
+     * @return the class
+     */
+    public abstract String getCss();
+
+    /**
+     * Get localized text.
+     * 
+     * @return the localized text
+     */
+    public abstract String getText();
+}
diff --git a/src/main/java/org/jenkinsci/plugins/cppcheck/CppcheckResult.java b/src/main/java/org/jenkinsci/plugins/cppcheck/CppcheckResult.java
@@ -1,6 +1,7 @@
 package org.jenkinsci.plugins.cppcheck;
 
 import com.thalesgroup.hudson.plugins.cppcheck.CppcheckSource;
+import com.thalesgroup.hudson.plugins.cppcheck.model.CppcheckFile;
 import com.thalesgroup.hudson.plugins.cppcheck.model.CppcheckWorkspaceFile;
 
 import hudson.XmlFile;
@@ -17,7 +18,12 @@
 import java.io.File;
 import java.io.IOException;
 import java.io.Serializable;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -294,6 +300,104 @@ public int getNumberErrorsAccordingConfiguration(
         }
     }
 
+    /**
+     * Compare current and previous source containers. The code first tries to
+     * find all exact matches (file, line, message) and then all approximate
+     * matches (file, message). It is possible the line number will change if
+     * a developer updates the source code somewhere above the issue. Move of
+     * the code to a different file e.g. during refactoring is not considered
+     * and one solved and one new issue will be highlighted in such case.
+     * 
+     * @return the result of the comparison
+     */
+    public Collection<CppcheckWorkspaceFile> diffCurrentAndPrevious() {
+        CppcheckSourceContainer cur = getCppcheckSourceContainer();
+        CppcheckResult prevResult = getPreviousResult();
+        List<CppcheckWorkspaceFile> curValues
+                = new ArrayList<CppcheckWorkspaceFile>(cur.getInternalMap().values());
+
+        if(prevResult == null) {
+            for(CppcheckWorkspaceFile file : curValues) {
+                file.setDiffState(CppcheckDiffState.UNCHANGED);
+            }
+
+            return curValues;
+        }
+
+        CppcheckSourceContainer prev = prevResult.getCppcheckSourceContainer();
+        Collection<CppcheckWorkspaceFile> prevValues = prev.getInternalMap().values();
+
+        // Exact match first
+        for(CppcheckWorkspaceFile curFile : curValues) {
+            CppcheckFile curCppFile = curFile.getCppcheckFile();
+
+            for(CppcheckWorkspaceFile prevFile : prevValues) {
+                CppcheckFile prevCppFile = prevFile.getCppcheckFile();
+
+                if (curCppFile.getLineNumber() == prevCppFile.getLineNumber()
+                        && curCppFile.getFileNameNotNull().equals(prevCppFile.getFileNameNotNull())
+                        && curCppFile.getMessage().equals(prevCppFile.getMessage())) {
+                    curFile.setDiffState(CppcheckDiffState.UNCHANGED);
+                    prevFile.setDiffState(CppcheckDiffState.UNCHANGED);
+                    break;
+                }
+            }
+        }
+
+        // Approximate match of the rest (ignore line numbers)
+        for(CppcheckWorkspaceFile curFile : curValues) {
+            if(curFile.getDiffState() != null) {
+                continue;
+            }
+
+            CppcheckFile curCppFile = curFile.getCppcheckFile();
+
+            for(CppcheckWorkspaceFile prevFile : prevValues) {
+                if(prevFile.getDiffState() != null) {
+                    continue;
+                }
+
+                CppcheckFile prevCppFile = prevFile.getCppcheckFile();
+
+                if (curCppFile.getFileNameNotNull().equals(prevCppFile.getFileNameNotNull())
+                        && curCppFile.getMessage().equals(prevCppFile.getMessage())) {
+                    curFile.setDiffState(CppcheckDiffState.UNCHANGED);
+                    prevFile.setDiffState(CppcheckDiffState.UNCHANGED);
+                    break;
+                }
+            }
+        }
+
+        // Label all new
+        for(CppcheckWorkspaceFile curFile : curValues) {
+            if(curFile.getDiffState() != null) {
+                continue;
+            }
+
+            curFile.setDiffState(CppcheckDiffState.NEW);
+        }
+
+        // Add and label all solved
+        for(CppcheckWorkspaceFile prevFile : prevValues) {
+            if(prevFile.getDiffState() != null) {
+                continue;
+            }
+
+            prevFile.setDiffState(CppcheckDiffState.SOLVED);
+            prevFile.setSourceIgnored(true);
+            curValues.add(prevFile);
+        }
+
+        // Sort according to the compare flag
+        Collections.sort(curValues, new Comparator<CppcheckWorkspaceFile>() {
+            public int compare(CppcheckWorkspaceFile a, CppcheckWorkspaceFile b) {
+                return a.getDiffState().ordinal() - b.getDiffState().ordinal();
+            }
+        });
+
+        return curValues;
+    }
+
     /**
      * Convert legacy data in format of cppcheck plugin version 1.14
      * to the new one that uses statistics.

diff --git a/src/main/resources/org/jenkinsci/plugins/cppcheck/CppcheckResult/details.jelly b/src/main/resources/org/jenkinsci/plugins/cppcheck/CppcheckResult/details.jelly
@@ -1,7 +1,7 @@
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:g="/jelly/cppcheck">
     <st:header name="Content-Type" value="text/html;charset=UTF-8"/>
 
-    <j:set var="cachedContainer" value="${it.cppcheckSourceContainer}"/>
+    <j:set var="cachedContainer" value="${it.diffCurrentAndPrevious()}"/>
 
     <j:if test="${cachedContainer != null}">
 
@@ -10,11 +10,15 @@
         <style type="text/css">
         #cppcheckDetails { width: auto; }
         #cppcheckDetails td { white-space: normal; }
+        #cppcheckDetails .new { background-color: #FFC8C8; }
+        #cppcheckDetails .solved { background-color: #C8FFC8; }
+        #cppcheckDetails .unchanged { }
         </style>
 
         <table class="pane sortable" id="cppcheckDetails">
             <thead>
                 <tr>
+                    <td class="pane-header">${%State}</td>
                     <td class="pane-header">${%File}</td>
                     <td class="pane-header">${%Line}</td>
                     <td class="pane-header">${%Severity}</td>
@@ -23,9 +27,11 @@
                 </tr>
             </thead>
             <tbody>
-                <j:forEach var="elt" items="${cachedContainer.internalMap.values()}">
+                <j:forEach var="elt" items="${cachedContainer}">
                     <j:set var="cppcheckfile" value="${elt.cppcheckFile}"/>
-                    <tr>
+
+                    <tr class="${elt.diffState.css}">
+                        <td class="pane">${elt.diffState.text}</td>
                         <td class="pane">
                             <j:if test="${elt.isSourceIgnored()}">
                                 ${cppcheckfile.fileName}
@@ -36,10 +42,10 @@
                         </td>
                         <td class="pane">
                             <j:if test="${elt.isSourceIgnored()}">
-                                ${cppcheckfile.lineNumber}
+                                ${cppcheckfile.lineNumberString}
                             </j:if>
                             <j:if test="${not elt.isSourceIgnored()}">
-                                <a href="source.${cppcheckfile.key}#${cppcheckfile.linkLineNumber}">${cppcheckfile.lineNumber}</a>
+                                <a href="source.${cppcheckfile.key}#${cppcheckfile.linkLineNumber}">${cppcheckfile.lineNumberString}</a>
                             </j:if>
                         </td>
                         <td class="pane">${cppcheckfile.severity}</td>

diff --git a/src/main/resources/org/jenkinsci/plugins/cppcheck/Messages.properties b/src/main/resources/org/jenkinsci/plugins/cppcheck/Messages.properties
@@ -18,3 +18,7 @@ cppcheck.AllErrors=All errors
 
 cppcheck.BuildStability=Build stability for Cppcheck errors.
 cppcheck.PortletName=Cppcheck Statistics
+
+cppcheck.DiffNew=new
+cppcheck.DiffSolved=solved
+cppcheck.DiffUnchanged=unchanged