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

Mon Sep 6 14:18:38 CEST 2004

Roberto Togni writes:
> On 2004.08.30 14:33, Diego Biurrun wrote:
> > I've included the main block of cvs-howto.txt from my local tree with
> > all the changes suggested above included.
> > 
> > Please comment.
> >
> [...]
> > 4. Adding new files/dirs:
> > 
> >     cvs add filename/dirname
> add also "cvs commit filename" here

OK

> > 6. Checking changes:
> > 
> >     cvs -z3 diff -u filename(s)
> >
> maybe cvs update before doing it? i usually do it, if version number  
> does not match with current cvs commit is aborted.
> you can also use cvs status to check for it iirc

Hmm, I don't think it's necessary to do it before cvs diff.  If there
has been a commit in the meantime you should notice it in the diff.
However, I would suggest doing cvs update before committing.

> > 7. Checking changelog:
> > 
> >     cvs -z3 log filename(s)
> >     cvs -z3 annotate filename(s)
> Please mention also cvsweb, it's a lot easier to track changes with  
> that than with log

Yes, I also like cvsweb quite a bit.

> > 9. Reverting broken commits
> > 
> >   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.
> I would add some clarification here (or maybe i  the policy section):
> Revert a change if and only if:
> - you made something stupid, like committing the wrong version of a  
> patch, some broken code or cosmetics, and you're going to recommit the  
> right version immediately
> - you committed something that was not intended to be committed (like  
> your private debug version of another file not involved in your  
> changes)
> 
> Never revert:
> - patches committed long time before the reversal (remove them in the  
> traditional way, giving a reason for it)
> - fix that didn't work as expected, or worked for you but not for  
> others: just commit the right fix
> 
> You cannot revert file add and file remove
> 
> So imho use reversal only for wrong use of cvs (technically wise or  
> policy wise), not for nonworking code

agree

> > 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 committing uncomplete/partial working code (that compiles  
> correctly) that implements a new feature (so it doesn't break current  
> features) is ok

I could live with that as long as it is not orphaned and never fixed.
If something is broken you should be willing to fix it up eventually.

> > 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.
> keep it this way, for exotic hardware or features nobody cares about  
> it's better to trust the patch committer than let the patch unapplied  
> (and maybe ask him if he wants to take mantainership of that code, if  
> currently unmantained).

Agree.  If it works for the author and does not break your setup it's
probably safe.  Of course the patch has to fulfill basic quality
standards and not be just an ugly workaround.

> > 3. You can commit unfinished stuff (for testing etc), but it must be
> > disabled
> >    (#ifdef etc) by default.
> same as 1, why duplicate it?

It's not the same IMO.  But apparently it is confusing, so I merged it.

> > 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.
> Remark _really_
> we should not encourage people to commit cosmetics after every patch

OK

> > 8. If you apply a patch by someone else, include his name and email
> > address in
> please add some "antispam" to mail address, don't put them unchanged on  
> the list; list is archived in a lot of public web pages.

Hmm, yes, probably a good idea.

> >    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
> this should apply only to code with an active mantainer if patch is a  
> fix, and probably also if it adds an useful feature.
> If a mantainer disappears for months we can't stop developement.
> Obviously you need to understand the code and the patch, not apply it  
> blindly
> That's also very similar to 5

I agree.  We should define some sort of policy about what to do with
unmaintained parts or with unavailable maintainers.

I propose that we allow commits in this case if a patch is sent to
dev-eng first.  Some time should pass to allow maintainers to react.
We need to define what an acceptable timeframe for this is.  A week?
A month?

> > 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.
> Strangely you forgot about this :)
> when someone submit a patch that changes/extends command line syntax,  
> ask him to send a patch also for the man page.

That's already in patches.txt.

> but imho do not reject an useful patch just because author did not  
> submit patch for docs

Just ask them to provide it, it worked fine so far.

> > 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.
> as asked in a previous message, i think subscribing to advuser can be  
> suggested, that list gets about 10 msg/month

Maybe, I'm not sure.

> should devels subscribe to -announce?

That's not necessary if you subscribe everywhere else, so I'd leave it
up to the individual developer to decide.

I pasted the updated sections I and II from cvs-howto.txt below.

Any more comments/suggestions?

Diego

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 update -dPA
    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).

4. Adding new files/dirs:

    cvs add filename/dirname
    cvs commit filename/dirname

5. Removing files:

    rm filename
    cvs remove filename
    cvs commit filename

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.

7. Checking changelog:

    cvs -z3 log filename(s)
    cvs -z3 annotate filename(s)

  You may also find viewcvs, a web frontend for CVS helpful. It's often more
  comfortable than using cvs log, cvs annotate and cvs diff. Find it here:
  http://www1.mplayerhq.hu/cgi-bin/cvsweb.cgi/main

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 & readd 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.)
   You can commit unfinished stuff (for testing etc), but it must be disabled
   (#ifdef etc) by default so it does not interfere with other developers'
   work.

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.

3. Do not commit unrelated changes together, split them into self-contained
   pieces.

4. Do not change behavior of the program (renaming options etc) without
   first discussing it on 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 (try to avoid this), separate them strictly from real
   changes.

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

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.

8. If you apply a patch by someone else, include the name and email address in
   the CVS log message. Since the mplayer-cvslog mailing list is publicly
   archived you should add some spam protection to the email address. Send an
   answer to mplayer-dev-eng (or wherever you got the patch from) saying that
   you applied the patch.

9. Do NOT commit to code actively maintained by others without permission. Send
   a patch to mplayer-dev-eng instead.

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. Update the documentation if you change behavior or add features. If you are
    unsure how best to do this, send a patch to mplayer-docs, the documentation
    maintainers will review and commit your stuff.

12. Revert a commit ONLY in case of a big blunder like committing something not
    intended to be committed or committing a wrong file, the wrong version of a
    patch, cosmetics or broken code and you are going to recommit the right
    thing immediately.

    Never revert changes made a long time ago or buggy code. Fix it in the
    normal way instead.