[Freeciv-Dev] Re: Documentation, Usability and Development

[Kevin Brown <kevin@xxxxxxxxxxxxxx>]

Petr Baudis <pasky@xxxxxxxxxxx> wrote:
> > > >    I sent in some huge patches, but several people complained about it,
> > > > saying that I had actually written 
> > > > code^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H
> > > > not thought out the design enough and their whiz-bang paper tiger was
> > > > better.  Since then I've heard nothing about it.
> > > Huge patches are bad idea, approval chance near zero, IMHO.
> > 
> > Yeah, but *WHY*?
> Because it is hard to review those patches. And if something will go
> wrong, and the patch will need to be reverted, it will get reverted
> as a whole, not only a part. Higher count of smaller patches are
> always better definitively.

So tell me then: what's the difference between reverting the entire
patch and not accepting it at all?  In terms of the final effect on
the code, there is *no* difference.  And yet, that's exactly the
comparison you're making.

But if you accept the patch, there is a chance (perhaps a good one,
even) that it won't need to be reverted, and the program will be
improved as a result.

So in the case where you accept the patch, there is some nonzero
chance that it'll pass muster, and the program will improve as a
result.  In the case where you reject the patch, there is no
improvement.

Sounds to me like accepting the patch is the way to go.

And one other thing: you can't always get there from here via small
patches.  Some things require large patches and there's no other way
around it.  Furthermore, a set of small patches isn't necessarily
going to be higher quality as a whole than a single large patch.
Oftentimes, quite the opposite will be true: the large patch will have
more cohesion because it's written by a single person with a very
clear idea of what's going on.

> >                   A patch should be disapproved only if it breaks something
> > in a very fundamental way.
> This way you can't easily check if it breaks something or not.

Sure you can: let the playtesters at it.  That's the whole *point*
behind accepting the patch for inclusion in the main (or, I prefer,
development) tree.

And of course you run your regression suite against it as well.

> <flamebait>
> Who was talking about naming conventions or formatting here..? :-)
> </flamebait>

Oh, well, I just figured that those things were implicitly included.
:-)


-- 
Kevin Brown                                           kevin@xxxxxxxxxxxxxx

    It's really hard to define what "unexpected behavior" means when you're
                       talking about Windows.