apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Branko ─îibej <br...@xbc.nu>
Subject Re: cvs commit: apr/threadproc/win32 proc.c
Date Wed, 03 Jul 2002 01:11:01 GMT
William A. Rowe, Jr. wrote:

> At 06:55 PM 7/2/2002, Brane wrote:
>
>> William A. Rowe, Jr. wrote:
>>
>>> If there is a problem, it is NOT in this patch you reverted.  It is 
>>> probably
>>> localized to apr_file_inherit_set().  That API didn't exist when the 
>>> original
>>> 'make inheritable duplicates' was added.
>>
>
>> The first order if business is to get HEAD working again, regardless.
>
>
> The code wasn't correct in the first place.  Ergo reverting to a 
> pseudo-working
> state isn't productive.


The difference  is between "working" and "not working", not "buggy" and 
"not buggy". I know why you made the change, and we all agree that the 
original was buggy, and I'm in the process of testing a better patch 
(O.K., you beat me to it, but anyway).

>   Neither is reverting and then reapplying a patch.
>
> The correct fix was 10 minutes.  A heads up to the list is ALWAYS 
> warranted
> unless vetoed code is checked into the repository.


Um. Sorry to disagree here, but the fix isn't entirely correct. You 
can't use SetHandleInformation for console buffers on pre-Win2k 
variants, including WinNT 4.0 and below. Do we care about that? Probably 
not for Apache, but we should for other reasons (not the least that I 
suspect Subversion won't work on NT4 now)

And your patch is obviously wrong because it won't detect a failure to 
make the handle inheritable. I know that's because the declarations of 
those functions are wrong, but making them correct is definitely more 
than 10 minutes' work. My patch avoids that problem by (again, 
temporarily) avoiding those APis.

I'm attaching my patch, just for comparison.

> We do NOT have to roll over each other every time that HEAD is broken.
> If it doesn't compile, fine.  But you are talking about a behavioral 
> problem
> with the apr_foo_set_inherit semantics, so you rip out a perfectly 
> legit patch.
> That's bad form.


<rant>
Are you aware this is the second time you broke apr_proc_create and 
friends, when a slightly more thorough test would have avoided it? And 
why do you think HEAD should remain non-working (but non-buggy?) while I 
figure out that we have a no-op funciton that's causing problems? That 
it's 3 a.m. and I threw away several hours of work trying to find a bug 
in Subversion that wasn't there? And you talk to me about bad form?
</rant>

I'll probably regret this last paragraph tomorrow, but I'm too pissed 
off to regret it now. Apologies to innocent bystanders. We should take 
this off list now, mano y mano.

-- 
Brane ─îibej   <brane@xbc.nu>   http://www.xbc.nu/brane/

Mime
View raw message