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=1180004&view=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=1180004&r1=1180003&r2=1180004&view=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 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 (IOException e) { // treat the same as EOF
>>> -                log.error(e.toString());
>>> -            }
>>> -            if (line!=null) {// i.e. not EOF
>>> -                try {
>>> -                    String[] lineValues = getQuotedData() ?
>>> -                            CSVSaveService.csvSplitString(line, 
>>> delim.charAt(0))
>>> -                            : JOrphanUtils.split(line, delim, false);
>>> -                            for (int a = 0; a < vars.length && a < 
>>> lineValues.length; a++) {
>>> -                                threadVars.put(vars[a], lineValues[a]);
>>> -                            }
>>> -                } catch (IOException e) { // Should only happen for 
>>> quoting errors
>>> -                   log.error("Unexpected error splitting '"+line+"' on 
>>> '"+delim.charAt(0)+"'");
>>> -                }
>>> -                // TODO - report unused columns?
>>> -                // TODO - provide option to set unused variables ?
>>> -            } else {
>>> -                if (getStopThread()) {
>>> -                    throw new JMeterStopThreadException("End of file 
>>> detected");
>>> -                }
>>> -                for (int a = 0; a < vars.length ; a++) {
>>> -                    threadVars.put(vars[a], EOFVALUE);
>>> +                String[] lineValues = getQuotedData() ?
>>> +                        CSVSaveService.csvSplitString(line, 
>>> delim.charAt(0))
>>> +                        : JOrphanUtils.split(line, delim, false);
>>> +                for (int a = 0; a < vars.length && a < lineValues.length; 
>>> a++) {
>>> +                    threadVars.put(vars[a], lineValues[a]);
>>>                 }
>>> +            } catch (IOException e) { // Should only happen for quoting 
>>> errors
>>> +                log.error("Unexpected error splitting '" + line + "' on '" 
>>> + delim.charAt(0) + "'");
>>> +            }
>>> +            // TODO - report unused columns?
>>> +            // TODO - provide option to set unused variables ?
>>> +        } else {
>>> +            if (getStopThread()) {
>>> +                throw new JMeterStopThreadException("End of file 
>>> detected");
>>>             }
>>> +            for (int a = 0; a < vars.length ; a++) {
>>> +                threadVars.put(vars[a], EOFVALUE);
>>> +            }
>>> +        }
>>>     }
>>>
>>>     /**
>>>
>>> Modified: jakarta/jmeter/trunk/xdocs/changes.xml
>>> URL: 
>>> http://svn.apache.org/viewvc/jakarta/jmeter/trunk/xdocs/changes.xml?rev=1180004&r1=1180003&r2=1180004&view=diff
>>> ==============================================================================
>>> --- jakarta/jmeter/trunk/xdocs/changes.xml (original)
>>> +++ jakarta/jmeter/trunk/xdocs/changes.xml Fri Oct  7 11:10:18 2011
>>> @@ -119,6 +119,7 @@ Mirror server now uses default port 8081
>>>  <h3>General</h3>
>>>  <ul>
>>>  <li>Bug 51937 - JMeter does not handle missing TestPlan entry well</li>
>>> +<li>Bug 51988 - CSV Data Set Configuration does not resolve default 
>>> delimiter for header parsing when variables field is empty</li>
>>>  </ul>
>>>
>>>  <!-- =================== Improvements =================== -->
>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: notifications-unsubscr...@jakarta.apache.org
>>> For additional commands, e-mail: notifications-h...@jakarta.apache.org
>>>
>>>
>>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@jakarta.apache.org
For additional commands, e-mail: dev-h...@jakarta.apache.org

Reply via email to