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

[Attila Kinali]

Yo,

DOCS/tech review part 2 :)

This time it's cvs-howto.txt:



>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.
Also be aware that a few "modern" distros think vi is evil and dont
install it anymore.


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

>7. Checking changelog:
>
>    cvs -z3 log filename(s)

Maybe annotate should be mentioned here too.


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

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

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

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


>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.
BTW the "(move it right)" here is a bit confusing, maybe a "dont move it
right" would be better.

>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.
Thus i think we should not mention -m as it generaly leads to smaller
comments.

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

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

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

Missing here: dont commit unrelated changes together.

>III. Beginners Guide	by David Holm
>====================
>
>When I first got CVS write access I got banned after only a few hours
>because I didn't fully understand this documentation. This part is for
>those of you who have just got CVS write access and want to avoid the
>most common pitfalls leading to CVS ban.
>I will introduce a step-by-step guide explaining how I'm making sure
>that my CVS commits are proper and won't get me banned.
>
>1. You should set up two directoress for MPlayer, one which contains the stable
>   version and has the :ext: option instead of :pserver: in CVS/Root.
>   The other should be your development directory and have the CVS/Root set to
>   :pserver: instead of :ext:, that way you can't commit development code
>   by accident (since only :ext: allows writes).
>   This is my setup:
>   ~/mplayer
>	    /main
>	    /main.dev
>    NOTE: I'll use these directory names from here on in the guide, what you
>    call your directories is entirely up to you. This is _only_ an example.

Although i use a similar system (ie separate dirs, one checkout with
pserv, one with ssh) i dont understand this :ext thing here :)
Maybe it should contain a small c&p capable tutorial how to achive these
two dirs.

>2. When you are satisfied with the changes in "main.dev" and think you are
>   ready to commit the changes to CVS start by doing the following in the
>   "~/mplayer" dir":
>   diff -Nur -x "CVS" -x ".*" main main.dev > dev2stable
>   dev2stable is the filename for the patchfile, it doesn't matter what you
>   call it.

Oh.. this is not right!
Diffs should be made against the version of which was used as a start
point. main and main.dev are not necessarily updated at the same time.
What i generealy do when working on the code is to copy the mplayer tree
to main.orig and main.working, then change main.working, do a diff
against main.orig. I use here a different directory then main because i
want to be able to update main while i work on somehting.

>3. Now comes one of the tricky parts, editing the patch. I prefer using mcedit
>   (comes with Midnight Commander) since it does syntax highlighting in patches
>   (= it uses colors to identify lines =), But most ASCII editors should do
>   (meaning don't use Star Office and save it as a Star Office document for
>   instance ;) I will try to explain this as good as I can.

You can switch off syntax highlighting for any editor 
(unless it's crap :)

>   Read through the patch and remove all occurrences of:
>
>     * diff -Nur.... that are affecting files YOU have NOT modified. These
>       occur when either main or main.dev are a different version (not checked
>       out at the same time)
>       EVERYTHING from the diff -Nur... line until the next diff -Nur... line
>       are changes to the file specified after the diff options, and ONLY that
>       file.

Diffing against a tree that is not exactly the same as you started with
is a bad thing [tm].
The possibility of overlooking a change made by someone else and thus
accidentialy reverting it again is just to high.

>     * Lines containing "Binary files..." if you add the 'a' switch to -N(a)ur
>       binary files will be added to the patch as well, making it huge and
>       putting a lot of unnecessary data in it (since you seldom commit any
>       binaries).

Why would anyone want to commit binaries to cvs ?

>     * If you find changes within a diff block that you don't want to commit
>       you can delete them if they are the only changes ranging from the
>       @@ -x,y +x,y @@ until the line before the next @@ -x,y +x,y @@. You
>       _cannot_ remove single lines after a @@ -x,y +x,y @@ because that will
>       break the patch!.

You can, you just have to adjust the numbers (yes, i do that quite
often)


>       When I end up in a situation where I have to remove just some lines from
>       a block, I leave it alone, remember (write down) which file it is in and
>       then edit the file in "main" after I've applied the patch.

This is the safe aproach, as changing the numbers might leed to a broken
patch file if done incorectly (yes, i do that quite often too :)

>     * Now it's time for applying the patch to the "main" (stable) directory.
>       This should be done in two steps:
>         1. enter "main" and run
>
>               patch -p1 --dry-run < ../dev2stable

a cvs up -dP should be done first!


>4. It's almost time for the final step, committing the changes. But first you
>   MUST make sure your changes compile without breaking anything and that it
>   follows the Policy mentioned in section 2. (Read it until your eyes are
>   bleeding if you want to keep CVS access!)
>   Don't worry about object files etc that will be created in your "main" dir,
>   they won't  be sent to CVS on a commit, you must use the add command to add
>   new files (discuss it on dev-eng before adding new files!).
>   Now to make sure your additions follow policy do the following on every file
>   you will commit:
>
>     cvs -z3 diff -u <filename> > <filename.d>

a `cvs diff -u .` in main/ should be performed instead so any
unintentional change is also captured.
btw: cvs diff -u |less works too


>   Of course the output file (<filename.d>) can have any name you want. This
>   will create a file showing the differences between the file on CVS and your
>   updated local file.
>   I will explain some of the policy rules I had a hard time understanding:
>
>     II.5: This means that if for instance you have lines in <filename.d> that
>           look something like this:
>
>             -
>             +
>
>           That means you have added or removed tabs or spaces on that line.
>           That qualifies as a cosmetic change and is disallowed. Edit the
>           file and put back/remove the added/removed tabs/spaces.
>           Rediff the file and make sure the cosmetic changes are fixed.

Mention here also the case of:
- some line
+ the same line as above


>     II.6: Make sure you read and understand this properly before committing
>           anything. Commit one file at a time!

Bad thing[tm], commit everything at once. The timeframe between the
cvs diff and cvs ci is usualy small enough that noone else commits
anythign in between. Also commiting everything at once generates less
mails on -cvs as commits are groupped together per directory.


>5. OK, you have a working patch following the CVS policy, excellent work. Now
>   for the final step, committing. This is really simple. Just run the
>   following command in "main" for each file you want to commit:
>
>     cvs -z3 commit -m "<comment (changes)>" <filename>
>     cvs -z3 commit <filename>

Again i think that -m should not be mentioned (see above)


			Attila Kinali