RE: [ros-diffs] [cwittich] 19254: add a check for SPDRP_CONFIGFLAGSwhich is not working yet

[Casper Hornstrup]

~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

[Alex Ionescu]

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
>   
>

[Casper Hornstrup]

>   -----Original Message-----
>  From: ros-dev-bounces(a)reactos.org [mailto:[e-mail]] 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

[Alex Ionescu]

Casper Hornstrup wrote:

>    
>
> >  -----Original Message-----
> > From: ros-dev-bounces(a)reactos.org [mailto:[e-mail]] 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

[Casper Hornstrup]

>   -----Original Message-----
>  From: ros-dev-bounces(a)reactos.org [mailto:[e-mail]] 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.

>   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. 
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