Re: svn commit: r219501 - /httpd/httpd/branches/2.0.x/STATUS

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

At 09:39 AM 7/18/2005, jorton@apache.org wrote:
>+       -1: jorton: this is a massive patch and extremely hard to review
>+           for actual interesting content; it is mixed in with all sorts
>+           of unrelated stuff.  It needs to at least be split up or
>+           the unrelated stuff removed.
>+
>+           unrelated change: s/apr_strnatcasecmp/strcasecmp/

apr_strnatcasecmp means something different than strcasecmp,
I'm guessing the author was cutting and pasting, and never noticed
the distinction.  (Was probably looking for portable strcasecmp, 
which we already ensure will be portable.)

>+           unrelated change: s/b/bb/ on variable+parameter names a few times

axed, good point.

>+           unrelated change: whitespaces changes all over the shop

minimized if possible.

The point is that the new code can actually be compared to httpd-2.1
- it makes sense to keep identical code in sync.

>+           spurious change:? send_request_body() appears to have been inlined

Not spurious.  We were wasting alot of cycles to make send_request_body
disjoint from ap_proxy_http_request.  It isn't; the actions performed
in ap_proxy_http_request already do all the work for us, we lost that
memory and repeated some of the process in the name of separate fn()'s.

By splitting this code across two functions it was very hard to review
the code.  I agree simple patch review is good, but the fact is that
the code itself was too hard for anyone to review, thus these bugs that
had crept into the refactoring a few years back.

The attached patch (available unmauled from
  http://people.apache.org/~wrowe/httpd-2.0-proxy-request-4.patch)
should address all of your valid concerns.

I consider your other concerns invalid because the original code
was otherwise illegible, duplicitous (which introduced even MORE
bugs), and never followed style conventions.  40% of the time I 
am trying to review code in an 80 column vim window in wrap mode,
and some of the proxy code looks like no other code in httpd.

I've tried to make the new patch as simple as possible, without
being so simple as to make review of proxy_http.c impossible.
Since only five people in this world can apparently read the
-old- code, I consider making the -code- more legible than the
actual -patch- a valid reason for a somewhat more complex patch.

Bill