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>
======================
