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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Aug 16, 2001 at 12:20:27PM -0700, Kevin Brown wrote:
> Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > IMHO it is hard to decide if a patch is high-risk or not. It should be
> > possible to minimize the risk by carefully reviewing and testing
> > it. It should be possible to turn the feature off if the patch is
> > controversially.
> 
> Well, there are certainly some metrics that can be used.  For
> instance, the more code that's actually *changed*, the greater the
> risk.  :-)  But yes, it's certainly going to be a bit subjective.
> Essentially, if enough people on the list agree that a patch is
> high-risk, then it's high-risk.

In MOST cases a large patch can be splitted.

> > > 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.
> > 
> > IMHO this shouldn't be measured with time but with approvals and
> > rejects. No formal system, just an email with yes or no (and a comment
> > in case of a no).
> 
> If there isn't a time limit, then a patch can be stalled indefinitely
> simply as a result of nobody commenting on it.  Someone has to apply
> the patch to CVS, so it's going to take some effort on somebody's part
> regardless.  The purpose of the time limit is to provide incentive to
> the rest of us to actually comment on the patches that are submitted.
> If we know we have to deal with the consequences of an unreviewed
> patch, we'll be a lot more inclined to look at them, don't you think?

Convinced.

> We can adjust the time limit to fit the circumstances.  If the CVS
> maintainers are falling behind, or if the rest of us are, then we
> increase the time limit.  But I think the time limit should be there.
> 
> > > 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.
> > 
> > Nack. The submitter has to provide the style. I don't think it is
> > dis-encouraging if the patch sender gets a "we love to apply the patch
> > but there are style issues with it, please correct them" email.
> 
> 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.

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

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

> > It looks like a lot of people play via
> > civserver. Is it possible to patch servers at the civserver with
> > controversially patches? So players can test new ideas in an easy
> > way. Let the server print out at the start of the game something like:
> > "
> >   This is cvs version 2001-xx-yy with patch xyz version n applied
> >   This patch does:
> >      ....
> >   Please provide feedback to freeciv-dev or to the author <aslkdj@xxxxxxxxx>
> > "
> 
> The only problem is that a lot of the changes to the servers,
> particularly new features, require corresponding changes to the
> client.  They're pretty strongly coupled.  You can do what you're
> talking about with server bugfixes and certain additional features,
> but it's pretty limited, I would imagine.

Yes I forgot this.

> Still, we should leverage all the resources we can!  So if a patch
> allows us to change the server without changing the clients, then I'm
> all for deploying such changes on one of the civserver games.
> 
> In fact, if we don't already have it, perhaps we should have a
> mechanism (accessible from the web page) where people can give
> feedback about a specific game they played, regardless of whether it's
> a stable server that was running it or a patched, development one.
> The mechanism should record the details automagically so that we'll
> know what version and environment they were working with.  Perhaps tie
> this in with the bug system?

What do other think about this?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "- Amiga Y2K fixes (a bit late, wouldn't you say?)"
    -- Linus Torvalds about linux 2.4.0 at 4 Jan 2001