[MPlayer-dev-eng] cvs-howto.txt

[Attila Kinali]

On Mon, Aug 30, 2004 at 02:33:23PM +0200, Diego Biurrun wrote:
> > One can never overtest things. Changes should be always tested as much
> > as possible to cover all possible bugs one can think off. Changing stuff
> > with the attitude of "if it breaks something, someone will complain"
> > isnt exactly what we want.
> 
> See Reimar's comment.

Yes I know, but people should not be encuraged to commit broken/half
broken code.

 
> > BTW the "(move it right)" here is a bit confusing, maybe a "dont move it
> > right" would be better.
> 
> Is this rule still valid?

Afaik yes, it makes it easiert to catch functional changes and what they
changed. But after rereading it, i must say that the sentence is not
very clear, especialy not for someone new with MPlayer.

 
> > >10. Subscribe to the mplayer-cvslog mailing list. The diffs of all CVS commits
> > >    are sent there and reviewed by all the other developers. Bugs and possible
> > >    improvements or general questions regarding commits are discussed there. We
> > >    expect you to react if problems with your code are uncovered.
> > 
> > Also -advusers should be on the MUST subscribe list.
> 
> Hmm, dunno, what do the others think?

I like (or rather liked, as i dont read -users anymore) -advusers to
forward confirmed bugs so other developers could read them who dont have
the time to read/dont care about -users, while still keeping the
bugreports of -dev-eng.
Maybe i should do some lobbying for this.

 
> I. TECH SIDE:
> =============
> 
> 1. Changing password:
> 
>   As you probably got a restricted CVS-only shell, it's not trivial:
> 
>     ssh LOGIN at mplayerhq.hu passwd
> 
>   Replace LOGIN with your login name. Leave 'passwd' unchanged, it's a command.
> 
>   Note: If you need a real shell for something, tell A'rpi.
> 
> 2. Checking out development source tree:
> 
>     export CVS_RSH=ssh
>     cvs -z3 -d:ext:LOGIN at mplayerhq.hu:/cvsroot/mplayer co -P main
> 
>   Replace LOGIN with your login name.
>   NOTE: cvs -d:pserver: mode doesn't allow writing, even with password!
> 
> 3. Committing changes:
> 
>     cvs -z3 commit filename(s)
> 
>   Do not use comments such as: "bug fix." or "files changed" or "dunno".
>   You don't have to include the filename in the comment, as comments are linked
>   to files. If you have made several independent changes, commit them
>   separately, not at the same time. You will be prompted for a comment in an
>   editor (see 'cvs -e', usually vi).

How about to start this section with: "Always add a clearly describing
comment about what the patch does. When commiting other people patches
use their patch description." ?


 
> 6. Checking changes:
> 
>     cvs -z3 diff -u filename(s)
> 
>   Doublecheck your changes before committing to avoid trouble later on.
>   This way you will see if your patch has debug stuff or indentation
>   changes and you can fix it before committing and triggering flames.

Add "Always doublecheck"

