httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From William A Rowe Jr <wr...@rowe-clan.net>
Subject STATUS and Backport Review efficiency
Date Mon, 08 Jun 2015 17:17:57 GMT
I've noticed this happening more and more often...

http://httpd.markmail.org/search/?q=+list%3Aorg.apache.httpd.cvs+%22vote+discarded%22

Here's one arbitrary example - there are many committers implicated here...


--- httpd/httpd/branches/2.4.x/STATUS (original) +++
httpd/httpd/branches/2.4.x/STATUS Sat May 16 10:17:05 2015 @@ -205,10
+205,17 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
http://svn.apache.org/r1640763 http://svn.apache.org/r1643179
http://svn.apache.org/r1656368 - 2.4.x patch:
http://people.apache.org/~jim/patches/httpd-2.4.x-ap_listeners_buckets-v2.patch
- +1: ylavic, jim + http://svn.apache.org/r1679714 + 2.4.x patch:
http://people.apache.org/~ylavic/httpd-2.4.x-ap_listeners_buckets-v3.patch
+ +1: ylavic wrowe: the statics are not explicitly initialized for clarity
ap_log_common is horribly named, perhaps ap_log_mpm_common()? + ylavic: v3
with s/ap_log_common/ap_log_mpm_common/ (jim's *vote discarded*). + globals
(including statics) not initialized are implicitely
initialized to zero + per C standard, and go to .bss (zeroed as a whole at
load time),
whereas the ones + explicitly initialized (even to 0 depending on the
compiler/flags)
go to .data and + have each to be set to their init value (which may take
some
cycles at load time). + I personaly don't see explicit initializations to
{0} as a win
(even for clarity...).

*) http: Make ap_die() robust against any HTTP error code and not modify
response status (finally logged) when nothing is to be done. PR 56035.


In this example, the patch was enhanced and the original reviewers' efforts
were thrown away. It's a shame to waste the limited review cycles.

Moving forwards, can we please do two things.  1) retain the original patch
and vote in the STATUS, adding a revised patch below the vote that can be
reviewed, and 2) show the changelog of patch 1 vs. patch 2 so that the
original reviewer doesn't have to bisect two patches to determine whether
the change had any measurable effect on the original patch review?

We might start an
http://svn.apache.org/repos/asf/httpd/httpd/patches/backports/2.[24].x/
tree, so that viewvc can be helpful in this respect, instead of multiple
patches residing in our individual public_html/ trees. WDYAT?

All of this is to say that we should pay extra close attention to
correctness when we first submit a backport patch proposal, and request 3)
is to please not use STATUS as a discussion tool.  If the
comment/observation doesn't fit in 240 characters (3 lines), it probably
belongs on the list instead, or can be a short note in STATUS with a long
explanation on dev.

Mime
View raw message