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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Aug 17, 2001 at 09:27:13AM -0700, Kevin Brown wrote:
> 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.  :-)

Mhh yes. But before a patch is rejected or the author asked for
changes for such a reason there has to by a guide line. It may be good
to start one when there is a conclusion about the macro-vs-function
and the normal-vs-real-proper-tile topics.

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

Never I had in mind a complete rejection. Either the author or the
maintainer (if the patch is "good" and the author won't do it) has to
do the cleanup.

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

You are right. However nobody has the courage to do so. At least that
was the answer I got.

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

I think this is sufficient.

> , and as such would be viewed as "risk" by the CVS maintainers,
> which would make them more reluctant to commit patches in general.

This would the same situation as now.

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

You assume that there must always a tree which is stable. Is this
really necessary? Let me define what kind of stableness I have in
mind: at all time the tree has to compile without errors (except
"exotic" parts like vms, amiga or beos) and a basic games (all ai and
2 humans plus ais) must be possible. There may be a chance that the
recently introduced feature will only 90 out of 100 times. This is
ok. It is also ok that the changes a feature made to the general code
base breaks 5 out of 100 times.

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

Not if the patch worked in civserver or another developer tested
it. So serveral kinds of feedback is needed:
 - general feedback about the idea
 - feedback about the style and correctness
 - feedback about actually trying out the patch

The question is when get you the feedback: before the cvs commit or
after it or after the commit to the first branch but before the secon
one?

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Sit, disk, sit. Good boy. Now spin up. Very good. Here's a netscape
  cookie for you. Fetch me some data. Come on, you can do it. No, not that
  data. Bad disk. Bad." 
    -- Calle Dybedahl, alt.sysadmin.recovery