On Sat, Oct 3, 2009 at 8:44 PM, Graham Leggett <[email protected]> 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.
