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

[Kevin Brown <kevin@xxxxxxxxxxxxxx>]

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.

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

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.

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

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

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.

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

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?


-- 
Kevin Brown                                           kevin@xxxxxxxxxxxxxx

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