William A. Rowe, Jr. wrote:
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)
Yes, we should. There are many places where console HANDLEs are borked in APR right now. I know that. I also only have so much time on my hands so I'm picking and choosing battles.
You are 100% right on - we should support odd HANDLE values (literally, a console HANDLE generally has an odd value since it would never align, and therefore signifies a non-NT object handle.)
But at the very least, apr_file_read on those handles is borked when the buffer passed in is greater than some 30720 bytes. Don't ask me, that one isn't our bug, it's ms's borked tty API.
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.
My patch isn't any more wrong than dozens of other places in the code where we never had a return value, and later discover we need one.
I agree. I committed the exact code to be changed in a separate commit, as we introduce a return value for apr_foo_inherit_set/unset. We've all agreed that we will break the next tag (apache 2.0.40) in some somewhat subtle ways regarding the return values, so let's just do it.
But it doesn't matter which order they are applied in. It matters that these are two seperate commits.
<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?
No. I've broken it at least a dozen times in the past two years. And it's been fixed reasonably promptly. And so?
As far as a slightly more thorough test... well that wouldn't be testing with Apache, would it? It wouldn't be testing with the apr/test suite, would it? Care for me to be a mindreader?
For whatever reasons, the Win2K and XP boxes I'm hacking on lately promoted the si.Handles to inherited all on their own. Sorry that things like that slip by. Blame m$ for leniency towards incorrect API calls.
Next time commit additional test cases to apr/test when you are pissed off, it helps us all solve these sorts of bugs and prevent them on the next trip around that code.
<rant>
<counterrant>
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?
I don't. But you are developing against HEAD of how many different code bases and projects? I have at least a half dozen CVS head checkouts here, several alpha and beta libraries. Why? So I can catch interactions with the *next* set of bugs that will be released.
And I am usually bit in the middle of the night, or when I am in a rush, or when I'd rather be doing something else. Take the well intentioned commit for apr_file_setaside(). It didn't work. I've commented it out for the moment so nobody trips. But the tree will be broken sometimes. cvs up -D"-24:00:00" and you will be much happier, and unstuck.
apr_proc_create stopped creating good processes for you. revert the patch in your own tree, and then YELL. Don't just commit every hack that happens to work out for you unless you are taking the time to figure out exactly what is going on. Obviously you know now. Obviously you didn't when you reverted the last two patches in one swoop. Thank you for your willingness to fix the broken code, I'm only asking that you would have done so before reverting the good code.
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?
I've been hacking on Apache for 2 years, and on APR for well over a year. Hell, I wrote or rewrote a very large quantity of the Win32 code, with alot of help from a very vocal peanut gallery. And you know what? Discussion [when you disagree with a commit or the old code] accomplishes alot more.
The code is certainly not perfect [when held up as a stand alone library.] Reverting patches without discussion is NOT forward progress to making the entire library better.
And you talk to me about bad form?
Yes. You aren't the only one who's been clobbered by other folks' commits. And if you want to get along as a Win32 committer, you need to deal with a broken tree a good deal of the time in cross platform projects.
Learn to checkout by good, known date ... and commit incremental fixes rather than reverting commits.
</counterrant>
</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.
Actually, remember there are others lurking here. These sorts of discussions about form or policies or whatever are productive, they help keep us all from repeating the same mistakes.
So I apologize you tripped over a case that wasn't apparent on my development OS's, that the existing API wasn't designed for. And I apologize that code I thought I had committed months ago to inherit.h wasn't there. And I hope your next 3am hacking session is alot more productive.
Bill
