[Freeciv-Dev] Re: commit early, commit often (was: Submit patch again?)

[Kevin Brown <kevin@xxxxxxxxxxxxxx>]

Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> In MOST cases a large patch can be splitted.

I'm assuming that a patch addresses one specific problem rather than
many.  I agree that a patch can be split if it attempts to address
multiple things.  Not sure how many of those we tend to get, though...

> > Perhaps, but what's more important, the style of the code or the
> > functionality?  Rejecting patches due to style is the same as valuing
> > style over substance, something I'm not at all comfortable with.  Like
> > I said, if someone has a problem with the style, they can submit a
> > patch to fix it.  Much better to apply the patch and fix the style
> > later than to never apply the patch.
> 
> Think about long term maintainability. How old ist freeciv now? First
> CVS files are from 1998. Somebody has to go over to code at certain
> times and cleanup intention and other things (naming comes to mind) or
> else the code will be "rotten" away in 3 years. Comitting a patch is a
> good time to do such things. Either the patch author can do it, or the
> maintainer or another person. I vote for the patch author.

I guess this could go either way.  I tend to believe that in the
general case, those who have problems with a patch should be
responsible for fixing it until it satisfies their criteria, as long
as the problem they have is something minor like aesthetics.

Otherwise, where does it stop?  If we *don't* make those who have
objections responsible for taking care of those objections (at least
when those objections are rather trivial in nature), then patches will
be rejected for essentially arbitrary reasons, and that's not good for
any of us.  No, I think it's better that patches get rejected only if
there are truly solid technical reasons for it.

But "solid technical reasons" still covers quite a lot of ground.
Consistent use of available macros and functions is a good example,
especially in light of some of the other discussion going on at the
moment.  :-)

I don't have a problem with asking the patch submitter to make
changes.  But there is a big difference between doing that and
rejecting the patch.  My point in all of this is that the patch
shouldn't be rejected on trivial grounds.  If the patch submitter is
willing to make the requested changes, fine, but don't reject the
patch just because he can't or won't unless the changes have a solid
technical reason for being requested.

> > In any case, much of the style is indentation and things like that,
> > but that can be taken care of by the CVS maintainers themselves simply
> > by running indent against the entire source tree from time to time.
> > As long as indent doesn't have any bugs that would break things...
> 
> This causes noise.

I'm not sure I follow.  If *all* of the source is run through indent
like this, then I can see a huge set of diffs being generated once,
but afterwards?

> > > I don't like the idea of two separate cvs branches. They just spread
> > > the manpower more. 
> > 
> > A little, perhaps.  The two branches aren't primarily for the
> > developers, they're for the users.  Lots of people just aren't going
> > to be interested in running the latest and greatest features, but
> > they're sure going to want bugfixes!
> 
> AFAI remember there aren't this many bugfixes in the past and the
> bugfixes got committed with hassle.

Well, what do you expect?  If recent history is anything to go by,
then in the past it was difficult to get *anything* committed, bugfix
or not.  So naturally, there aren't going to be that many bugfixes if
the code tends to change slowly over time.

Admittedly, part of the reason will be that code changes get the
highest scrutiny, so what gets committed tends to be relatively
bug-free anyway.  That's a good thing, but it certainly has a price:
the speed of development slows to a crawl, relative to other projects
that aren't as tightly controlled.

> > And look what happens if we don't split into stable and development:
> > we end up with very conservative application of patches because we
> > only have one source tree to work with and can't afford to break it
> > badly.  In other words, we end up right back where we are right now.
> > Having a development branch gives us a LOT more flexibility in terms
> > of what patches we can apply to it, because we don't have to be as
> > concerned about the "safety" of the source.  We can relax the
> > standards a lot for the development branch because we know the issues
> > will be ironed out through testing and bugfixing.  We will also know
> > that, in the worst case, we can fall back to the current stable branch
> > and start over if the development branch gets too hosed.  That way we
> > at least won't lose a bunch of bugfixes.
> 
> Basic CVS provides such safety. 

Only to the extent that you can revert to a previous version.  Not the
same thing at all.  Oh, you could get diffs between versions to
isolate the bugfix patches and reapply them, but that's quite a bit of
work, and as such would be viewed as "risk" by the CVS maintainers,
which would make them more reluctant to commit patches in general.

Unless there's some capability of CVS that I'm overlooking...

> I would like to see the acceptance not depending on the branch but
> on time. So after a release a lot of relative risky patches gets
> included. Than comes a freeze with only bugfixes and maybe also
> backouts. Release and repeat.

Well, this is more or less what I had in mind anyway, but how are you
supposed to isolate bugfixes from improvements with one tree?  Backing
out a feature patch that was applied before a bunch of bugfixes may
prove to be nontrivial, but with two trees it's much less of an issue
because the bugfixes are "safe" in the stable tree: you'll always have
a stable, up-to-date tree to fall back on if the development tree gets
messed up.

Even so, you still haven't addressed my main concern, namely that the
use of a single tree will result in a very conservative application of
patches: if I'm a CVS maintainer and I know that the application of a
feature patch may make a lot of work for me later if I have to back it
out, then I'm going to be a lot more reluctant to commit it.


Seems to me that CVS gives us a lot of capabilities with its ability
to manage separate trees and merge them later.  I think we'd be stupid
to not use that to our advantage.



-- 
Kevin Brown                                           kevin@xxxxxxxxxxxxxx

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