> 7. Checking changelog:
> 
>     cvs -z3 log filename(s)
>     cvs -z3 annotate filename(s)
> 
> 8. Renaming/moving files or content of files, removing empty directories:
> 
>   You CANNOT do that. Ask the CVS server admin (A'rpi) to do it!
>   Do NOT remove & re-add a file - it will kill the changelog!!!!
> 
>   Don't do a lot of cut'n'paste from one file to another without a very good
>   reason and discuss it on the mplayer-dev-eng mailing list first. It will make
>   those changes untraceable!
> 
>   Such actions are useless and treated as cosmetics in 99% of cases,
>   so try to avoid them.
> 
> 9. Reverting broken commits
> 
>   In case you committed something really broken and wish to undo it completely,
>   you can use the 'cvs admin -o' command, which removes entries from the
>   revision history of a file. It does not undo the changes related to that
>   revision, but for the corner case that you remove the last revision (and only
>   then!) this amounts to reverting a commit.
> 
>   Assuming that 1.123 is the last revision
> 
>     cvs -z3 admin -o1.123 filename
> 
>   will remove revision 1.123, thus reverting the file back to revision 1.122.
> 
>   ONLY use this command to delete the LAST revision of a file. Removing other
>   revisions will NOT undo the changes connected to that revision, so the last
>   revision will remain unmodified, only revision history will be lost and
>   holes left in the revision numbering.
> 
> 
> Contact A'rpi <arpi at thot.banki.hu> if you have technical problems with the CVS
> server.
> 
> 
> 
> II. POLICY / RULES:
> ===================
> 
> 1. You must not commit code which breaks MPlayer! (Meaning unfinished but
>    enabled code which breaks compilation or compiles but does not work.)

IMHO compiling but not working code in progress may be commited if and
only if it is declared as such and there is a comment in the code in
which cases it works and when it fails.

> 2. You don't have to over-test things. If it works for you, and you think it
>    should work for others, too, then commit. If your code has problems
>    (portability, exploits compiler bugs, unusual environment etc) they will be
>    reported and eventually fixed.

* "But it does not mean that you should skip reasonable test that you
  can perform yourself."
* code which is known to be exploitable should be never ever commited, 
  it will always backfire.

> 3. You can commit unfinished stuff (for testing etc), but it must be disabled
>    (#ifdef etc) by default.

merge with 1.

> 4. Do not change behavior of the program (renaming options etc) without
>    discussing it first at the mplayer-dev-eng mailing list. Do not remove
>    functionality from the code. Just improve!
> 
> 5. Do not commit changes to the build system (Makefiles, configure script)
>    which change behaviour, defaults etc, without asking first. The same
>    applies to compiler warning fixes, trivial looking fixes and to code
>    maintained by other developers. We usually have a reason for doing things
>    the way we do. Send your changes as patches to the mplayer-dev-eng mailing
>    list, and if the code maintainers say OK, you may commit. This does not
>    apply to files you wrote and/or maintain.
> 
> 6. We refuse source indentation and other cosmetic changes if they are mixed
>    with functional changes, such commits will be rejected and removed. Every
>    developer has his own indentation style, you should not change it. Of course
>    if you (re)write something, you can use your own style... (Many projects
>    force a given indentation style - we don't.) If you really need to make
>    indentation changes, separate them strictly from real changes.

"If you are changing other peoples code, try to use their indentation
style"
New indentation styles should only be used in seperate block (new
functions/files etc) never within other code. This will just screw up
the already messy indentation of code.

>    NOTE: If you had to put if(){ .. } over a large (> 5 lines) chunk of code,
>    do NOT change the indentation of the inner part (don't move it to the right)!

"Instead move the if() to the left by one or two spaces"

> 7. Always fill out the comment at committing. Describe in a few lines what you
>    changed and why. You can refer to mailing list postings if you fix a
>    particular bug. Comments such as "fixed!" or "Changed it." are unacceptable.

References to mailinglist should not be links to archives other than
ours (those might die at some time) and _always_ include the msg id of
the mail (works offline or if the archive is gone).

> 8. If you apply a patch by someone else, include his name and email address in
>    the CVS comment! Do NOT commit patches for other developer's code (code not
>    maintained by you) without his permission! If he didn't commit - he probably
>    has a reason! Send an answer to mplayer-dev-eng (or wherever you got the
>    patch from) saying that you applied it.
> 
> 9. You will have write access to DOCS/. If you are unsure about changes you
>    make to the documentation, send a patch to mplayer-docs, the documentation
>    maintainers will review and commit your stuff.
> 
> 10. Subscribe to the mplayer-cvslog mailing list. The diffs of all CVS commits
>     are sent there and reviewed by all the other developers. Bugs and possible
>     improvements or general questions regarding commits are discussed there. We
>     expect you to react if problems with your code are uncovered.
> 
> 11. Do not commit unrelated changes together.
> 
> Also read DOCS/tech/patches.txt !!!!

We should soon add a "read code-documentation.txt" too :)

> 
> We think our rules are not too hard. If you have comments, contact us.

			Attila Kinali