httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Leggett <minf...@sharp.fm>
Subject Re: svn commit: r821333 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_cache.xml modules/cache/cache_util.c
Date Sun, 04 Oct 2009 00:44:29 GMT
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.

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

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

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.

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

Regards,
Graham
--

Mime
View raw message