RE: [ros-diffs] [cwittich] 19254: add a check for SPDRP_CONFIGFLAGSwhich is not working yet
~1700 lines to add a check? In the future, please do reformatting in its own patch without semantic changes. This patch would require an hour or more to review if anyone dared to do it and even if someone did, that person would have be superhuman to be able to keep track of that many lines of information.
The risk of introducing a bug is higher with some types of operations than others. Formatting is a relatively safe operation while semantic changes to complex code is a highly risky operation. This means that reviewing a formatting patch is not as important as reviewing a patch with semantic changes to complex code (and thus it doesn't hurt so much if a formatting change is large). When mixing safe and unsafe operations, the overall risk of a introducing a bug in a given patch is the same as if the changes were spread over several patches. However, the risk of the individual patch is equal to risk of the most risky operation in the patch. So if a 1 line highly risky change is hidden in a 1700 line otherwise safe patch, then the overall risk for the whole patch is highly risky.
It may not seem important to have only a few bugs in releases now. We can always use the excuse that ReactOS is alpha software. However, if we ever want to have an 1.x release which is more than a toy-OS, then we have to learn to build better software. And I would prefer that we learn it before 1 day before the deadline for 1.0.0 since that might give us some years to get really good at it. I've written several school papers the night before the deadline, and they were never any good ;-( Another side effect of starting early is that some might even be able to use ReactOS before 1.0.0.
Casper
While your points are valid, newdev is his baby and he should be allowed to do with it as he damn well pleases. Don't tell me you've never done this kind of operation locally on your disk? Christopher just chose to commit it. It's why I now run a local branch and only commit final code; it lets me do what I want with code that I wrote 100% of, without being criticzed about design choices before the application is complete.
Note: If this wasn't his app or was a really old app we depend on, I would agree with you. But as far as I'm concerned this is his application and for now there's no point in reviewing it until he considers it done and ready for review.
Best regards, Alex Ionescu
Casper Hornstrup wrote:
~1700 lines to add a check? In the future, please do reformatting in its own patch without semantic changes. This patch would require an hour or more to review if anyone dared to do it and even if someone did, that person would have be superhuman to be able to keep track of that many lines of information.
The risk of introducing a bug is higher with some types of operations than others. Formatting is a relatively safe operation while semantic changes to complex code is a highly risky operation. This means that reviewing a formatting patch is not as important as reviewing a patch with semantic changes to complex code (and thus it doesn't hurt so much if a formatting change is large). When mixing safe and unsafe operations, the overall risk of a introducing a bug in a given patch is the same as if the changes were spread over several patches. However, the risk of the individual patch is equal to risk of the most risky operation in the patch. So if a 1 line highly risky change is hidden in a 1700 line otherwise safe patch, then the overall risk for the whole patch is highly risky.
It may not seem important to have only a few bugs in releases now. We can always use the excuse that ReactOS is alpha software. However, if we ever want to have an 1.x release which is more than a toy-OS, then we have to learn to build better software. And I would prefer that we learn it before 1 day before the deadline for 1.0.0 since that might give us some years to get really good at it. I've written several school papers the night before the deadline, and they were never any good ;-( Another side effect of starting early is that some might even be able to use ReactOS before 1.0.0.
Casper
-----Original Message----- From: ros-dev-bounces@reactos.org [mailto:ros-dev-bounces@reactos.org] On Behalf Of Alex Ionescu Sent: 15. november 2005 21:36 To: ReactOS Development List Subject: Re: [ros-dev] RE: [ros-diffs] [cwittich] 19254: add a check forSPDRP_CONFIGFLAGSwhich is not working yet
While your points are valid, newdev is his baby and he should be allowed to do with it as he damn well pleases. Don't tell me you've never done this kind of operation locally on your disk? Christopher just chose to commit it. It's why I now run a local branch and only commit final code; it lets me do what I want with code that I wrote 100% of, without being criticzed about design choices before the application is complete.
Note: If this wasn't his app or was a really old app we depend on, I would agree with you. But as far as I'm concerned this is his application and for now there's no point in reviewing it until he considers it done and ready for review.
Best regards, Alex Ionescu
No way. Contributing something doesn't give you any more right over it than anyone else in the community (except copyright). If someone can't accept that, then they shouldn't contribute to a community effort like ReactOS. If newdev is Christopher's "baby", then ntoskrnl must be David's "baby". So what were you doing in David's code?
I'm sorry to hear that you believe that reviewing is just about critiquing someones work and that you are obviously so afraid to answer questions about your code that you choose to keep it a secret for as long as possible and commit it in big batches.
There is very much a point in reviewing as early as possible. It is to find bugs as early as possible.
Casper
Casper Hornstrup wrote:
-----Original Message----- From: ros-dev-bounces@reactos.org [mailto:ros-dev-bounces@reactos.org] On Behalf Of Alex Ionescu Sent: 15. november 2005 21:36 To: ReactOS Development List Subject: Re: [ros-dev] RE: [ros-diffs] [cwittich] 19254: add a check forSPDRP_CONFIGFLAGSwhich is not working yet
While your points are valid, newdev is his baby and he should be allowed to do with it as he damn well pleases. Don't tell me you've never done this kind of operation locally on your disk? Christopher just chose to commit it. It's why I now run a local branch and only commit final code; it lets me do what I want with code that I wrote 100% of, without being criticzed about design choices before the application is complete.
Note: If this wasn't his app or was a really old app we depend on, I would agree with you. But as far as I'm concerned this is his application and for now there's no point in reviewing it until he considers it done and ready for review.
Best regards, Alex Ionescu
No way. Contributing something doesn't give you any more right over it than anyone else in the community (except copyright).
I did not argue against that.
If someone can't accept that, then they shouldn't contribute to a community effort like ReactOS.
I agree.
If newdev is Christopher's "baby", then ntoskrnl must be David's "baby". So what were you doing in David's code?
He abandonned his code and it needed a new maintainer. I think you're (purposedly) confusing "maintaining" and "owning" while trying to make a point which doesn't exist. Ge didn't agree with a change I made in Freeldr, and I reverted it. Why? Because he's maintaining it and I don't want to make his life hard. If Magnus spent 6 months writing a DX library from scratch, does that mean I can come in and rewrite half his code because I don't like it? No it doesn't. As the maintainer and guy that wrote 100% of the library he should have a choice in design decisions made with it. That doesn't mean he has to be Hitler, but it does give him some authority over his own code.
I'm sorry to hear that you believe that reviewing is just about critiquing someones work
Never said that.
and that you are obviously so afraid to answer questions about your code that you choose to keep it a secret for as long as possible and commit it in big batches.
What kind of delluded paranoid argument are you getting into? I never said any of these things.
There is very much a point in reviewing as early as possible. It is to find bugs as early as possible.
I agree and never said anything against this.
Casper
Sometimes I think you like to invent things just to have something to say. I never said that "I'm afraid to answer questions" or that "Reviewing is only about critiquing". I pointed out that I find it stupid and wasteful to make community decisions on something that the writer hasn't done yet! Imagine a program or DLL as being a book. It starts out in 3 phases
1) Manuscript being written. At this moment, entire sentences are added and removed, sometimes 3, 4 times in a row until the author decides what to do. Entire chapters can disappear. Plot twists can be removed or added. 2) Manuscript is written. The author is happy with his work and thinks its mostly complete. His editor and publisher will now do reviews, which can still result in modifications present in the book, sometimes even major. Sometimes the author might not agree, but usually they are team decisions, and many times it's mistakes that the author himself hadn't noticed 3) Published book.
Now when coding with a SVN system, imagine that EVERY sentence modification in the manuscript is a commit, so therefore becomes a new manuscript. Do you think that the author sends the editor a new copy EVERY time he makes a sentence?
Oh yes, I'm starting a book, and my book starts out with "In a butiful summer afternoon". I send it to the editor. Editor tells me "beautiful, not butiful." I respond "oh yes, I was really tired that night, I fixed it a long time ago. Here is a new come: "In a beautiful summer afternoon, Jake went to the store". Editor responds "Well, you didn't tell us about Jake yet!". Author says "Yes, I didn't write his character in yet, here is a new copy of the manuscript (which is only 1 line at this moment) which introduces Jake.."
Are you starting to realize how pathetic and stupid this woudl be? Society itself would crumble if people worked like this. The same must apply to code. The coder starts out with his own internal revisions, as he slowly writes his functions and fixes his bug. r40 might have a 1000 line function, that by r42 the coder decided was useless and removed. If he had published r40, then everyone would've reviwed it, maybe even made some helpful comments about it, only to see it gone in r42, because nobody is in the mind of the coder and can possibly know his ultimate design. Even when David commited code to the kernel (I checked the logs), it wasn't 1 line at a time. He commited entire chunks of functions which already revealed internal testing and decisions he made during that time. And so does almost anyone committing code.
The difference here is important because it's a difference between pre-written code, in which constant and progressive changes are needed, and non-existant code, in which a whole library is written from scratch. The "small patches and commit every 100 lines" theory does not, and cannot hold, for something being written from scratch, especially if you're a top-down developer. It might only work for people that have already thought of the design for entire months, and start out with their DllMain and then write the 100 stubs they know they will need. But most developers don't do that; some are not aware of the potential issues which might arise. You can have developed your whole program, only to realize it fails miserable on MP platforms because you never tested. And this kind of mistake happens even to the best programmers, sometimes years later. Look at the Audio subsystem in NT. It was rewritten THREE times, going from 2/3 user mode 1/3 kernel mode to 1/2 user mode 1/2 kernel mode to 100% user mode in Vista. And we're talking about a company with dedicated teams and years of experience to find these kinds of design flaws. But it still took them 15 years to realize it. So don't tell me that a coder can't realize a mistake after only a month of testing.
That's why I commited ws2_32 as a "large patch". Not because I was afraid of reviews. I WANT reviews. It's why I had a local SVN branch and did the kind of diffs that are usually used when modifying a library, so that if anyone DOES want to peer into my own thoughts and decisgn decisions, they can. But if they had done that at r1 and started changing things around, then by r4 all those changes would've been irrelevant. Or comments like "hey, you didn't think about ..".. Of course I didn't! I haven't reached that stage in my implementation phase yet!
I'm sorry for making this so lenghty but I felt very insulted by your comments which seemed to imply that I hate reviews, that I'm afraid of them, and I have unusual ideas about FOSS code contributions.
Best regards, Alex Ionescu
-----Original Message----- From: ros-dev-bounces@reactos.org [mailto:ros-dev-bounces@reactos.org] On Behalf Of Alex Ionescu Sent: 15. november 2005 22:44 To: ReactOS Development List Subject: Re: [ros-dev] RE: [ros-diffs] [cwittich] 19254: add a checkforSPDRP_CONFIGFLAGSwhich is not working yet
He abandonned his code and it needed a new maintainer. I think you're (purposedly) confusing "maintaining" and "owning" while trying to make a point which doesn't exist. Ge didn't agree with a change I made in Freeldr, and I reverted it. Why? Because he's maintaining it and I don't want to make his life hard. If Magnus spent 6 months writing a DX library from scratch, does that mean I can come in and rewrite half his code because I don't like it? No it doesn't. As the maintainer and guy that wrote 100% of the library he should have a choice in design decisions made with it. That doesn't mean he has to be Hitler, but it does give him some authority over his own code.
Any developer can argue for/against a change, so why make the distinction between owner and non-owner? I would sleep so much better at night if the reason you reverted the freeldr change was that Gé provided a sound technical argument against the change (have to build, test and distribute different .isos for different sub-architectures, only to save a few bytes in the freeldr/setupldr binaries) and not because he was maintaining freeldr. Maintainers come and go. The source code stays.
There is very much a point in reviewing as early as possible. It is to find bugs as early as possible.
I agree and never said anything against this.
Ok, maybe I interpreted it wrong. Maybe you or someone else could explain how the following should be interpreted then?
"It's why I now run a local branch and only commit final code; it lets me do what I want with code that I wrote 100% of, without being criticzed about design choices before the application is complete."
Note that I'm not against local branches at all. What I care about is what and how changes get into the ReactOS repository.
Casper
Sometimes I think you like to invent things just to have something to say. I never said that "I'm afraid to answer questions" or that "Reviewing is only about critiquing". I pointed out that I find it stupid and wasteful to make community decisions on something that the writer hasn't done yet! Imagine a program or DLL as being a book. It starts out in 3 phases
And I argue that it is not stupid and wasteful. It gives more developers a chance to comment on other developers work. Other developers may have experience that may be beneficial in the development of that particular feature. Other developers may have knowledge of another corner of ReactOS where similar code exists and code could be shared. Other developers may see problems with the direction the developer is going and could bring it to the attention of the developer before going a thousand miles in a "bad" direction. Other developers may have experience that could add to achieving a better solution. Holding off the commit/review, decreases the usefulness of the distributed nature of the development of ReactOS.
- Manuscript being written. At this moment, entire sentences are added
and removed, sometimes 3, 4 times in a row until the author decides what to do. Entire chapters can disappear. Plot twists can be removed or added. 2) Manuscript is written. The author is happy with his work and thinks its mostly complete. His editor and publisher will now do reviews, which can still result in modifications present in the book, sometimes even major. Sometimes the author might not agree, but usually they are team decisions, and many times it's mistakes that the author himself hadn't noticed 3) Published book.
Maybe if the editor was brought in from the start, he could have prevented wasting time on writing those sections just to remove them later.
you're a top-down developer. It might only work for people that have already thought of the design for entire months, and start out with their DllMain and then write the 100 stubs they know they will need. But most developers don't do that; some are not aware of the potential issues which might arise.
There is no need to think about the entire design. You just set a goal for each commit that will bring the software one step closer to where you want it to be. Be it preparing for something by refactoring the existing code, adding or deleting code or something entirely different. The important thing is to try to have it work better or at least just as good as before the commit.
You can have developed your whole program, only to realize it fails miserable on MP platforms because you never tested. And this kind of mistake happens even to the best programmers, sometimes years later. Look at the Audio subsystem in NT. It was rewritten THREE times, going from 2/3 user mode 1/3 kernel mode to 1/2 user mode 1/2 kernel mode to 100% user mode in Vista. And we're talking about a company with dedicated teams and years of experience to find these kinds of design flaws. But it still took them 15 years to realize it. So don't tell me that a coder can't realize a mistake after only a month of testing.
I don't see what you are arguing for/against here.
Casper
-
Alex Ionescu -
Casper Hornstrup