I don't know that rejection policies are very community friendly. Treating SVN code changes like the keeper of the Bridge of Death (Monty Python Reference, smile) may find less people willing to do in this do-ocracy. Many of us rather like what's left of our anarco-sydicalist commune (oh look, there's another :-) ).
--- Jonathon -- Improov <[EMAIL PROTECTED]> wrote: > David, > > I agree with the rejection policy. > > One suggestion on procedure to take when > self-reviewing a patch before submitting it. Look at > the > diff to see if there are changes we cannot account > for. Using KDiff also makes things easier, even > when dealing with formatting changes. > > In Emacs, it's also easy to go through every set of > changes, do an interactive-search for 1st > entry of each set, and see if the 2nd entry is > highlighted similarly. Meaning, if it is > highlighted similarly, the set of changes is bogus > (formatting only). > > In general, we should submit patches that are > intentional, ie just the intended changes only. We > should not submit patches that contain unintended > changes that have extra unintended footprints. > > Jonathon > > David E. Jones wrote: > > Moving this back to the mailing list... > > > > > > On Jan 15, 2007, at 5:34 PM, Adrian Crum wrote: > > > >> David E. Jones wrote: > >>> When reviewing a patch lines with formatting > changes only are > >>> EXTREMELY time consuming, and the patch > attached that issue is a > >>> good example of a painful one. For each of > those line you have to > >>> stare at it looking at every character trying > to figure out what the > >>> point of the bloody change was, or to make sure > that the change is > >>> "invisible"... > >> > >> That really puzzles me. You keep mentioning > formatting changes when I > >> don't see any. I didn't make any formatting > changes when modifying the > >> files. Even now when I look at the patch in Jira, > it shows only the > >> lines I changed in the file. > >> > >> *shrug* > > > > As others may have the same question I'll answer > it on the mailing list. > > Below is the first section from the > framework_v2.patch file on the > > [https://issues.apache.org/jira/browse/OFBIZ-605] > issue. > > > > Each change is marked with a "-" for a line > removed or a "+" for a line > > added. > > > > First Set: formatting, or "invisible" change > > > > Second Set: add comment (not necessary, BTW as it > describes something > > that no longer exists; if people want to see old > stuff they can look at > > the SVN history); remove tabstyles.css link tag > > > > Third Set: formatting, or "invisible" change > > > > Of all of these changes, only the "remove > tabstyles.css link tag" was > > actually intended and necessary, but getting to > this fact when reviewing > > the changes takes some time... this making it more > difficult to review, > > commit, etc. > > > > People tend to slip things into patches > accidentally all the time. I'm > > tempted to begun the voting process for a new > policy that says that if > > there is anything in the patch file not described > in the summary of the > > patch, then it will be rejected... > > > > -David > > > > > > ======================== > > Index: common/webcommon/includes/header.ftl > > > =================================================================== > > --- common/webcommon/includes/header.ftl > (revision 494159) > > +++ common/webcommon/includes/header.ftl > (working copy) > > @@ -27,7 +27,7 @@ > > <head> > > <meta http-equiv="Content-Type" > content="text/html; charset=UTF-8"/> > > <title>${layoutSettings.companyName}: <#if > > > (page.titleProperty)?has_content>${uiLabelMap[page.titleProperty]}<#else>${(page.title)?if_exists}</#if></title> > > > > > - <link rel="shortcut icon" > > > href="<@ofbizContentUrl>/images/ofbiz.ico</@ofbizContentUrl>" > /> > > + <link rel="shortcut icon" > > > href="<@ofbizContentUrl>/images/ofbiz.ico</@ofbizContentUrl>" > /> > > <script language="javascript" > > > src="<@ofbizContentUrl>/images/calendar1.js</@ofbizContentUrl>" > > > type="text/javascript"></script> > > <script language="javascript" > > > src="<@ofbizContentUrl>/images/selectall.js</@ofbizContentUrl>" > > > type="text/javascript"></script> > > <script language="javascript" > > > src="<@ofbizContentUrl>/images/fieldlookup.js</@ofbizContentUrl>" > > > type="text/javascript"></script> > > @@ -37,8 +37,8 @@ > > <link rel="stylesheet" > > > href="<@ofbizContentUrl>${styleSheet}</@ofbizContentUrl>" > type="text/css"/> > > </#list> > > <#else> > > + <#-- tabstyles.css has been moved to > maincss.css --> > > <link rel="stylesheet" > > > href="<@ofbizContentUrl>/images/maincss.css</@ofbizContentUrl>" > > > type="text/css"/> > > - <link rel="stylesheet" > > > href="<@ofbizContentUrl>/images/tabstyles.css</@ofbizContentUrl>" > > > type="text/css"/> > > </#if> > > ${layoutSettings.extraHead?if_exists} > > </head> > > @@ -51,7 +51,7 @@ > > <tr> > > <#if > layoutSettings.headerImageUrl?exists> > > <td align="left" width="1%"><img > > alt="${layoutSettings.companyName}" > > > src="<@ofbizContentUrl>${layoutSettings.headerImageUrl}</@ofbizContentUrl>"/></td> > > > > > - </#if> > > + </#if> > > <td align="right" width="1%" > nowrap="nowrap" <#if > > > layoutSettings.headerRightBackgroundUrl?has_content>background="${layoutSettings.headerRightBackgroundUrl}"</#if>> > > > > > <div class="insideHeaderText"> > > <#if person?has_content> > > ====================== > > > >
