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

[Kevin Brown <kevin@xxxxxxxxxxxxxx>]

Reinier Post <rp@xxxxxxxxxx> wrote:
> On Wed, Aug 15, 2001 at 11:33:32PM -0700, Kevin Brown wrote:
> > As a developer, feedback on a patch is CRITICAL.
> 
> OK.  (BTW, there's no need to argue that point.)

Ah, OK.  Wasn't sure, since patch feedback is sometimes hard to get
here.  :-)

> > You needn't be an expert on an area of code to have useful comments
> > about a patch.  It doesn't take that long to get your feet wet with
> > new sections of the code (I managed to do so for the GTK client's map
> > drawing capabilities in a day or two).  I'll admit that there are
> > parts that are fairly involved, but that's not true of all the code.
> 
> For me, it's a question of dividing attention between C code and the
> other things that surround Freeciv.  (Playing ...)

I'll be the first to admit that I'm probably unusual in that I tend to
spend more time messing with the code than playing, probably because
I'm such a green player that I get more gratification from fixing the
code than getting my butt kicked by the AIs.  :-)

> > Perhaps.  I totally agree with the savegames submission idea, where
> > appropriate.  But there are lots of patches that don't require playing
> > a full game to determine whether or not the patch is worthwhile.  Take
> > my GTK+ client speedup patch (which I'll be releasing as a separate
> > patch dependent on the patch I supplied for PR#892), for instance.
> > The benefits of that patch are obvious simply by starting up the
> > client with the appropriate option to activate it and moving around
> > the map a bit.
> 
> OK, still it would help to give explicit instructions for testing.
> "Start a game, turn off FOW, and move around the map a bit."
> Dumb it down to the level of those who haven't seen the code.

Yeah, definitely agree here.  I also think the author should describe
the patch (what it's good for, the technical details, etc.).
Basically, all the things you'd expect from someone who's submitting a
bug report and fix, since that's basically what it is.

> > > Another problem is that patches get lost or outdated too easily.
> > 
> > This is because they're not being committed to CVS!
> 
> Not sure.  If they were in CVS (on separate branches?)
> their status might be even harder to follow.

The followups to this have been quite insightful.  I certainly don't
think we should create a separate CVS branch for every patch!  But I
think it makes sense to do so for the patches that appear to be very
complicated and otherwise difficult to back out.  In other words, you
create a separate branch for the high-risk patches, and then merge the
branch back into the main trunk once it's been tested.

It's very important for the high-risk branch to be built and tested by
the community.  Otherwise it'll stagnate.

> > In my little time here on the list, there's one impression I get that
> > seems to come through loud and clear: there is a great reluctance to
> > commit patches to the CVS tree, sometimes for the most trivial of
> > reasons (coding style, for instance).
> 
> Yes, the policy has been to keep the source tree stable and clean.
> (Note: I'm just echoing what others have said here, I'm not a
> core developer.)

Yes, and that policy has been very evident!  :-)

However, I think that policy hasn't been particularly good for the
developers here, as I'm sure you've figured out by now...

The policy is completely appropriate ... for a mission-critical piece
of software which only requires bugfixes.  This doesn't exactly
describe Freeciv, except perhaps for those Freeciv fanatics whose
lives depend on getting their next fix (probably more people than
you'd expect!).  :-)


OK, so here's how I think it should work:

1.  We split the source into two development lines: stable and
    development.  That way, the quality of the source can be
    maintained for at least one of the branches, with some caveats
    (namely, that it's more important to have bug-free code than
    pretty code, so bugfixes should almost always go in).

2.  Bugfixes typically get applied against both stable and
    development, depending on the nature of the fix.  If it's a
    particularly involved fix for a bug that most users aren't going
    to encounter, then serious consideration shoud be given to
    applying it against the development branch only.  But for
    bugfixes, the default should be to apply it against both branches,
    since both branches are presumably going to benefit.  Obviously
    fixes to new features that appear in the development tree should
    be applied only against the development tree.

3.  For either branch, if a patch seems particularly involved, it may
    make sense to create a separate subbranch for it, then merge it
    back into its main branch when it's been sufficiently tested.  It
    should be VERY rare that this happens against the stable branch.
    Perhaps such splits should be limited to the development branch.

4.  All patches get a couple of days of review time before being
    applied, but if nobody says anything about a patch, that
    automatically means that it gets applied.

5.  We need to make sure that each branch and subbranch has binary
    builds that are made available on the web site.  We need lots of
    people to test this stuff, and you'll get more people testing
    something if there's a ready-made binary available than if they
    have to build it from the source themselves.  This might be more
    difficult to arrange for the Windows client, so it would be useful
    to have some information about how many people run which client.
    If enough people run the Windows client, then effort should be
    made to have Windows binaries made available for each branch and
    subbranch.

6.  On a periodic basis (say, three to four months), we declare a
    feature freeze in the development branch, subject to whether or
    not the code is in a suitable state for that.  New feature patches
    can still be submitted, but they'll be queued up instead of being
    applied against the tree.  Bugfixes get applied as usual.

7.  Patches get applied to the appropriate places even if there are
    trivial objections to them (such as style).  If someone has a
    problem with the style of a patch, they can supply another patch
    to fix it after it's been applied!  If the person responsible for
    CVS commits has such a problem with the patch, then it's his
    responsibility to fix it before applying it to the tree.  But a
    patch should NOT EVER be rejected for trivial reasons!  The only
    reason to reject a bugfix patch is if there is substantial
    objection to it on this forum.  So people here should be given a
    couple of days to comment on a patch before it gets applied
    against CVS, unless it's an "emergency" bugfix against the stable
    tree.

8.  Under no circumstances should a patch be ignored.  Ignoring a
    patch is the same thing as telling the submitter that his efforts
    are unimportant, which says all sorts of bad things about the
    project.

9.  We need to remember that this is a game, not an operating system.
    So it's actually okay to break things, even in the stable tree,
    from time to time, as long as they get fixed in a timely fashion.
    If a stable version ends up being fundamentally broken, we'll hear
    about it soon enough!  People can revert back to a previous
    version if they have to.  We don't want such breakage to happen
    often because we don't want our reputation tarnished TOO much, but
    neither should we be afraid of it to the point of paralysis.


Thoughts?


-- 
Kevin Brown                                           kevin@xxxxxxxxxxxxxx

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