[JENKINS-14481] Quickly identify only the new errors

- A new column added to the report details table. It can contain "new", "solved" or "unchanged" flags to distinguish state of comparison with the previous report (if any). The column is mainly defined for an ability of sorting. - "New" issues are additionally highlighted with red background, "solved" issues are green and "unchanged" has white color as usual. - Solved issues also don't have the links to the source code, because it isn't available (issue was solved and the code is no longer present). - New CppcheckDiffState enum/flag added and stored as transient in CppcheckWorkspaceFile class. It is a dynamic parameter that should not be saved to any XML file. - The real comparison is done in CppcheckDiffState.diffCurrentAndPrevious(). - If source file is not available, no line will be displayed in the report (zero was present before).
jenkinsci · Mar 10, 2014 · 0ee027c · 0ee027c
1 parent c7fbdb6
commit 0ee027c
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