Merge pull request #18 from miktap/master

[JENKINS-13109] Huge changelogs eat all the Java memory
jenkinsci · Apr 3, 2012 · a252310 · a252310
2 parents 11249ca + 5159c1c
commit a252310
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 17 deletions.
diff --git a/src/main/java/com/tek42/perforce/parse/ChangelistBuilder.java b/src/main/java/com/tek42/perforce/parse/ChangelistBuilder.java
@@ -48,6 +48,12 @@
  */
 public class ChangelistBuilder implements Builder<Changelist> {
 	private final Logger logger = LoggerFactory.getLogger("perforce");
+	//Maximum amount of files to be recorded to a changelist
+	private int maxFiles;
+
+	public ChangelistBuilder(int maxFiles) {
+	    this.maxFiles = maxFiles;
+	}
 
 	public String[] getBuildCmd(String p4exe, String id) {
 		return new String[] { p4exe, "describe", "-s", id };
@@ -152,8 +158,16 @@ public Changelist build(StringBuilder sb) throws PerforceException {
 				if(line.startsWith("Affected files")) {
 					logger.debug("reading files...");
 					List<Changelist.FileEntry> files = new ArrayList<Changelist.FileEntry>();
+			        int fileCount = 0;
 
 					while(lines.hasMoreElements()) {
+					    //Record a maximum of maxFiles files
+					    if(maxFiles > 0) {
+	                        if(fileCount >= maxFiles) {
+	                            break;
+	                        }
+					        fileCount++;
+					    }
 						String entry = lines.nextToken();
 						logger.debug("File Line: " + entry);
 						// if(!entry.startsWith("..."))

diff --git a/src/main/java/com/tek42/perforce/parse/Changes.java b/src/main/java/com/tek42/perforce/parse/Changes.java
@@ -59,12 +59,15 @@ public Changes(Depot depot) {
 	 * Returns a single changelist specified by its number.
 	 * 
 	 * @param number
+     * @param maxFiles 
+     *             The maximum number of affected files that will be recorded
+     *             to a changelist. With negative value include all the files.
 	 * @return
 	 * @throws PerforceException
 	 */
-	public Changelist getChangelist(int number) throws PerforceException {
+	public Changelist getChangelist(int number, int maxFiles) throws PerforceException {
 
-		ChangelistBuilder builder = new ChangelistBuilder();
+		ChangelistBuilder builder = new ChangelistBuilder(maxFiles);
 		Changelist change = builder.build(getPerforceResponse(builder.getBuildCmd(getP4Exe(), Integer.toString(number))));
 		if(change == null)
 			throw new PerforceException("Failed to retrieve changelist " + number);
@@ -111,10 +114,12 @@ private String getWorkspacePathForFile(String file) throws PerforceException {
 	 *            The last changelist number to start from
 	 * @param limit
 	 *            The maximum changes to return if less than 1, will return everything
+	 * @param maxFiles 
+	 *            The maximum amount of affected files in a changelist to be recorded
 	 * @return
 	 * @throws PerforceException
 	 */
-	public List<Changelist> getChangelists(String path, int lastChange, int limit) throws PerforceException {
+	public List<Changelist> getChangelists(String path, int lastChange, int limit, int maxFiles) throws PerforceException {
 		path = normalizePath(path);
 		if(lastChange > 0)
 			path += "@" + lastChange;
@@ -132,7 +137,7 @@ public List<Changelist> getChangelists(String path, int lastChange, int limit) t
 		List<Changelist> changes = new ArrayList<Changelist>();
 		for(String id : ids) {
                     try{
-			changes.add(getChangelist(new Integer(id)));
+			changes.add(getChangelist(new Integer(id), maxFiles));
                     } catch(Exception e){
                         throw new PerforceException("Could not retrieve changelists.\nResponse from perforce was:\n" + response, e);
                     }
@@ -416,13 +421,16 @@ private void addCommand(List<String> list, String... args) {
 	 * Converts a list of numbers to a list of changes.
 	 * 
 	 * @param numbers
+	 * @param maxFiles 
+	 *             The maximum number of affected files that will be recorded
+	 *             to a changelist. With negative value include all the files.
 	 * @return
 	 * @throws PerforceException
 	 */
-	public List<Changelist> getChangelistsFromNumbers(List<Integer> numbers) throws PerforceException {
+	public List<Changelist> getChangelistsFromNumbers(List<Integer> numbers, int maxFiles) throws PerforceException {
 		List<Changelist> changes = new ArrayList<Changelist>();
 		for(Integer id : numbers) {
-			changes.add(getChangelist(id));
+			changes.add(getChangelist(id, maxFiles));
 		}
 		return changes;
 	}

diff --git a/src/main/java/hudson/plugins/perforce/PerforceSCM.java b/src/main/java/hudson/plugins/perforce/PerforceSCM.java
@@ -183,6 +183,11 @@ public class PerforceSCM extends SCM {
      */
     int firstChange = -1;
 
+    /**
+     * Maximum amount of files that are recorded to a changelist, if < 1 show every file.
+     */
+    int fileLimit = 0;
+
     /**
      * P4 user name(s) or regex user pattern to exclude from SCM poll to prevent build trigger.
      * Multiple user names are deliminated by space.
@@ -274,6 +279,7 @@ public PerforceSCM(
             boolean pollOnlyOnMaster,
             String slaveClientNameFormat,
             int firstChange,
+            int fileLimit,
             PerforceRepositoryBrowser browser,
             String excludedUsers,
             String excludedFiles,
@@ -352,6 +358,7 @@ public PerforceSCM(
         this.dontUpdateClient = dontUpdateClient;
         this.slaveClientNameFormat = slaveClientNameFormat;
         this.firstChange = firstChange;
+        this.fileLimit = fileLimit;
         this.dontRenameClient = false;
         this.useOldClientName = false;
         this.p4Charset = Util.fixEmptyAndTrim(p4Charset);
@@ -809,7 +816,7 @@ public boolean checkout(AbstractBuild build, Launcher launcher,
                     } else {
                         changeNumbersTo = depot.getChanges().getChangeNumbersInRange(p4workspace, lastChange+1, newestChange);
                     }
-                    changes = depot.getChanges().getChangelistsFromNumbers(changeNumbersTo);
+                    changes = depot.getChanges().getChangelistsFromNumbers(changeNumbersTo, fileLimit);
                 }
 
 
@@ -1165,7 +1172,7 @@ private SCMRevisionState getCurrentDepotRevisionState(Workspace p4workspace, Abs
         }
         else {
             for (int changeNumber : changeNumbers) {
-                if (isChangelistExcluded(depot.getChanges().getChangelist(changeNumber), project, logger)) {
+                if (isChangelistExcluded(depot.getChanges().getChangelist(changeNumber, fileLimit), project, logger)) {
                     logger.println("Changelist "+changeNumber+" is composed of file(s) and/or user(s) that are excluded.");
                 } else {
                     return new PerforceSCMRevisionState(changeNumber);
@@ -1899,7 +1906,7 @@ public FormValidation doCheckChangeList(StaplerRequest req) {
             if (depot != null) {
                 try {
                     int number = Integer.parseInt(change);
-                    Changelist changelist = depot.getChanges().getChangelist(number);
+                    Changelist changelist = depot.getChanges().getChangelist(number, -1);
                     if (changelist.getChangeNumber() != number)
                         throw new PerforceException("broken");
                 } catch (Exception e) {
@@ -2707,6 +2714,24 @@ public String getFirstChange() {
         return Integer.valueOf(firstChange).toString();
     }
 
+    /**
+     * This is only for the config screen.  Also, it returns a string and not an int.
+     * This is because we want to show an empty value in the config option if it is not being
+     * used.  The default value of -1 is not exactly empty.  So if we are set to default of
+     * -1, we return an empty string.  Anything else and we return the actual change number.
+     *
+     * @return  fileLimit
+     */
+    public String getFileLimit() {
+        if (fileLimit <= 0)
+            return "";
+        return Integer.valueOf(fileLimit).toString();
+    }
+
+    public void setFileLimit(int fileLimit) {
+        this.fileLimit = fileLimit;
+    }
+
     /**
      * With Perforce the server keeps track of files in the workspace.  We never
      * want files deleted without the knowledge of the server so we disable the

diff --git a/src/main/resources/hudson/plugins/perforce/PerforceSCM/config.jelly b/src/main/resources/hudson/plugins/perforce/PerforceSCM/config.jelly
@@ -121,6 +121,10 @@
               checkUrl="'${rootURL}/scm/PerforceSCM/checkChangeList?port='+escape(this.form.elements['p4Port'].value)+'&amp;tool='+escape(this.form.elements['p4Tool'].value)+'&amp;user='+escape(this.form.elements['p4User'].value)+'&amp;pass='+escape(this.form.elements['p4Passwd'].value)+'&amp;change='+escape(this.value)"/>
     </f:entry>
 
+    <f:entry title="Changelist file limit" help="/plugin/perforce/help/fileLimit.html">
+      <f:textbox field="fileLimit"/>
+    </f:entry>
+
     <f:entry title="Client Line Endings" help="/plugin/perforce/help/lineEnd.html">
         <select name="lineEndValue" id="lineEndValue">
             <j:forEach var="value" items="${instance==null?descriptor['allLineEndChoices']:instance['allLineEndChoices']}">

diff --git a/src/main/webapp/help/fileLimit.html b/src/main/webapp/help/fileLimit.html
@@ -0,0 +1,8 @@
+<div>
+    <p>
+        The maximum amount of affected files in a changelist that will be recorded to the detailed changes view. Empty value or 0 or negative value will list all the files in a changelist.
+    </p>
+    <p>
+        Limiting the amount of files can prevent out of memory errors.
+    </p>
+</div>
diff --git a/src/test/java/hudson/plugins/perforce/PerforceSCMTest.java b/src/test/java/hudson/plugins/perforce/PerforceSCMTest.java
@@ -27,7 +27,7 @@ public void testConfigRoundtrip() throws Exception {
         PerforceSCM scm = new PerforceSCM(
         		"user", "pass", "client", "port", "", "exe", "sysRoot",
         		"sysDrive", "label", "counter", "shared", "charset", "charset2", false, true, true, true, true, true, false,
-                        false, true, false, false, false, false, "${basename}", 0, browser, "exclude_user", "exclude_file", true);
+                        false, true, false, false, false, false, "${basename}", 0, -1, browser, "exclude_user", "exclude_file", true);
         scm.setProjectPath("path");
         project.setScm(scm);
 
@@ -36,7 +36,7 @@ public void testConfigRoundtrip() throws Exception {
 
         // verify that the data is intact
         assertEqualBeans(scm, project.getScm(),
-                "p4User,p4Client,p4Port,p4Label,projectPath,p4Exe,p4SysRoot,p4SysDrive,forceSync,alwaysForceSync,dontUpdateClient,createWorkspace,updateView,slaveClientNameFormat,lineEndValue,firstChange,p4Counter,updateCounterValue,exposeP4Passwd,useViewMaskForPolling,viewMask,useViewMaskForSyncing,p4Charset,p4CommandCharset,p4Stream,useStreamDepot");
+                "p4User,p4Client,p4Port,p4Label,projectPath,p4Exe,p4SysRoot,p4SysDrive,forceSync,alwaysForceSync,dontUpdateClient,createWorkspace,updateView,slaveClientNameFormat,lineEndValue,firstChange,p4Counter,updateCounterValue,exposeP4Passwd,useViewMaskForPolling,viewMask,useViewMaskForSyncing,p4Charset,p4CommandCharset,p4Stream,useStreamDepot,fileLimit");
         assertEquals("exclude_user", scm.getExcludedUsers());
         assertEquals("exclude_file", scm.getExcludedFiles());
         //assertEqualBeans(scm.getBrowser(),p.getScm().getBrowser(),"URL");
@@ -48,7 +48,7 @@ public void testConfigRoundtripWithStream() throws Exception {
         PerforceSCM scm = new PerforceSCM(
         		"user", "pass", "client", "port", "", "exe", "sysRoot",
         		"sysDrive", "label", "counter", "shared", "charset", "charset2", false, true, true, true, true, true, false,
-                        false, true, false, false, false, false, "${basename}", 0, browser, "exclude_user", "exclude_file", true);
+                        false, true, false, false, false, false, "${basename}", 0, -1, browser, "exclude_user", "exclude_file", true);
         scm.setP4Stream("stream");
         scm.setUseStreamDepot(true);
         project.setScm(scm);
@@ -75,7 +75,7 @@ public void testConfigPasswordEnctyptionAndDecription() throws Exception {
         PerforceSCM scm = new PerforceSCM(
         		"user", password, "client", "port", "", "test_installation", "sysRoot",
         		"sysDrive", "label", "counter", "shared", "charset", "charset2", false, true, true, true, true, true, false,
-                        false, true, false, false, false, false, "${basename}", 0, browser, "exclude_user", "exclude_file", true);
+                        false, true, false, false, false, false, "${basename}", 0, -1, browser, "exclude_user", "exclude_file", true);
         scm.setProjectPath("path");
         project.setScm(scm);
 
@@ -84,7 +84,7 @@ public void testConfigPasswordEnctyptionAndDecription() throws Exception {
 
         // verify that the data is intact
         assertEqualBeans(scm, project.getScm(),
-                "p4User,p4Client,p4Port,p4Label,projectPath,p4Tool,p4SysRoot,p4SysDrive,forceSync,alwaysForceSync,dontUpdateClient,updateView,slaveClientNameFormat,lineEndValue,firstChange,p4Counter,updateCounterValue,exposeP4Passwd,useViewMaskForPolling,viewMask,useViewMaskForSyncing,p4Charset,p4CommandCharset,p4Stream,useStreamDepot");
+                "p4User,p4Client,p4Port,p4Label,projectPath,p4Tool,p4SysRoot,p4SysDrive,forceSync,alwaysForceSync,dontUpdateClient,updateView,slaveClientNameFormat,lineEndValue,firstChange,p4Counter,updateCounterValue,exposeP4Passwd,useViewMaskForPolling,viewMask,useViewMaskForSyncing,p4Charset,p4CommandCharset,p4Stream,useStreamDepot,fileLimit");
         assertEquals("exclude_user", scm.getExcludedUsers());
         assertEquals("exclude_file", scm.getExcludedFiles());
 
@@ -104,7 +104,7 @@ public void testDepotContainsUnencryptedPassword() throws Exception {
         PerforceSCM scm = new PerforceSCM(
         		"user", password, "client", "port", "", "test_installation", "sysRoot",
         		"sysDrive", "label", "counter", "shared", "charset", "charset2", false, true, true, true, true, true, false,
-                        false, true, false, false, false, false, "${basename}", 0, browser, "exclude_user", "exclude_file", true);
+                        false, true, false, false, false, false, "${basename}", 0, -1, browser, "exclude_user", "exclude_file", true);
         scm.setProjectPath("path");
         project.setScm(scm);
 
@@ -122,7 +122,7 @@ public void testConfigSaveReloadAndSaveDoesNotDoubleEncryptThePassword() throws
         PerforceSCM scm = new PerforceSCM(
         		"user", password, "client", "port", "", "test_installation", "sysRoot",
         		"sysDrive", "label", "counter", "shared", "charset", "charset2", false, true, true, true, true, true, false,
-                        false, true, false, false, false, false, "${basename}", 0, browser, "exclude_user", "exclude_file", true);
+                        false, true, false, false, false, false, "${basename}", 0, -1, browser, "exclude_user", "exclude_file", true);
         scm.setProjectPath("path");
         project.setScm(scm);
 
@@ -132,7 +132,7 @@ public void testConfigSaveReloadAndSaveDoesNotDoubleEncryptThePassword() throws
 
         // verify that the data is intact
         assertEqualBeans(scm, project.getScm(),
-                "p4User,p4Client,p4Port,p4Label,projectPath,p4Tool,p4SysRoot,p4SysDrive,forceSync,alwaysForceSync,dontUpdateClient,updateView,slaveClientNameFormat,lineEndValue,firstChange,p4Counter,updateCounterValue,exposeP4Passwd,useViewMaskForPolling,viewMask,useViewMaskForSyncing,p4Charset,p4CommandCharset,p4Stream,useStreamDepot");
+                "p4User,p4Client,p4Port,p4Label,projectPath,p4Tool,p4SysRoot,p4SysDrive,forceSync,alwaysForceSync,dontUpdateClient,updateView,slaveClientNameFormat,lineEndValue,firstChange,p4Counter,updateCounterValue,exposeP4Passwd,useViewMaskForPolling,viewMask,useViewMaskForSyncing,p4Charset,p4CommandCharset,p4Stream,useStreamDepot,fileLimit");
         assertEquals("exclude_user", scm.getExcludedUsers());
         assertEquals("exclude_file", scm.getExcludedFiles());