[Mono-devel-list] Re: [Mono-patches] mcs/class/System/System.Collections.Specialized ChangeLog,1.18,1.19 StringCollection.cs,1.5,1.6

[Paolo Molaro]

On 06/10/03 Gonzalo Paniagua Javier wrote:
> > - *  Copyright (C) 2001 John Barnette
> > - * (C) Ximian, Inc.  http://www.ximian.com
> 
> I don't think that removing those people from the list of authors is a
> good practice. Even if you rewrote the whole class, the 'namespace',
> 'class' and method/event/properties signatures are part of their work.
[...]
> I'd like to hear more opinions on best practices for this topic.

I agree with gonzalo on this one. Even if a class is rewritten, the
copyright should not be removed, because the code is never really
rewritten totally anyway. But there is also another fundamental issue,
here, see below. As for people adding their copyright and name all over
the place when they change one line of code, it's not such a bit deal,
other than making the guy that does that look childish.

The other fundamental issue is that some people have been given cvs
accounts, but don't follow the simple common sense rules for working on
a project. I realize the rules are not written anywhere specifically,
but we trusted people to be careful with their commits: I guess spelling
out the 'rules' for people that don't know how to work on a big project
is worthwhile. We currently don't want to enforce the rules, but if
someone abuses cvs, we'll have to take measures.

Even if you have cvs write access, that doesn't mean that you can
change code at will in all the directories or modules. Directories and
namespaces have a sort of unofficial ownership. If you're not the owner
of a piece of code and you want to make changes/fixes to it, there are
two cases:
1) the change is a typo fix or a one-liner build or trivial fix: in this
case almost anyone can commit (always remembering to add the proper
changelog entry to explain the change). Note, I said almost anyone,
because changes to certain directories should be almost always reviewed 
first, like changes to core stuff: corlib/System, System.Collections,
mini/, metadata/, System.IO.
2) the change is larger: in this case the patch *must* be sent to
mono-devel-list for review by the owner of the code and by the other
hackers (note: always submit patches to the list or bugzilla, though you
may put the owner of the code in the cc header: hackers come and go,
mailing a patch to a personal address only is a good way to get the
patch forgot and missed, plus getting the patches reviewed as well as
reviewing them, is a good thing, so try to get used to it).

Note: if the patch is an addition of code and doesn't change any of the
existing code, the rules are slightly relaxed: there is more freedom
in committing such changes, if they don't interfer with the existing
codebase.

Now, how do you get to be the owner of a chunk of code? The answer is
really simple: you wrote the code, so you're the unofficial owner.
There is also another way: after sending a few patches for the code, the
owner (or the core developers of mono, if the owner somehow disappeared)
trusts you and tells you you're free to commit without getting his
review first.
Here is for example a (partial) list of namespaces/directories with their 
owners (I may well have forgot some):

	debugger module and debug code in mono: martin
	mcs compiler: miguel, martin, ravi
	reflection/reflection.emit: lupus, zoltan
	io-layer: dick
	mini: lupus, dietmar
	test suite: nickd (though anyone should feel free to add test cases)
	System.IO: dick, ville
	security stuff: spouliot
	ilasm: jackson
	System.Web and related: gonzalo
	System.Xml: eno, piers
	Remoting: dietmar, lluis
	interop/marshal: dietmar
	threads: dick
	etc. etc...

So, if you're not the owner of the code, committing a rewrite without
getting a review first is not good cvsitizenship (especially when the
rewrite claimed to fix bugs, but not a single regression test has been
added to the suite).

Once you know you can commit a patch (because of the rules above) there
are a few other small rules to follow:
*) Always add a changelog entry with a meaningful explanation
*) If you fix a bug, add a regression test for it in the regression
suite
*) Don't commit unrelated changes together with a fix: do fine-grained
commits
*) Always check what you're committing: make sure you're only committing
what you need and make sure you don't change line endings and whitespace
(do a cvs diff of the files you're going to commit and check the changes)
*) don't do reformatting commits, unless you're the original author of
the code
*) when fixing bugs, don't follow the documentation blindly, it may well
be wrong: test the behaviour on the MS runtime or ask on the list for
discussion if unsure: don't be afraid of having your changes reviewed.
*) never remove copyright notices from the code
*) never remove licensing info from code
*) never commit code you didn't write yourself or code that doesn't
have a suitable license
*) follow the style conventions
*) keep an eye on performance considerations, especially for code in
core classes, ask on the list for guidance
*) do a regression test run and a bootstrapping build if making changes
to core functionality before committing

Also, remember to pat yourself on the back after the commit, smile and
think we're a step closer to a better free software world.

Thanks.

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better