Scott Gray wrote: > On 5/04/2010, at 5:11 PM, Adam Heath wrote: > >> [email protected] wrote: >>> Author: doogie >>> Date: Sun Apr 4 19:08:46 2010 >>> New Revision: 930737 >>> >>> URL: http://svn.apache.org/viewvc?rev=930737&view=rev >>> Log: >>> Make use of UtilObject.equalsHelper in storeAll. >>> >>> Modified: >>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java >>> >>> Modified: >>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java >>> URL: >>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java?rev=930737&r1=930736&r2=930737&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java >>> (original) >>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java >>> Sun Apr 4 19:08:46 2010 >>> @@ -1450,7 +1450,7 @@ public class GenericDelegator implements >>> if (value.containsKey(fieldName)) { >>> Object fieldValue = value.get(fieldName); >>> Object oldValue = existing.get(fieldName); >>> - if ((fieldValue == null && oldValue != null) >>> || (fieldValue != null && !fieldValue.equals(oldValue))) { >>> + if (UtilObject.equalsHelper(oldValue, >>> fieldValue)) { >>> toStore.put(fieldName, fieldValue); >>> atLeastOneField = true; >>> } >> Did anyone else review this commit? Or did you not understand the code? >> >> Yes, I screwed it up. But if no one reviews the commits, then how >> will much bigger problems be found? This was a simple problem to >> find, an obvious inverted test, yet no one noticed it. >> >> What's the point of having commits mailed to the list if no one will >> review them? > > Try to avoid dumping 10-20 commits at a time and I'll try to avoid skimming > over them.
Dumping 20 commits at once, and not doing anything for a week, vs: doing 2 a day, for 7 days, the same amount of review has to happen. And, it's better doing them all at once, because you don't have to constantly multi-task. And, besides, this little commit was not part of some larger commit flood anyways. >
