Re: svn commit: r1337026 - in /ofbiz/branches/release11.04: applications/accounting/src/org/ofbiz/accounting/thirdparty/clearcommerce/ framework/webapp/src/org/ofbiz/webapp/event/ specialpurpose/shark/src/org/ofbiz/shark/audit/ specialpurpose/shark/src/org...

[Adam Heath <do...@brainfood.com>]

On 05/10/2012 11:51 PM, ashish@apache.org wrote:
> Author: ashish
> Date: Fri May 11 04:51:55 2012
> New Revision: 1337026
>
> URL: http://svn.apache.org/viewvc?rev=1337026&view=rev
> Log:
> Applied similar fix as we did on trunk r1300463.
> On production systems you can't suppress Debug.log( message by the use of debug.properties file. It is always good to use Debug.* statements that are having log level setup in debug.properties file. The real problem comes with Debug.log( statement when you are printing any list or map object that contains so many records(or data) in it. Here I am changing all the occurrence of Debug.log( with Debug.logInfo(, Debug.logError( or Debug.logWarning( so that we can have better control of Debug.* statements on production system. :-)
>
> Bad use of Debug statement. On production system you will get false alarm that you are having error in code base although the resultant statement is only giving additional information instead of error message.
>
> If conditional check is used for some debug level lets say "Verbose" then the containing Debug statement will be Debug.logVerbose() instead of Debug.logInfo() - Comment from Jacopo and Adrian.

While I don't really care that this set of changes has been applied to 
12.04, please revert this change applied to 11.04 and 10.04.  It changes 
external interfaces, what is saved in a log file.  It's not a bug fix, 
but more of a cosmetic change.

Re: svn commit: r1337026 - in /ofbiz/branches/release11.04: applications/accounting/src/org/ofbiz/accounting/thirdparty/clearcommerce/ framework/webapp/src/org/ofbiz/webapp/event/ specialpurpose/shark/src/org/ofbiz/shark/audit/ specialpurpose/shark/src/org...

[Jacopo Cappellato <ja...@hotwaxmedia.com>]

Hi Adam,

On May 11, 2012, at 7:05 AM, Adam Heath wrote:

> On 05/10/2012 11:51 PM, ashish@apache.org wrote:
>> Author: ashish
>> Date: Fri May 11 04:51:55 2012
>> New Revision: 1337026
>> 
>> URL: http://svn.apache.org/viewvc?rev=1337026&view=rev
>> Log:
>> Applied similar fix as we did on trunk r1300463.
>> On production systems you can't suppress Debug.log( message by the use of debug.properties file. It is always good to use Debug.* statements that are having log level setup in debug.properties file. The real problem comes with Debug.log( statement when you are printing any list or map object that contains so many records(or data) in it. Here I am changing all the occurrence of Debug.log( with Debug.logInfo(, Debug.logError( or Debug.logWarning( so that we can have better control of Debug.* statements on production system. :-)
>> 
>> Bad use of Debug statement. On production system you will get false alarm that you are having error in code base although the resultant statement is only giving additional information instead of error message.
>> 
>> If conditional check is used for some debug level lets say "Verbose" then the containing Debug statement will be Debug.logVerbose() instead of Debug.logInfo() - Comment from Jacopo and Adrian.
> 
> While I don't really care that this set of changes has been applied to 12.04, please revert this change applied to 11.04 and 10.04.  It changes external interfaces, what is saved in a log file.  It's not a bug fix, but more of a cosmetic change.


Premise: I really don't care about this specific commit, we can revert it or change it or keep it is some branches and not in others, I don't care.
With that said: the argument that a log file is an interface, we do not change the interfaces in release branches and then we cannot change a log message in a branch is silly and dangerous... the idea that there is a rule that we cannot violate and that you can use (stretching your reasonings to their limits) to prevent a commit that you do not like to be backported is something that has caused big problems in the past (when we had huge discussion about what is a bug, how to deal with backward compatibility etc...).

There is no rule that prevents us from backporting any kind of fixes to branches, if there is general consensus: we all agree that the purpose of branches is to stabilize code, that is why we all agree that only bug fixes (and not new features) should be backported, unless there are good reasons for backporting something else.

If, like in this situation, you have a problem with a commit, please state the real problem you have with the commit without trying to demonstrate that it violates some untouchable rule: you may discover that the real issue you are having may be the same of others and your argument would make more sense.

For example here, a good argument could be the following:
"I have a large custom codebase based on 10.04 and this big commit that affects several files (without changing their behavior or fixing any bug) is causing several conflicts with my custom code; others may be in my same situation; shouldn't we consider to revert this?"

As a side note: the fact that you couldn't suppress a log in production and that the Debug.log(...) was used by code that could print huge lists if used in production by companies with big picking lists was the main issue here, if I well understand; this needs to be fixed and backported; however I agree it is questionable that the same fix has to be backported everywhere; it is a rather safe fix that shouldn't add new bugs but (as I mentioned above) I understand that custom code may be affected in the merge process.
So it makes sense to discuss the base way to go without enforcing a decision based on the *rules*.

Jacopo