httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
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 11:46:11 GMT
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.

Mime
View raw message