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 16:11:39 GMT
On Sun, Oct 4, 2009 at 9:22 AM, Graham Leggett <minfrin@sharp.fm> wrote:

> Jeff Trawick wrote:
>
> > My gut instinct when I see something odd is that I'd like to know what
> > that was for.
>
> First off, I am not in a position to tell you why it was done like that,
>  that came from the original contributor, so I don't know why you were
> asking me. Although having looked at it it is pretty obvious why.
>

You committed the code, so questions will be addressed to you based on the
the general expectation that you reviewed and understood it.  It isn't
appropriate for you to expect me to track down the original contributor and
ask them.



> The original author had added ap_log_error() into the code, and when
> they were done they removed them again, but forgot to remove the
> server_rec they had wired through that made ap_log_error() possible.
>
> And it's pretty obvious how it could be missed by me. The comment at the
> top clearly explained why the server_rec had been added. I expected to
> see ap_log_error() further down, but instead got distracted for a few
> hours by the fact the patch had completely rewritten the function and in
> its original form didn't fully work.
>
> And that was fine too, because you picked it up, creating the
> opportunity for it to be fixed.
>
> > 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."
>
> Just flag it to be looked at further, as in "you missed a spot".
>

I think that is a very extreme position.  I don't believe that anyone here
is going to say "you missed a spot" in lieu of something more specific about
a code construct (except when making some boilerplate change and missing 1
of n occurrences).


>
> > 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?
>
> It would have, yes.
>
> Rhetorical questions come across as "how could you have missed that
> moron" even though that may not have been the original intention.
>

How about just answering questions straight-up and seeing how that goes?
I'm not trying to say "how could you have missed that" with any of my
comments.

Mime
View raw message