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

Mon Aug 30 14:33:23 CEST 2004

Attila Kinali writes:
> 
> >3. Committing changes:
> >
> >    cvs -z3 commit -m "comment - what you changed and why" 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. If you leave out -m at the command line you
> >  will be prompted for a comment in an editor (usually vi).
> 
> editor should mean here $EDITOR, dunno what cvs does if it is not set.

>From the CVS book:

-e EDITOR

Invokes EDITOR for your commit message, if the commit message was not
specified on the command line with the -m option. Normally, if you
don't give a message with -m, CVS invokes the editor based on the
$CVSEDITOR, $VISUAL, or $EDITOR environment variables, which it checks
in that order. Failing that, it invokes the popular Unix editor vi.

> >6. Checking changes:
> >
> >    cvs -z3 diff -u filename(s)
> >
> >  It's recommended to check changes before committing. especially if you forget
> >  what you changed :)
> 
> This should be always done. Double checking that the commit does the
> right thing is necessary.

OK

> >7. Checking changelog:
> >
> >    cvs -z3 log filename(s)
> 
> Maybe annotate should be mentioned here too.

OK

> >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. For the corner case that you remove the last
> >  revision 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 from that revision in the last revision
> >  and leave holes in the revision history.
> 
> We should spell out that it only removes a revision, not the change, ie
> if you remove anything but the latest revision the code will still be
> there, but the revision history will be gone.

I thought it was clear already, but better to err on the side of
caution with such a touchy subject.

> >II. POLICY / RULES:
> >===================
> >
> >1. You shouldn't commit code which breaks MPlayer! (Meaning unfinished but
> >   enabled code which breaks compilation or compiles but does not work.)
> 
> should not -> MUST NOT :)

OK

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

> >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!
> >   Do not commit changes to the build system (Makefiles, configure script)
> >   which change behaviour, defaults etc, without asking (and your change being
> >   accepted) on the mplayer-dev-eng mailing list first. The same applies to
> >   compiler warning fixes and trivial looking fixes. We usually have a reason
> >   for doing things the way we do. Send them as patches to the mailing list,
> >   and if the code maintainers say OK, you may commit. This does not apply to
> >   files written and/or maintained by you.
> 
> Maybe a extra section "dont change other peoples code w/o good
> reason/discussion" should be added.

OK

Anyway, this paragraph can be split up further.

> >5. We refuse source indentation and other cosmetic 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.)
> >
> >   NOTE: If you had to put if(){ .. } over a large (> 5 lines) chunk of code,
> >   do NOT change the indentation of the inner part (move it right)!
> 
> 
> I think this changed a bit, iirc indentation changes are allowed if, and
> only if they are separate from functional changes. Of course not every
> functional change should be followed by an indentation fix.

OK

> BTW the "(move it right)" here is a bit confusing, maybe a "dont move it
> right" would be better.

Is this rule still valid?

> >6. Always fill out the comment at committing (-m switch of CVS, or in the
> >   editor if you left out -m). Describe in a few lines (usually one line is
> >   enough) 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 not
> >   acceptable.
> 
> It should be a bit more than a few lines. Most commit comments are too
> short IMHO. Every commit comment should include  what was changed
> and also a reason why. It makes bug hunting easier. Especialy if other
> peoples patches were applied, a c&p of their patch description doesnt
> hurt.

I completely agree.  I also don't like commit messages like "10l", at
least not without further explanation.  The changes may be obvious on
mplayer-cvslog, but they are not when you look at the cvs log alone.

> Thus i think we should not mention -m as it generaly leads to smaller
> comments.

OK

> >7. 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.
> >
> >8. A'rpi developed something called cvs-backup. It archives the CVS repository
> >   after each commit - so commits can be reversed (without messing up the
> >   changelog) if they are bad. If you think your bug fix or other change was
> >   bad and unneeded, ask A'rpi to reverse it instead of committing the previous
> >   version!
> 
> I thought that cvs-backup is long gone ?

Yes, I think so, paragraph removed.

> >9. You will have write access to DOCS/. This used to be different to avoid
> >   breaking docs or getting translations or the homepage desynced. If you are
> >   unsure about this, send a patch to mplayer-dev-eng or even better to
> >   mplayer-docs, the documentation maintainers will review and commit your
> >   stuff.
> 
> Docu patches should always go to -docs, -dev-eng is the wrong place for
> them.

OK, paragraph rephrased.

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

> Missing here: dont commit unrelated changes together.

OK

> >III. Beginners Guide	by David Holm
> >====================

I'm currently to lazy to rewrite this.  What do the others think?  Is
this tutorial worth keeping?

I've included the main block of cvs-howto.txt from my local tree with
all the changes suggested above included.

Please comment.

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

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)

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

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. You can commit unfinished stuff (for testing etc), but it must be disabled
   (#ifdef etc) by default.

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.

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