Re: svn commit: r1180004 - in /jakarta/jmeter/trunk: src/components/org/apache/jmeter/config/CSVDataSet.java xdocs/changes.xml

2011-10-07 Thread sebb
On 7 October 2011 12:10,  pmoua...@apache.org wrote:
 Author: pmouawad
 Date: Fri Oct  7 11:10:18 2011
 New Revision: 1180004

 URL: http://svn.apache.org/viewvc?rev=1180004view=rev
 Log:
 Bug 51988 - CSV Data Set Configuration does not resolve default delimiter for 
 header parsing when variables field is empty

 Modified:
    
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
    jakarta/jmeter/trunk/xdocs/changes.xml

 Modified: 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
 URL: 
 http://svn.apache.org/viewvc/jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java?rev=1180004r1=1180003r2=1180004view=diff
 ==
 --- 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java 
 (original)
 +++ 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java 
 Fri Oct  7 11:10:18 2011
 @@ -100,74 +100,74 @@ public class CSVDataSet extends ConfigTe
     public void iterationStart(LoopIterationEvent iterEvent) {
         FileServer server = FileServer.getFileServer();
         final JMeterContext context = getThreadContext();
 +        String delim = getDelimiter();
 +        if (delim.startsWith(\\t)) { // $NON-NLS-1$

This is a change from the previous code, which uses .equals().
I don't think startsWith() is suitable here.

 +            delim = \t;// Make it easier to enter a Tab // $NON-NLS-1$
 +        } else if (delim.length() == 0) {
 +            log.warn(Empty delimiter converted to ',');
 +            delim = ,;
 +        }
         if (vars == null) {
             String _fileName = getFilename();
             String mode = getShareMode();
             int modeInt = CSVDataSetBeanInfo.getShareModeAsInt(mode);
 -            switch(modeInt){
 -                case CSVDataSetBeanInfo.SHARE_ALL:
 -                    alias = _fileName;
 -                    break;
 -                case CSVDataSetBeanInfo.SHARE_GROUP:
 -                    alias = 
 _fileName+@+System.identityHashCode(context.getThreadGroup());
 -                    break;
 -                case CSVDataSetBeanInfo.SHARE_THREAD:
 -                    alias = 
 _fileName+@+System.identityHashCode(context.getThread());
 -                    break;
 -                default:
 -                    alias = _fileName+@+mode; // user-specified key
 -                    break;
 +            switch (modeInt) {
 +            case CSVDataSetBeanInfo.SHARE_ALL:
 +                alias = _fileName;
 +                break;
 +            case CSVDataSetBeanInfo.SHARE_GROUP:
 +                alias = _fileName + @ + 
 System.identityHashCode(context.getThreadGroup());
 +                break;
 +            case CSVDataSetBeanInfo.SHARE_THREAD:
 +                alias = _fileName + @ + 
 System.identityHashCode(context.getThread());
 +                break;
 +            default:
 +                alias = _fileName + @ + mode; // user-specified key
 +                break;
             }
             final String names = getVariableNames();
 -            if (names == null || names.length()==0) {
 +            if (names == null || names.length() == 0) {
                 String header = server.reserveFile(_fileName, 
 getFileEncoding(), alias, true);
                 try {
 -                    vars = CSVSaveService.csvSplitString(header, 
 getDelimiter().charAt(0));
 +                    vars = CSVSaveService.csvSplitString(header, 
 delim.charAt(0));
                     firstLineIsNames = true;
                 } catch (IOException e) {
 -                    log.warn(Could not split CSV header line,e);
 +                    log.warn(Could not split CSV header line, e);
                 }
             } else {
                 server.reserveFile(_fileName, getFileEncoding(), alias);
                 vars = JOrphanUtils.split(names, ,); // $NON-NLS-1$
             }
         }
 -            String delim = getDelimiter();
 -            if (delim.equals(\\t)) { // $NON-NLS-1$
 -                delim = \t;// Make it easier to enter a Tab // $NON-NLS-1$
 -            } else if (delim.length()==0){
 -                log.warn(Empty delimiter converted to ',');
 -                delim=,;
 -            }
 -            // TODO: fetch this once as per vars above?
 -            JMeterVariables threadVars = context.getVariables();
 -            String line = null;
 +        // TODO: fetch this once as per vars above?
 +        JMeterVariables threadVars = context.getVariables();
 +        String line = null;
 +        try {
 +            line = server.readLine(alias, getRecycle(), firstLineIsNames);
 +        } catch (IOException e) { // treat the same as EOF
 +            log.error(e.toString());
 +        }
 +        if (line != null) {// i.e. not EOF
             try {
 -                line = server.readLine(alias, getRecycle(), 
 firstLineIsNames);
 -            } catch 

Re: svn commit: r1180004 - in /jakarta/jmeter/trunk: src/components/org/apache/jmeter/config/CSVDataSet.java xdocs/changes.xml

2011-10-07 Thread sebb
On 7 October 2011 12:51, sebb seb...@gmail.com wrote:
 On 7 October 2011 12:10,  pmoua...@apache.org wrote:
 Author: pmouawad
 Date: Fri Oct  7 11:10:18 2011
 New Revision: 1180004

 URL: http://svn.apache.org/viewvc?rev=1180004view=rev
 Log:
 Bug 51988 - CSV Data Set Configuration does not resolve default delimiter 
 for header parsing when variables field is empty

 Modified:
    
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
    jakarta/jmeter/trunk/xdocs/changes.xml

 Modified: 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
 URL: 
 http://svn.apache.org/viewvc/jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java?rev=1180004r1=1180003r2=1180004view=diff
 ==
 --- 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java 
 (original)
 +++ 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java 
 Fri Oct  7 11:10:18 2011
 @@ -100,74 +100,74 @@ public class CSVDataSet extends ConfigTe
     public void iterationStart(LoopIterationEvent iterEvent) {
         FileServer server = FileServer.getFileServer();
         final JMeterContext context = getThreadContext();
 +        String delim = getDelimiter();
 +        if (delim.startsWith(\\t)) { // $NON-NLS-1$

 This is a change from the previous code, which uses .equals().
 I don't think startsWith() is suitable here.

 +            delim = \t;// Make it easier to enter a Tab // $NON-NLS-1$
 +        } else if (delim.length() == 0) {
 +            log.warn(Empty delimiter converted to ',');
 +            delim = ,;
 +        }
         if (vars == null) {
             String _fileName = getFilename();
             String mode = getShareMode();
             int modeInt = CSVDataSetBeanInfo.getShareModeAsInt(mode);
 -            switch(modeInt){
 -                case CSVDataSetBeanInfo.SHARE_ALL:
 -                    alias = _fileName;
 -                    break;
 -                case CSVDataSetBeanInfo.SHARE_GROUP:
 -                    alias = 
 _fileName+@+System.identityHashCode(context.getThreadGroup());
 -                    break;
 -                case CSVDataSetBeanInfo.SHARE_THREAD:
 -                    alias = 
 _fileName+@+System.identityHashCode(context.getThread());
 -                    break;
 -                default:
 -                    alias = _fileName+@+mode; // user-specified key
 -                    break;
 +            switch (modeInt) {
 +            case CSVDataSetBeanInfo.SHARE_ALL:
 +                alias = _fileName;
 +                break;
 +            case CSVDataSetBeanInfo.SHARE_GROUP:
 +                alias = _fileName + @ + 
 System.identityHashCode(context.getThreadGroup());
 +                break;
 +            case CSVDataSetBeanInfo.SHARE_THREAD:
 +                alias = _fileName + @ + 
 System.identityHashCode(context.getThread());
 +                break;
 +            default:
 +                alias = _fileName + @ + mode; // user-specified key
 +                break;
             }
             final String names = getVariableNames();
 -            if (names == null || names.length()==0) {
 +            if (names == null || names.length() == 0) {

Also, please don't change formatting when fixing bugs, unless directly
related - e.g. adding surrounding try/catch or if statement.

It makes it much harder to see what has actually changed.

                 String header = server.reserveFile(_fileName, 
 getFileEncoding(), alias, true);
                 try {
 -                    vars = CSVSaveService.csvSplitString(header, 
 getDelimiter().charAt(0));
 +                    vars = CSVSaveService.csvSplitString(header, 
 delim.charAt(0));
                     firstLineIsNames = true;
                 } catch (IOException e) {
 -                    log.warn(Could not split CSV header line,e);
 +                    log.warn(Could not split CSV header line, e);
                 }
             } else {
                 server.reserveFile(_fileName, getFileEncoding(), alias);
                 vars = JOrphanUtils.split(names, ,); // $NON-NLS-1$
             }
         }
 -            String delim = getDelimiter();
 -            if (delim.equals(\\t)) { // $NON-NLS-1$
 -                delim = \t;// Make it easier to enter a Tab // $NON-NLS-1$
 -            } else if (delim.length()==0){
 -                log.warn(Empty delimiter converted to ',');
 -                delim=,;
 -            }
 -            // TODO: fetch this once as per vars above?
 -            JMeterVariables threadVars = context.getVariables();
 -            String line = null;
 +        // TODO: fetch this once as per vars above?
 +        JMeterVariables threadVars = context.getVariables();
 +        String line = null;
 +        try {
 +            line = server.readLine(alias, getRecycle(), firstLineIsNames);
 +        } catch (IOException 

Re: svn commit: r1180004 - in /jakarta/jmeter/trunk: src/components/org/apache/jmeter/config/CSVDataSet.java xdocs/changes.xml

2011-10-07 Thread sebb
On 7 October 2011 13:01, sebb seb...@gmail.com wrote:
 On 7 October 2011 12:51, sebb seb...@gmail.com wrote:
 On 7 October 2011 12:10,  pmoua...@apache.org wrote:
 Author: pmouawad
 Date: Fri Oct  7 11:10:18 2011
 New Revision: 1180004

 URL: http://svn.apache.org/viewvc?rev=1180004view=rev
 Log:
 Bug 51988 - CSV Data Set Configuration does not resolve default delimiter 
 for header parsing when variables field is empty

 Modified:
    
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
    jakarta/jmeter/trunk/xdocs/changes.xml

 Modified: 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
 URL: 
 http://svn.apache.org/viewvc/jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java?rev=1180004r1=1180003r2=1180004view=diff
 ==
 --- 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
  (original)
 +++ 
 jakarta/jmeter/trunk/src/components/org/apache/jmeter/config/CSVDataSet.java
  Fri Oct  7 11:10:18 2011
 @@ -100,74 +100,74 @@ public class CSVDataSet extends ConfigTe
     public void iterationStart(LoopIterationEvent iterEvent) {
         FileServer server = FileServer.getFileServer();
         final JMeterContext context = getThreadContext();
 +        String delim = getDelimiter();
 +        if (delim.startsWith(\\t)) { // $NON-NLS-1$

 This is a change from the previous code, which uses .equals().
 I don't think startsWith() is suitable here.

 +            delim = \t;// Make it easier to enter a Tab // $NON-NLS-1$
 +        } else if (delim.length() == 0) {
 +            log.warn(Empty delimiter converted to ',');
 +            delim = ,;
 +        }
         if (vars == null) {
             String _fileName = getFilename();
             String mode = getShareMode();
             int modeInt = CSVDataSetBeanInfo.getShareModeAsInt(mode);
 -            switch(modeInt){
 -                case CSVDataSetBeanInfo.SHARE_ALL:
 -                    alias = _fileName;
 -                    break;
 -                case CSVDataSetBeanInfo.SHARE_GROUP:
 -                    alias = 
 _fileName+@+System.identityHashCode(context.getThreadGroup());
 -                    break;
 -                case CSVDataSetBeanInfo.SHARE_THREAD:
 -                    alias = 
 _fileName+@+System.identityHashCode(context.getThread());
 -                    break;
 -                default:
 -                    alias = _fileName+@+mode; // user-specified key
 -                    break;
 +            switch (modeInt) {
 +            case CSVDataSetBeanInfo.SHARE_ALL:
 +                alias = _fileName;
 +                break;
 +            case CSVDataSetBeanInfo.SHARE_GROUP:
 +                alias = _fileName + @ + 
 System.identityHashCode(context.getThreadGroup());
 +                break;
 +            case CSVDataSetBeanInfo.SHARE_THREAD:
 +                alias = _fileName + @ + 
 System.identityHashCode(context.getThread());
 +                break;
 +            default:
 +                alias = _fileName + @ + mode; // user-specified key
 +                break;
             }
             final String names = getVariableNames();
 -            if (names == null || names.length()==0) {
 +            if (names == null || names.length() == 0) {

 Also, please don't change formatting when fixing bugs, unless directly
 related - e.g. adding surrounding try/catch or if statement.

 It makes it much harder to see what has actually changed.

I now see that the additional changes were in the original patch.

If a submiited patch includes unnecessary changes, we have a choice:
- either we reject it, and ask the submitter to provide a new patch
- or we apply it, but not the spurious changes.
This is reasonably easy to do in Eclipse, but can be tedious.
Apply the patch, then do a compare against the base revision.
Revert the changes that aren't relevant.
Just before commiting, recheck changes against the server version.

Either way, commits should be the minimum necessary to solve the issue.

                 String header = server.reserveFile(_fileName, 
 getFileEncoding(), alias, true);
                 try {
 -                    vars = CSVSaveService.csvSplitString(header, 
 getDelimiter().charAt(0));
 +                    vars = CSVSaveService.csvSplitString(header, 
 delim.charAt(0));
                     firstLineIsNames = true;
                 } catch (IOException e) {
 -                    log.warn(Could not split CSV header line,e);
 +                    log.warn(Could not split CSV header line, e);
                 }
             } else {
                 server.reserveFile(_fileName, getFileEncoding(), alias);
                 vars = JOrphanUtils.split(names, ,); // $NON-NLS-1$
             }
         }
 -            String delim = getDelimiter();
 -            if (delim.equals(\\t)) { // $NON-NLS-1$
 -                delim = \t;// Make it