On Sat, Oct 3, 2009 at 8:44 PM, Graham Leggett <minfrin@sharp.fm> wrote:
Jeff Trawick wrote:

   +  *) mod_cache: Fix uri_meets_conditions() so that CacheEnable will
   +     match by scheme, or by a wildcarded hostname. PR 40169
   +     [Ryan Pendergast <rpender us.ibm.com <http://us.ibm.com>>,

   Graham Leggett]
   +


I guess it is "Submitted:" by both you and Ryan?  (Commit log doesn't match CHANGES?)

This is curious because this patch looks most like one attached to the issue by Peter Grandi.  The last patch attached by Ryan is a one-liner, and now marked obsolete.

I mixed up the names, thanks for the catch.

The patch is different enough that if I was the original contributor, I'd say "oi, that isn't my code any more, and the person who reviewed it changed it, so don't blame me for their bugs".

CHANGES doesn't make distinctions about who did what to code, it just lists the people responsible, usually in the order of contribution.

The commit log makes a clearer distinction that the code was originally submitted, and then reviewed and modified by me. They do not contradict one another.

IMO the "modified by me" part is lost.
 


some aspects of this patch change the style or handle certain issues that are ignored everywhere else

That's because the entire afternoon I devoted to this patch was spent painstakingly testing each path through the code, to make 100% sure that it worked both for the forward and reverse proxy case (the original patch didn't work in the reverse proxy case).

The original patch didn't follow the style of mod_cache at all, and I changed the majority of it to match. Obviously I didn't catch all of it, but then I prioritised whether it worked or not over and above where a brace was placed.


we have unused parameters all over the place (e.g., with hook functions); why we need to do "(void) s;" here?

Why are you asking why?

If it's wrong, highlight it, we fix it, we move on.

My gut instinct when I see something odd is that I'd like to know what that was for.  Yeah, I'm pretty sure it is wrong and needs to be removed, but I wondered why as well.  I suppose I could have said "I don't see any purpose for this, so delete."

also, since no debug provision utilizing s was committed, the developer has to add code anyway when wanting to log something; can't they just add s at that time instead of leaving it in the code?  it should take all of 15 seconds to do that

Did you think I just took the patch as it was and applied it blindly to httpd-trunk and committed it?


What I think is that you are not comfortable having a civil conversation about your commits.

Would "Please delete this unnecessary code" have been better received than explaining why I thought it was preferable to leave that minimal debug support out?

 
The fact that you can only find issues with the style, and not with the functionality indicates that the afternoon's work was a success.

Good for you for moving an outside patch forward; that's great, of course.
 

It's late, I'm tired and I'm off to bed. The style can be fixed tomorrow.

It will be appreciated.